[PATCH] Proposal on how to fix temporary dtors.

[Manuel Klimek]

Regarding LastBindTemporary and stacks:
My initial thought was that if VisitForTemporaryDtors can hit nested full-statements, it'd be incorrect anyway, so I assumed it can't happen.

Now that I look into it, it seems like I might be able to create a case where that happens (using a lambda inside an expression which itself of course has new full statements). I'm not sure yet what the right fix is, or whether there is something I'm not aware of yet that catches that. I'll investigate some more.

Generally, the idea is that the "stack" state is kept in the VisitXXXForTemporaryDtors, specifically VisitForTemporaryDtorsBranch, which:
1. Calls VisitForTemporaryDtors(E):
a) If that does not hit another VisitForTemporaryDtorsBranch, we'll just aggregate all temporary dtors below in a single block; the guard for that block now will be the topmost temporary dtor in that block, which is the last one we visited (due insert-at-top for blocks), thus LastBindTemporary.
b) If that does hit another VisitForTemporaryDtorsBranch, that one will have run through (a), and then have added all temporary dtors that need to run before the inner branch point, if any; if we found any after the inner branch (that should be executed before the inner branch), it will be in LastBindTemporary, and we'll add the branch; otherwise, we know that there was no temporary added since the inner branch, so we do not need to add another branch

I should probably add an implementation comment with that exact content for VisitForTemporaryDtorsBranch, unless you guys come up with a better idea on how to implement the behavior.

================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h:98-99
@@ -97,2 +97,4 @@
                     ExplodedNode *Pred);
+  void HandleBindTemporary(const CXXBindTemporaryExpr *BTE, const CFGBlock *B,
+                           ExplodedNode *Pred);
 
----------------
Jordan Rose wrote:
> Now that I think about it, this name is wrong. It's really "HandleCleanupTemporaryBranch" or something. The CXXBindTemporaryExpr just tells you which temporary it is. (And similar for similarly-named functions.)
Done.

================
Comment at: lib/Analysis/CFG.cpp:429-438
@@ -422,3 +428,12 @@
                                             bool BindToTemporary);
+  // Adds cleanup logic for temporaries in case of a conditional path.
+  //
+  // 1. Does nothing if 'E' does not construct temporary objects.
+  // 2. Equivalent to VisitForTemporaryDtors if 'E' contains temporary
+  //    destructors, but they are already handled in a branch due to a child
+  //    statement of 'E'.
+  // 3. Otherwise, creates a new decision node that conditionally executes the
+  //    temporary destructor and routes back to the current block on entry.
+  CFGBlock *VisitForTemporaryDtorsBranch(Stmt *E, bool BindToTemporary);
 
   // NYS == Not Yet Supported
----------------
Jordan Rose wrote:
> You might as well make this a proper doc comment.
Done.

================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:766-767
@@ +765,4 @@
+      if (getAnalysisManager().options.includeTemporaryDtorsInCFG()) {
+        VisitCXXBindTemporaryExpr(cast<CXXBindTemporaryExpr>(S), Pred, PreVisit,
+                                  Next);
+      } else {
----------------
Jordan Rose wrote:
> Calling this VisitCXXBindTemporaryExpr but then only having it actually be called when the flag is set seems like a bad idea if anyone ever needs to add behavior that should happen all the time. Either the check should be moved inside the method or it should be called something else.
Good point. Moved the if inside.

================
Comment at: test/Analysis/temporaries.cpp:316
@@ +315,3 @@
+      ++y;
+      if (true) (void)0; else (void)check(NoReturnDtor());
+    }
----------------
Jordan Rose wrote:
> This and the `true ? ...` below will get optimized out at CFG construction time. If that's correct, you should comment it; if you want to test path sensitivity, you should take a boolean input variable and then fully constrain it at the top of the function.
This was a regression test that broke with the last version of the patch... Funnily enough it only broke with the conditional operator case, not the if() case. I'll run the CFG for both and verify what they do / did in the patch, and update this with my findings...

http://reviews.llvm.org/D3627