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.
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
Avoid accessing a destroyed object in V0Fitter #21195
Conversation
References to internal data of a destructed temporary were being passed to a function. Now the temporaries are 'pinned' to stay on the stack.
The code-checks are being triggered in jenkins. |
IB RelVals for the ASAN build were failing because of this bug. E.g. |
@davidlt thought you'd want to know |
+code-checks |
please test |
I have something similar done over weekend: #21172 |
The tests are being triggered in jenkins. |
A new Pull Request was created by @Dr15Jones (Chris Jones) for master. It involves the following packages: RecoVertex/V0Producer @perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
-1 Tested at: f51a5e7 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: You can see the results of the tests here: I found follow errors while testing this PR Failed tests: RelVals
When I ran the RelVals I found an error in the following worklfows: runTheMatrix-results/10042.0_ZMM_13+ZMM_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+ALCAFull_2017+HARVESTFull_2017/step5_ZMM_13+ZMM_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+ALCAFull_2017+HARVESTFull_2017.log The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped) |
This is similar to #21172, which lacks however the protection @Dr15Jones @davidlt : we have to evaluate which is preferable, and close the other PR, your opinion is welcome! |
I kept the original line: |
auto const& negImpact = negTransTkPtr->impactPointTSCP(); | ||
if (!posImpact.isValid() || !negImpact.isValid()) continue; | ||
FreeTrajectoryState const & posState = posImpact.theState(); | ||
FreeTrajectoryState const & negState = negImpact.theState(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is semantically simpler in terms of copies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you grep for "impactPointTSCP" you will find the same style allovertheplace
(and is not minimal only in terms of copy also in terms of virtual call and actual computation...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, not in this scope (even if I am not sure auto const&
is correct and should not be
auto const
.
I was only saying that there are many other places where isValid si first tested on the fly and then impactPointTSCP accesses again...
see
https://cmssdt.cern.ch/dxr/CMSSW/source/L1Trigger/L1TNtuples/plugins/L1MuonRecoTreeProducer.cc#829
fro instance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dr15Jones could you also look/fix lines 176-177 in your patch? When I checked DXR trajectoryStateClosestToPoint
also returns a copy: https://github.com/cms-sw/cmssw/blob/master/TrackingTools/TransientTrack/interface/BasicTransientTrack.h#L51
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dr15Jones ignore my comment above, const extends temporary lifetime in those lines.
@cmsbuild please test |
The tests are being triggered in jenkins. |
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
Comparison job queued. |
Comparison is ready There are some workflows for which there are errors in the baseline: Comparison Summary:
|
+1
|
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. @davidlange6, @slava77, @smuzaffar (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
References to internal data of a destructed temporary were being
passed to a function. Now the temporaries are 'pinned' to stay
on the stack.