[UBSAN][L1] Fix read out of bounds issue

[smuzaffar]

This should fix the UBSAN error like [a]. There are few issues with this code

• It allocates a temp ap_uint<12> temp[p2eg::CRYSTAL_IN_ETA + 2][p2eg::CRYSTAL_IN_PHI + 4]; but then assign value to temp[CRYSTAL_IN_ETA + 1][CRYSTAL_IN_PHI + 7].
• Loop https://github.com/cms-sw/cmssw/blob/master/L1Trigger/L1CaloTrigger/interface/Phase2L1RCT.h#L1267-L1295 only reads values for seed eta and phi so I think there is no need to loop over every item
• Same as loop https://github.com/cms-sw/cmssw/blob/master/L1Trigger/L1CaloTrigger/interface/Phase2L1RCT.h#L1342-L1385.
• Todo: Finally we only read temp memory for seed eta and phi + some offset. So we do not need to allocate temp[p2eg::CRYSTAL_IN_ETA + 2][p2eg::CRYSTAL_IN_PHI + 4]; a smaller memory just to hold the values for seed eta and phi + some offset should be enough.

I am opening this PR but would like @cms-sw/l1-l2 to review this code and may be propose a batter change

[a] https://cmssdt.cern.ch/SDT/cgi-bin/buildlogs/raw/el8_amd64_gcc12/CMSSW_14_2_UBSAN_X_2024-10-11-2300/pyRelValMatrixLogs/run/23634.0_TTbar_14TeV+2026D95/step2_TTbar_14TeV+2026D95.log

src/L1Trigger/L1CaloTrigger/interface/Phase2L1RCT.h:1257:24: runtime error: index 25 out of bounds for type 'ap_uint [24]'
    #0 0x14e16aba5450 in p2eg::getBremsValuesNeg(p2eg::crystal (*) [20], ap_uint<5>, ap_uint<5>) src/L1Trigger/L1CaloTrigger/interface/Phase2L1RCT.h:1257
    #1 0x14e16abedbad in p2eg::getClusterFromRegion3x4(p2eg::crystal (*) [20]) src/L1Trigger/L1CaloTrigger/interface/Phase2L1RCT.h:1441
    #2 0x14e16aa9cd7c in Phase2L1CaloEGammaEmulator::produce(edm::Event&, edm::EventSetup const&) src/L1Trigger/L1CaloTrigger/plugins/Phase2L1CaloEGammaEmulator.cc:337

[cmsbuild]

cms-bot internal usage

[cmsbuild]

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46372/42213

Code check has found code style and quality issues which could be resolved by applying following patch(s)

• code-format:
  https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46372/42213/code-format.patch
  e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46372/42213/code-format.patch | patch -p1
  You can also run scram build code-format to apply code format directly

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46372/42215

[cmsbuild]

A new Pull Request was created by @smuzaffar for master.

It involves the following packages:

• L1Trigger/L1CaloTrigger (l1, upgrade)

@Moanwar, @aloeliger, @cmsbuild, @epalencia, @srimanob, @subirsarkar can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @missirol, @mmusich this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

[smuzaffar]

please test

[smuzaffar]

test parameters:

• workflow = 23634.0

[smuzaffar]

please test for CMSSW_14_2_UBSAN_X

[cmsbuild]

+1

Size: This PR adds an extra 24KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-08e216/42161/summary.html
COMMIT: 5fa788a
CMSSW: CMSSW_14_2_X_2024-10-14-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/46372/42161/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

• You potentially added 1 lines to the logs
• Reco comparison results: 23 differences found in the comparisons
• DQMHistoTests: Total files compared: 45
• DQMHistoTests: Total histograms compared: 3429312
• DQMHistoTests: Total failures: 1697
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3427595
• DQMHistoTests: Total skipped: 20
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 44 files compared)
• Checked 198 log files, 167 edm output root files, 45 DQM output files
• TriggerResults: no differences found

[cmsbuild]

-1

Failed Tests: UnitTests
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-08e216/42185/summary.html
COMMIT: 5fa788a
CMSSW: CMSSW_14_2_UBSAN_X_2024-10-11-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/46372/42185/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

• @bsunanda Run3-hcx375 Extend the topology modes to accommodate different scenarios during the Run2 period #46351
• @mmusich Improvements to hltPhase2UpgradeIntegrationTests #46362
• @bsunanda Run3-376Y Modify the testing code of HcalTopology #46363
• @iarspider [LLVM Analyzer] Avoid storing pointer to a temporary string's internal buffer in SiPixelFakeGenErrorDBObjectESSource::produce and SiPixelTemplateDBObjectReader::analyze  #46181
• @fabiocos DQM: restore Muon POG sequence (and CSC Monitor), revert #46293 #46364
• @bsunanda Run3-hcx376 Get rid of the obsolete constructor of the HcalTopology class #46357

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-08e216/42185/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-08e216/42185/git-merge-result

Unit Tests

I found 1 errors in the following unit tests:

---> test TestDQMOfflineConfigurationGotAll had ERRORS

[smuzaffar]

@cms-sw/l1-l2 , can you please review this PR ?

[aloeliger]

Some of this goes beyond my ability to comment on. @skkwan Could you please take a look at this and comment on these changes?

[skkwan]

Hi @smuzaffar, thanks for raising this issue and thanks Andrew for the tag. The out-of-bounds error can be resolved by increasing the temp array size and I'm happy to open a PR with just a few changes to fix it, if that's OK with you and @aloeliger. The reasoning for the +7 index in temp[i + 1][j + 7] comes from the window size for the bremsstrahlung correction; I made a few schematics to illustrate why it needs to be +7 and not +4. [1]

Regarding the for loops, our reasoning is to keep the emulator as close as possible to the firmware, because it also helps us debug the performance in the future. There are multiple places where the emulator has suboptimal usage of C++ resources, but it accurately reflects the firmware code which meets timing and resource constraints. Sascha Savin ([email protected]) can also elaborate on how the firmware will evolve if it's helpful. Thanks!

[1] https://www.dropbox.com/scl/fi/epzhvcon2ohd7ouvj56bw/2024-10-15-Indexing.pdf?rlkey=d6szhm3wdv8afdiwtht13git3&dl=0

[smuzaffar]

thanks @skkwan for looking in to it. Feel free to open a PR to apply the changes you feel necessary to avoid the out of bound issue

[skkwan]

Hi @smuzaffar, @aloeliger, I've opened PR #46472 regarding the out-of-bounds. I've checked that it doesn't affect the original outputs, but please let me know if this solves the error in your tests. Thanks.

[smuzaffar]

closing this in favor of #46472