r180639 - [analyzer] An ObjC for-in loop runs 0 times if the collection is nil.

[Jordan Rose]

Author: jrose
Date: Fri Apr 26 16:43:01 2013
New Revision: 180639

URL: http://llvm.org/viewvc/llvm-project?rev=180639&view=rev
Log:
[analyzer] An ObjC for-in loop runs 0 times if the collection is nil.

In an Objective-C for-in loop "for (id element in collection) {}", the loop
will run 0 times if the collection is nil. This is because the for-in loop
is implemented using a protocol method that returns 0 when there are no
elements to iterate, and messages to nil will result in a 0 return value.

At some point we may want to actually model this message send, but for now
we may as well get the nil case correct, and avoid the false positives that
would come with this case.

<rdar://problem/13744632>

Modified:
    cfe/trunk/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
    cfe/trunk/test/Analysis/objc-for.m

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp?rev=180639&r1=180638&r2=180639&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp Fri Apr 26 16:43:01 2013
@@ -759,38 +759,81 @@ static bool isKnownNonNilCollectionType(
   }
 }
 
-void ObjCLoopChecker::checkPostStmt(const ObjCForCollectionStmt *FCS,
-                                    CheckerContext &C) const {
-  ProgramStateRef State = C.getState();
-  
-  // Check if this is the branch for the end of the loop.
-  SVal CollectionSentinel = State->getSVal(FCS, C.getLocationContext());
-  if (CollectionSentinel.isZeroConstant())
-    return;
-  
+/// Assumes that the collection is non-nil.
+///
+/// If the collection is known to be nil, returns NULL to indicate an infeasible
+/// path.
+static ProgramStateRef checkCollectionNonNil(CheckerContext &C,
+                                             ProgramStateRef State,
+                                             const ObjCForCollectionStmt *FCS) {
+  if (!State)
+    return NULL;
+
+  SVal CollectionVal = C.getSVal(FCS->getCollection());
+  Optional<DefinedSVal> KnownCollection = CollectionVal.getAs<DefinedSVal>();
+  if (!KnownCollection)
+    return State;
+
+  ProgramStateRef StNonNil, StNil;
+  llvm::tie(StNonNil, StNil) = State->assume(*KnownCollection);
+  if (StNil && !StNonNil) {
+    // The collection is nil. This path is infeasible.
+    return NULL;
+  }
+
+  return StNonNil;
+}
+
+/// Assumes that the collection elements are non-nil.
+///
+/// This only applies if the collection is one of those known not to contain
+/// nil values.
+static ProgramStateRef checkElementNonNil(CheckerContext &C,
+                                          ProgramStateRef State,
+                                          const ObjCForCollectionStmt *FCS) {
+  if (!State)
+    return NULL;
+
   // See if the collection is one where we /know/ the elements are non-nil.
-  const Expr *Collection = FCS->getCollection();
-  if (!isKnownNonNilCollectionType(Collection->getType()))
-    return;
-  
-  // FIXME: Copied from ExprEngineObjC.
+  if (!isKnownNonNilCollectionType(FCS->getCollection()->getType()))
+    return State;
+
+  const LocationContext *LCtx = C.getLocationContext();
   const Stmt *Element = FCS->getElement();
-  SVal ElementVar;
+
+  // FIXME: Copied from ExprEngineObjC.
+  Optional<Loc> ElementLoc;
   if (const DeclStmt *DS = dyn_cast<DeclStmt>(Element)) {
     const VarDecl *ElemDecl = cast<VarDecl>(DS->getSingleDecl());
     assert(ElemDecl->getInit() == 0);
-    ElementVar = State->getLValue(ElemDecl, C.getLocationContext());
+    ElementLoc = State->getLValue(ElemDecl, LCtx);
   } else {
-    ElementVar = State->getSVal(Element, C.getLocationContext());
+    ElementLoc = State->getSVal(Element, LCtx).getAs<Loc>();
   }
 
-  if (!ElementVar.getAs<Loc>())
-    return;
+  if (!ElementLoc)
+    return State;
 
   // Go ahead and assume the value is non-nil.
-  SVal Val = State->getSVal(ElementVar.castAs<Loc>());
-  State = State->assume(Val.castAs<DefinedOrUnknownSVal>(), true);
-  C.addTransition(State);
+  SVal Val = State->getSVal(*ElementLoc);
+  return State->assume(Val.castAs<DefinedOrUnknownSVal>(), true);
+}
+
+void ObjCLoopChecker::checkPostStmt(const ObjCForCollectionStmt *FCS,
+                                    CheckerContext &C) const {
+  // Check if this is the branch for the end of the loop.
+  SVal CollectionSentinel = C.getSVal(FCS);
+  if (CollectionSentinel.isZeroConstant())
+    return;
+
+  ProgramStateRef State = C.getState();
+  State = checkCollectionNonNil(C, State, FCS);
+  State = checkElementNonNil(C, State, FCS);
+
+  if (!State)
+    C.generateSink();
+  else if (State != C.getState())
+    C.addTransition(State);
 }
 
 namespace {

Modified: cfe/trunk/test/Analysis/objc-for.m
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/objc-for.m?rev=180639&r1=180638&r2=180639&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/objc-for.m (original)
+++ cfe/trunk/test/Analysis/objc-for.m Fri Apr 26 16:43:01 2013
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -analyze -analyzer-checker=osx.cocoa.Loops,debug.ExprInspection -verify %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,osx.cocoa.Loops,debug.ExprInspection -verify %s
 
 void clang_analyzer_eval(int);
 
@@ -56,3 +56,15 @@ void testWithVarInFor() {
     clang_analyzer_eval(x != nil); // expected-warning{{UNKNOWN}}
 }
 
+void testNonNil(id a, id b) {
+  clang_analyzer_eval(a != nil); // expected-warning{{UNKNOWN}}
+  for (id x in a)
+    clang_analyzer_eval(a != nil); // expected-warning{{TRUE}}
+
+  if (b != nil)
+    return;
+  for (id x in b)
+    *(volatile int *)0 = 1; // no-warning
+  clang_analyzer_eval(b != nil); // expected-warning{{FALSE}}
+}
+