From: Jordan Rose <jordan_rose@apple.com>
Date: Sat, 8 Sep 2012 01:47:28 +0000 (+0000)
Subject: [analyzer] ObjCSelfInitChecker should always clean up in postCall checks.
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=82f2ad456a82da1b9cb7ddfc994c8f5fa44b59e6;p=clang

[analyzer] ObjCSelfInitChecker should always clean up in postCall checks.

ObjCSelfInitChecker stashes information in the GDM to persist it across
function calls; it is stored in pre-call checks and retrieved post-call.
The post-call check is supposed to clear out the stored state, but was
failing to do so in cases where the call did not have a symbolic return
value.

This was actually causing the inappropriate cache-out from r163361.
Per discussion with Anna, we should never actually cache out when
assuming the receiver of an Objective-C message is non-nil, because
we guarded that node generation by checking that the state has changed.
Therefore, the only states that could reach this exact ExplodedNode are
ones that should have merged /before/ making this assumption.

r163361 has been reverted and the test case removed, since it won't
actually test anything interesting now.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@163449 91177308-0d34-0410-b5e6-96231b3b80d8
---

diff --git a/lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp b/lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp
index 2fb022928e..dc902b944f 100644
--- a/lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp
+++ b/lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp
@@ -140,7 +140,8 @@ static void addSelfFlag(ProgramStateRef state, SVal val,
                         SelfFlagEnum flag, CheckerContext &C) {
   // We tag the symbol that the SVal wraps.
   if (SymbolRef sym = val.getAsSymbol())
-    C.addTransition(state->set<SelfFlag>(sym, getSelfFlags(val, C) | flag));
+    state = state->set<SelfFlag>(sym, getSelfFlags(val, state) | flag);
+  C.addTransition(state);
 }
 
 static bool hasSelfFlag(SVal val, SelfFlagEnum flag, CheckerContext &C) {
@@ -310,7 +311,7 @@ void ObjCSelfInitChecker::checkPostCall(const CallEvent &CE,
       const Expr *CallExpr = CE.getOriginExpr();
       if (CallExpr)
         addSelfFlag(state, state->getSVal(CallExpr, C.getLocationContext()),
-                                          prevFlags, C);
+                    prevFlags, C);
       return;
     }
   }
diff --git a/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp b/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
index abe18bf835..4f1a76e89e 100644
--- a/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
+++ b/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
@@ -192,8 +192,10 @@ void ExprEngine::VisitObjCMessage(const ObjCMessageExpr *ME,
         }
         
         // Generate a transition to non-Nil state.
-        if (notNilState != State)
+        if (notNilState != State) {
           Pred = Bldr.generateNode(currStmt, Pred, notNilState);
+          assert(Pred && "Should have cached out already!");
+        }
       }
     } else {
       // Check for special class methods.
@@ -245,9 +247,7 @@ void ExprEngine::VisitObjCMessage(const ObjCMessageExpr *ME,
       }
     }
 
-    // Evaluate the call if we haven't cached out.
-    if (Pred)
-      defaultEvalCall(Bldr, Pred, *UpdatedMsg);
+    defaultEvalCall(Bldr, Pred, *UpdatedMsg);
   }
   
   ExplodedNodeSet dstPostvisit;
diff --git a/test/Analysis/retain-release-crashes.m b/test/Analysis/retain-release-crashes.m
deleted file mode 100644
index 40171a39c9..0000000000
--- a/test/Analysis/retain-release-crashes.m
+++ /dev/null
@@ -1,62 +0,0 @@
-// 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);
-
-@interface NSObject
-- (id)init;
-@end
-
-@interface Foo : NSObject
-@end
-
-void touch(id, SEL);
-id getObject();
-int getInt();
-
-
-@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;
-}
-@end