Unavailable method checker

[Jordan Rose]

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:
>> 
>>> 
>>> On Sep 23, 2013, at 4:38 , Richard <tarka.t.otter at gmail.com> wrote:
>>>> 
>>>> This sounds exactly what is required. I have this mostly working, except for a bug that I cannot seem to track down. I have implemented the event dispatch in the CallAndMessageChecker as:
>>>> 
>>>> {	
>>>>   ...
>>>>   if (StNull && !StNonNull) {
>>>>     if (!BT_call_null)
>>>>       BT_call_null.reset(
>>>>         new BuiltinBug("Called function pointer is null (null dereference)"));
>>>>     emitBadCall(BT_call_null.get(), C, Callee);
>>>>   } else if (StNull) {
>>>>     // Called functions are assumed to be non-null, so this is
>>>>     // an implicit null dereference.
>>>>     if (ExplodedNode *N = C.generateSink(StNull)) {
>>>>       ImplicitNullDerefEvent Event = { L, false, N, &C.getBugReporter() };
>>>>       dispatchEvent(Event);
>>>>     }
>>>>   }
>>>> 
>>>>   C.addTransition(StNonNull);
>>>> }
>>>> 
>>>> This makes the unavailable method checker work fine, but causes 2 failures in some C++ tests, dtor.cpp:431 and method-call-path-notes.cpp:37. I am having a hard time working out why, any suggestions? In both cases the sink seems to abort the analysis early, checkPreCall is never called in the CallAndMessageChecker for the c++ methods that should be generating warnings. I am not sure where I should be looking here.
>>>> 
>>>> Otherwise the functionality works well, and using C.getPredecessor() instead of generating a sink for the Event makes everything work fine.
>>>> Richard.
>>> 
>>> Well, you did show me another bug: there's a missing return after emitBadCall. But that's not in the new code...
>>> 
>>> Aha: it's possible the value is UnknownVal, and thus StNull, StNonNull, and the original state are all the same. That means that the regular addTransition gets ignored, but the generateSink does not. That seems like a bug in CheckerContext::addTransitionImpl...if you remove this optimization, does it start working?
>>> 
>>>     if (!State || (State == Pred->getState() && !Tag && !MarkAsSink))
>>>       return Pred;
>>> 
>>> I'm not sure we can simply remove the optimization, but it'll help narrow down the problem.
>>> 
>>> Jordan
>>> 
>>> 
>> 
>> 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131001/b99cf9af/attachment.html>