[analyzer] Address Jordan’s review of r178309 - do not register an extra visitor for nil receiver

We can check if the receiver is nil in the node that corresponds to the StmtPoint of the message send.
At that point, the receiver is guaranteed to be live. We will find at least one unreclaimed node due to
my previous commit (look for StmtPoint instead of PostStmt) and the fact that the nil receiver nodes are tagged.

+ a couple of extra tests.

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

3 files changed, 38 insertions, 35 deletions

diff --git a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h
index 6eb5f259b3..2e5f207f4b 100644
--- a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h
+++ b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h
@@ -160,25 +160,11 @@ private:
 /// \brief Prints path notes when a message is sent to a nil receiver.
 class NilReceiverBRVisitor
   : public BugReporterVisitorImpl<NilReceiverBRVisitor> {
-
-  /// \brief The reciever to track. If null, all receivers should be tracked.
-  const Expr *TrackedReceiver;
-
-  /// If the visitor is tracking the value directly responsible for the
-  /// bug, we are going to employ false positive suppression.
-  bool EnableNullFPSuppression;
-    
 public:
-    NilReceiverBRVisitor(const Expr *InTrackedReceiver = 0,
-                         bool InEnableNullFPSuppression = false):
-      TrackedReceiver(InTrackedReceiver),
-      EnableNullFPSuppression(InEnableNullFPSuppression) {}
   
   void Profile(llvm::FoldingSetNodeID &ID) const {
     static int x = 0;
     ID.AddPointer(&x);
-    ID.AddPointer(TrackedReceiver);
-    ID.AddBoolean(EnableNullFPSuppression);
   }
 
   PathDiagnosticPiece *VisitNode(const ExplodedNode *N,
@@ -186,7 +172,9 @@ public:
                                  BugReporterContext &BRC,
                                  BugReport &BR);
 
-  static const Expr *getReceiver(const Stmt *S);
+  /// If the statement is a message send expression with nil receiver, returns
+  /// the receiver expression. Returns NULL otherwise.
+  static const Expr *getNilReceiver(const Stmt *S, const ExplodedNode *N);
 };
 
 /// Visitor that tries to report interesting diagnostics from conditions.
diff --git a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index 11218c086b..241388d18f 100644
--- a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -824,12 +824,6 @@ bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N,
     S = Ex;
   }
 
-  // The message send could be null if the receiver is null.
-  if (const Expr *Receiver = NilReceiverBRVisitor::getReceiver(S)) {
-    report.addVisitor(new NilReceiverBRVisitor(Receiver,
-                                               EnableNullFPSuppression));
-  }
-
   const Expr *Inner = 0;
   if (const Expr *Ex = dyn_cast<Expr>(S)) {
     Ex = Ex->IgnoreParenCasts();
@@ -862,7 +856,14 @@ bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N,
   
   ProgramStateRef state = N->getState();
 
-  // See if the expression we're interested refers to a variable. 
+  // The message send could be nil due to the receiver being nil.
+  // At this point in the path, the receiver should be live since we are at the
+  // message send expr. If it is nil, start tracking it.
+  if (const Expr *Receiver = NilReceiverBRVisitor::getNilReceiver(S, N))
+    trackNullOrUndefValue(N, Receiver, report, IsArg, EnableNullFPSuppression);
+
+
+  // See if the expression we're interested refers to a variable.
   // If so, we can track both its contents and constraints on its value.
   if (Inner && ExplodedGraph::isInterestingLValueExpr(Inner)) {
     const MemRegion *R = 0;
@@ -985,11 +986,18 @@ bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N,
   return true;
 }
 
-const Expr *NilReceiverBRVisitor::getReceiver(const Stmt *S) {
+const Expr *NilReceiverBRVisitor::getNilReceiver(const Stmt *S,
+                                                 const ExplodedNode *N) {
   const ObjCMessageExpr *ME = dyn_cast<ObjCMessageExpr>(S);
   if (!ME)
     return 0;
-  return ME->getInstanceReceiver();
+  if (const Expr *Receiver = ME->getInstanceReceiver()) {
+    ProgramStateRef state = N->getState();
+    SVal V = state->getSVal(Receiver, N->getLocationContext());
+    if (state->isNull(V).isConstrainedTrue())
+      return Receiver;
+  }
+  return 0;
 }
 
 PathDiagnosticPiece *NilReceiverBRVisitor::VisitNode(const ExplodedNode *N,
@@ -1000,24 +1008,15 @@ PathDiagnosticPiece *NilReceiverBRVisitor::VisitNode(const ExplodedNode *N,
   if (!P)
     return 0;
 
-  const Expr *Receiver = getReceiver(P->getStmt());
+  const Expr *Receiver = getNilReceiver(P->getStmt(), N);
   if (!Receiver)
     return 0;
 
-  // Are we tracking a different reciever?
-  if (TrackedReceiver && TrackedReceiver != Receiver)
-    return 0;
-
-  ProgramStateRef state = N->getState();
-  SVal V = state->getSVal(Receiver, N->getLocationContext());
-  if (!state->isNull(V).isConstrainedTrue())
-    return 0;
-
   // The receiver was nil, and hence the method was skipped.
   // Register a BugReporterVisitor to issue a message telling us how
   // the receiver was null.
   bugreporter::trackNullOrUndefValue(N, Receiver, BR, /*IsArg*/ false,
-                                    EnableNullFPSuppression);
+                                     /*EnableNullFPSuppression*/ false);
   // Issue a message saying that the method was skipped.
   PathDiagnosticLocation L(Receiver, BRC.getSourceManager(),
                                      N->getLocationContext());
diff --git a/test/Analysis/inlining/inline-defensive-checks.m b/test/Analysis/inlining/inline-defensive-checks.m
index a757af3d2f..0404ee6df8 100644
--- a/test/Analysis/inlining/inline-defensive-checks.m
+++ b/test/Analysis/inlining/inline-defensive-checks.m
@@ -76,6 +76,22 @@ int testNilReceiver(Foo* fPtr) {
   return mem->x; // expected-warning {{Access to instance variable 'x' results in a dereference of a null pointer}}
 }
 
+int suppressNilReceiverRetNullCond(Foo* fPtr) {
+  unsigned zero = 0;
+  fPtr = retInputOrNil(fPtr);
+  // On a path where fPtr is nzil, mem should be nil.
+  Foo *mem = [fPtr getFooPtr];
+  return mem->x;
+}
+
+int suppressNilReceiverRetNullCondCast(id fPtr) {
+  unsigned zero = 0;
+  fPtr = retInputOrNil(fPtr);
+  // On a path where fPtr is nzil, mem should be nil.
+  Foo *mem = ((id)([(Foo*)(fPtr) getFooPtr]));
+  return mem->x;
+}
+
 int dontSuppressNilReceiverRetNullCond(Foo* fPtr) {
   unsigned zero = 0;
   fPtr = retInputOrNil(fPtr);