[PATCH] [analyzer] Enable limited support for temporary destructors

[Pavel Labath]

  Yes, I am still very much interested in pushing this through, although I did get a bit sidetracked by the lifetime extension issue. It appears that it is not entirely clear whether a construct like this should extend the lifetime of the temporary or not <http://llvm.org/bugs/show_bug.cgi?id=17003>. While I agree that it makes sense to extend, not even the clang compiler does that currently. Therefore, I would propose to make sure code like this doesn't crash the analyzer (I have made D1513 for that purpose and I think it should go in first (review, please? :) )), and then this patch could go in, without the lifetime extension (at least we will be consistent with the compiler).

  If it turns out later that we do want to extend the lifetime, then I think the same technique that we use for normal temporaries should work - just branch once more on the condition of the ?: operator. In fact, I think that if we change the CFG construction, then the rest of the analyzer should "just work". I see no fundamental difference between these two cases (running the destructor after the full statement or the block expression) -- the liveness analysis should make sure the value of the condition remains in the state and can be queried later.


================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1362
@@ +1361,3 @@
+  // If the value is already available, we don't need to do anything.
+  if(Pred->getState()->getSVal(Condition, Pred->getLocationContext()).isUnknownOrUndef()) {
+    // Resolve the condition in the presence of nested '||' and '&&'.
----------------
Jordan Rose wrote:
> This looks like it's from the other patch, doesn't it?
It is related to it, but I didn't want to put it there, as it is not necessary if we don't have temporary destructors. Basically, this will only be false when we run through the branches the second time. (Ok, it can be false also in other cases, but in these cases the normal ResolveCondition process will return the same value anyway).
The catch is that when you evaluate the branches the first time you always branch on the leaf values. Only in the second run you can have compound logical expressions like (a && b) if b doesn't contain temporaries.

================
Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:99-102
@@ -98,2 +98,6 @@
     Ty = AT->getElementType();
-    LValue = State->getLValue(Ty, SVB.makeZeroArrayIndex(), LValue);
+    // FIXME: The if test is just a temporary workaround, because
+    // ProcessTemporaryDtor sends us NULL regions. It will not be necessary once
+    // we can properly process temporary destructors.
+    if (LValue.getAsRegion())
+      LValue = State->getLValue(Ty, SVB.makeZeroArrayIndex(), LValue);
----------------
Jordan Rose wrote:
> This could probably be put outside the loop, huh?
Yes, indeed it can.

================
Comment at: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:810-814
@@ -809,1 +809,7 @@
 
+  // Temporary object destructor processing is currently broken, so we never
+  // inline them.
+  // FIXME: Remove this once temp destructors are working.
+  if ((*currBldrCtx->getBlock())[currStmtIdx].getAs<CFGTemporaryDtor>())
+    return false;
+
----------------
Jordan Rose wrote:
> Does it make sense to check the call kind first? These are all simple operations but they do require several dereferences.
Well... I wanted to make sure we don't attempt the inline under any circumstances. E.g. if I put it lower, then we would still inline synthesized destructor calls because of the check below (I didn't test that, but it certainly seems so). It would be possible to redesign the code to run this check only if isBodyAutosynthesized et al. return true but that would make the code hairy and i'm not sure if it's worth the trouble.


http://llvm-reviews.chandlerc.com/D1259