[UBSAN]Fix runtime error for invalid bool value assignment by smuzaffar · Pull Request #46907 · cms-sw/cmssw

smuzaffar · 2024-12-10T12:52:24Z

For UBSAN IBs, few workflows show runtime errors like [a]. Looks like this happens when we try to swap uninitialized bool values here e.g from https://github.com/cms-sw/cmssw/blob/master/SimDataFormats/GeneratorProducts/src/HepMCProduct.cc#L149 . If I am not wrong, there is no need to use swap for boolean types, just assignment should be enough

Also removed using fTimeOffset comments

[a]

 gcc/include/c++/12.3.1/bits/move.h:204:11: runtime error: load of value 224, which is not a valid value for type 'bool'

    #0 0x14a7a7ba4fbc in std::enable_if<std::__and_<std::__not_<std::__is_tuple_like<bool> >, std::is_move_constructible<bool>, std::is_move_assignable<bool> >::value, void>::type std::swap<bool>(bool&, bool&) /data/cmsbld/jenkins/workspace/build-any-ib/w/el8_amd64_gcc12/external/gcc/12.3.1-40d504be6370b5a30e3947a6e575ca28/include/c++/12.3.1/bits/move.h:204
    #1 0x14a7a7ba4fbc in edm::HepMCProduct::swap(edm::HepMCProduct&) src/SimDataFormats/GeneratorProducts/src/HepMCProduct.cc:135
    #2 0x14a77bcb7e7d in edm::Wrapper<edm::HepMCProduct>::Wrapper<edm::HepMCProduct>(edm::WrapperBase::Emplace, edm::HepMCProduct&&) src/DataFormats/Common/interface/Wrapper.h:89
    #3 0x14a77bcb7e7d in edm::OrphanHandle<edm::HepMCProduct> edm::Event::emplaceImpl<edm::HepMCProduct, edm::HepMCProduct>(unsigned int, edm::HepMCProduct&&) src/FWCore/Framework/interface/Event.h:454
    #4 0x14a77bc8719c in edm::OrphanHandle<edm::HepMCProduct> edm::Event::emplace<edm::HepMCProduct, edm::HepMCProduct>(edm::EDPutTokenT<edm::HepMCProduct>, edm::HepMCProduct&&) src/FWCore/Framework/interface/Event.h:434
    #5 0x14a77bc8719c in ExternalGeneratorFilter::filter(edm::StreamID, edm::Event&, edm::EventSetup const&) const src/GeneratorInterface/Core/plugins/ExternalGeneratorFilter.cc:270

cmsbuild · 2024-12-10T12:52:44Z

cms-bot internal usage

cmsbuild · 2024-12-10T12:53:53Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46907/42957

cmsbuild · 2024-12-10T12:54:16Z

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

It involves the following packages:

SimDataFormats/GeneratorProducts (generators)

@bbilin, @cmsbuild, @lviliani, @menglu21, @mkirsano can you please review it and eventually sign? Thanks.
@apsallid, @bsunanda, @denizsun, @fabiocos, @martinamalberti, @missirol, @mmusich, @rovere, @salimcerci, @youyingli 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 · 2024-12-10T13:01:18Z

test parameters:

relvals = 570.0,571.0,577.0

smuzaffar · 2024-12-10T13:01:26Z

please test

smuzaffar · 2024-12-10T13:01:45Z

please test for CMSSW_15_0_UBSAN_X

cmsbuild · 2024-12-10T14:23:11Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4303dc/43351/summary.html
COMMIT: adae936
CMSSW: CMSSW_15_0_UBSAN_X_2024-12-09-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/46907/43351/install.sh to create a dev area with all the needed externals and cmssw changes.

cmsbuild · 2024-12-10T16:04:28Z

-1

Failed Tests: RelVals-INPUT
Size: This PR adds an extra 16KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4303dc/43350/summary.html
COMMIT: adae936
CMSSW: CMSSW_15_0_X_2024-12-09-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/46907/43350/install.sh to create a dev area with all the needed externals and cmssw changes.

DAS Queries: The DAS query tests failed, see the summary page for details.

RelVals-INPUT

2024.000001DAS Error
2024.002001DAS Error
2024.001001DAS Error

Comparison Summary

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: 3512684
DQMHistoTests: Total failures: 484
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3512180
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 208 log files, 175 edm output root files, 49 DQM output files
TriggerResults: found differences in 1 / 47 workflows

smuzaffar · 2024-12-18T10:34:35Z

please test

cmsbuild · 2024-12-18T12:20:57Z

+1

Size: This PR adds an extra 16KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4303dc/43542/summary.html
COMMIT: adae936
CMSSW: CMSSW_15_0_X_2024-12-17-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/46907/43542/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially added 2 lines to the logs
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3512684
DQMHistoTests: Total failures: 423
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3512241
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 208 log files, 175 edm output root files, 49 DQM output files
TriggerResults: no differences found

smuzaffar · 2025-01-06T14:23:13Z

please test

lets refresh the tests

@cms-sw/generators-l2 can you please review this? this just contains changes needed for code-format

cmsbuild · 2025-01-06T16:19:55Z

+1

Size: This PR adds an extra 16KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4303dc/43638/summary.html
COMMIT: adae936
CMSSW: CMSSW_15_0_X_2025-01-06-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/46907/43638/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially removed 2 lines from the logs
Reco comparison results: 6 differences found in the comparisons
DQMHistoTests: Total files compared: 52
DQMHistoTests: Total histograms compared: 3821397
DQMHistoTests: Total failures: 444
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3820933
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 51 files compared)
Checked 220 log files, 187 edm output root files, 52 DQM output files
TriggerResults: found differences in 1 / 50 workflows

lviliani · 2025-01-09T09:16:54Z

+1

cmsbuild · 2025-01-09T09:17:17Z

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

mandrenguyen · 2025-01-09T10:18:38Z

+1

makortel · 2025-01-09T15:55:08Z

SimDataFormats/GeneratorProducts/src/HepMCProduct.cc

-  //std::swap(fTimeOffset, other.fTimeOffset);
+  isVtxGenApplied_ = other.isVtxGenApplied_;
+  isVtxBoostApplied_ = other.isVtxBoostApplied_;
+  isPBoostApplied_ = other.isPBoostApplied_;


Sorry but strictly speaking this is not a correct implementation of swap(), because the values from this do not end up in other (see https://en.cppreference.com/w/cpp/named_req/Swappable). I'm puzzled though why

cmssw/DataFormats/Common/interface/Wrapper.h

Lines 89 to 94 in 684a740

template <typename T>

Wrapper<T>::Wrapper(std::unique_ptr<T> ptr) : WrapperBase(), obj{construct_()}, present(ptr.get() != nullptr) {

if (present) {

obj = std::move(*ptr);

}

}

ends up calling swap().

Ah, it's because the move constructor and assignment are implemented with swap()

cmssw/SimDataFormats/GeneratorProducts/src/HepMCProduct.cc

Lines 146 to 150 in 684a740

HepMCProduct::HepMCProduct(HepMCProduct&& other) : evt_(nullptr) { swap(other); }

HepMCProduct& HepMCProduct::operator=(HepMCProduct&& other) {

swap(other);

return *this;

}

Maybe the real problem is that the move constructor does not initialize the bools?

ah you are right @makortel . Let me revert this change and initialize bools in move constructor

thanks @makortel , indeed properly initializing data members for move constructor fixes the issue. I have opened #47074

[UBSAN]Fix runtime error for invalid bool value assignment

adae936

cmsbuild added this to the CMSSW_15_0_X milestone Dec 10, 2024

cmsbuild added generators-pending pending-signatures tests-pending orp-pending code-checks-pending labels Dec 10, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 10, 2024

cmsbuild added tests-started and removed tests-pending labels Dec 10, 2024

cmsbuild added tests-rejected and removed tests-started labels Dec 10, 2024

cmsbuild added tests-started and removed tests-rejected labels Dec 18, 2024

cmsbuild added tests-approved and removed tests-started labels Dec 18, 2024

cmsbuild added tests-started and removed tests-approved labels Jan 6, 2025

cmsbuild added tests-approved and removed tests-started labels Jan 6, 2025

cmsbuild added generators-approved fully-signed and removed generators-pending pending-signatures labels Jan 9, 2025

cmsbuild added orp-approved and removed orp-pending labels Jan 9, 2025

cmsbuild merged commit e944ebc into cms-sw:master Jan 9, 2025
10 checks passed

cmsbuild mentioned this pull request Jan 9, 2025

[DAQ] Input source improvements and initial Phase-2 DTH format support #47068

Merged

makortel reviewed Jan 9, 2025

View reviewed changes

This was referenced Jan 9, 2025

Moved construction of ProductResolvers to a separate function #47051

Merged

L1 tracking update #47067

Merged

smuzaffar mentioned this pull request Jan 9, 2025

[UBSAN]Properly initialize datamembers for move operator #47074

Merged

cmsbuild mentioned this pull request Jan 9, 2025

Improve behavior of streamer system #47073

Merged

	template <typename T>
	Wrapper<T>::Wrapper(std::unique_ptr<T> ptr) : WrapperBase(), obj{construct_()}, present(ptr.get() != nullptr) {
	if (present) {
	obj = std::move(*ptr);
	}
	}

	HepMCProduct::HepMCProduct(HepMCProduct&& other) : evt_(nullptr) { swap(other); }
	HepMCProduct& HepMCProduct::operator=(HepMCProduct&& other) {
	swap(other);
	return *this;
	}

Conversation

smuzaffar commented Dec 10, 2024

Uh oh!

cmsbuild commented Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmsbuild commented Dec 10, 2024

Uh oh!

cmsbuild commented Dec 10, 2024

Uh oh!

smuzaffar commented Dec 10, 2024

Uh oh!

smuzaffar commented Dec 10, 2024

Uh oh!

smuzaffar commented Dec 10, 2024

Uh oh!

cmsbuild commented Dec 10, 2024

Uh oh!

cmsbuild commented Dec 10, 2024

RelVals-INPUT

Comparison Summary

Uh oh!

smuzaffar commented Dec 18, 2024

Uh oh!

cmsbuild commented Dec 18, 2024

Comparison Summary

Uh oh!

smuzaffar commented Jan 6, 2025

Uh oh!

cmsbuild commented Jan 6, 2025

Comparison Summary

Uh oh!

lviliani commented Jan 9, 2025

Uh oh!

cmsbuild commented Jan 9, 2025

Uh oh!

mandrenguyen commented Jan 9, 2025

Uh oh!

Uh oh!

makortel Jan 9, 2025

Choose a reason for hiding this comment

Uh oh!

smuzaffar Jan 9, 2025

Choose a reason for hiding this comment

Uh oh!

smuzaffar Jan 9, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cmsbuild commented Dec 10, 2024 •

edited

Loading