On Oct 10, 2013, at 8:06 , Richard <[email protected]> wrote: > > On 5 Oct 2013, at 03:24, Jordan Rose <[email protected]> wrote: > >> >> On Oct 3, 2013, at 3:21 , Richard <[email protected]> wrote: >> >>> >>> On 2 Oct 2013, at 03:23, Jordan Rose <[email protected]> wrote: >>> >>>> >>>> On Oct 1, 2013, at 9:40 , Jordan Rose <[email protected]> wrote: >>>> >>>>> [+cfe-commits, which I didn't really mean to remove] >>>>> >>>>> On Sep 27, 2013, at 1:01 , Richard <[email protected]> wrote: >>>>> >>>>>> >>>>>> On 25 Sep 2013, at 18:44, Jordan Rose <[email protected]> wrote: >>>>>> >>>>>> 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: >>>>>> >>>>>> C.addTransition(StNonNull, this); >>>>>> >>>>>> 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. >>>>>> >>>>>> 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? >>>>> >>>>> Hm, I wonder if this is from the missing return at the end of the null >>>>> check clause in checkPreStmt. Transitioning to StNonNull when we know the >>>>> value is null is just wrong. I'll commit that back soon, but meanwhile >>>>> can you try it locally? >>>>> >>>>> (That probably also means you don't have to add the ", this" to the final >>>>> addTransition there.) >>>> >>>> Committed in r191805. >>>> >>>> Jordan >>>> >>> >>> 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. >>> >>> 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 CXXMemberCallExpr fixes the bug, >>> and does not cause any new ones to materialise. >> >> Member pointers can most certainly be null, but we don't handle it yet: >> >> struct Foo { >> int bar(); >> }; >> >> void test(Foo *obj) { >> int (Foo::*mp)() = 0; >> (obj->*mp)(); // should warn but don't >> } >> >> 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 there we cache out. >> >> 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? >> >> Jordan >> > > > 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 > NodeBuilder::generateNodeImpl where IsNew is set to false for the null object > call in CallAndMessageChecker::checkPreCall if there is a previous sink > generated. In this case, 0 is always returned, so there is no node to report > the bug on. > > 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?
Now I'm confused. Why would we have already generated a sink? None of the relevant checks here actually warn unless we're already fully-constrained, which means we should only be generating sinks in cases where there's no alternative. And that means we should never try to generate the second sink, because the first sink would have finished that path. Can you attach your latest patch(es), so that we're on the same page? Jordan
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
