1 files changed, 109 insertions, 107 deletions

diff --git a/lib/Analysis/CallAndMessageChecker.cpp b/lib/Analysis/CallAndMessageChecker.cpp
index ebc3a4fd56..920f21a22e 100644
--- a/lib/Analysis/CallAndMessageChecker.cpp
+++ b/lib/Analysis/CallAndMessageChecker.cpp
@@ -27,21 +27,27 @@ class VISIBILITY_HIDDEN CallAndMessageChecker
   BugType *BT_call_arg;
   BugType *BT_msg_undef;
   BugType *BT_msg_arg;
-  BugType *BT_struct_ret;
-  BugType *BT_void_ptr;
+  BugType *BT_msg_ret;
 public:
   CallAndMessageChecker() :
     BT_call_null(0), BT_call_undef(0), BT_call_arg(0),
-    BT_msg_undef(0), BT_msg_arg(0), BT_struct_ret(0), BT_void_ptr(0) {}
+    BT_msg_undef(0), BT_msg_arg(0), BT_msg_ret(0) {}
 
   static void *getTag() {
     static int x = 0;
     return &x;
   }
+
   void PreVisitCallExpr(CheckerContext &C, const CallExpr *CE);
   void PreVisitObjCMessageExpr(CheckerContext &C, const ObjCMessageExpr *ME);
+
 private:
   void EmitBadCall(BugType *BT, CheckerContext &C, const CallExpr *CE);
+  void EmitNilReceiverBug(CheckerContext &C, const ObjCMessageExpr *ME,
+                          ExplodedNode *N);
+    
+  void HandleNilReceiver(CheckerContext &C, const GRState *state,
+                         const ObjCMessageExpr *ME);    
 };
 } // end anonymous namespace
 
@@ -142,111 +148,107 @@ void CallAndMessageChecker::PreVisitObjCMessageExpr(CheckerContext &C,
     }
   }
 
-  // Check if the receiver was nil and then return value a struct.
+  // Check if the receiver was nil and then returns a value that may
+  // be garbage.
   if (const Expr *Receiver = ME->getReceiver()) {
-    SVal L_untested = state->getSVal(Receiver);
-    // Assume that the receiver is not NULL.
-    DefinedOrUnknownSVal L = cast<DefinedOrUnknownSVal>(L_untested);
-    const GRState *StNotNull = state->Assume(L, true);
-
-    // Assume that the receiver is NULL.
-    const GRState *StNull = state->Assume(L, false);
-
-    if (StNull) {
-      QualType RetTy = ME->getType();
-      if (RetTy->isRecordType()) {
-        if (C.getPredecessor()->getParentMap().isConsumedExpr(ME)) {
-          // The [0 ...] expressions will return garbage.  Flag either an
-          // explicit or implicit error.  Because of the structure of this
-          // function we currently do not bifurfacte the state graph at
-          // this point.
-          // FIXME: We should bifurcate and fill the returned struct with
-          //  garbage.
-          if (ExplodedNode* N = C.GenerateSink(StNull)) {
-            if (!StNotNull) {
-              if (!BT_struct_ret) {
-                std::string sbuf;
-                llvm::raw_string_ostream os(sbuf);
-                os << "The receiver in the message expression is 'nil' and "
-                  "results in the returned value (of type '"
-                   << ME->getType().getAsString()
-                   << "') to be garbage or otherwise undefined";
-                BT_struct_ret = new BuiltinBug(os.str().c_str());
-              }
-              
-              EnhancedBugReport *R = new EnhancedBugReport(*BT_struct_ret, 
-                                                   BT_struct_ret->getName(), N);
-              R->addRange(Receiver->getSourceRange());
-              R->addVisitorCreator(bugreporter::registerTrackNullOrUndefValue, 
-                                   Receiver);
-              C.EmitReport(R);
-              return;
-            }
-            else
-              // Do not report implicit bug.
-              return;
-          }
-        }
-      } else {
-        ASTContext &Ctx = C.getASTContext();
-        if (RetTy != Ctx.VoidTy) {
-          if (C.getPredecessor()->getParentMap().isConsumedExpr(ME)) {
-            // sizeof(void *)
-            const uint64_t voidPtrSize = Ctx.getTypeSize(Ctx.VoidPtrTy);
-            // sizeof(return type)
-            const uint64_t returnTypeSize = Ctx.getTypeSize(ME->getType());
-            
-            if (voidPtrSize < returnTypeSize) {
-              if (ExplodedNode* N = C.GenerateSink(StNull)) {
-                if (!StNotNull) {
-                  if (!BT_struct_ret) {
-                    std::string sbuf;
-                    llvm::raw_string_ostream os(sbuf);
-                    os << "The receiver in the message expression is 'nil' and "
-                      "results in the returned value (of type '"
-                       << ME->getType().getAsString()
-                       << "' and of size "
-                       << returnTypeSize / 8
-                       << " bytes) to be garbage or otherwise undefined";
-                    BT_void_ptr = new BuiltinBug(os.str().c_str());
-                  }
-              
-                  EnhancedBugReport *R = new EnhancedBugReport(*BT_void_ptr, 
-                                                     BT_void_ptr->getName(), N);
-                  R->addRange(Receiver->getSourceRange());
-                  R->addVisitorCreator(
-                          bugreporter::registerTrackNullOrUndefValue, Receiver);
-                  C.EmitReport(R);
-                  return;
-                } else
-                  // Do not report implicit bug.
-                  return;
-              }
-            }
-            else if (!StNotNull) {
-              // Handle the safe cases where the return value is 0 if the
-              // receiver is nil.
-              //
-              // FIXME: For now take the conservative approach that we only
-              // return null values if we *know* that the receiver is nil.
-              // This is because we can have surprises like:
-              //
-              //   ... = [[NSScreens screens] objectAtIndex:0];
-              //
-              // What can happen is that [... screens] could return nil, but
-              // it most likely isn't nil.  We should assume the semantics
-              // of this case unless we have *a lot* more knowledge.
-              //
-              SVal V = C.getValueManager().makeZeroVal(ME->getType());
-              C.GenerateNode(StNull->BindExpr(ME, V));
-              return;
-            }
-          }
-        }
-      }
+    DefinedOrUnknownSVal receiverVal =
+      cast<DefinedOrUnknownSVal>(state->getSVal(Receiver));
+    
+    const GRState *notNullState, *nullState;
+    llvm::tie(notNullState, nullState) = state->Assume(receiverVal);
+    
+    if (nullState && !notNullState) {
+      HandleNilReceiver(C, nullState, ME);
+      C.setDoneEvaluating(); // FIXME: eventually remove.
+      return;
     }
-    // Do not propagate null state.
-    if (StNotNull)
-      C.GenerateNode(StNotNull);
+    
+    assert(notNullState);
+    state = notNullState;
   }
+  
+  // Add a state transition if the state has changed.
+  C.addTransition(state);
+}
+
+void CallAndMessageChecker::EmitNilReceiverBug(CheckerContext &C,
+                                               const ObjCMessageExpr *ME,
+                                               ExplodedNode *N) {
+  
+  if (!BT_msg_ret)
+    BT_msg_ret =
+      new BuiltinBug("Receiver in message expression is "
+                     "'nil' and returns a garbage value");
+  
+  llvm::SmallString<200> buf;
+  llvm::raw_svector_ostream os(buf);
+  os << "The receiver of message '" << ME->getSelector().getAsString()
+     << "' is nil and returns a value of type '"
+     << ME->getType().getAsString() << "' that will be garbage";
+  
+  EnhancedBugReport *report = new EnhancedBugReport(*BT_msg_ret, os.str(), N);
+  const Expr *receiver = ME->getReceiver();
+  report->addRange(receiver->getSourceRange());
+  report->addVisitorCreator(bugreporter::registerTrackNullOrUndefValue, 
+                            receiver);
+  C.EmitReport(report);  
+}
+
+void CallAndMessageChecker::HandleNilReceiver(CheckerContext &C,
+                                              const GRState *state,
+                                              const ObjCMessageExpr *ME) {
+  
+  // Check the return type of the message expression.  A message to nil will
+  // return different values depending on the return type and the architecture.
+  QualType RetTy = ME->getType();
+
+  if (RetTy->isStructureType()) {
+    // FIXME: At some point we shouldn't rely on isConsumedExpr(), but instead
+    // have the "use of undefined value" be smarter about where the
+    // undefined value came from.
+    if (C.getPredecessor()->getParentMap().isConsumedExpr(ME)) {
+      if (ExplodedNode* N = C.GenerateSink(state))
+        EmitNilReceiverBug(C, ME, N);
+      return;
+    }
+
+    // The result is not consumed by a surrounding expression.  Just propagate
+    // the current state.
+    C.addTransition(state);
+    return;
+  }
+
+  // Other cases: check if the return type is smaller than void*.
+  ASTContext &Ctx = C.getASTContext();
+  if (RetTy != Ctx.VoidTy &&
+      C.getPredecessor()->getParentMap().isConsumedExpr(ME)) {
+    // Compute: sizeof(void *) and sizeof(return type)
+    const uint64_t voidPtrSize = Ctx.getTypeSize(Ctx.VoidPtrTy);
+    const uint64_t returnTypeSize = Ctx.getTypeSize(ME->getType());
+
+    if (voidPtrSize < returnTypeSize) {
+      if (ExplodedNode* N = C.GenerateSink(state))
+        EmitNilReceiverBug(C, ME, N);
+      return;
+    }
+
+    // Handle the safe cases where the return value is 0 if the
+    // receiver is nil.
+    //
+    // FIXME: For now take the conservative approach that we only
+    // return null values if we *know* that the receiver is nil.
+    // This is because we can have surprises like:
+    //
+    //   ... = [[NSScreens screens] objectAtIndex:0];
+    //
+    // What can happen is that [... screens] could return nil, but
+    // it most likely isn't nil.  We should assume the semantics
+    // of this case unless we have *a lot* more knowledge.
+    //
+    SVal V = C.getValueManager().makeZeroVal(ME->getType());
+    C.GenerateNode(state->BindExpr(ME, V));
+    return;
+  }
+  
+  C.addTransition(state);
 }