<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, May 1, 2014 at 7:16 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"><div style="word-wrap:break-word"><div class=""><br><div><div>On Apr 29, 2014, at 11:05 , Alex McCarthy <<a href="mailto:alexmc@google.com" target="_blank">alexmc@google.com</a>> wrote:</div>
<br><blockquote type="cite"><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 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>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>


</div></blockquote></div><br></div><div>It sounds about right to me, but (4) is interesting. Ted and Anna and I have talked about similar things before. It's a good solution for the analyzer, but it might make the rest of the compiler unhappy. IIRC, all of the existing compiler flow-sensitive analyses <i>do</i> include temporary destructors right now. They probably aren't doing anything so special with them, but changing the CFG so that CXXBindTemporaryExpr can be a terminator is something that potentially affects all clients.</div>
</div></blockquote><div><br></div><div>Given that the terminator expression with temp dtors inside logical binary ops is basically always not evaluated, thus not in the Environment, I don't expect much fallout from clang's warnings.</div>
<div>In the end, I hope we have tests for those warnings that we can just run, and fix the warnings that break? :)</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>If we do this under a flag, what behavior should we use elsewhere? The existing thing, of visiting all the conditions again?</div></div></blockquote><div><br></div><div>As Ted said, the idea would be to have the flag so everybody can test the new behavior on their codebase and make sure we have no regressions the tests don't cover, and then quickly replace the existing check.</div>
<div><br></div><div>Cheers,</div><div>/Manuel</div></div></div></div>