[analyzer] Apply the suppression rules to the nil receiver only if the value participates in the computation of the nil we warn about.

We should only suppress a bug report if the IDCed or null returned nil value is directly related to the value we are warning about. This was
not the case for nil receivers - we would suppress a bug report that had an IDCed nil receiver on the path regardless of how it’s
related to the warning.

1) Thread EnableNullFPSuppression parameter through the visitors to differentiate between tracking the value which
is directly responsible for the bug and other values that visitors are tracking (ex: general tracking of nil receivers).
2) in trackNullOrUndef specifically address the case when a value of the message send is nil due to the receiver being nil.

llvm-svn: 178309

3 files changed, 61 insertions, 40 deletions

diff --git a/clang/lib/StaticAnalyzer/Checkers/IdempotentOperationChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/IdempotentOperationChecker.cpp
index 579ba9cf807..271ba4702c5 100644
--- a/clang/lib/StaticAnalyzer/Checkers/IdempotentOperationChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/IdempotentOperationChecker.cpp
@@ -423,12 +423,12 @@ void IdempotentOperationChecker::checkEndAnalysis(ExplodedGraph &G,
       if (LHSRelevant) {
         const Expr *LHS = i->first->getLHS();
         report->addRange(LHS->getSourceRange());
-        FindLastStoreBRVisitor::registerStatementVarDecls(*report, LHS);
+        FindLastStoreBRVisitor::registerStatementVarDecls(*report, LHS, false);
       }
       if (RHSRelevant) {
         const Expr *RHS = i->first->getRHS();
         report->addRange(i->first->getRHS()->getSourceRange());
-        FindLastStoreBRVisitor::registerStatementVarDecls(*report, RHS);
+        FindLastStoreBRVisitor::registerStatementVarDecls(*report, RHS, false);
       }
 
       BR.emitReport(report);
diff --git a/clang/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp
index f0ca8a8312b..93812f71485 100644
--- a/clang/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp
@@ -91,7 +91,8 @@ UndefCapturedBlockVarChecker::checkPostStmt(const BlockExpr *BE,
         BugReport *R = new BugReport(*BT, os.str(), N);
         if (const Expr *Ex = FindBlockDeclRefExpr(BE->getBody(), VD))
           R->addRange(Ex->getSourceRange());
-        R->addVisitor(new FindLastStoreBRVisitor(*V, VR));
+        R->addVisitor(new FindLastStoreBRVisitor(*V, VR,
+                                             /*EnableNullFPSuppression*/false));
         R->disablePathPruning();
         // need location of block
         C.emitReport(R);
diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index 3146e6c60e2..31ba5729eac 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -137,11 +137,12 @@ class ReturnVisitor : public BugReporterVisitorImpl<ReturnVisitor> {
     MaybeUnsuppress,
     Satisfied
   } Mode;
-  bool InitiallySuppressed;
+
+  bool EnableNullFPSuppression;
 
 public:
   ReturnVisitor(const StackFrameContext *Frame, bool Suppressed)
-    : StackFrame(Frame), Mode(Initial), InitiallySuppressed(Suppressed) {}
+    : StackFrame(Frame), Mode(Initial), EnableNullFPSuppression(Suppressed) {}
 
   static void *getTag() {
     static int Tag = 0;
@@ -151,7 +152,7 @@ public:
   virtual void Profile(llvm::FoldingSetNodeID &ID) const {
     ID.AddPointer(ReturnVisitor::getTag());
     ID.AddPointer(StackFrame);
-    ID.AddBoolean(InitiallySuppressed);
+    ID.AddBoolean(EnableNullFPSuppression);
   }
 
   /// Adds a ReturnVisitor if the given statement represents a call that was
@@ -162,7 +163,8 @@ public:
   /// the statement is a call that was inlined, we add the visitor to the
   /// bug report, so it can print a note later.
   static void addVisitorIfNecessary(const ExplodedNode *Node, const Stmt *S,
-                                    BugReport &BR) {
+                                    BugReport &BR,
+                                    bool InEnableNullFPSuppression) {
     if (!CallEvent::isCallStmt(S))
       return;
     
@@ -207,13 +209,13 @@ public:
     assert(Eng && "Cannot file a bug report without an owning engine");
     AnalyzerOptions &Options = Eng->getAnalysisManager().options;
 
-    bool InitiallySuppressed = false;
-    if (Options.shouldSuppressNullReturnPaths())
+    bool EnableNullFPSuppression = false;
+    if (InEnableNullFPSuppression && Options.shouldSuppressNullReturnPaths())
       if (Optional<Loc> RetLoc = RetVal.getAs<Loc>())
-        InitiallySuppressed = State->isNull(*RetLoc).isConstrainedTrue();
+        EnableNullFPSuppression = State->isNull(*RetLoc).isConstrainedTrue();
 
     BR.markInteresting(CalleeContext);
-    BR.addVisitor(new ReturnVisitor(CalleeContext, InitiallySuppressed));
+    BR.addVisitor(new ReturnVisitor(CalleeContext, EnableNullFPSuppression));
   }
 
   /// Returns true if any counter-suppression heuristics are enabled for
@@ -272,12 +274,14 @@ public:
     // make sure to track it into any further inner functions.
     if (!State->isNull(V).isConstrainedTrue()) {
       BR.markInteresting(V);
-      ReturnVisitor::addVisitorIfNecessary(N, RetE, BR);
+      ReturnVisitor::addVisitorIfNecessary(N, RetE, BR,
+                                           EnableNullFPSuppression);
       return 0;
     }
       
     // If we're returning 0, we should track where that 0 came from.
-    bugreporter::trackNullOrUndefValue(N, RetE, BR);
+    bugreporter::trackNullOrUndefValue(N, RetE, BR, /*IsArg*/ false,
+                                       EnableNullFPSuppression);
 
     // Build an appropriate message based on the return value.
     SmallString<64> Msg;
@@ -289,7 +293,7 @@ public:
       // the report is resurrected as valid later on.
       ExprEngine &Eng = BRC.getBugReporter().getEngine();
       AnalyzerOptions &Options = Eng.getAnalysisManager().options;
-      if (InitiallySuppressed && hasCounterSuppression(Options))
+      if (EnableNullFPSuppression && hasCounterSuppression(Options))
         Mode = MaybeUnsuppress;
 
       if (RetE->getType()->isObjCObjectPointerType())
@@ -360,7 +364,8 @@ public:
       if (!State->isNull(*ArgV).isConstrainedTrue())
         continue;
 
-      if (bugreporter::trackNullOrUndefValue(N, ArgE, BR, /*IsArg=*/true))
+      if (bugreporter::trackNullOrUndefValue(N, ArgE, BR, /*IsArg=*/true,
+                                             EnableNullFPSuppression))
         BR.removeInvalidation(ReturnVisitor::getTag(), StackFrame);
 
       // If we /can't/ track the null pointer, we should err on the side of
@@ -390,7 +395,7 @@ public:
   PathDiagnosticPiece *getEndPath(BugReporterContext &BRC,
                                   const ExplodedNode *N,
                                   BugReport &BR) {
-    if (InitiallySuppressed)
+    if (EnableNullFPSuppression)
       BR.markInvalid(ReturnVisitor::getTag(), StackFrame);
     return 0;
   }
@@ -403,6 +408,7 @@ void FindLastStoreBRVisitor ::Profile(llvm::FoldingSetNodeID &ID) const {
   ID.AddPointer(&tag);
   ID.AddPointer(R);
   ID.Add(V);
+  ID.AddBoolean(EnableNullFPSuppression);
 }
 
 PathDiagnosticPiece *FindLastStoreBRVisitor::VisitNode(const ExplodedNode *Succ,
@@ -487,10 +493,11 @@ PathDiagnosticPiece *FindLastStoreBRVisitor::VisitNode(const ExplodedNode *Succ,
     if (V.isUndef() || V.getAs<loc::ConcreteInt>()) {
       if (!IsParam)
         InitE = InitE->IgnoreParenCasts();
-      bugreporter::trackNullOrUndefValue(StoreSite, InitE, BR, IsParam);
+      bugreporter::trackNullOrUndefValue(StoreSite, InitE, BR, IsParam,
+                                         EnableNullFPSuppression);
     } else {
       ReturnVisitor::addVisitorIfNecessary(StoreSite, InitE->IgnoreParenCasts(),
-                                           BR);
+                                           BR, EnableNullFPSuppression);
     }
   }
 
@@ -520,7 +527,8 @@ PathDiagnosticPiece *FindLastStoreBRVisitor::VisitNode(const ExplodedNode *Succ,
           if (const VarRegion *OriginalR = BDR->getOriginalRegion(VR)) {
             if (Optional<KnownSVal> KV =
                 State->getSVal(OriginalR).getAs<KnownSVal>())
-              BR.addVisitor(new FindLastStoreBRVisitor(*KV, OriginalR));
+              BR.addVisitor(new FindLastStoreBRVisitor(*KV, OriginalR,
+                                                      EnableNullFPSuppression));
           }
         }
       }
@@ -807,7 +815,8 @@ static const Expr *peelOffOuterExpr(const Stmt *S,
 
 bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N,
                                         const Stmt *S,
-                                        BugReport &report, bool IsArg) {
+                                        BugReport &report, bool IsArg,
+                                        bool EnableNullFPSuppression) {
   if (!S || !N)
     return false;
 
@@ -815,6 +824,12 @@ 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();
@@ -881,7 +896,8 @@ bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N,
       // got initialized.
       if (const MemRegion *RR = getLocationRegionIfReference(Inner, N)) {
         if (Optional<KnownSVal> KV = LVal.getAs<KnownSVal>())
-          report.addVisitor(new FindLastStoreBRVisitor(*KV, RR));
+          report.addVisitor(new FindLastStoreBRVisitor(*KV, RR,
+                                                      EnableNullFPSuppression));
       }
     }
 
@@ -921,7 +937,8 @@ bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N,
         report.addVisitor(ConstraintTracker);
 
         // Add visitor, which will suppress inline defensive checks.
-        if (N->getState()->isNull(V).isConstrainedTrue()) {
+        if (N->getState()->isNull(V).isConstrainedTrue() &&
+            EnableNullFPSuppression) {
           BugReporterVisitor *IDCSuppressor =
             new SuppressInlineDefensiveChecksVisitor(V.castAs<DefinedSVal>(),
                                                      N);
@@ -930,7 +947,8 @@ bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N,
       }
 
       if (Optional<KnownSVal> KV = V.getAs<KnownSVal>())
-        report.addVisitor(new FindLastStoreBRVisitor(*KV, R));
+        report.addVisitor(new FindLastStoreBRVisitor(*KV, R,
+                                                     EnableNullFPSuppression));
       return true;
     }
   }
@@ -943,7 +961,8 @@ bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N,
   // sure that function isn't pruned in our output.
   if (const Expr *E = dyn_cast<Expr>(S))
     S = E->IgnoreParenCasts();
-  ReturnVisitor::addVisitorIfNecessary(N, S, report);
+
+  ReturnVisitor::addVisitorIfNecessary(N, S, report, EnableNullFPSuppression);
 
   // Uncomment this to find cases where we aren't properly getting the
   // base value that was dereferenced.
@@ -966,15 +985,11 @@ bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N,
   return true;
 }
 
-BugReporterVisitor *
-FindLastStoreBRVisitor::createVisitorObject(const ExplodedNode *N,
-                                            const MemRegion *R) {
-  assert(R && "The memory region is null.");
-
-  ProgramStateRef state = N->getState();
-  if (Optional<KnownSVal> KV = state->getSVal(R).getAs<KnownSVal>())
-    return new FindLastStoreBRVisitor(*KV, R);
-  return 0;
+const Expr *NilReceiverBRVisitor::getReceiver(const Stmt *S) {
+  const ObjCMessageExpr *ME = dyn_cast<ObjCMessageExpr>(S);
+  if (!ME)
+    return 0;
+  return ME->getInstanceReceiver();
 }
 
 PathDiagnosticPiece *NilReceiverBRVisitor::VisitNode(const ExplodedNode *N,
@@ -984,13 +999,15 @@ PathDiagnosticPiece *NilReceiverBRVisitor::VisitNode(const ExplodedNode *N,
   Optional<PreStmt> P = N->getLocationAs<PreStmt>();
   if (!P)
     return 0;
-  const ObjCMessageExpr *ME = P->getStmtAs<ObjCMessageExpr>();
-  if (!ME)
-    return 0;
-  const Expr *Receiver = ME->getInstanceReceiver();
+
+  const Expr *Receiver = getReceiver(P->getStmt());
   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())
@@ -999,7 +1016,8 @@ PathDiagnosticPiece *NilReceiverBRVisitor::VisitNode(const ExplodedNode *N,
   // 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);
+  bugreporter::trackNullOrUndefValue(N, Receiver, BR, /*IsArg*/ false,
+                                    EnableNullFPSuppression);
   // Issue a message saying that the method was skipped.
   PathDiagnosticLocation L(Receiver, BRC.getSourceManager(),
                                      N->getLocationContext());
@@ -1009,7 +1027,8 @@ PathDiagnosticPiece *NilReceiverBRVisitor::VisitNode(const ExplodedNode *N,
 
 // Registers every VarDecl inside a Stmt with a last store visitor.
 void FindLastStoreBRVisitor::registerStatementVarDecls(BugReport &BR,
-                                                       const Stmt *S) {
+                                                const Stmt *S,
+                                                bool EnableNullFPSuppression) {
   const ExplodedNode *N = BR.getErrorNode();
   std::deque<const Stmt *> WorkList;
   WorkList.push_back(S);
@@ -1031,7 +1050,8 @@ void FindLastStoreBRVisitor::registerStatementVarDecls(BugReport &BR,
 
         if (V.getAs<loc::ConcreteInt>() || V.getAs<nonloc::ConcreteInt>()) {
           // Register a new visitor with the BugReport.
-          BR.addVisitor(new FindLastStoreBRVisitor(V.castAs<KnownSVal>(), R));
+          BR.addVisitor(new FindLastStoreBRVisitor(V.castAs<KnownSVal>(), R,
+                                                   EnableNullFPSuppression));
         }
       }
     }