<p dir="ltr">Did this patch strictly add new warnings? Do you know if it eliminated any false positives?</p>
<p dir="ltr">-Alex</p>
<div class="gmail_quote">On May 9, 2014 4:29 AM, "Manuel Klimek" <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr">Just a short update:<div>I ran this over our internal codebase, and found a roughly 0.5% change in warnings. All the ones I investigated were new warnings of the form:</div><div>SomeType x = ...;</div><div>
CHECK(some.Other() || something.Else()); // this is the macro that evaluates to a no-return destructor iff the condition is false</div>
<div>...</div><div>doSomethingWith(x);</div><div><br></div><div>And now we would get a warning that 'x' is never used after being initialized (I assume because it now always thinks the noreturn dtor is run). I'll have to investigate some more.</div>
<div><br></div><div>Cheers,</div><div>/Manuel</div><div><br></div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, May 7, 2014 at 6:55 PM, Jordan Rose <span dir="ltr"><<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Wow, this actually came out very nicely. Good work, Manuel and Alex! I have a few comments on individual pieces, but overall I'm actually pretty confident in this. Have you run it over a large codebase yet?<br>
<div><br>
> often we know when one destructor has executed that we must also execute the dtor of things that were generated previously; this makes the code and the CFG significantly more complex, though; the biggest downside seems to be that we potentially generate more states; not sure whether it's worth it<br>
><br>
</div><div>> often we do not actually need a new block (for example, if the temporary is created unconditionally); in that case, we could just run with a single block; again this would make the code more complex though, and I'm not sure it's worth the effort, unless we feel it's an important invariant on the CFG<br>
<br>
</div>We do still have this knowledge: it's whether the logical expression immediately dominating the last temporary processed is the same as the logical expression dominating the current temporary. That's not perfect (it doesn't handle the `A() || B() || C()` case), but it works in the simple cases, and handles unconditional creation as well. That said, if we do this we need to make sure the state is cleaned up correctly, as I mention below.<br>
<br>
I'm not sure how changing either of these behaviors would generate more states. It seems like if you have different sets of destructors to run, the states are already necessarily different, and once you've reached the common set of destructors, you'd have the same opportunity to "cache out", i.e. match an existing node in the ExplodedGraph.<br>
<br>
================<br>
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:54-56<br>
@@ -53,1 +53,5 @@<br>
<br>
+REGISTER_TRAIT_WITH_PROGRAMSTATE(<br>
+ InitializedTemporariesSet,<br>
+ llvm::ImmutableSet<const CXXBindTemporaryExpr *>);<br>
+<br>
----------------<br>
This needs to include the current StackFrameContext as well (for recursive functions). You can get that from the current LocationContext.<br>
<div><br>
================<br>
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:771<br>
@@ +770,3 @@<br>
+ !State->contains<InitializedTemporariesSet>(BTE));<br>
+ State = State->add<InitializedTemporariesSet>(BTE);<br>
+ StmtBldr.generateNode(BTE, Pred, State);<br>
----------------<br>
</div><div>Manuel Klimek wrote:<br>
> Alex McCarthy wrote:<br>
> > If includeTemporaryDtorsInCFG is not set, should we avoid this state modification?<br>
> That's a good question (for Jordan ;)<br>
><br>
> Another question is whether we still want to do the runCheckersForPre / runCheckersForPost enchilada.<br>
</div>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.<br>
<br>
(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.)<br>
<br>
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.)<br>
<div><br>
================<br>
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1451<br>
@@ -1433,1 +1450,3 @@<br>
<br>
+ if (const CXXBindTemporaryExpr *BTE =<br>
+ dyn_cast<CXXBindTemporaryExpr>(Condition)) {<br>
----------------<br>
</div><div>Manuel Klimek wrote:<br>
> Alex McCarthy wrote:<br>
> > 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.<br>
> ><br>
> > This would involve adding a new HandleTemporary helper (or similar) that you'd call instead of HandleBranch, and that would call processTemporary<br>
> 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.<br>
</div>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.<br>
<br>
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.<br>
<div><br>
================<br>
Comment at: test/Analysis/temporaries.cpp:275<br>
@@ -228,2 +274,3 @@<br>
+ }<br>
<br>
void testBinaryOperatorShortcut(bool value) {<br>
----------------<br>
</div><div>Manuel Klimek wrote:<br>
> Alex McCarthy wrote:<br>
> > Can you add a test that cleans up multiple temporaries in one condition? Maybe something like this (not sure if this compiles, we might need to add a check2(obj1, obj2) function):<br>
> > class Dtor {<br>
> > ~Dtor() {}<br>
> > };<br>
> ><br>
> > void testMultipleTemporaries(bool value) {<br>
> > if (value) {<br>
> > // ~Dtor should run before ~NoReturnDtor() because construction order is guaranteed by comma operator<br>
> > if (!value || check((NoReturnDtor(), Dtor())) || value) {<br>
> > clang_analyzer_eval(true); // no warning, unreachable code<br>
> > }<br>
> > }<br>
> > }<br>
> ><br>
> > We could also make ~Dtor emit a warning which would only be triggered if Dtor was constructed with a certain value (e.g. check(Dtor(/*warning=*/true), NoReturnDtor()), then see if clang warns on that error that should never happen.<br>
> Added the test, and it works as expected. The problem is that I have no idea how to verify the construction order, as (according to comments I've seen in the code) the analyzer does not inline destructors (or at least temporary destructors).<br>
</div>We've turned that off until we can get it right, yes. We can turn that back on after this patch. I'm happy to make this a CFG-dump test for now.<br>
<br>
<a href="http://reviews.llvm.org/D3627" target="_blank">http://reviews.llvm.org/D3627</a><br>
<br>
<br>
</blockquote></div><br></div>
</blockquote></div>