[analyzer] Skip Pre/Post handlers for ObjC calls when receiver is nil.

In Objective-C, method calls with nil receivers are essentially no-ops. They
do not fault (although the returned value may be garbage depending on the
declared return type and architecture). Programmers are aware of this
behavior and will complain about a false alarm when the analyzer
diagnoses API violations for method calls when the receiver is known to
be nil.

Rather than require each individual checker to be aware of this behavior
and suppress a warning when the receiver is nil, this commit
changes ExprEngineObjC so that VisitObjCMessage skips calling checker
pre/post handlers when the receiver is definitely nil. Instead, it adds a
new event, ObjCMessageNil, that is only called in that case.

The CallAndMessageChecker explicitly cares about this case, so I've changed it
to add a callback for ObjCMessageNil and moved the logic in PreObjCMessage
that handles nil receivers to the new callback.

rdar://problem/18092611

Differential Revision: http://reviews.llvm.org/D12123

llvm-svn: 247653

2 files changed, 107 insertions, 34 deletions

diff --git a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
index d26a76abe8e..c464d301b6e 100644
--- a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
@@ -177,7 +177,9 @@ void CheckerManager::runCheckersForStmt(bool isPreVisit,
 namespace {
   struct CheckObjCMessageContext {
     typedef std::vector<CheckerManager::CheckObjCMessageFunc> CheckersTy;
-    bool IsPreVisit, WasInlined;
+
+    ObjCMessageVisitKind Kind;
+    bool WasInlined;
     const CheckersTy &Checkers;
     const ObjCMethodCall &Msg;
     ExprEngine &Eng;
@@ -185,14 +187,28 @@ namespace {
     CheckersTy::const_iterator checkers_begin() { return Checkers.begin(); }
     CheckersTy::const_iterator checkers_end() { return Checkers.end(); }
 
-    CheckObjCMessageContext(bool isPreVisit, const CheckersTy &checkers,
+    CheckObjCMessageContext(ObjCMessageVisitKind visitKind,
+                            const CheckersTy &checkers,
                             const ObjCMethodCall &msg, ExprEngine &eng,
                             bool wasInlined)
-      : IsPreVisit(isPreVisit), WasInlined(wasInlined), Checkers(checkers),
+      : Kind(visitKind), WasInlined(wasInlined), Checkers(checkers),
         Msg(msg), Eng(eng) { }
 
     void runChecker(CheckerManager::CheckObjCMessageFunc checkFn,
                     NodeBuilder &Bldr, ExplodedNode *Pred) {
+
+      bool IsPreVisit;
+
+      switch (Kind) {
+        case ObjCMessageVisitKind::Pre:
+          IsPreVisit = true;
+          break;
+        case ObjCMessageVisitKind::MessageNil:
+        case ObjCMessageVisitKind::Post:
+          IsPreVisit = false;
+          break;
+      }
+
       const ProgramPoint &L = Msg.getProgramPoint(IsPreVisit,checkFn.Checker);
       CheckerContext C(Bldr, Eng, Pred, L, WasInlined);
 
@@ -202,19 +218,29 @@ namespace {
 }
 
 /// \brief Run checkers for visiting obj-c messages.
-void CheckerManager::runCheckersForObjCMessage(bool isPreVisit,
+void CheckerManager::runCheckersForObjCMessage(ObjCMessageVisitKind visitKind,
                                                ExplodedNodeSet &Dst,
                                                const ExplodedNodeSet &Src,
                                                const ObjCMethodCall &msg,
                                                ExprEngine &Eng,
                                                bool WasInlined) {
-  CheckObjCMessageContext C(isPreVisit,
-                            isPreVisit ? PreObjCMessageCheckers
-                                       : PostObjCMessageCheckers,
-                            msg, Eng, WasInlined);
+  auto &checkers = getObjCMessageCheckers(visitKind);
+  CheckObjCMessageContext C(visitKind, checkers, msg, Eng, WasInlined);
   expandGraphWithCheckers(C, Dst, Src);
 }
 
+const std::vector<CheckerManager::CheckObjCMessageFunc> &
+CheckerManager::getObjCMessageCheckers(ObjCMessageVisitKind Kind) {
+  switch (Kind) {
+  case ObjCMessageVisitKind::Pre:
+    return PreObjCMessageCheckers;
+    break;
+  case ObjCMessageVisitKind::Post:
+    return PostObjCMessageCheckers;
+  case ObjCMessageVisitKind::MessageNil:
+    return ObjCMessageNilCheckers;
+  }
+}
 namespace {
   // FIXME: This has all the same signatures as CheckObjCMessageContext.
   // Is there a way we can merge the two?
@@ -616,6 +642,11 @@ void CheckerManager::_registerForPostStmt(CheckStmtFunc checkfn,
 void CheckerManager::_registerForPreObjCMessage(CheckObjCMessageFunc checkfn) {
   PreObjCMessageCheckers.push_back(checkfn);
 }
+
+void CheckerManager::_registerForObjCMessageNil(CheckObjCMessageFunc checkfn) {
+  ObjCMessageNilCheckers.push_back(checkfn);
+}
+
 void CheckerManager::_registerForPostObjCMessage(CheckObjCMessageFunc checkfn) {
   PostObjCMessageCheckers.push_back(checkfn);
 }
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
index da66a46e18e..1d77714a002 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
@@ -139,6 +139,74 @@ void ExprEngine::VisitObjCMessage(const ObjCMessageExpr *ME,
   CallEventRef<ObjCMethodCall> Msg =
     CEMgr.getObjCMethodCall(ME, Pred->getState(), Pred->getLocationContext());
 
+  // There are three cases for the receiver:
+  //   (1) it is definitely nil,
+  //   (2) it is definitely non-nil, and
+  //   (3) we don't know.
+  //
+  // If the receiver is definitely nil, we skip the pre/post callbacks and
+  // instead call the ObjCMessageNil callbacks and return.
+  //
+  // If the receiver is definitely non-nil, we call the pre- callbacks,
+  // evaluate the call, and call the post- callbacks.
+  //
+  // If we don't know, we drop the potential nil flow and instead
+  // continue from the assumed non-nil state as in (2). This approach
+  // intentionally drops coverage in order to prevent false alarms
+  // in the following scenario:
+  //
+  // id result = [o someMethod]
+  // if (result) {
+  //   if (!o) {
+  //     // <-- This program point should be unreachable because if o is nil
+  //     // it must the case that result is nil as well.
+  //   }
+  // }
+  //
+  // We could avoid dropping coverage by performing an explicit case split
+  // on each method call -- but this would get very expensive. An alternative
+  // would be to introduce lazy constraints.
+  // FIXME: This ignores many potential bugs (<rdar://problem/11733396>).
+  // Revisit once we have lazier constraints.
+  if (Msg->isInstanceMessage()) {
+    SVal recVal = Msg->getReceiverSVal();
+    if (!recVal.isUndef()) {
+      // Bifurcate the state into nil and non-nil ones.
+      DefinedOrUnknownSVal receiverVal =
+          recVal.castAs<DefinedOrUnknownSVal>();
+      ProgramStateRef State = Pred->getState();
+
+      ProgramStateRef notNilState, nilState;
+      std::tie(notNilState, nilState) = State->assume(receiverVal);
+
+      // Receiver is definitely nil, so run ObjCMessageNil callbacks and return.
+      if (nilState && !notNilState) {
+        StmtNodeBuilder Bldr(Pred, Dst, *currBldrCtx);
+        bool HasTag = Pred->getLocation().getTag();
+        Pred = Bldr.generateNode(ME, Pred, nilState, nullptr,
+                                 ProgramPoint::PreStmtKind);
+        assert((Pred || HasTag) && "Should have cached out already!");
+        if (!Pred)
+          return;
+        getCheckerManager().runCheckersForObjCMessageNil(Dst, Pred,
+                                                         *Msg, *this);
+        return;
+      }
+
+      ExplodedNodeSet dstNonNil;
+      StmtNodeBuilder Bldr(Pred, dstNonNil, *currBldrCtx);
+      // Generate a transition to the non-nil state, dropping any potential
+      // nil flow.
+      if (notNilState != State) {
+        bool HasTag = Pred->getLocation().getTag();
+        Pred = Bldr.generateNode(ME, Pred, notNilState);
+        assert((Pred || HasTag) && "Should have cached out already!");
+        if (!Pred)
+          return;
+      }
+    }
+  }
+
   // Handle the previsits checks.
   ExplodedNodeSet dstPrevisit;
   getCheckerManager().runCheckersForPreObjCMessage(dstPrevisit, Pred,
@@ -160,38 +228,12 @@ void ExprEngine::VisitObjCMessage(const ObjCMessageExpr *ME,
     if (UpdatedMsg->isInstanceMessage()) {
       SVal recVal = UpdatedMsg->getReceiverSVal();
       if (!recVal.isUndef()) {
-        // Bifurcate the state into nil and non-nil ones.
-        DefinedOrUnknownSVal receiverVal =
-            recVal.castAs<DefinedOrUnknownSVal>();
-
-        ProgramStateRef notNilState, nilState;
-        std::tie(notNilState, nilState) = State->assume(receiverVal);
-
-        // There are three cases: can be nil or non-nil, must be nil, must be
-        // non-nil. We ignore must be nil, and merge the rest two into non-nil.
-        // FIXME: This ignores many potential bugs (<rdar://problem/11733396>).
-        // Revisit once we have lazier constraints.
-        if (nilState && !notNilState) {
-          continue;
-        }
-
-        // Check if the "raise" message was sent.
-        assert(notNilState);
         if (ObjCNoRet.isImplicitNoReturn(ME)) {
           // If we raise an exception, for now treat it as a sink.
           // Eventually we will want to handle exceptions properly.
           Bldr.generateSink(ME, Pred, State);
           continue;
         }
-
-        // Generate a transition to non-Nil state.
-        if (notNilState != State) {
-          bool HasTag = Pred->getLocation().getTag();
-          Pred = Bldr.generateNode(ME, Pred, notNilState);
-          assert((Pred || HasTag) && "Should have cached out already!");
-          if (!Pred)
-            continue;
-        }
       }
     } else {
       // Check for special class methods that are known to not return