<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, May 5, 2014 at 6:37 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><div class="h5"><br><div><div>On May 5, 2014, at 6:02 , Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>> wrote:</div>
<br><blockquote type="cite"><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><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></div></blockquote><br></div></div></div><div>Well, Clang's warnings don't evaluate things in the same way as the analyzer. The few warnings that <i>do</i> track any sort of evaluation (like -Wsometimes-uninitialized, IIRC?) can probably handle the current CFG, and probably wouldn't be able to handle the new one.</div>
</div></blockquote><div><br></div><div>I think the DeadCode one does (that might be the same you mention, I'll take a look). I'd be surprised if any check would be correct with the current CFG and temporary destructors in logical operators if it matters for the check (and if there are no logical ops, I'd assume all temp dtors are unconditionally called anyway, so there'll not be any terminators?)</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>I have a hard time thinking of which warnings the presence of temporary destructors would affect, which unfortunately probably means we don't have many tests. But on the other hand, that means we probably won't affect anything important.</div>
</div></blockquote><div><br></div><div>Yea, I concur that I'd hope people have put a test on anything they really care about :) but that's also why the proposal is to use an experimental-flag first, and have it run on at least our internal codebase, which will already validate a ton of warnings...</div>
<div><br></div><div>Cheers,</div><div>/Manuel</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"><span class="HOEnZb"><font color="#888888"><div>
<br></div><div>Jordan</div><br></font></span></div>
</blockquote></div><br></div></div>