This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Implement path notes for temporary destructors.
ClosedPublic

Authored by NoQ on Feb 9 2018, 2:29 PM.

Details

Summary

Temporaries are destroyed at the end of their CXXBindTemporaryExpr, which can be picked up from their CFGTemporaryDtor. Note that lifetime-extended temporaries are different: they are destroyed via CFGAutomaticObjectDtor for their reference, and they don't have a CFGTemoraryDtor of their own. Unless, well, they are copied with an elidable copy-constructor, in which case the temporary destructor is there, but it still fires immediately, and lifetime-extended copy is still dealt with via an automatic destructor.

I think this is one of the places where diagnostics may become confusing due to presence of elidable constructors and their respective destructors. In the second test case nobody would expect anything to be destroyed on the line on which we show the "Calling '~C'" note. So we may need to either think of actually eliding elidable constructors together with their destructors, or at least explaining to the user that the constructor is elidable as part of the path note.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Feb 9 2018, 2:29 PM
NoQ updated this revision to Diff 133687.Feb 9 2018, 2:33 PM

Minor indent fix.

george.karpenkov accepted this revision.Feb 9 2018, 2:44 PM
george.karpenkov added inline comments.
lib/StaticAnalyzer/Core/PathDiagnostic.cpp
586 ↗(On Diff #133687)

auto?

test/Analysis/inlining/temp-dtors-path-notes.cpp
17 ↗(On Diff #133687)

Should we have "returning from ~C"? At least we do have it for inlined functions.

This revision is now accepted and ready to land.Feb 9 2018, 2:44 PM
NoQ added inline comments.Feb 9 2018, 2:47 PM
test/Analysis/inlining/temp-dtors-path-notes.cpp
17 ↗(On Diff #133687)

Not here, because the bug occurs within ~C. But we should definitely test it, yeah.

NoQ added a comment.Feb 9 2018, 2:48 PM

Another question is why do we have such inconsistency between Calling constructor for 'C' and Calling '~C', i.e. why not Calling destructor for 'C'. Seems accidental.

NoQ updated this revision to Diff 133692.Feb 9 2018, 2:55 PM

Add a test for returning from destructor.

NoQ marked 2 inline comments as done.Feb 9 2018, 2:55 PM
This revision was automatically updated to reflect the committed changes.