[analyzer] Refactor StoreSiteFinder and extract DefaultStoreHandler

[vsavchenko]

Summary

After this patch, custom StoreHandlers will also work as expected.

[vsavchenko]

Rebase

[NoQ]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

2047

WDYM?

Also maybe downscope it?

2063

Isn't DS always null here?

[vsavchenko]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

2047

I meant that the only client for it is undef value printer and we can probably get rid of it altogether, or at least what I did in the very last patch from the stack.

2063

Duh! Yes!
Good catch, I'll remove DS declaration!

[vsavchenko]

Fix review remarks

[NoQ]

Great, thanks!

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

struct StoreInfo {
   enum Kind {
     /// The value got stored into the region during initialization:
     ///   int x = 42;
     Initialization,
     /// The value got stored into the region during assignment:
     ///   int x;
     ///   x = 42;
     Assignment,
+    /// The value got stored into the parameter region as the result
+    /// of a call.
+    CallArgument,
     /// The value got stored into the region as block capture.
     /// Block data is modeled as a separate region, thus whenever
     /// the analyzer sees a captured variable, its value is copied
     /// into a special block region.
     BlockCapture
   };
 
   /// The type of store operation.
   Kind StoreKind;
   /// The node where the store happened.
   const ExplodedNode *StoreSite;
   /// The expression where the value comes from.
   /// NOTE: might be null.
-  Expr *SourceOfTheValue;
+  const Expr *SourceOfTheValue;
   /// Symbolic value that is being stored.
   SVal Value;
   /// Memory regions involved in the store operation.
   ///   Dest <- Origin
   /// NOTE: Origin might be null, when the stored value doesn't come
   ///       from another region.
   const MemRegion *Dest, *Origin;
 };
virtual Result track(SVal V, const MemRegion *R, TrackingOptions Opts = {},
                        const StackFrameContext *Origin = nullptr);
 
   /// Handle the store operation and produce the note.
   ///
   /// \param SI The information fully describing the store.
   /// \param Opts Tracking options specifying how we got to it.
   ///
   /// NOTE: this method is designed for sub-trackers and visitors.
-  virtual PathDiagnosticPieceRef handle(StoreInfo SI, TrackingOptions Opts);
+  virtual PathDiagnosticPieceRef handle(StoreInfo SI, BugReporterContext &BRC,
+                                        TrackingOptions Opts);
 
   /// Add custom expression handler with the highest priority.
   ///
   /// It means that it will be asked for handling first, and can prevent
   /// other handlers from running if decides to interrupt.
   void addHighPriorityHandler(ExpressionHandlerPtr SH) {
     ExpressionHandlers.push_front(std::move(SH));
   }
public:
 
   /// Handle the given store and produce the node.
   ///
   /// \param SI The information fully describing the store.
   /// \param Opts Tracking options specifying how we are tracking the value.
   ///
   /// \return the produced note, null if the handler doesn't support this kind
   ///         of stores.
-  virtual PathDiagnosticPieceRef handle(StoreInfo SI, TrackingOptions Opts) = 0;
+  virtual PathDiagnosticPieceRef handle(StoreInfo SI, BugReporterContext &BRC,
+                                        TrackingOptions Opts) = 0;
 
   Tracker &getParentTracker() { return ParentTracker; }
 };
 
 /// Visitor that tracks expressions and values.
 class TrackingBugReporterVisitor : public BugReporterVisitor {
 private:
   TrackerRef ParentTracker;

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

 
 /// Finds last store into the given region,
 /// which is different from a given symbolic value.
 class StoreSiteFinder final : public TrackingBugReporterVisitor {
   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;
+  TrackingOptions Options;
   const StackFrameContext *OriginSFC;
 
 public:
   /// \param V We're searching for the store where \c R received this value.
   /// \param R The region we're tracking.
   /// \param TKind May limit the amount of notes added to the bug report.
   /// \param OriginSFC Only adds notes when the last store happened in a
   ///        different stackframe to this one. Disregarded if the tracking kind
   ///        is thorough.
   ///        This is useful, because for non-tracked regions, notes about
   ///        changes to its value in a nested stackframe could be pruned, and
   ///        this visitor can prevent that without polluting the bugpath too
   ///        much.
   StoreSiteFinder(bugreporter::TrackerRef ParentTracker, KnownSVal V,
-                  const MemRegion *R, bool InEnableNullFPSuppression,
-                  TrackingKind TKind,
+                  const MemRegion *R, TrackingOptions Options,
                   const StackFrameContext *OriginSFC = nullptr)
-      : TrackingBugReporterVisitor(ParentTracker), R(R), V(V),
-        EnableNullFPSuppression(InEnableNullFPSuppression), TKind(TKind),
+      : TrackingBugReporterVisitor(ParentTracker), R(R), V(V), Options(Options),
         OriginSFC(OriginSFC) {
     assert(R);
   }
 
   void Profile(llvm::FoldingSetNodeID &ID) const override;
 
   PathDiagnosticPieceRef VisitNode(const ExplodedNode *N,
                                    BugReporterContext &BRC,
                                    PathSensitiveBugReport &BR) override;
 };
 
 void StoreSiteFinder::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);
+  ID.AddInteger(static_cast<int>(Options.Kind));
+  ID.AddBoolean(Options.EnableNullFPSuppression);
 }
 
 /// 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;
PathDiagnosticPieceRef StoreSiteFinder::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();
 
-    getParentTracker().track(InitE, StoreSite,
-                             {TKind, EnableNullFPSuppression});
+    getParentTracker().track(InitE, StoreSite, Options);
   }
 
   // Let's try to find the region where the value came from.
   const MemRegion *OldRegion = nullptr;
 
   // If we have init expression, it might be simply a reference
   // to a variable, so we can use it.
   if (InitE) {
if (NodeWithoutBinding) {
       // correct, can be confusing.
       StoreManager::FindUniqueBinding FB(V.getAsLocSymbol());
       BRC.getStateManager().iterBindings(NodeWithoutBinding->getState(), FB);
       if (FB)
         OldRegion = FB.getRegion();
     }
   }
 
-  if (TKind == TrackingKind::Condition &&
+  if (Options.Kind == TrackingKind::Condition && OriginSFC &&
       !OriginSFC->isParentOf(StoreSite->getStackFrame()))
     return nullptr;
 
   // Okay, we've found the binding. Emit an appropriate message.
   SmallString<256> sbuf;
   llvm::raw_svector_ostream os(sbuf);
 
+  StoreInfo SI = {StoreInfo::Assignment, // default kind
+                  StoreSite,
+                  InitE,
+                  V,
+                  R,
+                  OldRegion};
+
   if (Optional<PostStmt> PS = StoreSite->getLocationAs<PostStmt>()) {
     const Stmt *S = PS->getStmt();
-    const char *action = nullptr;
     const auto *DS = dyn_cast<DeclStmt>(S);
     const auto *VR = dyn_cast<VarRegion>(R);
 
     if (DS) {
-      action = R->canPrintPretty() ? "initialized to " :
-                                     "Initializing to ";
+      SI.StoreKind = StoreInfo::Initialization;
     } else if (isa<BlockExpr>(S)) {
-      action = R->canPrintPretty() ? "captured by block as " :
-                                     "Captured by block as ";
+      SI.StoreKind = StoreInfo::BlockCapture;
       if (VR) {
         // 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>())
-              getParentTracker().track(
-                  *KV, OriginalR, {TKind, EnableNullFPSuppression}, OriginSFC);
+            getParentTracker().track(State->getSVal(OriginalR), OriginalR,
+                                     Options, OriginSFC);
           }
         }
       }
     }
-    if (action)
-      showBRDiagnostics(action, os, R, V, OldRegion, DS);
-
-  } else if (StoreSite->getLocation().getAs<CallEnter>()) {
-    if (const auto *VR = dyn_cast<VarRegion>(R))
-      showBRParamDiagnostics(os, VR, V, OldRegion);
+  } else if (SI.StoreSite->getLocation().getAs<CallEnter>() &&
+             isa<VarRegion>(SI.Dest)) {
+    SI.StoreKind = StoreInfo::CallArgument;
   }
 
-  if (os.str().empty())
-    showBRDefaultDiagnostics(os, R, V, OldRegion);
-
-  if (TKind == bugreporter::TrackingKind::Condition)
-    os << WillBeUsedForACondition;
-
-  // Construct a new PathDiagnosticPiece.
-  ProgramPoint P = StoreSite->getLocation();
-  PathDiagnosticLocation L;
-  if (P.getAs<CallEnter>() && InitE)
-    L = PathDiagnosticLocation(InitE, BRC.getSourceManager(),
-                               P.getLocationContext());
-
-  if (!L.isValid() || !L.asLocation().isValid())
-    L = PathDiagnosticLocation::create(P, BRC.getSourceManager());
-
-  if (!L.isValid() || !L.asLocation().isValid())
-    return nullptr;
-
-  return std::make_shared<PathDiagnosticEventPiece>(L, os.str());
+  return getParentTracker().handle(SI, BRC, Options);
 }
 
 //===----------------------------------------------------------------------===//
 // Implementation of TrackConstraintBRVisitor.
 //===----------------------------------------------------------------------===//
 
 void TrackConstraintBRVisitor::Profile(llvm::FoldingSetNodeID &ID) const {
   static int tag = 0;
static const ExplodedNode* findNodeForExpression(const ExplodedNode *N,
   }
   return N;
 }
 
 //===----------------------------------------------------------------------===//
 //                            Tracker implementation
 //===----------------------------------------------------------------------===//
 
+class DefaultStoreHandler final : public StoreHandler {
+public:
+  using StoreHandler::StoreHandler;
+
+  PathDiagnosticPieceRef handle(StoreInfo SI, BugReporterContext &BRC,
+                                TrackingOptions Opts) override {
+    // Okay, we've found the binding. Emit an appropriate message.
+    SmallString<256> Buffer;
+    llvm::raw_svector_ostream OS(Buffer);
+    const char *Action = nullptr;
+
+    switch (SI.StoreKind) {
+    case StoreInfo::Initialization:
+      // We don't need to check here, all these conditions were
+      // checked by StoreSiteFinder, when it figured out that it is
+      // initialization.
+      Action =
+          SI.Dest->canPrintPretty() ? "initialized to " : "Initializing to ";
+      showBRDiagnostics(
+          Action, OS, SI.Dest, SI.Value, SI.Origin,
+          cast<DeclStmt>(SI.StoreSite->getLocationAs<PostStmt>()->getStmt()));
+      break;
+    case StoreInfo::BlockCapture:
+      Action = SI.Dest->canPrintPretty() ? "captured by block as "
+                                         : "Captured by block as ";
+      showBRDiagnostics(Action, OS, SI.Dest, SI.Value, SI.Origin, nullptr);
+      break;
+    case StoreInfo::CallArgument:
+      showBRParamDiagnostics(OS, cast<VarRegion>(SI.Dest), SI.Value, SI.Origin);
+      break;
+    case StoreInfo::Assignment:
+      showBRDefaultDiagnostics(OS, SI.Dest, SI.Value, SI.Origin);
+      break;
+    }
+
+    if (Opts.Kind == bugreporter::TrackingKind::Condition)
+      OS << WillBeUsedForACondition;
+
+    // Construct a new PathDiagnosticPiece.
+    ProgramPoint P = SI.StoreSite->getLocation();
+    PathDiagnosticLocation L;
+    if (P.getAs<CallEnter>() && SI.SourceOfTheValue)
+      L = PathDiagnosticLocation(SI.SourceOfTheValue, BRC.getSourceManager(),
+                                 P.getLocationContext());
+
+    if (!L.isValid() || !L.asLocation().isValid())
+      L = PathDiagnosticLocation::create(P, BRC.getSourceManager());
+
+    if (!L.isValid() || !L.asLocation().isValid())
+      return nullptr;
+
+    return std::make_shared<PathDiagnosticEventPiece>(L, OS.str());
+  }
+};
+
 class DefaultExpressionHandler final : public ExpressionHandler {
 public:
   using ExpressionHandler::ExpressionHandler;
 
   Tracker::Result handle(const Expr *Inner, const ExplodedNode *InputNode,
                          const ExplodedNode *LVNode,
                          TrackingOptions Opts) override {
     ProgramStateRef LVState = LVNode->getState();
if (BO->getOpcode() == BO_Mul) {
         track(BO->getLHS());
     }
 
     return CombinedResult;
   }
 };
 
 Tracker::Tracker(PathSensitiveBugReport &Report) : Report(Report) {
+  // Default expression handlers.
   addHighPriorityHandler<DefaultExpressionHandler>();
   addLowPriorityHandler<PRValueHandler>();
-  // TODO: split trackExpressionValue and FindLastStoreBRVisitor into handlers
-  //       and add them here.
+  // Default store handlers.
+  addHighPriorityHandler<DefaultStoreHandler>();
 }
 
 Tracker::Result Tracker::track(const Expr *E, const ExplodedNode *N,
                                TrackingOptions Opts) {
   if (!E || !N)
     return {};
 
   const Expr *Inner = peelOffOuterExpr(E, N);
Tracker::Result Tracker::track(const Expr *E, const ExplodedNode *N,
   }
 
   return CombinedResult;
 }
 
 Tracker::Result Tracker::track(SVal V, const MemRegion *R, TrackingOptions Opts,
                                const StackFrameContext *Origin) {
   if (auto KV = V.getAs<KnownSVal>()) {
-    Report.addVisitor<StoreSiteFinder>(
-        this, *KV, R, Opts.EnableNullFPSuppression, Opts.Kind, Origin);
+    Report.addVisitor<StoreSiteFinder>(this, *KV, R, Opts, Origin);
     return {true};
   }
   return {};
 }
 
-PathDiagnosticPieceRef Tracker::handle(StoreInfo SI, TrackingOptions Opts) {
+PathDiagnosticPieceRef Tracker::handle(StoreInfo SI, BugReporterContext &BRC,
+                                       TrackingOptions Opts) {
   // Iterate through the handlers in the order according to their priorities.
   for (StoreHandlerPtr &Handler : StoreHandlers) {
-    if (PathDiagnosticPieceRef Result = Handler->handle(SI, Opts))
+    if (PathDiagnosticPieceRef Result = Handler->handle(SI, BRC, Opts))
       // If the handler produced a non-null piece, return it.
       // There is no need in asking other handlers.
       return Result;
   }
   return {};
 }
 
 bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode,