L1 DT AM - heap-buffer-overflow fix + slight logic improvement in confirmed TPs #39411

jaimeleonh · 2022-09-16T08:07:59Z

PR description:

This PR tries to solve issue #39338, where a heap-buffer-overflow error was encountered and associated to one line in the MPQuality filter. It also includes some improved logic in a different class.

PR validation:

Passed code-format and code-checks tests

cmsbuild · 2022-09-16T08:16:52Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39411/32123

This PR adds an extra 20KB to repository

cmsbuild · 2022-09-16T08:17:19Z

A new Pull Request was created by @jaimeleonh (Jaime Leon Holgado) for master.

It involves the following packages:

L1Trigger/DTTriggerPhase2 (upgrade, l1)

@rekovic, @epalencia, @cmsbuild, @AdrianoDee, @srimanob, @cecilecaillol can you please review it and eventually sign? Thanks.
@battibass, @JanFSchulte, @Martin-Grunewald, @missirol, @trtomei, @beaucero this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

cecilecaillol · 2022-09-16T08:55:31Z

please test

cmsbuild · 2022-09-16T13:34:19Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b7785b/27594/summary.html
COMMIT: 752f9ba
CMSSW: CMSSW_12_6_X_2022-09-15-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/39411/27594/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 51
DQMHistoTests: Total histograms compared: 3618332
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3618308
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
Checked 212 log files, 49 edm output root files, 51 DQM output files
TriggerResults: no differences found

cecilecaillol · 2022-09-16T13:38:56Z

+l1

perrotta · 2022-09-17T08:24:31Z

L1Trigger/DTTriggerPhase2/src/MuonPathAssociator.cc

+                  if (dtLId.layer() != best_layer) {
+                    minx = std::abs(x_inSL3 - x_wire);
+                    next_wire = best_wire;
+                    next_tdc = best_tdc;
+                    next_layer = best_layer;
+                    next_lat = best_lat;
+
+                    best_wire = (*digiIt).wire();
+                    best_tdc = (*digiIt).time();
+                    best_layer = dtLId.layer();
+                    best_lat = lat;
+                    matched_digis++;
+                  } else if (dtLId.layer() ==
+                             best_layer) {  // same layer than stored, just substituting the hit, no matched_digis++;
+                    best_wire = (*digiIt).wire();
+                    best_tdc = (*digiIt).time();
+                    best_layer = dtLId.layer();
+                    best_lat = lat;
+                  }


Why not:

Suggested change

if (dtLId.layer() != best_layer) {

minx = std::abs(x_inSL3 - x_wire);

next_wire = best_wire;

next_tdc = best_tdc;

next_layer = best_layer;

next_lat = best_lat;

best_wire = (*digiIt).wire();

best_tdc = (*digiIt).time();

best_layer = dtLId.layer();

best_lat = lat;

matched_digis++;

} else if (dtLId.layer() ==

best_layer) { // same layer than stored, just substituting the hit, no matched_digis++;

best_wire = (*digiIt).wire();

best_tdc = (*digiIt).time();

best_layer = dtLId.layer();

best_lat = lat;

}

if (dtLId.layer() != best_layer) {

minx = std::abs(x_inSL3 - x_wire);

next_wire = best_wire;

next_tdc = best_tdc;

next_layer = best_layer;

next_lat = best_lat;

matched_digis++;

}

best_wire = (*digiIt).wire();

best_tdc = (*digiIt).time();

best_layer = dtLId.layer();

best_lat = lat;

perrotta · 2022-09-17T08:26:08Z

L1Trigger/DTTriggerPhase2/src/MuonPathAssociator.cc

+                  // buggy, as we could have stored as next LayerX -> LayerY -> LayerX, and this should
+                  // count only as 2 hits. However, as we confirm with at least 2 hits, having 2 or more
+                  // makes no difference
+                  else if (dtLId.layer() != next_layer)


Why is the else if needed after a continue in case of the complementary possibility?

L1Trigger/DTTriggerPhase2/src/MuonPathAssociator.cc

perrotta · 2022-09-17T08:30:31Z

L1Trigger/DTTriggerPhase2/src/MuonPathAssociator.cc

+                  if (dtLId.layer() != best_layer) {
+                    minx = std::abs(x_inSL1 - x_wire);
+                    next_wire = best_wire;
+                    next_tdc = best_tdc;
+                    next_layer = best_layer;
+                    next_lat = best_lat;
+
+                    best_wire = (*digiIt).wire();
+                    best_tdc = (*digiIt).time();
+                    best_layer = dtLId.layer();
+                    best_lat = lat;
+                    matched_digis++;
+                  } else if (dtLId.layer() ==
+                             best_layer) {  // same layer than stored, just substituting the hit, no matched_digis++;
+                    best_wire = (*digiIt).wire();
+                    best_tdc = (*digiIt).time();
+                    best_layer = dtLId.layer();
+                    best_lat = lat;
+                  }


Suggested change

if (dtLId.layer() != best_layer) {

minx = std::abs(x_inSL1 - x_wire);

next_wire = best_wire;

next_tdc = best_tdc;

next_layer = best_layer;

next_lat = best_lat;

best_wire = (*digiIt).wire();

best_tdc = (*digiIt).time();

best_layer = dtLId.layer();

best_lat = lat;

matched_digis++;

} else if (dtLId.layer() ==

best_layer) { // same layer than stored, just substituting the hit, no matched_digis++;

best_wire = (*digiIt).wire();

best_tdc = (*digiIt).time();

best_layer = dtLId.layer();

best_lat = lat;

}

if (dtLId.layer() != best_layer) {

minx = std::abs(x_inSL1 - x_wire);

next_wire = best_wire;

next_tdc = best_tdc;

next_layer = best_layer;

next_lat = best_lat;

matched_digis++;

}

best_wire = (*digiIt).wire();

best_tdc = (*digiIt).time();

best_layer = dtLId.layer();

best_lat = lat;

perrotta · 2022-09-17T08:31:08Z

L1Trigger/DTTriggerPhase2/src/MuonPathAssociator.cc

+                  else if (dtLId.layer() != next_layer)
+                    matched_digis++;


Suggested change

else if (dtLId.layer() != next_layer)

matched_digis++;

matched_digis++;

L1Trigger/DTTriggerPhase2/src/MuonPathAssociator.cc

perrotta

I am not asking for updating the PR, just take into account these possible simplifications in case you need to update it further eventually

L1Trigger/DTTriggerPhase2/src/MuonPathAssociator.cc

perrotta · 2022-09-20T08:50:30Z

@jaimeleonh , do I understand correctly that the bug fix for #39338 is not included in this PR any more? Or could you please point it out to me which is the code change that fixes it?

jaimeleonh · 2022-09-20T10:56:56Z

@jaimeleonh , do I understand correctly that the bug fix for #39338 is not included in this PR any more? Or could you please point it out to me which is the code change that fixes it?

The bug fix is included in the changes in L1Trigger/DTTriggerPhase2/src/MPQualityEnhancerFilter.cc.

srimanob · 2022-10-02T17:11:33Z

@cmsbuild please test

Just re-trigger the test after 2 weeks.

srimanob · 2022-10-02T17:13:15Z

@jaimeleonh @perrotta
Sorry to miss this PR. Do we converge on this? I still see suggestion which is not answered. Thanks.

cmsbuild · 2022-10-02T20:54:58Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b7785b/27909/summary.html
COMMIT: 6b32207
CMSSW: CMSSW_12_6_X_2022-10-02-0000/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39411/27909/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

@slava77 comparisons for the following workflows were not done due to missing matrix map:

/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-b7785b/41834.0_TTbar_14TeV+2026D94+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal

Summary:

No significant changes to the logs found
Reco comparison results: 8 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3432650
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3432622
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 204 log files, 49 edm output root files, 49 DQM output files
TriggerResults: no differences found

srimanob · 2022-10-05T10:28:09Z

Hi @jaimeleonh @cms-sw/l1-l2
What will be follow up of this PR? Thx.

jaimeleonh · 2022-10-05T10:30:54Z

Hi @jaimeleonh @cms-sw/l1-l2 What will be follow up of this PR? Thx.

Sorry, I missed your previous comment. What I understood from @perrotta is that his last suggestions didn't have to be included in this PR, so I left them for future PRs. I resolved the conversation about these suggestions in any case.

srimanob · 2022-10-05T10:40:11Z

+Upgrade

cmsbuild · 2022-10-05T10:40:33Z

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

perrotta · 2022-10-05T11:01:00Z

L1Trigger/DTTriggerPhase2/src/MuonPathAssociator.cc

@@ -390,6 +390,8 @@ void MuonPathAssociator::correlateMPaths(edm::Handle<DTDigiCollection> dtdigis,
              double x_inSL3 = SL1metaPrimitive->x - SL1metaPrimitive->tanPhi * (VERT_PHI1_PHI3 + l_shift);
              for (auto digiIt = (dtLayerId_It.second).first; digiIt != (dtLayerId_It.second).second; ++digiIt) {
                DTWireId wireId(dtLId, (*digiIt).wire());


Still for a possible future improvement: this can be moved after L394, so that it gets only executed if needed

perrotta · 2022-10-05T11:03:14Z

+1

Bearing in mind that some further optimization is possible (and welcome)

jaimeleonh added 2 commits September 16, 2022 10:00

Fix confirmed and primos filter

f3704c3

Code-checks and Code-format

752f9ba

cmsbuild added this to the CMSSW_12_6_X milestone Sep 16, 2022

cmsbuild added code-checks-pending l1-pending orp-pending pending-signatures tests-pending upgrade-pending labels Sep 16, 2022

jaimeleonh mentioned this pull request Sep 16, 2022

[12_5_X] L1 DT AM - heap-buffer-overflow fix + slight logic improvement in confirmed TPs #39412

Merged

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 16, 2022

cmsbuild added tests-started and removed tests-pending labels Sep 16, 2022

cmsbuild added tests-approved and removed tests-started labels Sep 16, 2022

cmsbuild added l1-approved and removed l1-pending labels Sep 16, 2022

perrotta reviewed Sep 17, 2022

View reviewed changes

Modifications from PR

6b32207

cmsbuild added code-checks-pending l1-pending and removed l1-approved tests-approved code-checks-approved labels Sep 19, 2022

cmsbuild added l1-approved and removed l1-pending labels Sep 20, 2022

perrotta reviewed Sep 20, 2022

View reviewed changes

L1Trigger/DTTriggerPhase2/src/MuonPathAssociator.cc Show resolved Hide resolved

L1Trigger/DTTriggerPhase2/src/MuonPathAssociator.cc Show resolved Hide resolved

cmsbuild added orp-approved orp-pending and removed orp-pending orp-approved labels Sep 20, 2022

cmsbuild added tests-started and removed tests-approved labels Oct 2, 2022

cmsbuild added tests-approved and removed tests-started labels Oct 2, 2022

cmsbuild added fully-signed upgrade-approved and removed pending-signatures upgrade-pending labels Oct 5, 2022

perrotta reviewed Oct 5, 2022

View reviewed changes

cmsbuild added orp-approved and removed orp-pending labels Oct 5, 2022

cmsbuild merged commit efda258 into cms-sw:master Oct 5, 2022

cecilecaillol mentioned this pull request Oct 27, 2022

heap-buffer-overflow in MPQualityEnhancerFilter::filterCousins() #39338

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

L1 DT AM - heap-buffer-overflow fix + slight logic improvement in confirmed TPs #39411

L1 DT AM - heap-buffer-overflow fix + slight logic improvement in confirmed TPs #39411

jaimeleonh commented Sep 16, 2022 •

edited

Loading

cmsbuild commented Sep 16, 2022

cmsbuild commented Sep 16, 2022

cecilecaillol commented Sep 16, 2022

cmsbuild commented Sep 16, 2022

cecilecaillol commented Sep 16, 2022

perrotta Sep 17, 2022

perrotta Sep 17, 2022

perrotta Sep 17, 2022

perrotta Sep 17, 2022

perrotta left a comment •

edited

Loading

perrotta commented Sep 20, 2022

jaimeleonh commented Sep 20, 2022

srimanob commented Oct 2, 2022

srimanob commented Oct 2, 2022

cmsbuild commented Oct 2, 2022

srimanob commented Oct 5, 2022

jaimeleonh commented Oct 5, 2022

srimanob commented Oct 5, 2022

cmsbuild commented Oct 5, 2022

perrotta Oct 5, 2022

perrotta commented Oct 5, 2022

	else if (dtLId.layer() != next_layer)
	matched_digis++;
	matched_digis++;

L1 DT AM - heap-buffer-overflow fix + slight logic improvement in confirmed TPs #39411

L1 DT AM - heap-buffer-overflow fix + slight logic improvement in confirmed TPs #39411

Conversation

jaimeleonh commented Sep 16, 2022 • edited Loading

PR description:

PR validation:

cmsbuild commented Sep 16, 2022

cmsbuild commented Sep 16, 2022

cecilecaillol commented Sep 16, 2022

cmsbuild commented Sep 16, 2022

Comparison Summary

cecilecaillol commented Sep 16, 2022

perrotta Sep 17, 2022

Choose a reason for hiding this comment

perrotta Sep 17, 2022

Choose a reason for hiding this comment

perrotta Sep 17, 2022

Choose a reason for hiding this comment

perrotta Sep 17, 2022

Choose a reason for hiding this comment

perrotta left a comment • edited Loading

Choose a reason for hiding this comment

perrotta commented Sep 20, 2022

jaimeleonh commented Sep 20, 2022

srimanob commented Oct 2, 2022

srimanob commented Oct 2, 2022

cmsbuild commented Oct 2, 2022

Comparison Summary

srimanob commented Oct 5, 2022

jaimeleonh commented Oct 5, 2022

srimanob commented Oct 5, 2022

cmsbuild commented Oct 5, 2022

perrotta Oct 5, 2022

Choose a reason for hiding this comment

perrotta commented Oct 5, 2022

jaimeleonh commented Sep 16, 2022 •

edited

Loading

perrotta left a comment •

edited

Loading