This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Introduce correct lifetime extension behavior in simple cases.
ClosedPublic

Authored by NoQ on Feb 19 2018, 5:36 PM.

Details

Summary

This patch uses the reference to MaterializeTemporaryExpr stored in the construction context since D43477 in order to model that expression correctly, following the plan described in http://lists.llvm.org/pipermail/cfe-dev/2018-February/056898.html

MaterializeTemporaryExpr is an expression that takes an rvalue object (or one of its xvalue sub-objects) and transforms it into a reference to that object. From the analyzer's point of view, the value of the sub-expression would be a NonLoc, and the value of MaterializeTemporaryExpr would be a Loc that this NonLoc is stored in.

For now this behavior has been achieved by creating a Loc out of thin air and re-binding the NonLoc value into the newly created location. This, however, is incorrect, because the constructor that has constructed the respective NonLoc value was operating on a different memory region, so all of the subsequent method calls, including the destructor, should have the exact same "this" region that the constructor was using.

The problem with behaving correctly is that we've already forgot the original storage. It is technically impossible to reliably recover it from the NonLoc.

The patch addresses it by relying on the ConstructionContext to inform the constructor that lifetime extension is taking place, so that the constructor could store the current "this" region in the program state for later use. The region is uniquely determined by the MaterializeTemporaryExpr itself. Then when time comes to model MaterializeTemporaryExpr, it looks up itself in the program state to find the respective region, and avoids the hassle of creating a new region and copying the value in case it does indeed find the existing correct region.

The temporary region's liveness (in the sense of removeDeadBindings) is extended until the MaterializeTemporaryExpr is resolved, in order to keep the store bindings around, because it wouldn't be referenced from anywhere else in the program state.

Situations where correct lifetime extension behavior is needed require relatively awkward test cases, because most of the time you don't care about the object's address as long as its contents are correct.

This patch is enough to assert that for every initialized temporary (lifetime-extended or not), a destructor is called (with the correct address) on all existing test cases(!). I added a new test case (with BinaryConditionalOperator) on which this assertion still fails (due to lack of construction context and other problems) - i've actually seen this operator used on C++ objects. There are more cases that we don't handle yet; i'd see what i can do about them.

Diff Detail

Repository
rC Clang

Event Timeline

NoQ created this revision.Feb 19 2018, 5:36 PM
NoQ updated this revision to Diff 135186.Feb 20 2018, 6:39 PM

Add one more FIXME test (dont_forget_destructor_around_logical_op in temporaries.cpp) which demonstrates a situation where we fail to call the temporary destructor after calling the temporary constructor. It should be fixed once we implement return value construction properly, as described at the "Return by value" part of http://lists.llvm.org/pipermail/cfe-dev/2018-February/056898.html

dcoughlin accepted this revision.Feb 20 2018, 7:55 PM

Looks good to me. I have a comment about simplifying createTemporaryRegionIfNeeded() (if possible) inline.

lib/StaticAnalyzer/Core/ExprEngine.cpp
278–295

Would it be safe for TR = MRMgr.getCXXTempObjectRegion(Init, LC); to be the else branch of if (const MaterializeTemporaryExpr *MT = dyn_cast<MaterializeTemporaryExpr>(Result)) rather than its own if statement?

Ideally the number paths through this function on which we call MRMgr.getCXXTempObjectRegion() would be small and very clear.

285

Would it be safe for the body of the if (!TR) to be the else branch of if constCXXTempObjectionRegion *const *TRPtr = ... rather then its own if statement?

497

Nit: There is a typo in the name here ("Materializatinos"). I guess this is the sub-atomic particle created as a byproduct of materializing a temporary! We have a lot of materializatinos in Cupertino.

test/Analysis/lifetime-extension.cpp
45

Yay!

78

Nice.

This revision is now accepted and ready to land.Feb 20 2018, 7:55 PM
a.sidorin accepted this revision.Feb 21 2018, 1:42 AM

Thank you! Just some of nits inline.

lib/StaticAnalyzer/Core/ExprEngine.cpp
401
for (const auto &I : State->get<TemporaryMaterializations>()) {
  auto *LCtx = I.first.second;
  if (LCtx == FromLC || (LCtx->isParentOf(From) && (!To || To->isParentOf(LCtx)))
     return false;
}
return true;

is a bit shorter but less evident so I won't insist.

403

EndLC? (similar to iterators)

407

const auto &?

510

As I see from line 350, Value is always non-null. And there is same check on line 659. Am I missing something?

NoQ updated this revision to Diff 135312.Feb 21 2018, 12:50 PM
NoQ marked 4 inline comments as done.
  • Address comments.
  • Add a FIXME test case that demonstrates that automatic destructors don't fire after lifetime extension through a POD field, even though lifetime extension itself seems to work correctly. I should probably also add a test case for the situation where sub-object adjustments actually kick in (here they suddenly don't), because even though they are correctly handled in createTemporaryRegionIfNeeded, they aren't implemented in findConstructionContexts.
lib/StaticAnalyzer/Core/ExprEngine.cpp
278–295

Nope, it also covers the case when we have an MTE but it's not static. I guess it's still more clear to add it twice than to have it as a fallback for an indefinite amount of cases.

285

Yep. Fxd.

403

Sounds neat, i'd make another quick patch on all things together.

497

Fixed to prevent further lifetime extensino of typos through autocompletinos.

510

Yep, all of these are non-null by construction. This actually makes me wonder if it was correct to remove VisitCXXBindTemporaryExpr from ExprEngine - might have done it too early (this is what makes the similar if in printInitializedTemporariesForContext redundant). The dont_forget_destructor_around_logical_op test also suggests that - seems to actually be a regression. I guess i'm going to revert the VisitCXXBindTemporaryExpr change in a follow-up commit.

Also we should allow ostream << MR to accept null pointers. It's a global operator overload anyway.

test/Analysis/lifetime-extension.cpp
45

These worked before (with old lifetime extension, since temporary constructors were inlined), i only added the run-line to see that.

NoQ added a comment.Feb 22 2018, 8:16 PM

Add a FIXME test case that demonstrates that automatic destructors don't fire after lifetime extension through a POD field, even though lifetime extension itself seems to work correctly.

This has always been broken - the CFG element for the automatic destructor is simply missing in this case. But previously we didn't inline the constructor, so the whole thing was invalidated anyway. I wonder how Sema lives without it.

Now the interesting part here is rC288563 which seems to have removed the need to skipRValueSubobjectAdjustments() three days after i added that code in rC288263 ¯\_(ツ)_/¯

This revision was automatically updated to reflect the committed changes.