Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consumers of Event and Run numbers are not using the proper type #47288

Open
1 of 3 tasks
vlimant opened this issue Feb 7, 2025 · 17 comments
Open
1 of 3 tasks

Consumers of Event and Run numbers are not using the proper type #47288

vlimant opened this issue Feb 7, 2025 · 17 comments

Comments

@vlimant
Copy link
Contributor

vlimant commented Feb 7, 2025

In many places, some code is extracting the event and run numbers and using it with "random" C++ type instead of using the types defined in

https://github.com/cms-sw/cmssw/blob/master/DataFormats/Provenance/interface/RunLumiEventNumber.h

It actually causes major trouble when the event number goes beyond the casting type (see preshower issue under https://indico.cern.ch/event/1503577/#2-egm-pog-data-vs-mc-compariso).

  • As a first step, Ecal will provide a PR for ESDataFormatter and ESDigiToRaw.
  • A second step would be to review all packer/unpacker software for such out-of-range casting
  • A finale step would be to review all code for such out-of-range casting
@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 7, 2025

cms-bot internal usage

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 7, 2025

A new Issue was created by @vlimant.

@Dr15Jones, @antoniovilela, @makortel, @mandrenguyen, @rappoccio, @sextonkennedy, @smuzaffar can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

@makortel
Copy link
Contributor

makortel commented Feb 7, 2025

assign EventFilter/ESDigiToRaw

(to start with, please assign further)

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 7, 2025

New categories assigned: daq

@emeschi,@smorovic you have been requested to review this Pull request/Issue and eventually sign? Thanks

@thomreis
Copy link
Contributor

thomreis commented Feb 7, 2025

Fix for ES packer in #47290 . Backports in #47291 , #47292 , #47293, and #47294 .

@thomreis
Copy link
Contributor

thomreis commented Feb 7, 2025

The ECAL packer seems to have this issue as well, although it does not seem to lead to zero digis when unpacking as for the ES.

@smorovic
Copy link
Contributor

smorovic commented Feb 7, 2025

+daq
yes, signed int event-id, if used, wraps for runs over ~5 hours at 100 kHz

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 7, 2025

This issue is fully signed and ready to be closed.

@vlimant
Copy link
Contributor Author

vlimant commented Feb 7, 2025

I don't think we can close this issue just yet though

@smorovic
Copy link
Contributor

smorovic commented Feb 7, 2025

-daq
to have it pending

@Dr15Jones
Copy link
Contributor

@smuzaffar @makortel So I did a bit of poking around. The standard says the compiler can do integer conversion (e.g. assign the value of an unsigned long long to an int). I did find a gcc warning that can be issued when it is done -Wconversion. Maybe we should turn that one for one IB build?

@smuzaffar
Copy link
Contributor

@Dr15Jones , I have opened cms-sw/cmsdist#9678 to see how many and where we get such conversions

@makortel
Copy link
Contributor

makortel commented Feb 7, 2025

Here is the previous episode in -Wconversion #39098

@thomreis
Copy link
Contributor

thomreis commented Feb 7, 2025

The ECAL packer seems to have this issue as well, although it does not seem to lead to zero digis when unpacking as for the ES.

The ECAL packer issue is addressed in #47295 . This one is not as critical as the ES one since not the full event number is used for the packing and the packed bits are not affected.

@smuzaffar
Copy link
Contributor

enabling -Wconversion results in over 500K overall warings https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2cad9b/44269/build-logs/ and 26K unique warnings

@Dr15Jones
Copy link
Contributor

@smuzaffar Is there an easy way to get the list of unique warnings?

@smuzaffar
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants