<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><blockquote type="cite"><div class="gmail_extra"><div class="gmail_quote">On Mon, Mar 24, 2014 at 11:29 AM, 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: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex; position: static; z-index: auto;">
<div dir="ltr">That's very helpful, thanks Jordan!<div><br></div><div><span>Pavel's attempt to fix the liveliness problems was <a href="http://llvm-reviews.chandlerc.com/rL189090" target="_blank">http://llvm-reviews.chandlerc.com/rL189090</a>, correct? I'll give that a more thorough read.</span></div>
<div><span><br></span></div><div><span>I take it that from the "sometimes-y-ness" of the bug, we don't have any better regression test coverage than what's already in temporaries.cpp? In particular, we don't have any test cases covering the looping misbehavior you described?</span></div></div></blockquote></div></div></blockquote><div><br></div><div>We don't use the LiveVariables analysis for anything but persisting analyzer values, and I don't think there are any problematic cases other than the nested logical operators, which means there aren't really observable test cases with the trunk behavior. The two test cases we do have are test/Analysis/live-variables.m and .cpp (really just two versions of the same test case).</div><div><br></div><br><blockquote type="cite"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex; position: static; z-index: auto;"><div dir="ltr">
<div><span><br></span></div><div><span>Would you mind speeding up my search by pointing me at 1) the IRGen code that does this sort of variable synthesis 2) similar code in the clang analyzer that does any sort of variable synthesis? (I think Ted said we do something similar to track which objects have been constructed so we know which destructors need to be called)</span></div></div></blockquote></div></div></blockquote><div><br></div><div>That's what we need to implement. The place we already do this sort of thing is for static variables, which you can find in CFG.cpp controlled by the "AddStaticInitBranches" build option, in CoreEngine::HandleBlockExit, and in ExprEngine::processStaticInitializer. I'm not sure we'd do the exact same thing here—someone needs to figure out what design makes sense.</div><div><br></div><div>I'm not so familiar with IRGen, but you can see it doing the same thing in CGCleanup.cpp's SetupCleanupBlockActivation, PopCleanupBlock, and EmitCleanup (amid a ton of other logic).</div><br><blockquote type="cite"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex; position: static; z-index: auto;"><div class="gmail_extra"><div><div class="h5"><div class="gmail_quote">On Mon, Mar 24, 2014 at 10:15 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: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex; position: static; z-index: auto;">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>On Mon, Mar 24, 2014 at 6:09 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: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex; position: static; z-index: auto;"><div style="word-wrap:break-word"><div><div><div><br></div></div><div><div>A while ago Ted mentioned that we should just synthesize shadow bool variables of some kind to track whether or not something's been initialized. I'm starting to think more and more that that's the right idea. (It's basically what IRGen does.) We don't exactly have an implementation for it, though.</div>
</div></div></div></blockquote><div><br></div></div><div>If I remember correctly that was also Chandler's favorite opinion from the start (just do whatever IRGen does).</div><div> </div></div></div></div></blockquote></div></div></div></div></blockquote></div></div></blockquote></div><br><div>Oops. Credit to Chandler for this!</div><div><br></div><div><br></div><div><blockquote type="cite"><div dir="ltr">And one more question: what do you think that the impact would be of submitting this change without fixing the logical operator liveliness problem? Would that only cause noreturn destructor-based results to be sometimes broken (better than the current always broken behavior), or do you think this change is likely to introduce regressions in other areas?</div></blockquote></div><div><div dir="ltr"><br></div></div><div dir="ltr">As I see it, this would go from destructors of temporary objects being executed "never" to being exceuted "always, except when they participate in logical expressions". That may seem strictly better, but I really don't like the unpredictability aspect of that.</div><div dir="ltr"><br></div><div dir="ltr">Patch comments:</div><div dir="ltr"><br></div><div dir="ltr"><div dir="ltr"></div><blockquote type="cite"><div dir="ltr">Index: cfe/lib/Analysis/CFG.cpp</div><div dir="ltr">===================================================================</div><div dir="ltr">--- cfe/lib/Analysis/CFG.cpp</div><div dir="ltr">+++ cfe/lib/Analysis/CFG.cpp</div><div dir="ltr">@@ -3899,6 +3899,8 @@</div><div dir="ltr"> OS << " (EXIT)]\n";</div><div dir="ltr"> else if (&B == cfg->getIndirectGotoBlock())</div><div dir="ltr"> OS << " (INDIRECT GOTO DISPATCH)]\n";</div><div dir="ltr">+ else if (B.hasNoReturnElement())</div><div dir="ltr">+ OS << " (NORETURN)]\n";</div><div dir="ltr"> else</div><div dir="ltr"> OS << "]\n";</div></blockquote><div dir="ltr"> </div><div>This is fine to commit.</div><div><br></div><div><div></div></div><blockquote type="cite"><div><div> return getBooleanOption(IncludeTemporaryDtorsInCFG,</div><div> "cfg-temporary-dtors",</div><div>- /* Default = */ false);</div><div>+ /* Default = */ true);</div></div></blockquote><div><br></div><div>Not yet. ;-)</div><div><br></div><div><div></div></div><blockquote type="cite"><div><div>Index: cfe/lib/StaticAnalyzer/Core/CallEvent.cpp</div><div>===================================================================</div><div>--- cfe/lib/StaticAnalyzer/Core/CallEvent.cpp</div><div>+++ cfe/lib/StaticAnalyzer/Core/CallEvent.cpp</div><div>@@ -955,7 +955,6 @@</div><div> CFGElement E = (*B)[CalleeCtx->getIndex()];</div><div> assert(E.getAs<CFGImplicitDtor>() &&</div><div> "All other CFG elements should have exprs");</div><div>- assert(!E.getAs<CFGTemporaryDtor>() && "We don't handle temporaries yet");</div></div></blockquote><div><br></div><div>I'm concerned about removing this without actually doing some work to identify the triggering expression. This is used to place path notes, and not providing a better location here seems...suboptimal. Is there any way for us to recover the full-expression that included this destructor call?</div><div><br></div><div>Something to think about, but I guess it unblocks your work to commit this, so okay.</div><div><br></div><div><div></div></div><blockquote type="cite"><div><div>- assert(I != E);</div><div>+ // We might not have found a matching block if we're handling a temporary</div><div>+ // destructor.</div></div></blockquote><div><br></div><div>This is the problem we have to solve to get destructors in logical expressions to work. I'm surprised that removing this assertion works; the next thing you'll hit here is an llvm_unreachable.</div><div><br></div><div><div></div></div><blockquote type="cite"><div><div>+static const MemRegion *getRegionForConstructedObject(const CXXConstructExpr *CE,</div><div>+ ExplodedNode *Pred,</div><div>+ ExprEngine *ExprEngine,</div><div>+ unsigned int CurrStmtIdx) {</div></div></blockquote><div><br></div><div>This is a nice refactoring anyway, even without the destructor change. You could do that in a separate commit, just to make it clearer what's changing afterwards.</div><div><br></div><div>Also, watch your 80-column limit, and don't indent parameters all the way to the end if they don't all line up. Whatever clang-format does is probably good here, but if you're doing it manually you can either break after the return type or double-indent.</div><div><br></div><div>Double-also, please don't shadow type names. We use "ExprEngine &Eng" enough in the analyzer that that's probably the best choice (including the reference type).</div><div><br></div><div><div></div></div><blockquote type="cite"><div><div>+ // Is this a destructor? If so, we might be in the middle of an assignment</div><div>+ // to a local or member: look ahead one more element to see what we find.</div><div>+ if (Optional<CFGImplicitDtor> DtorElem = Next.getAs<CFGImplicitDtor>()) {</div><div>+ return getRegionForConstructedObject(CE, Pred, ExprEngine, CurrStmtIdx + 1);</div><div>+ }</div></div></blockquote><div><br></div><div>Does it make sense to do this first, inline, rather than doing a recursive call? </div><div><br></div><div><blockquote type="cite">+ // If this is an array, find a RecordDecl in the array so we know which<br>+ // deconstructor to call. As described below, this currently just runs the<br>+ // first destructor.<br>+ if (ObjectType->isArrayType()) {<br>+ SValBuilder &SVB = State->getStateManager().getSValBuilder();<br>+ ASTContext &Ctx = SVB.getContext();<br>+<br>+ while (const ArrayType *ArrayType = Ctx.getAsArrayType(ObjectType)) {<br>+ ObjectType = ArrayType->getElementType();<br>+ }<br>+ }<br></blockquote><br></div><div>makeZeroElementRegion already does this, and in fact it relies on you <i>not</i> doing it ahead of time. Perhaps its interface should be changed to make this more clear. (Sorry for being unclear last time this came up.)</div><div><br></div><div><div></div></div><blockquote type="cite"><div><div>Index: cfe/lib/StaticAnalyzer/Core/PathDiagnostic.cpp</div><div>===================================================================</div><div>--- cfe/lib/StaticAnalyzer/Core/PathDiagnostic.cpp</div><div>+++ cfe/lib/StaticAnalyzer/Core/PathDiagnostic.cpp</div><div>@@ -571,7 +571,10 @@</div><div> return PathDiagnosticLocation::createEnd(CallerBody, SM, CallerCtx);</div><div> return PathDiagnosticLocation::create(CallerInfo->getDecl(), SM);</div><div> }</div><div>- case CFGElement::TemporaryDtor:</div><div>+ case CFGElement::TemporaryDtor: {</div><div>+ const CFGTemporaryDtor &Dtor = Source.castAs<CFGTemporaryDtor>();</div><div>+ return PathDiagnosticLocation(Dtor.getBindTemporaryExpr(), SM, CallerCtx);</div><div>+ }</div></div></blockquote><div><br></div><div>This is all right. I wonder if it's better to show the bind-temporary expression or the full-expression where the cleanup actually happens.</div><div><br></div><div>Jordan</div></div></body></html>