This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] When constructing a temporary without construction context, track it for destruction anyway.
ClosedPublic

Authored by NoQ on Feb 22 2018, 6:13 PM.

Details

Summary

This brings back code that was doing so earlier (when we had no constructed contexts at all) but was removed too early in D43104, as discussed in D43497. This allows us to call the correct destructor sequence in the end of the operator && in the test, even if not all of them are inlined, because we keep track of all temporaries we've constructed in order to resolve situations when the number or nature of temporaries is not known in compile-time.

The test case provided in D43497 was not entirely correct: the global variable would anyway be invalidated by the other destructor that we don't inline yet. Though, yeah, it needs to be fixed as well, but it isn't addressed by the fix that i've been thinking of (this patch).

Cleanup process misbehaves when the node after the cleanup is caching out: in this case we take the node that wasn't cleaned up and hit the assertion. This was fixed as well. It's a bit disappointing that our process of constructing a single exploded node is so complicated and error-prone.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Feb 22 2018, 6:13 PM
dcoughlin added inline comments.Feb 24 2018, 10:47 AM
lib/StaticAnalyzer/Core/ExprEngine.cpp
2234 ↗(On Diff #135580)

Can you add a comment explaining the criteria under which the DidCacheOutOnCleanup part can be removed?

2257 ↗(On Diff #135580)

I realize this isn't new code in this patch, but can you use the result of Bldr.generateNode() to determine whether we cached out and also to get the new Pred? That seems cleaner than querying the ExplodedNodeSet.

2262 ↗(On Diff #135580)

It sounds like this means if we did cache out then there are places where the initialized temporaries are not cleared (that is, we have extra gunk in the state that we don't want).

Is that true? If so, then this relaxation of the assertion doesn't seem right to me.

Do we need to introduce a new program point when calling Bldr.generateNode on the cleaned up state (for example, with a new tag or a new program point kind)? This would make it so that when we cache out when generating a node for the state with the cleaned up temporaries we know that it is safe to early return from processEndOfFunction(). It would be safe because we would know that the other node (the one we cached out because of) has already had its temporaries cleared and notified the checkers about the end of the function, etc.

NoQ updated this revision to Diff 135994.Feb 26 2018, 3:01 PM
NoQ marked 3 inline comments as done.

Fix cleanup node generation logic.

lib/StaticAnalyzer/Core/ExprEngine.cpp
2262 ↗(On Diff #135580)

Yep, this is a bug and also it can be simplified a lot.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 27 2018, 1:05 PM
This revision was automatically updated to reflect the committed changes.