<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Apr 29, 2014 at 8:05 PM, Alex McCarthy <span dir="ltr"><<a href="mailto:alexmc@google.com" target="_blank">alexmc@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>Jordan/Ted, could you comment on the approach described below? Manuel, please correct any mistakes in my write up :)</div>
<div><br></div>Manuel did some excellent debugging over the last couple of days, and this morning he and I sold ourselves on an approach to fix the temporary destructor issue described in <a href="http://llvm.org/bugs/show_bug.cgi?id=18159" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=18159</a> by following Jordan's recommendation to track temporary construction. Instead of changing the liveliness algorithms, we'll write patches that make the following changes:<div>
<br></div><div>1. Add new test cases to test/Analysis/temporaries.cpp that change boolean values during condition evaluation (e.g. if (!a || !b || !(a = false) || NoReturnDtr()) )</div><div>2. Add a flag to wrap the new temporary tracking behavior introduced below, off by default</div>
<div>3. Track temporary creation in ProgramState whenever we process a CXXBindTemporaryExpr, similar to how we handle static variable initialization (sample patch attached, nowhere near ready to commit)</div><div>4. Modify VisitBinaryOperatorForTemporaryDtors (lib/Analysis/CFG.cpp:3512) and VisitConditionalOperatorForTemporaryDtors (lib/Analysis/CFG.cpp:3624) to set the temporary cleanup block's terminator to be the CXXBindTemporaryExpr instead of a conditional only if the new flag is turned on (exact implementation might differ slightly, maybe by using a different expr/stmt class)</div>
<div>5. Add a new helper to call a temporary object's destructor if the temporary object is tracked in </div></div></blockquote><div><br></div><div>I think "call the destructor" is somewhat confusing - I think the idea is to generate the exploded state that visits the block that calls the desctructor iff the constructor was executed (true-branch), and visits the false-branch otherwise. This can be basically done equivalently to how we handle known Condition SVals in processBranch.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>ProgramState, similar to CoreEngine::HandleBranch (lib/StaticAnalyzer/Core/CoreEngine.cpp:453) and ExprEngine::processBranch (lib/StaticAnalyzer/Core/ExprEngine.cpp:1400). Alternative: modify processBranch to add special case logic for temporary destructor branching.</div>
<div>6. Modify HandleBlockExit (lib/StaticAnalyzer/Core/CoreEngine.cpp:345) to handle blocks with a CXXBindTemporaryExpr terminator by calling the new helper described above</div><div>7. Fix the assert caused by blocks with no statements and terminator conditions that fires in ResolveCondition (lib/StaticAnalyzer/Core/ExprEngine.cpp:1383). Manuel's working on understanding this better, and it might have a separate resolution.<br>
</div></div></blockquote><div><br></div><div>I think with the method described above, this assert will not fire any more (in fact, I think we'll add a new assert ;)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div>
</div><div>8. Do some large-scale testing, then set both the new temporary tracking behavior flag and the include-temporary-dtors flag to true by default if things look good.</div><div><br></div><div>What do you think?</div>
<span class="HOEnZb"><font color="#888888">
</font></span></div><div class="gmail_extra"><span class="HOEnZb"><font color="#888888"><br clear="all"><div><span style="color:rgb(153,153,153)">-Alex</span><br></div></font></span><div><div class="h5">
<br><br><div class="gmail_quote">On Tue, Apr 29, 2014 at 8:51 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">
<div dir="ltr">FYI, I figured ResolveCondition out (at least I think so). I'll send you a patch adding some code comments, where you can correct my understanding. Alex and I also now believe we have a plan how to fix the overall problem.</div>
<div><div>
<div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Apr 29, 2014 at 2:16 PM, 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">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>On Tue, Apr 29, 2014 at 3:27 AM, 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"><div style="word-wrap:break-word"><br><div><div><div>On Apr 28, 2014, at 9:35 , Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>> wrote:</div>
<br><blockquote type="cite"><div dir="ltr">Hi Jordan,<div><br></div><div>I have looked into the remaining problems today; let me quickly summarize so we make sure we're on the same page.</div><div>There are basically 2 problems left:</div>
<div>1. The invariant</div>
<div>As you mentioned on the rollback CL, the current layout of the CFG regarding temporary destructors violates the invariants assumed by other parts of the analyzer: namely, a CFG block with a terminator must not be empty.</div>
<div>After looking into it, I fully agree that we need a principled solution here; the question is: do we want to keep the invariant, and fix the CFG with temporary destructors to match the invariant, or do we want to say the invariant doesn't hold, and we should fix the rest of the code to not assume that invariant (and work with the current layout)? (I don't even know whether the former is possible theoretically).</div>
<div>Any hints on what your gut tells you the right solutions is would be helpful, and minimize dead-ends ;)</div><div>How would you want the CFG to look here?</div></div></blockquote><div><br></div></div><div>I'm not actually sure why we care that a CFG block with a terminator is not empty. It might be because we expect the terminator condition to have been computed within the block, and that it's trying to catch mistakes in constructing the CFG. The possible things that could be relying on this:</div>
<div><br></div><div>- maybe we ask for the first CFGElement unilaterally if the block isn't the entrance or exit block</div><div>- worse, maybe we assume the block <i>is</i> the entrance or exit block if it's "empty" (since the terminator isn't included in the element count)</div>
<div>- other things I haven't thought of</div><div><br></div><div>But really I don't think this is an important invariant, and if the right solution to (2) involves removing it, then so be it.</div></div></div></blockquote>
<div><br></div></div><div>Ok, I've tried to look into what exactly is happening in ExprEngine.cpp:1355, ResolveCondition.</div><div>I have not yet found out what ResolveCondition should actually do :)</div><div>It seems like if it's called with a logical binary operator, it's always called with the TerminatorCondition of the given CFGBlock.</div>
<div>Most of the time, it will just return that condition (namely if the condition is the last statement in the block).</div><div>If the condition is not the last statement in the block, it will drill down through the condition into the innermost RHS of logical binary operators, and then return that.</div>
<div><br></div><div>Can you explain what's going on here? (That's the point where the analyzer currently assumes the block is not empty, assert(I != E) triggers).</div><div><br></div><div>Thanks!</div><span><font color="#888888"><div>
/Manuel</div></font></span><div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><div><br></div><br><blockquote type="cite"><div dir="ltr">
<div><br></div><div>2. Taking all the correct branches</div>
<div>So far I'm not sure I have seen a case where we're not running into (1), but I'm not sure (the case where the temporary with the no-return dtor is the last in a sequence of binary logical ops seems to be (1) at least).</div>
<div>The correct solution seems to be to add tracking lifetime of temporaries in conditionals correctly. alexmc is working on a patch here.</div></div></blockquote><br></div></div><div>I agree; it's the particular value of "correctly" that makes the problem difficult. ;-) Very glad Alex has been able to spend time on this.</div>
<span><font color="#888888"><div><br></div><div>Jordan</div><br></font></span></div></blockquote></div></div><br></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div></div></div>
</blockquote></div><br></div></div>