[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.

llvm-svn: 178381

clang/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.

clang/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,6 +856,13 @@ bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N,
   
   ProgramStateRef state = N->getState();
 
+  // 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)) {
@@ -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());

clang/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);