<div dir="ltr">Find attached the CFG for this code, which I think nicely visualizes the proposed new flow of temp dtor decisions.<br><div><br></div><div><div>void f() {</div><div> struct Dtor {</div><div> ~Dtor();</div>
<div> operator bool() const;</div><div> };</div><div> extern bool coin();</div><div> if (Dtor() || coin() || Dtor() ? coin() || coin() || Dtor()</div><div> : coin() && Dtor()) {</div>
<div> }</div><div>}</div></div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, May 7, 2014 at 9:39 AM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I also thought again about the problem of where to reset the bound state of the bind-temporary. The problem with the current solution is that it puts pretty strong requirements on how the CFG looks: we need to guarantee that for every constructed temporary, we will control-flow through the decision node that branches to the destructor call. At a first glance this may seem benign, but when I first thought about how to construct the CFG in that case, I believed the "optimum" to be one where we don't have any decision blocks when there is no control flow, and where we use the knowledge from where we are in a logical operator chain to unconditionally flow into the next destructor block.<br>
For example:<br>
A() || B() || C() || D()<br>
When we are in the destructor block for D(), we *know* that we have to call the destructor of C(), thus we could unconditionally flow into C's destructor block without first flowing into its decision block (like the current code does).<br>
I'm not sure it makes sense to do that (as it's more complex), but on the other hand I'm not sure it makes sense to preclude that with an implementation detail of how we track the temp ctor calls.<br>
<div><div class="h5"><br>
================<br>
Comment at: lib/Analysis/CFG.cpp:3540<br>
@@ -3582,60 +3539,3 @@<br>
AbstractConditionalOperator *E, bool BindToTemporary) {<br>
- // First add destructors for condition expression. Even if this will<br>
- // unnecessarily create a block, this block will be used at least by the full<br>
- // expression.<br>
- autoCreateBlock();<br>
- CFGBlock *ConfluenceBlock = VisitForTemporaryDtors(E->getCond());<br>
- if (badCFG)<br>
- return NULL;<br>
- if (BinaryConditionalOperator *BCO<br>
- = dyn_cast<BinaryConditionalOperator>(E)) {<br>
- ConfluenceBlock = VisitForTemporaryDtors(BCO->getCommon());<br>
- if (badCFG)<br>
- return NULL;<br>
- }<br>
-<br>
- // Try to add block with destructors for LHS expression.<br>
- CFGBlock *LHSBlock = NULL;<br>
- Succ = ConfluenceBlock;<br>
- Block = NULL;<br>
- LHSBlock = VisitForTemporaryDtors(E->getTrueExpr(), BindToTemporary);<br>
- if (badCFG)<br>
- return NULL;<br>
-<br>
- // Try to add block with destructors for RHS expression;<br>
- Succ = ConfluenceBlock;<br>
- Block = NULL;<br>
- CFGBlock *RHSBlock = VisitForTemporaryDtors(E->getFalseExpr(),<br>
- BindToTemporary);<br>
- if (badCFG)<br>
- return NULL;<br>
-<br>
- if (!RHSBlock && !LHSBlock) {<br>
- // If neither LHS nor RHS expression had temporaries to destroy don't create<br>
- // more blocks.<br>
- Block = ConfluenceBlock;<br>
- return Block;<br>
- }<br>
-<br>
- Block = createBlock(false);<br>
- Block->setTerminator(CFGTerminator(E, true));<br>
- assert(Block->getTerminator().isTemporaryDtorsBranch());<br>
-<br>
- // See if this is a known constant.<br>
- const TryResult &KnownVal = tryEvaluateBool(E->getCond());<br>
-<br>
- if (LHSBlock) {<br>
- addSuccessor(Block, LHSBlock, !KnownVal.isFalse());<br>
- } else if (KnownVal.isFalse()) {<br>
- addSuccessor(Block, NULL);<br>
- } else {<br>
- addSuccessor(Block, ConfluenceBlock);<br>
- std::reverse(ConfluenceBlock->pred_begin(), ConfluenceBlock->pred_end());<br>
- }<br>
-<br>
- if (!RHSBlock)<br>
- RHSBlock = ConfluenceBlock;<br>
-<br>
- addSuccessor(Block, RHSBlock, !KnownVal.isTrue());<br>
-<br>
- return Block;<br>
+ CFGBlock *CondBlock = VisitForTemporaryDtors(E->getCond());<br>
+ CFGBlock *TrueBlock = VisitForTemporaryDtors(E->getTrueExpr());<br>
----------------<br>
</div></div><div class="">Alex McCarthy wrote:<br>
> Noob question: If we return FalseBlock below, do we need to visit CondBlock and TrueBlock even if they aren't returned? If we do need to visit them, is the order important? (Consider documenting this if this isn't super-obvious to someone with more clang experience than me :) )<br>
</div>Yes, the CFG blocks are hooked up even if they aren't returned. Actually, most of the hooking up goes via the Block and Succ members, and afaiu the returned value is mostly for convenience (and usually equals Block).<br>
I'm not sure whether it makes sense to document that here, as it seems to be an invariant used throughout the CFG builder.<br>
<div class=""><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 class="">Alex McCarthy wrote:<br>
> If includeTemporaryDtorsInCFG is not set, should we avoid this state modification?<br>
</div>That's a good question (for Jordan ;)<br>
<br>
Another question is whether we still want to do the runCheckersForPre / runCheckersForPost enchilada.<br>
<div class=""><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 class="">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>
</div>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 class=""><br>
================<br>
Comment at: test/Analysis/temporaries.cpp:269<br>
@@ +268,3 @@<br>
+ if (i < 3 && (i >= 2 || check(NoReturnDtor()))) {<br>
+ // FIXME: Figure out why we don't run into this for i == 2.<br>
+ // If we add if (i == 2 && (...)) we run into this case, though.<br>
----------------<br>
</div><div class="">Alex McCarthy wrote:<br>
> Don't we hit the NoReturnDtor when i == 0, which will prevent us from getting to i==2? This seems correct as-is.<br>
</div>/me headdesks.<br>
<div class=""><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 class="">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>
</div>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>
<br>
<a href="http://reviews.llvm.org/D3627" target="_blank">http://reviews.llvm.org/D3627</a><br>
<br>
<br>
</blockquote></div><br></div>