Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[UBSAN]Fix runtime error for invalid bool value assignment #46907

Merged
merged 1 commit into from
Jan 9, 2025
Merged

[UBSAN]Fix runtime error for invalid bool value assignment #46907

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions SimDataFormats/GeneratorProducts/src/HepMCProduct.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ void HepMCProduct::addHepMCData(HepMC::GenEvent* evt) {

void HepMCProduct::applyVtxGen(HepMC::FourVector const& vtxShift) {
//std::cout<< " applyVtxGen called " << isVtxGenApplied_ << endl;
//fTimeOffset = 0;

if (isVtxGenApplied())
return;
Expand Down Expand Up @@ -126,16 +125,14 @@ HepMCProduct::HepMCProduct(HepMCProduct const& other) : evt_(nullptr) {
isVtxGenApplied_ = other.isVtxGenApplied_;
isVtxBoostApplied_ = other.isVtxBoostApplied_;
isPBoostApplied_ = other.isPBoostApplied_;
//fTimeOffset = other.fTimeOffset;
}

// swap
void HepMCProduct::swap(HepMCProduct& other) {
std::swap(evt_, other.evt_);
std::swap(isVtxGenApplied_, other.isVtxGenApplied_);
std::swap(isVtxBoostApplied_, other.isVtxBoostApplied_);
std::swap(isPBoostApplied_, other.isPBoostApplied_);
//std::swap(fTimeOffset, other.fTimeOffset);
isVtxGenApplied_ = other.isVtxGenApplied_;
isVtxBoostApplied_ = other.isVtxBoostApplied_;
isPBoostApplied_ = other.isPBoostApplied_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

// assignment: use copy/swap idiom for exception safety.
Expand Down