<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Apr 29, 2014, at 11:05 , Alex McCarthy <<a href="mailto:alexmc@google.com">alexmc@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><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">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>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><br></div><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><br></div><div>Jordan</div></body></html>