[cfe-commits] r163361 - in /cfe/trunk: lib/StaticAnalyzer/Core/ExprEngineObjC.cpp test/Analysis/retain-release-crashes.m

[Anna Zaks]

On Sep 6, 2012, at 5:06 PM, Jordan Rose <jordan_rose at apple.com> wrote:

> 
> On Sep 6, 2012, at 16:58 , Anna Zaks <ganna at apple.com> wrote:
> 
>> 
>> On Sep 6, 2012, at 4:44 PM, Jordan Rose <jordan_rose at apple.com> wrote:
>> 
>>> Author: jrose
>>> Date: Thu Sep  6 18:44:36 2012
>>> New Revision: 163361
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=163361&view=rev
>>> Log:
>>> [analyzer] Don't crash if we cache out while evaluating an ObjC message.
>>> 
>>> A bizarre series of coincidences led us to generate a previously-seen
>>> node in the middle of processing an Objective-C message, where we assume
>>> the receiver is non-nil.
>> 
>> Should we cash out? / Why are we cashing out?
> 
> I couldn't exactly figure it out, but that is definitely what is happening. We generate a new node and that new node is null, even though the state and predecessor are both valid. If we run under -analyzer-purge=none, we either won't cache out or we're doing it somewhere more equipped to deal with it. See the radar and the test case for (slightly) more info.
> 
> I'm also a little bothered by not being able to reduce it further, but it's a definite crash that's affecting internal users, and this fix shouldn't result in any new false positives or false negatives. (The whole point of caching out is that we will have traversed the path from that point on anyway, right?)
> 
> If you're worried we're going to miss something, we could put a tag on the non-nil assumption, which will ensure that it only caches out at the exact same point in the ExprEngine's evaluation.

It is important to find out why we cash out because we might just silently drop coverage. Too aggressive caching out is very difficult to diagnose (and easy to introduce), so when this happens, we should try to find out why. Just adding an if statement is going to mask the issue. 

> Jordan
> 
> 
>>> We were assuming that such an assumption would
>>> never "cache out" like this, and blithely went on using a null ExplodedNode
>>> as the predecessor for the next step in evaluation.
>>> 
>>> Although the test case committed here is complicated, this could in theory
>>> happen in other ways as well, so the correct fix is just to test if the
>>> non-nil assumption results in an ExplodedNode we've seen before.
>>> 
>>> <rdar://problem/12243648>
>>> 
>>> Added:
>>>  cfe/trunk/test/Analysis/retain-release-crashes.m
>>> Modified:
>>>  cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
>>> 
>>> Modified: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp?rev=163361&r1=163360&r2=163361&view=diff
>>> ==============================================================================
>>> --- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp (original)
>>> +++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp Thu Sep  6 18:44:36 2012
>>> @@ -245,8 +245,9 @@
>>>     }
>>>   }
>>> 
>>> -    // Evaluate the call.
>>> -    defaultEvalCall(Bldr, Pred, *UpdatedMsg);
>>> +    // Evaluate the call if we haven't cached out.
>>> +    if (Pred)
>>> +      defaultEvalCall(Bldr, Pred, *UpdatedMsg);
>>> }
>>> 
>>> ExplodedNodeSet dstPostvisit;
>>> 
>>> Added: cfe/trunk/test/Analysis/retain-release-crashes.m
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/retain-release-crashes.m?rev=163361&view=auto
>>> ==============================================================================
>>> --- cfe/trunk/test/Analysis/retain-release-crashes.m (added)
>>> +++ cfe/trunk/test/Analysis/retain-release-crashes.m Thu Sep  6 18:44:36 2012
>>> @@ -0,0 +1,62 @@
>>> +// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -analyze -analyzer-checker=core,osx.cocoa.RetainCount,osx.cocoa.SelfInit,debug.ExprInspection -verify -w %s
>>> +
>>> +// This file contains crash regression tests; please do not remove any checkers
>>> +// from the RUN line because they may have been necessary to produce the crash.
>>> +// (Adding checkers should be fine.)
>>> +
>>> +void clang_analyzer_eval(int);
>>> +
>>> + at interface NSObject
>>> +- (id)init;
>>> + at end
>>> +
>>> + at interface Foo : NSObject
>>> + at end
>>> +
>>> +void touch(id, SEL);
>>> +id getObject();
>>> +int getInt();
>>> +
>>> +
>>> + at implementation Foo
>>> +// Bizarre crash related to the ExprEngine reaching a previously-seen
>>> +// ExplodedNode /during/ the processing of a message. Removing any
>>> +// parts of this test case seem not to trigger the crash any longer.
>>> +// <rdar://problem/12243648>
>>> +- (id)init {
>>> +  // Step 0: properly call the superclass's initializer
>>> +  self = [super init];
>>> +  if (!self) return self;
>>> +
>>> +  // Step 1: Perturb the state with a new conjured symbol.
>>> +  int value = getInt();
>>> +
>>> +  // Step 2: Loop. Some loops seem to trigger this, some don't.
>>> +  // The original used a for-in loop.
>>> +  while (--value) {
>>> +    // Step 3: Make it impossible to retain-count 'self' by calling
>>> +    // a function that takes a "callback" (in this case, a selector).
>>> +    // Note that this does not trigger the crash if you use a message!
>>> +    touch(self, @selector(hi));
>>> +  }
>>> +
>>> +  // Step 4: Use 'self', so that we know it's non-nil.
>>> +  [self bar];
>>> +
>>> +  // Step 5: Once again, make it impossible to retain-count 'self'...
>>> +  // ...while letting ObjCSelfInitChecker mark this as an interesting
>>> +  // message, since 'self' is an argument...
>>> +  // ...but this time do it in such a way that we'll also assume that
>>> +  // 'other' is non-nil. Once we've made the latter assumption, we
>>> +  // should cache out.
>>> +  id other = getObject();
>>> +  [other use:self withSelector:@selector(hi)];
>>> +
>>> +  // Step 6: Check that we did, in fact, keep the assumptions about 'self'
>>> +  // and 'other' being non-nil.
>>> +  clang_analyzer_eval(other != 0); // expected-warning{{TRUE}}
>>> +  clang_analyzer_eval(self != 0); // expected-warning{{TRUE}}
>>> +
>>> +  return self;
>>> +}
>>> + at end
>>> 
>>> 
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> 
>