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

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

diff --git a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h
index 6eb5f259b3ad6082e560d60f69eba67c779e1217..2e5f207f4b4c163fec2468e9ce975cbba2a3286b 100644 (file)
--- 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 11218c086b9e3d2b09ff6b2c7d82fdee7cee92cf..241388d18f492cb4aba036e6a24ee29b631f7a86 100644 (file)
--- 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 a757af3d2fa8d4068aba3138aad6fd81b97d44a7..0404ee6df81322bea513eac115460ec6d1696050 100644 (file)
--- 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);