[analyzer][NFC] Prepare visitors for different tracking kinds

[Szelethus]

Summary

When we're tracking a variable that is responsible for a null pointer dereference or some other sinister programming error, we of course would like to gather as much information why we think that the variable has that specific value as possible. However, the newly introduced condition tracking shows that tracking all values this thoroughly could easily cause an intolerable growth in the bug report's length.

Take, for instance the following report as an example: BEFORE condition tracking, AFTER condition tracking. Having an already lengthy, bug report with 18 events grew to 45 is inexcusable.

There are a variety of heuristics we discussed on the mailing list to combat this, all of them requiring to differentiate in between tracking a "regular value" and a "condition".

This patch introduces the new bugreporter::TrackingKind enum, adds it to FindLastStoreBRVisitor as a non-optional argument, and moves some functions around to make the code a little more coherent.

[NoQ]

Approve!

null pointer dereference

...should in my opinion be tracked in a mild manner, as if it's a condition, because D62978. I think the definition we're looking for is "when we care only about why it's null, regardless of what it is or where it comes from".

Say, in case of MallocChecker the malloc() call is the "origin" of the tracked pointer; we need to track it back to its original malloc. Similarly, in case of null dereference, the collapse point should be treated as an "origin" of the null pointer. The same for condition.

[Charusso]

Document!!4!44! It is great you have started to limit the notes, thanks!

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h

110

What about the following?

enum class TrackKind {
  Full,     // comment what it does
  Condition // comment what it does
};

Please consider the following example by unspoken naming conventions:

enum class ColoringKind {
  RedColoring,
  BlueColoring
}

would be

enum class Color { Red, Blue }

where each trivial what is does, by name.

Please also note that, the thoroughness not working with redecls, assumptions, loop conditions, anything we failed to inline, etc... so it is not really that full tracking. But in our capabilities, it is the full what we could do.

[xazax.hun]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

81

Do we need this overload? What about a default argument?

[Szelethus]

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h

110

There's a good reasoning behind this -- if you take a look at this, and followup patches, the behavior the different TrackingKinds cause may differ from visitor to visitor, so I decided to document each behavior there. That way, you can't avoid the documents even if you didn't glance over this enum.

Please also note that, the thoroughness not working with redecls, assumptions, loop conditions, anything we failed to inline, etc... so it is not really that full tracking. But in our capabilities, it is the full what we could do.

I don't see what you mean here?

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

81

I don't want to expose this functionality outside of this file. I don't insist though!

[NoQ]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

81

I think we'll have to expose it because different checkers would want different tracking kinds for their values. For instance, null dereference checker could really use condition tracking as its default tracking mode.

[Szelethus]

• Make sure that the tracking kind is forwarded for each invocation of trackExpressionValue in BugReporterVisitor.cpp
• Expose the tracking kind to all users
• Add some more docs

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h

public:
   virtual void Profile(llvm::FoldingSetNodeID &ID) const = 0;
 
   /// Generates the default final diagnostic piece.
   static std::shared_ptr<PathDiagnosticPiece>
   getDefaultEndPath(BugReporterContext &BRC, const ExplodedNode *N,
                     BugReport &BR);
 };
 
+namespace bugreporter {
+
+/// Specifies the type of tracking for an expression.
+enum class TrackingKind {
+  /// Default tracking kind -- specifies that as much information should be
+  /// gathered about the tracked expression value as possible.
+  Thorough,
+  /// Specifies that a more moderate tracking should be used for the expression
+  /// value. This will essentially make sure that functions relevant to the it
+  /// aren't pruned, but otherwise relies on the user reading the code or
+  /// following the arrows.
+  Condition
+};
+
+/// Attempts to add visitors to track expression value back to its point of
+/// origin.
+///
+/// \param N A node "downstream" from the evaluation of the statement.
+/// \param E The expression value which we are tracking
+/// \param R The bug report to which visitors should be attached.
+/// \param EnableNullFPSuppression Whether we should employ false positive
+///         suppression (inlined defensive checks, returned null).
+///
+/// \return Whether or not the function was able to add visitors for this
+///         statement. Note that returning \c true does not actually imply
+///         that any visitors were added.
+bool trackExpressionValue(const ExplodedNode *N, const Expr *E, BugReport &R,
+                          TrackingKind TKind = TrackingKind::Thorough,
+                          bool EnableNullFPSuppression = true);
+
+const Expr *getDerefExpr(const Stmt *S);
+
+} // namespace bugreporter
+
 /// Finds last store into the given region,
 /// which is different from a given symbolic value.
 class FindLastStoreBRVisitor final : public BugReporterVisitor {
   const MemRegion *R;
   SVal V;
   bool Satisfied = false;
 
   /// If the visitor is tracking the value directly responsible for the
   /// bug, we are going to employ false positive suppression.
   bool EnableNullFPSuppression;
 
+  using TrackingKind = bugreporter::TrackingKind;
+  TrackingKind TKind;
+
 public:
   /// Creates a visitor for every VarDecl inside a Stmt and registers it with
   /// the BugReport.
   static void registerStatementVarDecls(BugReport &BR, const Stmt *S,
-                                        bool EnableNullFPSuppression);
+                                        bool EnableNullFPSuppression,
+                                        TrackingKind TKind);
 
+  /// \param V We're searching for the store where \c R received this value.
+  /// \param R The region we're tracking.
+  /// \param EnableNullFPSuppression Whether we should employ false positive
+  ///         suppression (inlined defensive checks, returned null).
+  /// \param TKind May limit the amount of notes added to the bug report.
   FindLastStoreBRVisitor(KnownSVal V, const MemRegion *R,
-                         bool InEnableNullFPSuppression)
-      : R(R), V(V), EnableNullFPSuppression(InEnableNullFPSuppression) {}
+                         bool InEnableNullFPSuppression,
+                         TrackingKind TKind)
+      : R(R), V(V), EnableNullFPSuppression(InEnableNullFPSuppression),
+        TKind(TKind) {
+    assert(R);
+  }
 
   void Profile(llvm::FoldingSetNodeID &ID) const override;
 
   std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
                                                  BugReporterContext &BRC,
                                                  BugReport &BR) override;
 };
 

 public:
   void Profile(llvm::FoldingSetNodeID &ID) const override;
 
   std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
                                                  BugReporterContext &BRC,
                                                  BugReport &R) override;
 };
 
-namespace bugreporter {
-
-/// Attempts to add visitors to track expression value back to its point of
-/// origin.
-///
-/// \param N A node "downstream" from the evaluation of the statement.
-/// \param E The expression value which we are tracking
-/// \param R The bug report to which visitors should be attached.
-/// \param EnableNullFPSuppression Whether we should employ false positive
-///         suppression (inlined defensive checks, returned null).
-///
-/// \return Whether or not the function was able to add visitors for this
-///         statement. Note that returning \c true does not actually imply
-///         that any visitors were added.
-bool trackExpressionValue(const ExplodedNode *N, const Expr *E, BugReport &R,
-                          bool EnableNullFPSuppression = true);
-
-const Expr *getDerefExpr(const Stmt *S);
-
-} // namespace bugreporter
-
 } // namespace ento
 
 } // namespace clang
 
 #endif // LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_BUGREPORTERVISITORS_H

clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp

void MIGChecker::checkReturnAux(const ReturnStmt *RS, CheckerContext &C) const {
   auto R = llvm::make_unique<BugReport>(
       BT,
       "MIG callback fails with error after deallocating argument value. "
       "This is a use-after-free vulnerability because the caller will try to "
       "deallocate it again",
       N);
 
   R->addRange(RS->getSourceRange());
-  bugreporter::trackExpressionValue(N, RS->getRetValue(), *R, false);
+  bugreporter::trackExpressionValue(N, RS->getRetValue(), *R,
+                                    bugreporter::TrackingKind::Thorough, false);
   C.emitReport(std::move(R));
 }
 
 void ento::registerMIGChecker(CheckerManager &Mgr) {
   Mgr.registerChecker<MIGChecker>();
 }
 
 bool ento::shouldRegisterMIGChecker(const LangOptions &LO) {
   return true;
 }

clang/lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp

if (Name.equals("CFArrayGetValueAtIndex")) {
     ProgramStateRef StOutBound = State->assumeInBound(Idx, *Size, false, T);
     if (StOutBound && !StInBound) {
       ExplodedNode *N = C.generateErrorNode(StOutBound);
       if (!N)
         return;
       initBugType();
       auto R = llvm::make_unique<BugReport>(*BT, "Index is out of bounds", N);
       R->addRange(IdxExpr->getSourceRange());
-      bugreporter::trackExpressionValue(N, IdxExpr, *R,
-                                        /*EnableNullFPSuppression=*/false);
+      bugreporter::trackExpressionValue(
+          N, IdxExpr, *R, bugreporter::TrackingKind::Thorough, false);
       C.emitReport(std::move(R));
       return;
     }
   }
 }
 
 ProgramStateRef
 ObjCContainersChecker::checkPointerEscape(ProgramStateRef State,

clang/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp

if (Optional<UndefinedVal> V =
 
         os << "Variable '" << VD->getName()
            << "' is uninitialized when captured by block";
 
         auto R = llvm::make_unique<BugReport>(*BT, os.str(), N);
         if (const Expr *Ex = FindBlockDeclRefExpr(BE->getBody(), VD))
           R->addRange(Ex->getSourceRange());
         R->addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
-            *V, VR, /*EnableNullFPSuppression*/ false));
+            *V, VR, /*EnableNullFPSuppression*/ false,
+            bugreporter::TrackingKind::Thorough));
         R->disablePathPruning();
         // need location of block
         C.emitReport(std::move(R));
       }
     }
   }
 }
 
 void ento::registerUndefCapturedBlockVarChecker(CheckerManager &mgr) {
   mgr.registerChecker<UndefCapturedBlockVarChecker>();
 }
 
 bool ento::shouldRegisterUndefCapturedBlockVarChecker(const LangOptions &LO) {
   return true;
 }

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

 
 static const Expr *peelOffPointerArithmetic(const BinaryOperator *B) {
   if (B->isAdditiveOp() && B->getType()->isPointerType()) {
     if (B->getLHS()->getType()->isPointerType()) {
       return B->getLHS();
     } else if (B->getRHS()->getType()->isPointerType()) {
       return B->getRHS();
     }
   }
   return nullptr;
 }
 
 /// Given that expression S represents a pointer that would be dereferenced,
 /// try to find a sub-expression from which the pointer came from.
 /// This is used for tracking down origins of a null or undefined value:
 /// "this is null because that is null because that is null" etc.
 /// We wipe away field and element offsets because they merely add offsets.
enum {
     Initial,
     MaybeUnsuppress,
     Satisfied
   } Mode = Initial;
 
   bool EnableNullFPSuppression;
   bool ShouldInvalidate = true;
   AnalyzerOptions& Options;
+  bugreporter::TrackingKind TKind;
 
 public:
-  ReturnVisitor(const StackFrameContext *Frame,
-                bool Suppressed,
-                AnalyzerOptions &Options)
+  ReturnVisitor(const StackFrameContext *Frame, bool Suppressed,
+                AnalyzerOptions &Options, bugreporter::TrackingKind TKind)
       : CalleeSFC(Frame), EnableNullFPSuppression(Suppressed),
-        Options(Options) {}
+        Options(Options), TKind(TKind) {}
 
   static void *getTag() {
     static int Tag = 0;
     return static_cast<void *>(&Tag);
   }
 
   void Profile(llvm::FoldingSetNodeID &ID) const override {
     ID.AddPointer(ReturnVisitor::getTag());
     ID.AddPointer(CalleeSFC);
     ID.AddBoolean(EnableNullFPSuppression);
   }
 
   /// Adds a ReturnVisitor if the given statement represents a call that was
   /// inlined.
   ///
   /// This will search back through the ExplodedGraph, starting from the given
   /// node, looking for when the given statement was processed. If it turns out
   /// 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,
-                                    bool InEnableNullFPSuppression) {
+                                    bool InEnableNullFPSuppression,
+                                    bugreporter::TrackingKind TKind) {
     if (!CallEvent::isCallStmt(S))
       return;
 
     // First, find when we processed the statement.
     // If we work with a 'CXXNewExpr' that is going to be purged away before
     // its call take place. We would catch that purge in the last condition
     // as a 'StmtPoint' so we have to bypass it.
     const bool BypassCXXNewExprEval = isa<CXXNewExpr>(S);
static void addVisitorIfNecessary(const ExplodedNode *Node, const Stmt *S,
     bool EnableNullFPSuppression = false;
     if (InEnableNullFPSuppression &&
         Options.ShouldSuppressNullReturnPaths)
       if (Optional<Loc> RetLoc = RetVal.getAs<Loc>())
         EnableNullFPSuppression = State->isNull(*RetLoc).isConstrainedTrue();
 
     BR.addVisitor(llvm::make_unique<ReturnVisitor>(CalleeContext,
                                                    EnableNullFPSuppression,
-                                                   Options));
+                                                   Options, TKind));
   }
 
   std::shared_ptr<PathDiagnosticPiece>
   visitNodeInitial(const ExplodedNode *N,
                    BugReporterContext &BRC, BugReport &BR) {
     // Only print a message at the interesting return statement.
     if (N->getLocationContext() != CalleeSFC)
       return nullptr;
visitNodeInitial(const ExplodedNode *N,
 
     // Ignore aggregate rvalues.
     if (V.getAs<nonloc::LazyCompoundVal>() ||
         V.getAs<nonloc::CompoundVal>())
       return nullptr;
 
     RetE = RetE->IgnoreParenCasts();
 
-    // If we're returning 0, we should track where that 0 came from.
-    bugreporter::trackExpressionValue(N, RetE, BR, EnableNullFPSuppression);
+    // Let's track the return value.
+    bugreporter::trackExpressionValue(
+        N, RetE, BR, TKind, EnableNullFPSuppression);
 
     // Build an appropriate message based on the return value.
     SmallString<64> Msg;
     llvm::raw_svector_ostream Out(Msg);
 
     bool WouldEventBeMeaningless = false;
 
     if (State->isNull(V).isConstrainedTrue()) {
for (unsigned I = 0, E = Call->getNumArgs(); I != E; ++I) {
       const Expr *ArgE = Call->getArgExpr(I);
       if (!ArgE)
         continue;
 
       // Is it possible for this argument to be non-null?
       if (!State->isNull(*ArgV).isConstrainedTrue())
         continue;
 
-      if (bugreporter::trackExpressionValue(N, ArgE, BR, EnableNullFPSuppression))
+      if (trackExpressionValue(N, ArgE, BR, TKind, EnableNullFPSuppression))
         ShouldInvalidate = false;
 
       // If we /can't/ track the null pointer, we should err on the side of
       // false negatives, and continue towards marking this report invalid.
       // (We will still look at the other arguments, though.)
     }
 
     return nullptr;

 // Implementation of FindLastStoreBRVisitor.
 //===----------------------------------------------------------------------===//
 
 void FindLastStoreBRVisitor::Profile(llvm::FoldingSetNodeID &ID) const {
   static int tag = 0;
   ID.AddPointer(&tag);
   ID.AddPointer(R);
   ID.Add(V);
+  ID.AddInteger(static_cast<int>(TKind));
   ID.AddBoolean(EnableNullFPSuppression);
 }
 
+void FindLastStoreBRVisitor::registerStatementVarDecls(
+    BugReport &BR, const Stmt *S, bool EnableNullFPSuppression,
+    TrackingKind TKind) {
+
+  const ExplodedNode *N = BR.getErrorNode();
+  std::deque<const Stmt *> WorkList;
+  WorkList.push_back(S);
+
+  while (!WorkList.empty()) {
+    const Stmt *Head = WorkList.front();
+    WorkList.pop_front();
+
+    ProgramStateManager &StateMgr = N->getState()->getStateManager();
+
+    if (const auto *DR = dyn_cast<DeclRefExpr>(Head)) {
+      if (const auto *VD = dyn_cast<VarDecl>(DR->getDecl())) {
+        const VarRegion *R =
+        StateMgr.getRegionManager().getVarRegion(VD, N->getLocationContext());
+
+        // What did we load?
+        SVal V = N->getSVal(S);
+
+        if (V.getAs<loc::ConcreteInt>() || V.getAs<nonloc::ConcreteInt>()) {
+          // Register a new visitor with the BugReport.
+          BR.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
+              V.castAs<KnownSVal>(), R, EnableNullFPSuppression, TKind));
+        }
+      }
+    }
+
+    for (const Stmt *SubStmt : Head->children())
+      WorkList.push_back(SubStmt);
+  }
+}
+
 /// Returns true if \p N represents the DeclStmt declaring and initializing
 /// \p VR.
 static bool isInitializationOfVar(const ExplodedNode *N, const VarRegion *VR) {
   Optional<PostStmt> P = N->getLocationAs<PostStmt>();
   if (!P)
     return false;
 
   const DeclStmt *DS = P->getStmtAs<DeclStmt>();
if (isInitializationOfVar(Pred, VR)) {
       InitE = VR->getDecl()->getInit();
     }
   }
 
   // If this is a post initializer expression, initializing the region, we
   // should track the initializer expression.
   if (Optional<PostInitializer> PIP = Pred->getLocationAs<PostInitializer>()) {
     const MemRegion *FieldReg = (const MemRegion *)PIP->getLocationValue();
-    if (FieldReg && FieldReg == R) {
+    if (FieldReg == R) {
       StoreSite = Pred;
       InitE = PIP->getInitializer()->getInit();
     }
   }
 
   // Otherwise, see if this is the store site:
   // (1) Succ has this binding and Pred does not, i.e. this is
   //     where the binding first occurred.
FindLastStoreBRVisitor::VisitNode(const ExplodedNode *Succ,
   Satisfied = true;
 
   // If we have an expression that provided the value, try to track where it
   // came from.
   if (InitE) {
     if (!IsParam)
       InitE = InitE->IgnoreParenCasts();
 
-    bugreporter::trackExpressionValue(StoreSite, InitE, BR,
-                                      EnableNullFPSuppression);
+    bugreporter::trackExpressionValue(
+        StoreSite, InitE, BR, TKind, EnableNullFPSuppression);
   }
 
   // Okay, we've found the binding. Emit an appropriate message.
   SmallString<256> sbuf;
   llvm::raw_svector_ostream os(sbuf);
 
   if (Optional<PostStmt> PS = StoreSite->getLocationAs<PostStmt>()) {
     const Stmt *S = PS->getStmt();
if (DS) {
         // See if we can get the BlockVarRegion.
         ProgramStateRef State = StoreSite->getState();
         SVal V = StoreSite->getSVal(S);
         if (const auto *BDR =
               dyn_cast_or_null<BlockDataRegion>(V.getAsRegion())) {
           if (const VarRegion *OriginalR = BDR->getOriginalRegion(VR)) {
             if (auto KV = State->getSVal(OriginalR).getAs<KnownSVal>())
               BR.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
-                  *KV, OriginalR, EnableNullFPSuppression));
+                  *KV, OriginalR, EnableNullFPSuppression, TKind));
           }
         }
       }
     }
     if (action)
       showBRDiagnostics(action, os, R, V, DS);
 
   } else if (StoreSite->getLocation().getAs<CallEnter>()) {
TrackControlDependencyCondBRVisitor::VisitNode(const ExplodedNode *N,
 
   if (ControlDeps.isControlDependent(OriginB, NB)) {
     if (const Expr *Condition = NB->getLastCondition()) {
       // Keeping track of the already tracked conditions on a visitor level
       // isn't sufficient, because a new visitor is created for each tracked
       // expression, hence the BugReport level set.
       if (BR.addTrackedCondition(N)) {
         bugreporter::trackExpressionValue(
-            N, Condition, BR, /*EnableNullFPSuppression=*/false);
+            N, Condition, BR, bugreporter::TrackingKind::Condition,
+            /*EnableNullFPSuppression=*/false);
         return constructDebugPieceForTrackedCondition(Condition, N, BRC);
       }
     }
   }
 
   return nullptr;
 }
 
if (PathDiagnosticLocation::getStmt(N) == Inner)
       return N;
     N = N->getFirstPred();
   }
   return N;
 }
 
 bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode,
                                        const Expr *E, BugReport &report,
+                                       bugreporter::TrackingKind TKind,
                                        bool EnableNullFPSuppression) {
+
   if (!E || !InputNode)
     return false;
 
   const Expr *Inner = peelOffOuterExpr(E, InputNode);
   const ExplodedNode *LVNode = findNodeForExpression(InputNode, Inner);
   if (!LVNode)
     return false;
 
   ProgramStateRef LVState = LVNode->getState();
 
   // We only track expressions if we believe that they are important. Chances
   // are good that control dependencies to the tracking point are also improtant
   // because of this, let's explain why we believe control reached this point.
   // TODO: Shouldn't we track control dependencies of every bug location, rather
   // than only tracked expressions?
   if (LVState->getAnalysisManager().getAnalyzerOptions().ShouldTrackConditions)
     report.addVisitor(llvm::make_unique<TrackControlDependencyCondBRVisitor>(
           InputNode));
 
   // 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(Inner, LVNode))
-    trackExpressionValue(LVNode, Receiver, report, EnableNullFPSuppression);
+    trackExpressionValue(
+        LVNode, Receiver, report, TKind, EnableNullFPSuppression);
 
   // Track the index if this is an array subscript.
   if (const auto *Arr = dyn_cast<ArraySubscriptExpr>(Inner))
     trackExpressionValue(
-        LVNode, Arr->getIdx(), report, /*EnableNullFPSuppression*/ false);
+        LVNode, Arr->getIdx(), report, TKind, /*EnableNullFPSuppression*/false);
 
   // 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 (ExplodedGraph::isInterestingLValueExpr(Inner)) {
     SVal LVal = LVNode->getSVal(Inner);
 
     const MemRegion *RR = getLocationRegionIfReference(Inner, LVNode);
     bool LVIsNull = LVState->isNull(LVal).isConstrainedTrue();
 
     // If this is a C++ reference to a null pointer, we are tracking the
     // pointer. In addition, we should find the store at which the reference
     // got initialized.
     if (RR && !LVIsNull)
       if (auto KV = LVal.getAs<KnownSVal>())
         report.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
-              *KV, RR, EnableNullFPSuppression));
+              *KV, RR, EnableNullFPSuppression, TKind));
 
     // In case of C++ references, we want to differentiate between a null
     // reference and reference to null pointer.
     // If the LVal is null, check if we are dealing with null reference.
     // For those, we want to track the location of the reference.
     const MemRegion *R = (RR && LVIsNull) ? RR :
         LVNode->getSVal(Inner).getAsRegion();
 
if (R) {
         if (!DV->isZeroConstant() && LVState->isNull(*DV).isConstrainedTrue() &&
             EnableNullFPSuppression)
           report.addVisitor(
               llvm::make_unique<SuppressInlineDefensiveChecksVisitor>(*DV,
                                                                       LVNode));
 
       if (auto KV = V.getAs<KnownSVal>())
         report.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
-              *KV, R, EnableNullFPSuppression));
+              *KV, R, EnableNullFPSuppression, TKind));
       return true;
     }
   }
 
   // If the expression is not an "lvalue expression", we can still
   // track the constraints on its contents.
   SVal V = LVState->getSValAsScalarOrLoc(Inner, LVNode->getLocationContext());
 
   ReturnVisitor::addVisitorIfNecessary(
-    LVNode, Inner, report, EnableNullFPSuppression);
+    LVNode, Inner, report, EnableNullFPSuppression, TKind);
 
   // Is it a symbolic value?
   if (auto L = V.getAs<loc::MemRegionVal>()) {
     report.addVisitor(llvm::make_unique<UndefOrNullArgVisitor>(L->getRegion()));
 
     // FIXME: this is a hack for fixing a later crash when attempting to
     // dereference a void* pointer.
     // We should not try to dereference pointers at all when we don't care
if (auto L = V.getAs<loc::MemRegionVal>()) {
     if (ExplodedGraph::isInterestingLValueExpr(Inner))
       RVal = LVState->getRawSVal(L.getValue(), Inner->getType());
     else if (CanDereference)
       RVal = LVState->getSVal(L->getRegion());
 
     if (CanDereference)
       if (auto KV = RVal.getAs<KnownSVal>())
         report.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
-            *KV, L->getRegion(), EnableNullFPSuppression));
+            *KV, L->getRegion(), EnableNullFPSuppression, TKind));
 
     const MemRegion *RegionRVal = RVal.getAsRegion();
     if (RegionRVal && isa<SymbolicRegion>(RegionRVal)) {
       report.markInteresting(RegionRVal);
       report.addVisitor(llvm::make_unique<TrackConstraintBRVisitor>(
             loc::MemRegionVal(RegionRVal), /*assumption=*/false));
     }
   }
NilReceiverBRVisitor::VisitNode(const ExplodedNode *N,
   else {
     OS << "No method is called";
   }
   OS << " because the receiver is nil";
 
   // 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::trackExpressionValue(N, Receiver, BR,
-                               /*EnableNullFPSuppression*/ false);
+  bugreporter::trackExpressionValue(
+      N, Receiver, BR, bugreporter::TrackingKind::Thorough,
   // Issue a message saying that the method was skipped.
   PathDiagnosticLocation L(Receiver, BRC.getSourceManager(),
                                      N->getLocationContext());
   return std::make_shared<PathDiagnosticEventPiece>(L, OS.str());
 }
 
 //===----------------------------------------------------------------------===//
-// Implementation of FindLastStoreBRVisitor.
-//===----------------------------------------------------------------------===//
-
-// Registers every VarDecl inside a Stmt with a last store visitor.
-void FindLastStoreBRVisitor::registerStatementVarDecls(BugReport &BR,
-                                                const Stmt *S,
-                                                bool EnableNullFPSuppression) {
-  const ExplodedNode *N = BR.getErrorNode();
-  std::deque<const Stmt *> WorkList;
-  WorkList.push_back(S);
-
-  while (!WorkList.empty()) {
-    const Stmt *Head = WorkList.front();
-    WorkList.pop_front();
-
-    ProgramStateManager &StateMgr = N->getState()->getStateManager();
-
-    if (const auto *DR = dyn_cast<DeclRefExpr>(Head)) {
-      if (const auto *VD = dyn_cast<VarDecl>(DR->getDecl())) {
-        const VarRegion *R =
-        StateMgr.getRegionManager().getVarRegion(VD, N->getLocationContext());
-
-        // What did we load?
-        SVal V = N->getSVal(S);
-
-        if (V.getAs<loc::ConcreteInt>() || V.getAs<nonloc::ConcreteInt>()) {
-          // Register a new visitor with the BugReport.
-          BR.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
-              V.castAs<KnownSVal>(), R, EnableNullFPSuppression));
-        }
-      }
-    }
-
-    for (const Stmt *SubStmt : Head->children())
-      WorkList.push_back(SubStmt);
-  }
-}
-
-//===----------------------------------------------------------------------===//
 // Visitor that tries to report interesting diagnostics from conditions.
 //===----------------------------------------------------------------------===//
 
 /// Return the tag associated with this visitor.  This tag will be used
 /// to make all PathDiagnosticPieces created by this visitor.
 const char *ConditionBRVisitor::getTag() {
   return "ConditionBRVisitor";
 }