Move two StopReasonName arrays into source files #42628

hahnjo · 2023-08-22T13:09:11Z

PR description:

This works around root-project/root#13429 by not having std::string arrays in headers that are double-freed.

PR validation:

Not needed

PR backport:

Probably not needed.

cmsbuild · 2023-08-22T13:13:48Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42628/36682

This PR adds an extra 16KB to repository

cmsbuild · 2023-08-22T13:14:11Z

A new Pull Request was created by @hahnjo (Jonas Hahnfeld) for master.

It involves the following packages:

DataFormats/TrackCandidate (reconstruction)
DataFormats/TrackReco (reconstruction)

@cmsbuild, @mandrenguyen, @clacaputo can you please review it and eventually sign? Thanks.
@VourMa, @JanFSchulte, @rovere, @VinInn, @missirol, @gpetruc, @mmusich, @mtosi this is something you requested to watch as well.
@perrotta, @dpiparo, @antoniovilela, @rappoccio you are the release manager for this.

cms-bot commands are listed here

hahnjo · 2023-08-22T13:14:51Z

@smuzaffar could you please test this PR with ROOT6 IBs? I hope this works around the problem while we work on a proper solution.

iarspider · 2023-08-22T13:17:27Z

please test with CMSSW_13_3_ROOT6_X

smuzaffar · 2023-08-22T13:46:12Z

test parameters:

addpkg = Alignment/OfflineValidation

smuzaffar · 2023-08-22T13:46:18Z

please test for CMSSW_13_3_ROOT6_X

smuzaffar · 2023-08-22T13:49:09Z

please test

lets test production arch/IB

cmsbuild · 2023-08-22T14:25:12Z

-1

Failed Tests: Build
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b50767/34415/summary.html
COMMIT: 2c532f0
CMSSW: CMSSW_13_3_X_2023-08-21-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/42628/34415/install.sh to create a dev area with all the needed externals and cmssw changes.

Build

I found compilation error when building:

>> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_3_X_2023-08-21-2300/src/DQM/TrackingMonitor/src/V0Monitor.cc
>> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_3_X_2023-08-21-2300/src/DQM/TrackingMonitor/src/VertexMonitor.cc
>> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_3_X_2023-08-21-2300/src/DQM/TrackingMonitor/src/dEdxAnalyzer.cc
>> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_3_X_2023-08-21-2300/src/DQM/TrackingMonitor/src/dEdxHitAnalyzer.cc
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_3_X_2023-08-21-2300/src/DQM/TrackingMonitor/src/TrackAnalyzer.cc: In member function 'void tadqm::TrackAnalyzer::bookHistosForHitProperties(dqm::legacy::DQMStore::IBooker&)':
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_3_X_2023-08-21-2300/src/DQM/TrackingMonitor/src/TrackAnalyzer.cc:718:33: error: invalid application of 'sizeof' to incomplete type 'const string []' {aka 'const std::__cxx11::basic_string []'}
  718 |     size_t StopReasonNameSize = sizeof(StopReasonName::StopReasonName) / sizeof(std::string);
      |                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_3_X_2023-08-21-2300/src/DQM/TrackingMonitor/src/TrackBuildingAnalyzer.cc: In member function 'void TrackBuildingAnalyzer::initHisto(dqm::legacy::DQMStore::IBooker&, const edm::ParameterSet&)':
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_3_X_2023-08-21-2300/src/DQM/TrackingMonitor/src/TrackBuildingAnalyzer.cc:332:33: error: invalid application of 'sizeof' to incomplete type 'const string []' {aka 'const std::__cxx11::basic_string []'}
  332 |     size_t StopReasonNameSize = sizeof(StopReasonName::StopReasonName) / sizeof(std::string);

cmsbuild · 2023-08-22T14:27:42Z

-1

Failed Tests: Build
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b50767/34416/summary.html
COMMIT: 2c532f0
CMSSW: CMSSW_13_3_ROOT6_X_2023-08-21-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/42628/34416/install.sh to create a dev area with all the needed externals and cmssw changes.

Build

I found compilation error when building:

>> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_3_ROOT6_X_2023-08-21-2300/src/DQM/TrackingMonitor/src/V0Monitor.cc
>> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_3_ROOT6_X_2023-08-21-2300/src/DQM/TrackingMonitor/src/VertexMonitor.cc
>> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_3_ROOT6_X_2023-08-21-2300/src/DQM/TrackingMonitor/src/dEdxAnalyzer.cc
>> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_3_ROOT6_X_2023-08-21-2300/src/DQM/TrackingMonitor/src/dEdxHitAnalyzer.cc
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_3_ROOT6_X_2023-08-21-2300/src/DQM/TrackingMonitor/src/TrackAnalyzer.cc: In member function 'void tadqm::TrackAnalyzer::bookHistosForHitProperties(dqm::legacy::DQMStore::IBooker&)':
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_3_ROOT6_X_2023-08-21-2300/src/DQM/TrackingMonitor/src/TrackAnalyzer.cc:718:33: error: invalid application of 'sizeof' to incomplete type 'const string []' {aka 'const std::__cxx11::basic_string []'}
  718 |     size_t StopReasonNameSize = sizeof(StopReasonName::StopReasonName) / sizeof(std::string);
      |                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_3_ROOT6_X_2023-08-21-2300/src/DQM/TrackingMonitor/src/TrackBuildingAnalyzer.cc: In member function 'void TrackBuildingAnalyzer::initHisto(dqm::legacy::DQMStore::IBooker&, const edm::ParameterSet&)':
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_3_ROOT6_X_2023-08-21-2300/src/DQM/TrackingMonitor/src/TrackBuildingAnalyzer.cc:332:33: error: invalid application of 'sizeof' to incomplete type 'const string []' {aka 'const std::__cxx11::basic_string []'}
  332 |     size_t StopReasonNameSize = sizeof(StopReasonName::StopReasonName) / sizeof(std::string);

This works around root-project/root#13429 by not having std::string arrays in headers that are double-freed.

cmsbuild · 2023-08-22T15:55:33Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42628/36684

This PR adds an extra 36KB to repository

makortel · 2023-08-22T18:28:13Z

DataFormats/TrackCandidate/interface/TrajectoryStopReasons.h

-      "NOT_STOPPED"                    // 11 (be careful, NOT_STOPPED needs to be the last,
-                                       //     its index differs from the enumeration value)
-  };
+  extern const std::string StopReasonName[];


How about making this

Suggested change

extern const std::string StopReasonName[];

extern const std::array<std::string_view, static_cast<size_t>(StopReason::SIZE)>;

?

Please feel free to make further changes as necessary / wanted. This PR is meant to provide a minimal workaround for root-project/root#13429 which appears to work, up to your decision to merge or wait for a proper fix for the underlying problem in Cling (working on it).

Incidentally, I would argue that this array of names doesn't need any C++ data type, plain const char * should be fine. I would still argue that it's better to move the strings into the source files because it means the compiler doesn't have to materialize them in all TUs that include this header, and I think it's not critical for performance.

cmsbuild · 2023-08-22T21:34:41Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b50767/34424/summary.html
COMMIT: deae987
CMSSW: CMSSW_13_3_ROOT6_X_2023-08-21-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/42628/34424/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially removed 11 lines from the logs
Reco comparison results: 15 differences found in the comparisons
DQMHistoTests: Total files compared: 48
DQMHistoTests: Total histograms compared: 3153095
DQMHistoTests: Total failures: 3
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3153070
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
Checked 207 log files, 159 edm output root files, 48 DQM output files
TriggerResults: no differences found

hahnjo · 2023-08-23T06:29:46Z

please test

Testing against production IB doesn't seem to have worked, not sure if you want to restart that before considering a merge...

iarspider · 2023-08-23T07:06:34Z

please test

cmsbuild · 2023-08-23T09:58:52Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b50767/34440/summary.html
COMMIT: deae987
CMSSW: CMSSW_13_3_X_2023-08-22-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/42628/34440/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially removed 14 lines from the logs
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 48
DQMHistoTests: Total histograms compared: 3153095
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3153073
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
Checked 207 log files, 159 edm output root files, 48 DQM output files
TriggerResults: no differences found

emanueleusai · 2023-08-25T02:53:24Z

+1

mandrenguyen · 2023-08-29T05:30:50Z

+1
No reco changes

cmsbuild · 2023-08-29T05:31:13Z

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, @antoniovilela, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

perrotta · 2023-08-29T07:38:49Z

+1

Further improvements, as for example the one suggested in Move two StopReasonName arrays into source files #42628 (review), can be dealt with elsewhere

cmsbuild added this to the CMSSW_13_3_X milestone Aug 22, 2023

cmsbuild added reconstruction-pending pending-signatures tests-pending orp-pending code-checks-pending tracking labels Aug 22, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 22, 2023

cmsbuild added tests-started and removed tests-pending labels Aug 22, 2023

cmsbuild added tests-rejected and removed tests-started labels Aug 22, 2023

Move two StopReasonName arrays into source files

deae987

This works around root-project/root#13429 by not having std::string arrays in headers that are double-freed.

hahnjo force-pushed the DataFormats-arrays branch from 2c532f0 to deae987 Compare August 22, 2023 15:45

cmsbuild added dqm-pending tests-pending code-checks-pending and removed tests-rejected code-checks-approved labels Aug 22, 2023

cmsbuild added the tests-started label Aug 22, 2023

makortel reviewed Aug 22, 2023

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Aug 22, 2023

cmsbuild added tests-started and removed tests-approved labels Aug 23, 2023

cmsbuild added tests-approved and removed tests-started labels Aug 23, 2023

hahnjo mentioned this pull request Aug 23, 2023

[Testing] Delete the object held by gROOT after we reset the globals. cms-sw/root#186

Closed

cmsbuild added dqm-approved and removed dqm-pending labels Aug 25, 2023

cmsbuild added reconstruction-approved fully-signed and removed reconstruction-pending pending-signatures labels Aug 29, 2023

cmsbuild added orp-approved and removed orp-pending labels Aug 29, 2023

cmsbuild merged commit 1640346 into cms-sw:master Aug 29, 2023

hahnjo deleted the DataFormats-arrays branch August 29, 2023 07:48

mmusich mentioned this pull request Aug 31, 2023

[ROOT6 and ROOT628] Unit tests Miscellanea and PrimaryVertex failing #42468

Closed

This was referenced Sep 4, 2023

[ROOT] Unit Tests testDataFormatsScoutingRunX failing in ROOT IBs #41222

Closed

Llvm16 cms-sw/root#185

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move two StopReasonName arrays into source files #42628

Move two StopReasonName arrays into source files #42628

hahnjo commented Aug 22, 2023

cmsbuild commented Aug 22, 2023

cmsbuild commented Aug 22, 2023

hahnjo commented Aug 22, 2023

iarspider commented Aug 22, 2023

smuzaffar commented Aug 22, 2023

smuzaffar commented Aug 22, 2023

smuzaffar commented Aug 22, 2023

cmsbuild commented Aug 22, 2023

cmsbuild commented Aug 22, 2023

cmsbuild commented Aug 22, 2023

makortel Aug 22, 2023

hahnjo Aug 23, 2023

cmsbuild commented Aug 22, 2023

hahnjo commented Aug 23, 2023

iarspider commented Aug 23, 2023

cmsbuild commented Aug 23, 2023

emanueleusai commented Aug 25, 2023

mandrenguyen commented Aug 29, 2023

cmsbuild commented Aug 29, 2023

perrotta commented Aug 29, 2023

	extern const std::string StopReasonName[];
	extern const std::array<std::string_view, static_cast<size_t>(StopReason::SIZE)>;

Move two StopReasonName arrays into source files #42628

Move two StopReasonName arrays into source files #42628

Conversation

hahnjo commented Aug 22, 2023

PR description:

PR validation:

PR backport:

cmsbuild commented Aug 22, 2023

cmsbuild commented Aug 22, 2023

hahnjo commented Aug 22, 2023

iarspider commented Aug 22, 2023

smuzaffar commented Aug 22, 2023

smuzaffar commented Aug 22, 2023

smuzaffar commented Aug 22, 2023

cmsbuild commented Aug 22, 2023

Build

cmsbuild commented Aug 22, 2023

Build

cmsbuild commented Aug 22, 2023

makortel Aug 22, 2023

Choose a reason for hiding this comment

hahnjo Aug 23, 2023

Choose a reason for hiding this comment

cmsbuild commented Aug 22, 2023

Comparison Summary

hahnjo commented Aug 23, 2023

iarspider commented Aug 23, 2023

cmsbuild commented Aug 23, 2023

Comparison Summary

emanueleusai commented Aug 25, 2023

mandrenguyen commented Aug 29, 2023

cmsbuild commented Aug 29, 2023

perrotta commented Aug 29, 2023