<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><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><br></div><div>Cheers,</div><div>/Manuel</div>
<div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Apr 7, 2014 at 6:20 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"><div style="word-wrap:break-word"><div>Oops, no, that was my mistake. Thanks for catching this.</div>
<br><div><div><div>
<div>On Apr 5, 2014, at 18:16 , Alex McCarthy <<a href="mailto:alexmc@google.com" target="_blank">alexmc@google.com</a>> wrote:</div><br></div></div><blockquote type="cite"><div><div><div dir="ltr">It looks like the simple case test in r205661 was commented out in favor of the more complex test. Here's a patch to uncomment the simple test. We can also delete the simple case if you'd prefer.</div>
<div class="gmail_extra"><br clear="all"><div><span style="color:rgb(153,153,153)">-Alex</span><br></div>
<br><br><div class="gmail_quote">On Fri, Apr 4, 2014 at 7:14 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr">Great, thanks for the improvement!<br><br>I've started to dive into live variable analysis, but I think it'll take me a while to dig into this one. I'll ping this thread once I have more well-researched questions.</div>
<span><font color="#888888">
</font></span><div class="gmail_extra"><span><font color="#888888"><br clear="all"><div><span style="color:rgb(153,153,153)">-Alex</span><br></div></font></span><div><div>
<br><br><div class="gmail_quote">On Fri, Apr 4, 2014 at 7:08 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">
<div style="word-wrap:break-word"><div>Here's a trivial modification that shows two temporary destructors:</div><div><br></div><div><div style="margin:0px;font-size:11px;font-family:Menlo"> <span style="color:rgb(187,44,162)">bool</span> testNamedCustomDestructor2() {</div>
<div style="margin:0px;font-size:11px;font-family:Menlo"> <span style="color:rgb(187,44,162)">if</span> (CheckCustomDestructor c = (CheckCustomDestructor(), CheckCustomDestructor()))</div><div style="margin:0px;font-size:11px;font-family:Menlo;color:rgb(187,44,162)">
<span> </span>return<span> </span>true<span>;</span></div><div style="margin:0px;font-size:11px;font-family:Menlo;color:rgb(187,44,162)"><span> </span>return<span> </span>false<span>;</span></div>
<div style="margin:0px;font-size:11px;font-family:Menlo"> }</div></div><div><br></div><div>This also works with your patch, so I added it to dtor.cpp and committed everything in r205661. Thanks!</div><div>Jordan</div><br>
<div><div><div>On Apr 3, 2014, at 11:55 , Alex McCarthy <<a href="mailto:alexmc@google.com" target="_blank">alexmc@google.com</a>> wrote:</div><br></div><blockquote type="cite"><div>
<div dir="ltr"><div>I've attached a looping version of this patch. What do you think? I'm not a fan of the code duplication, but it's not too bad.</div><div><br></div><div>The logic change in this patch actually doesn't affect the test that I uncommented: it fixes another bug that appears when analyzing dtor.cpp with cfg-temporary-dtors=true. If you apply just the test change in this patch and revert the lib change, you'll see this failure (noise omitted):</div>
<div><br></div><div>******************** TEST 'Clang :: Analysis/dtor.cpp' FAILED ********************<br></div><div>error: 'warning' diagnostics seen but not expected: <br></div><div> File /Users/alexmc/Documents/dev/llvm/tools/clang/test/Analysis/dtor.cpp Line 394: Undefined or garbage value returned to caller</div>
<div><br></div><div>Here's the line that causes the error:</div><div><div> struct CheckCustomDestructor {</div><div> bool bool_;</div><div> CheckCustomDestructor():bool_(true) {}</div><div> ~CheckCustomDestructor();</div>
<div> operator bool() const { return bool_; } // warning generated here</div><div> }; </div></div><div><br></div><div>The lib change is the fix you recommended in <a href="http://permalink.gmane.org/gmane.comp.compilers.clang.devel/35520" target="_blank">http://permalink.gmane.org/gmane.comp.compilers.clang.devel/35520</a> for this error. Now that the error's fixed, it's safe to use cfg-temporary-dtors=true to analyze the whole file. Does that make sense? Do you have any recommendations on how I should rework this patch?</div>
<div><br></div><div>Thanks!</div></div><div class="gmail_extra"><br clear="all"><div><span style="color:rgb(153,153,153)">-Alex</span><br></div>
<br><br><div class="gmail_quote">On Tue, Apr 1, 2014 at 8:10 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">
<div style="word-wrap:break-word"><div>I like using a loop instead of a recursion; that's all. I'm not sure I had thought of the "multiple destructors in a row" case before I said my previous advice. I'd be fine to commit this as is...</div>
<div><br></div><div>...except I don't see how it fixes the test case, and once I can see that I'd like to have a more complicated test case here as well.</div><div><br></div><div>Jordan</div><div><br></div><br><div>
<div><div>On Apr 1, 2014, at 11:05 , Alex McCarthy <<a href="mailto:alexmc@google.com" target="_blank">alexmc@google.com</a>> wrote:</div><br></div><blockquote type="cite"><div>
<div dir="ltr">Maybe I misinterpreted your previous feedback: I also don't the "check one ahead" approach in the patch.<div><br></div><div>Here's another version that uses a recursive solution like the original patch, which I think handles this much more elegantly. It's an early recurse and return so there shouldn't be much wasted extra work. What do you think?</div>
</div><div class="gmail_extra"><br clear="all"><div><span style="color:rgb(153,153,153)">-Alex</span><br></div>
<br><br><div class="gmail_quote">On Tue, Apr 1, 2014 at 9:46 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div style="word-wrap:break-word"><div>I've committed all the cleanup patches, but I'm not sure the destructor patch is correct. What if there were two temporaries within the assignment? Wouldn't we see CFGImplicitDtors for both of them? (Maybe a loop can solve this problem, although it's kind of unfortunate.)</div>
<div><br></div><div>Jordan</div><div><br></div><br><div><div><div><div>On Apr 1, 2014, at 6:50 , Alex McCarthy <<a href="mailto:alexmc@google.com" target="_blank">alexmc@google.com</a>> wrote:</div><br></div>
</div><blockquote type="cite"><div><div dir="ltr">And two more patches: the first is the refactor to split out a getRegionForConstructedObject from ExprEngineCXX.cpp: this can be applied to head.<div><br>
</div>
<div>The second fixes spurious garbage value warnings when using temporary destructor analysis. It has to be applied after both the first refactoring attached to this mail (0001-Refactor-out-a-getRegionForConstructedObject-helper-.patch) and the previous patch to add new tests (0001-Prep-work-for-fixing-C-analysis-handling-of-temporar.patch).</div>
</div><div class="gmail_extra"><br clear="all"><div><span style="color:rgb(153,153,153)">-Alex</span><br></div>
<br><br><div class="gmail_quote">On Mon, Mar 31, 2014 at 8:29 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr">And here's a second patch, to be applied after the first, that fixes the crash while processing c++11 initializer lists. Thanks for explaining the intent of makeZeroElementRegion: I'm not sure why the FIXME I removed was there in the first place, but this seems like a much better fix than the one I had before.</div>
<span><font color="#888888">
</font></span><div class="gmail_extra"><span><font color="#888888"><br clear="all"><div><span style="color:rgb(153,153,153)">-Alex</span><br></div></font></span><div>
<br><br><div class="gmail_quote">On Mon, Mar 31, 2014 at 8:03 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr">Here's a new version of the simple patch. nullptr tests are gone, and I've uncommented tests that generate incorrect warnings. As discussed, I've left a few tests commented out because they trigger crashes or asserts: I think having the tests there but commented out serves as a cheap/poor form of documentation of the current state of these bugs in clang, and it should make upcoming patches that contain fixes smaller and easier to read. Let me know if you'd like me to remove those too.<div>
<br></div><div>Since I don't have clang/llvm commit access, are you able to commit this on my behalf once this looks good to oyu?<br><br>Thanks!</div></div><div class="gmail_extra"><span><font color="#888888"><br clear="all">
<div><span style="color:rgb(153,153,153)">-Alex</span><br>
</div></font></span><div>
<br><br><div class="gmail_quote">On Mon, Mar 31, 2014 at 3:47 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">
<div><br>
On Mar 31, 2014, at 10:40, Alex McCarthy <<a href="mailto:alexmc@google.com" target="_blank">alexmc@google.com</a>> wrote:<br>
<br>
> Thanks for your advice, Jordan.<br>
><br>
> I've split the NORETURN printing changes to CFG.cpp and the new test additions into a separate patch (attached). Hopefully this should be a safe no-op from a behavior perspective: debug output changes slightly, and the new test cases more accurately reflects current clang behavior. Does this look safe to commit?<br>
<br>
</div>Rather than commenting out tests, I'd prefer you put them in with the "wrong" expected-warnings, and then put a FIXME next to the warnings. Commented-out tests become dead tests all too easily.<br>
<br>
I would drop the tests from nullptr.cpp. That file's for testing nullptr and nullptr_t specifically, not null pointers in general, and the new tests don't actually test any new behavior if we already believe that nullptr properly converts to a null pointer value.<br>
<span><font color="#888888"><br>
Jordan<br>
<br>
</font></span></blockquote></div><br></div></div>
</blockquote></div><br></div></div>
</blockquote></div><br></div>
</div><span><0001-Refactor-out-a-getRegionForConstructedObject-helper-.patch></span><span><0002-Fix-spurious-Undefined-or-garbage-value-returned-to-.patch></span></blockquote></div><br></div></blockquote>
</div><br></div>
</div><span><0002-Fix-spurious-Undefined-or-garbage-value-returned-to-.patch></span></blockquote></div><br></div></blockquote></div><br></div>
</div><span><0001-Loop-Fix-spurious-Undefined-or-garbage-value-returned-to-.patch></span></blockquote></div><br></div></blockquote></div><br></div></div></div>
</blockquote></div><br></div>
</div></div><span><0001-Re-enable-simple-case-test-that-was-introduced-in-r2.patch></span></blockquote></div><br></div>
</blockquote></div><br></div></div></div>