[PATCH] Proposal on how to fix temporary dtors.

[Manuel Klimek]

Ok, I think I worked through the issues. Please take another look :D

================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:54-56
@@ -53,1 +53,5 @@
 
+REGISTER_TRAIT_WITH_PROGRAMSTATE(
+    InitializedTemporariesSet,
+    llvm::ImmutableSet<const CXXBindTemporaryExpr *>);
+
----------------
Jordan Rose wrote:
> Manuel Klimek wrote:
> > Jordan Rose wrote:
> > > This needs to include the current StackFrameContext as well (for recursive functions). You can get that from the current LocationContext.
> > Can you elaborate on how I would put that into a datastructure? Just use a std::pair? (doesn't seem to work with ImmutableSet though)
> > 
> > Also, I seem unable to write a test that breaks because of this - any hints would be highly welcome.
> We'd have to write a specialization of ImutProfileInfo for std::pair, but that's probably not a bad idea anyway. Please feel free to add one to ImmutableSet.h.
> 
> I would guess the test case would look something like this:
> 
>   static bool test(bool isInner) {
>     if (isInner || NoReturnDtor() || test(true)) {
>       *(volatile int *)0 = 1; // should warn
>     }
>   }
>   void topLevel() {
>     test(false);
>   }
> 
Sent patch for review & added test.

================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:771
@@ +770,3 @@
+             !State->contains<InitializedTemporariesSet>(BTE));
+      State = State->add<InitializedTemporariesSet>(BTE);
+      StmtBldr.generateNode(BTE, Pred, State);
----------------
Jordan Rose wrote:
> Manuel Klimek wrote:
> > Alex McCarthy wrote:
> > > If includeTemporaryDtorsInCFG is not set, should we avoid this state modification?
> > That's a good question (for Jordan ;)
> > 
> > Another question is whether we still want to do the runCheckersForPre / runCheckersForPost enchilada.
> We're trying to run pre/post-checks on all non-trivial statements, so yes, we should still do it. It's //almost// simple enough that we can hoist that out of the big switch, but I'm not sure that's correct for all statement kinds, and no one's looked into it.
> 
> (But now that we have C++11, I'm //really// inclined to hoist the thing into a closure, so that people don't have to write that "loop over the nodes generated by pre-checkers" loop themselves.)
> 
> As for the state modification, what I mainly care about is that we //clean up// the map. Since that doesn't happen when temporary destructors are off, yes, we should be checking here. (Note that this is also important if we stop generating conditions for every temporary destructor—we need to make sure that the state is clean by the end of the full-expression even if we skip some of the conditions.)
Done.

================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1451
@@ -1433,1 +1450,3 @@
 
+  if (const CXXBindTemporaryExpr *BTE = 
+      dyn_cast<CXXBindTemporaryExpr>(Condition)) {
----------------
Jordan Rose wrote:
> Manuel Klimek wrote:
> > Alex McCarthy wrote:
> > > I think it's worth pulling this out into a new processTemporary helper method rather than adding it to processBranch: you're not reusing any of the logic in processBranch in the temporary dtor case.
> > > 
> > > This would involve adding a new HandleTemporary helper (or similar) that you'd call instead of HandleBranch, and that would call processTemporary
> > I agree. I mainly haven't done that, as somewhere in that chain there is an interface, and the method is abstract. I found only one use, so I assume somebody else implements that interface outside of the clang codebase. I'd like to hear what Jordan thinks on that issue, architecture-wise.
> These days the interface isn't serving us much real purpose. In theory CoreEngine is just traversing CFGs, and doesn't actually know very much about C ef al. It then hands the CFGElements off to its SubEngine, which actually processes them and tells the CoreEngine where to go next if there's a branch. ExprEngine is the particularly SubEngine that understands the AST / the language.
> 
> In practice there's a lot of linguistic knowledge coded into CoreEngine, just because there's a lot of linguistic knowledge in the CFG and in ProgramPoints these days. But either way, SubEngine is more of an interface than a real base class; I don't think Ted ever expected that we'd get subclassing more than one level deep. It's okay to split this up into helper methods.
Done.

http://reviews.llvm.org/D3627