<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; "><div><br></div><div><div>On 5 Oct 2013, at 03:24, Jordan Rose <<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=us-ascii"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Oct 3, 2013, at 3:21 , Richard <<a href="mailto:tarka.t.otter@gmail.com">tarka.t.otter@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=us-ascii"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On 2 Oct 2013, at 03:23, Jordan Rose <<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Oct 1, 2013, at 9:40 , Jordan Rose <<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>[+cfe-commits, which I didn't really mean to remove]</div><br><div><div>On Sep 27, 2013, at 1:01 , Richard <<a href="mailto:tarka.t.otter@gmail.com">tarka.t.otter@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div>On 25 Sep 2013, at 18:44, Jordan Rose <<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>> wrote:</div><br><div>Yes, the SVal was an UnknownVal, that was it. I have fixed the dtor.cpp test by forcing a transition for StNonNull at the end of checkPreStmt with:</div><div><br></div><div><div style="margin: 0px; font-size: 14px; font-family: Monaco; "> C.addTransition(StNonNull, <span style="color: #bb2ca2">this</span>);</div></div><div><br></div><div>Unfortunately the other test failure remains, method-call-path-notes.cpp. This one is a bit trickier, checkPreCall is now being called and the null state is triggering a bug report. The problem is the generateSink() method in emitBadCall() is returning null, so the bug is never actually reported.</div><div><br></div><div>I guess the previously generated sink is causing the new sink to not be generated, but I do not know how to work around this. The generated graph is also a bit odd, I have attached it in case it is any help. Any suggestions here?</div></div></blockquote><div><br></div><div>Hm, I wonder if <i>this</i> is from the missing return at the end of the null check clause in checkPreStmt. Transitioning to StNonNull when we <i>know</i> the value is null is just wrong. I'll commit that back soon, but meanwhile can you try it locally?</div><div><br></div><div>(That probably also means you don't have to add the ", this" to the final addTransition there.)</div></div></div></blockquote><br></div><div>Committed in r191805.</div><div><br></div><div>Jordan</div><br></div></blockquote></div><br><div>Unfortunately this does not fix either issue, as in both cases we have an UnknownVal for the SVal, so we never hit the early return. </div><div><br></div><div>I am wondering if C++ member expressions should have special handling in the CallAndMessageChecker. Is it possible for a C++ member function to be null? I'm afraid my knowledge of C++ is insufficient to offer anything more constructive at this point, but doing an early return from CallAndMessageChecker::checkPreStmt for <span style="font-family: Monaco; font-size: 14px; ">CXXMemberCallExpr </span>fixes the bug, and does not cause any new ones to materialise.</div></div></blockquote><br></div><div>Member pointers can most certainly be null, but we don't handle it yet:</div><div><br></div><div><div><font face="Menlo"><span style="font-size: 11px;">struct Foo {</span></font></div><div><font face="Menlo"><span style="font-size: 11px;"><span class="Apple-tab-span" style="white-space:pre"> </span>int bar();</span></font></div><div><font face="Menlo"><span style="font-size: 11px;">};</span></font></div><div><font face="Menlo"><span style="font-size: 11px;"><br></span></font></div><div><font face="Menlo"><span style="font-size: 11px;">void test(Foo *obj) {</span></font></div><div><font face="Menlo"><span style="font-size: 11px;"><span class="Apple-tab-span" style="white-space:pre"> </span>int (Foo::*mp)() = 0;</span></font></div><div><font face="Menlo"><span style="font-size: 11px;"><span class="Apple-tab-span" style="white-space:pre"> </span>(obj->*mp)(); // should warn but don't</span></font></div><div><font face="Menlo"><span style="font-size: 11px;">}</span></font></div><div><br></div><div>Instead, let's think about why that would fix the problem. We shouldn't be generating a sink node anymore in the non-null case, and either we changed the state or we didn't. With the change to CheckerContext::addTransition, though, checkPreStmt generates a new node tagged with the checker, but with the same state as before. Then when we get to checkPreCall, we try to do the same thing again, and <i>there</i> we cache out.</div><div><br></div><div>I think what this means is that rather than completely removing the check in CheckerContext::addTransition, we should either check if the predecessor node is tagged, or just document that if nothing changed, you don't get a new node unless you explicitly pass the checker as a tag. How many places would need to change if you had to do that?</div><div><br></div><div>Jordan</div></div><br></div></blockquote></div><div><br></div>I'm not sure if this is exactly what is happening. It is not at the beginning of addTransition where we are failing to generate a sink. If MarkAsSink is true, this optimisation will always be skipped. The problem is in <span style="font-family: Monaco; font-size: 14px; ">NodeBuilder::generateNodeImpl</span> where IsNew is set to false for the null object call in <span style="font-family: Monaco; font-size: 14px; ">CallAndMessageChecker::checkPreCall</span> if there is a previous sink generated. In this case, 0 is always returned, so there is no node to report the bug on. <div><br></div><div>I am not sure why generating a sink on the not null path returns a cached sink from the null path though. Is it because the true state and the not true state both have the same predecessor with no other changes? If so, what is the solution here?</div><div><br></div><div>Richard.<br><div><div><br></div></div></div></body></html>