Unavailable method checker

[Jordan Rose]

On Oct 10, 2013, at 8:06 , Richard <tarka.t.otter at gmail.com> wrote:

> 
> On 5 Oct 2013, at 03:24, Jordan Rose <jordan_rose at apple.com> wrote:
> 
>> 
>> On Oct 3, 2013, at 3:21 , Richard <tarka.t.otter at gmail.com> wrote:
>> 
>>> 
>>> On 2 Oct 2013, at 03:23, Jordan Rose <jordan_rose at apple.com> wrote:
>>> 
>>>> 
>>>> On Oct 1, 2013, at 9:40 , Jordan Rose <jordan_rose at apple.com> wrote:
>>>> 
>>>>> [+cfe-commits, which I didn't really mean to remove]
>>>>> 
>>>>> On Sep 27, 2013, at 1:01 , Richard <tarka.t.otter at gmail.com> wrote:
>>>>> 
>>>>>> 
>>>>>> On 25 Sep 2013, at 18:44, Jordan Rose <jordan_rose at apple.com> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131010/73e29ea3/attachment.html>