Page MenuHomePhabricator

[analyzer] Find correct region for simple temporary destructor calls and inline them if possible.
ClosedPublic

Authored by NoQ on Feb 8 2018, 5:39 PM.

Details

Summary

When a temporary is constructed into a CXXBindTemporaryExpr (i.e. when it needs a destructor), we can (since D43056/D43062) pick up the construction context and be sure that the temporary is initialized correctly.

Now, store the initialized temporary region in the program state with CXXBindTemporaryExpr as its key, and when the time comes to call the destructor, lookup the CXXBindTemporaryExpr in the CFGTemporaryDtor element, then lookup the region in the program state.

For this purpose, the existing program state trait is used - the one that was added by @klimek for making sure that the control flow for ternary operators with temporaries is correct. Now the region is also stuffed into it. Technically, it should be possible to reconstruct the region by having just the temporary-expression and the location context. However, this would require duplicating a bit of logic from the CFG.

When the this-region is available this way, give ourselves the privilege to attempt inlining it. Of course, normal limitations of what we will or will not inline will still apply.

Additionally, prevent the temporary from being garbage-collected from the program state until the destructor call. I don't think SymRegion::isLiveRegion() should be taught to query ExprEngine regarding live temporaries - we should be just fine treating it as a regular program state trait value liveness process. Also in this case LiveVariables analysis isn't going to help much: temporaries definitely outlive their CXXBindTemporaryExprs sometimes. So i have an impression that this is the right way to solve the liveness issue this time.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Feb 8 2018, 5:39 PM
NoQ added inline comments.Feb 8 2018, 5:39 PM
lib/StaticAnalyzer/Core/ExprEngine.cpp
68–71 ↗(On Diff #133545)

For consistency.

dcoughlin added inline comments.Feb 8 2018, 6:29 PM
lib/StaticAnalyzer/Core/ExprEngine.cpp
939 ↗(On Diff #133545)

Can we add an assertion somewhere else (like when leaving a stack frame context) to make sure that the expected temporaries have all been removed from InitializedTemporaries?

NoQ updated this revision to Diff 133672.Feb 9 2018, 1:04 PM
NoQ marked an inline comment as done.

Assert that the destructors are cleaned up. This assertion is pretty important because it says that we didn't miss any destructors for initialized temporaries during analysis.

Disable tracking initialized temporaries when cfg-temporary-dtors is off - a bug i introduced when moving the code, which was found by the newly added assertion :)

NoQ updated this revision to Diff 133956.Feb 12 2018, 4:10 PM
  • Test const C &c = coin ? C(x, y) : C(z, w);.
  • Fix comments surrounding the assertion.
NoQ updated this revision to Diff 134341.Feb 14 2018, 5:13 PM

All right, so the assertion that we actually destroy all temporaries in our InitializedTemporaries map is still violated quite often - every time we do any sort of lifetime extension, we're actually calling the destructor on a different object, because createTemporaryRegionIfNeeded() has moved the object to a different place. I made a large brain dump on this matter in http://lists.llvm.org/pipermail/cfe-dev/2018-February/056898.html . For now it means that the assertion is still not going in. I added it in a commented-out form. In fact, @klimek has tried that long before me, and failed in a similar manner. If only i added the assertion in the right place (not in processCallExit, but in processEndOfFunction, which is also called for the top frame), i would have seen it earlier :) So i've reinvented quite a bit of a wheel here.

  • Move the assertion to the right place and disable it, explaining that lifetime extension is broken.
  • Move the similar operator-new assertion to the right place as well, while we're at it.
  • Add a flag to disable inlining of temporary destructors, which is different from having them at all. It's on by default but does nothing until cfg-temporary-dtors are also enabled.
  • Add a test for that flag. This flag cannot be tested in analyzer-config.cpp because it is never read unless cfg-temporary-dtors takes a non-default value, so ConfigDumper doesn't see it.
NoQ updated this revision to Diff 134351.Feb 14 2018, 6:20 PM

Whoops - recall that we still need to cleanup the temporaries map even, now that we know that we cannot assert that it's already empty. Clean up the map and enable the assertion that becomes tautological until the respective FIXME is fixed.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 15 2018, 11:19 AM
Closed by commit rL325282: [analyzer] Compute the correct this-region for temporary destructors. (authored by dergachev, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.
This revision was not accepted when it landed; it landed in state Needs Review.Feb 15 2018, 11:20 AM
Closed by commit rC325282: [analyzer] Compute the correct this-region for temporary destructors. (authored by dergachev, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.