[UBSAN] DataFormats/CaloTowers initialization fix #35068

abdoulline · 2021-08-30T10:32:38Z

PR description:

Fixes DataFormats/CaloTowers/interface/CaloTower.h UBSAN issue #35033

PR validation:

runTheMatrix.py -l 159.3 --ibeos --useInput all

in CMSSW_12_1_UBSAN_X_2021-08-27-2300 on cmsdev25

cmsbuild · 2021-08-30T10:39:29Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35068/24950

This PR adds an extra 16KB to repository

cmsbuild · 2021-08-30T10:39:49Z

A new Pull Request was created by @abdoulline (Salavat Abdullin) for master.

It involves the following packages:

DataFormats/CaloTowers (reconstruction)

@jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@rovere this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

mseidel42 · 2021-08-30T10:47:31Z

Thanks a lot @abdoulline !

slava77 · 2021-08-30T13:29:48Z

@cmsbuild please test

cmsbuild · 2021-08-30T16:42:51Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-63d5f7/18132/summary.html
COMMIT: d0fdb6d
CMSSW: CMSSW_12_1_X_2021-08-29-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35068/18132/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: 2686 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 3000352
DQMHistoTests: Total failures: 4219
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2996111
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: found differences in 1 / 38 workflows

abdoulline · 2021-08-31T05:04:41Z

Rather enigmatic results in bin-by-bin DQM comparison for the only particular wf 11634.911 (DD4) :
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_12_1_X_2021-08-29-2300+63d5f7/45071/dqm-histo-comparison-summary.html
massive changes include: ECAL/HCAL SimHits, Pixels etc... (?)

slava77 · 2021-09-01T15:55:42Z

@cms-sw/geometry-l2
is DD4 HEP workflow (again) expected to have some instabilities?

slava77 · 2021-09-01T15:55:51Z

@cmsbuild please test

makortel · 2021-09-01T16:01:02Z

@cms-sw/geometry-l2
is DD4 HEP workflow (again) expected to have some instabilities?

I saw this recently too, maybe it is time to open a dedicated issue #35109

slava77 · 2021-09-01T17:51:57Z

DataFormats/CaloTowers/src/CaloTower.cc

+      hadLvl1_(hcal_tp) {
+  subdet_ = HcalEmpty;
+  inHO_ = false;
+  inHBHEgap_ = false;


Suggested change

hadLvl1_(hcal_tp) {

subdet_ = HcalEmpty;

inHO_ = false;

inHBHEgap_ = false;

hadLvl1_(hcal_tp),

subdet_(HcalEmpty),

inHO_(false),

inHBHEgap_(false) {

the initializer list is usually preferred over the body of the constructor.

I'm not sure I understand your comment (suggestion).
Those 3 variables aren't constructior parameters, but private members.
Look similar to https://github.com/cms-sw/cmssw/pull/35064/files

my suggestion is as displayed; a more proper phrasing is to use initialization in the member initializer list instead of using assignment in the constructor body
e.g. https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c49-prefer-initialization-to-assignment-in-constructors

it's perhaps even better to initialize in the member declaration part in CaloTower.h

OK, thanks. Mea culpa (misunderstanding) ...

cmsbuild · 2021-09-01T19:04:38Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-63d5f7/18218/summary.html
COMMIT: d0fdb6d
CMSSW: CMSSW_12_1_X_2021-08-31-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35068/18218/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: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 3000404
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3000376
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: no differences found

cmsbuild · 2021-09-01T20:17:34Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35068/25009

This PR adds an extra 16KB to repository

cmsbuild · 2021-09-01T20:18:01Z

Pull request #35068 was updated. @jpata, @cmsbuild, @slava77 can you please check and sign again.

slava77 · 2021-09-01T21:43:47Z

@cmsbuild please test

cmsbuild · 2021-09-02T01:49:47Z

-1

Failed Tests: RelVals-INPUT
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-63d5f7/18233/summary.html
COMMIT: 0017f58
CMSSW: CMSSW_12_1_X_2021-09-01-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35068/18233/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals-INPUT

136.72413136.72413_RunJetHT2016B_reminiaodUL+RunJetHT2016B_reminiaodUL+REMININANO_data2016UL_HIPM_met+HARVESTDR2_REMININANO_data2016UL_HIPM_met/step2_RunJetHT2016B_reminiaodUL+RunJetHT2016B_reminiaodUL+REMININANO_data2016UL_HIPM_met+HARVESTDR2_REMININANO_data2016UL_HIPM_met.log

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 2 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 3000404
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3000376
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: no differences found

abdoulline · 2021-09-02T09:11:25Z

#35121 temporarily disables wf 136.72413

I've re-run runTheMatrix.py -l 159.3 --ibeos --useInput all
in CMSSW_12_1_UBSAN_X_2021-08-27-2300 and it looks OK (as expected).

slava77 · 2021-09-02T13:04:03Z

+reconstruction

for #35068 0017f58

code changes are mostly technical in line with the PR description and the follow up review
jenkins tests pass and comparisons with the baseline show no differences (apparently the cases with the uninitialized values identified by UBSAN never made it to the monitored quantities)

cmsbuild · 2021-09-02T13:04:27Z

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

qliphy · 2021-09-02T13:50:23Z

+1

qliphy · 2021-09-02T13:50:58Z

merge

CaloTower initialization fix

d0fdb6d

cmsbuild added this to the CMSSW_12_1_X milestone Aug 30, 2021

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Aug 30, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 30, 2021

slava77 mentioned this pull request Aug 30, 2021

[UBSAN] Undefined behavior in DataFormats/* reco packages #35033

Closed

cmsbuild added tests-started and removed tests-pending labels Aug 30, 2021

cmsbuild added tests-approved and removed tests-started labels Aug 30, 2021

cmsbuild added tests-started and removed tests-approved labels Sep 1, 2021

makortel mentioned this pull request Sep 1, 2021

Instabilities in 11634.911 (DD4Hep) workflow comparisons #35109

Open

slava77 reviewed Sep 1, 2021

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Sep 1, 2021

cmsbuild added code-checks-pending tests-pending and removed code-checks-approved labels Sep 1, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 1, 2021

cmsbuild added tests-started and removed tests-pending labels Sep 1, 2021

cmsbuild added tests-rejected and removed tests-started labels Sep 2, 2021

cmsbuild added fully-signed reconstruction-approved and removed reconstruction-pending pending-signatures labels Sep 2, 2021

cmsbuild added orp-approved and removed orp-pending labels Sep 2, 2021

cmsbuild merged commit ab84da9 into cms-sw:master Sep 2, 2021

This was referenced Sep 2, 2021

Added esConsumes to modules in SimMuon/MCTruth #35134

Merged

[GCC11] Fix compilation warning for CommonTools #35144

Merged

[GCC11] Fix compilation warning for EcalMatacqDigi #35148

Merged

[GCC11] Fix compilation warning for FW/ToyIntProducers #35143

Merged

abdoulline deleted the CaloTower_initialization_fix branch October 27, 2021 05:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[UBSAN] DataFormats/CaloTowers initialization fix #35068

[UBSAN] DataFormats/CaloTowers initialization fix #35068

abdoulline commented Aug 30, 2021

cmsbuild commented Aug 30, 2021

cmsbuild commented Aug 30, 2021

mseidel42 commented Aug 30, 2021

slava77 commented Aug 30, 2021

cmsbuild commented Aug 30, 2021

abdoulline commented Aug 31, 2021 •

edited

Loading

slava77 commented Sep 1, 2021

slava77 commented Sep 1, 2021

makortel commented Sep 1, 2021

slava77 Sep 1, 2021

abdoulline Sep 1, 2021

slava77 Sep 1, 2021

slava77 Sep 1, 2021

abdoulline Sep 1, 2021 •

edited

Loading

cmsbuild commented Sep 1, 2021

cmsbuild commented Sep 1, 2021

cmsbuild commented Sep 1, 2021

slava77 commented Sep 1, 2021

cmsbuild commented Sep 2, 2021

abdoulline commented Sep 2, 2021

slava77 commented Sep 2, 2021

cmsbuild commented Sep 2, 2021

qliphy commented Sep 2, 2021

qliphy commented Sep 2, 2021

[UBSAN] DataFormats/CaloTowers initialization fix #35068

[UBSAN] DataFormats/CaloTowers initialization fix #35068

Conversation

abdoulline commented Aug 30, 2021

PR description:

PR validation:

cmsbuild commented Aug 30, 2021

cmsbuild commented Aug 30, 2021

mseidel42 commented Aug 30, 2021

slava77 commented Aug 30, 2021

cmsbuild commented Aug 30, 2021

Comparison Summary

abdoulline commented Aug 31, 2021 • edited Loading

slava77 commented Sep 1, 2021

slava77 commented Sep 1, 2021

makortel commented Sep 1, 2021

slava77 Sep 1, 2021

Choose a reason for hiding this comment

abdoulline Sep 1, 2021

Choose a reason for hiding this comment

slava77 Sep 1, 2021

Choose a reason for hiding this comment

slava77 Sep 1, 2021

Choose a reason for hiding this comment

abdoulline Sep 1, 2021 • edited Loading

Choose a reason for hiding this comment

cmsbuild commented Sep 1, 2021

Comparison Summary

cmsbuild commented Sep 1, 2021

cmsbuild commented Sep 1, 2021

slava77 commented Sep 1, 2021

cmsbuild commented Sep 2, 2021

RelVals-INPUT

Comparison Summary

abdoulline commented Sep 2, 2021

slava77 commented Sep 2, 2021

cmsbuild commented Sep 2, 2021

qliphy commented Sep 2, 2021

qliphy commented Sep 2, 2021

abdoulline commented Aug 31, 2021 •

edited

Loading

abdoulline Sep 1, 2021 •

edited

Loading