[analyzer] NFC: Re-implement stack hints as a side map in BugReport.

That's one of the few random entities in the PathDiagnostic interface that
are specific to the Static Analyzer. By moving them out we could let
everybody use path diagnostics without linking against Static Analyzer.

Differential Revision: https://reviews.llvm.org/D67381

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@371658 91177308-0d34-0410-b5e6-96231b3b80d8

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

@@ -70,6 +70,50 @@ class SValBuilder;
 using DiagnosticForConsumerMapTy =
     llvm::DenseMap<PathDiagnosticConsumer *, std::unique_ptr<PathDiagnostic>>;
 
+/// Interface for classes constructing Stack hints.
+///
+/// If a PathDiagnosticEvent occurs in a different frame than the final
+/// diagnostic the hints can be used to summarize the effect of the call.
+class StackHintGenerator {
+public:
+  virtual ~StackHintGenerator() = 0;
+
+  /// Construct the Diagnostic message for the given ExplodedNode.
+  virtual std::string getMessage(const ExplodedNode *N) = 0;
+};
+
+/// Constructs a Stack hint for the given symbol.
+///
+/// The class knows how to construct the stack hint message based on
+/// traversing the CallExpr associated with the call and checking if the given
+/// symbol is returned or is one of the arguments.
+/// The hint can be customized by redefining 'getMessageForX()' methods.
+class StackHintGeneratorForSymbol : public StackHintGenerator {
+private:
+  SymbolRef Sym;
+  std::string Msg;
+
+public:
+  StackHintGeneratorForSymbol(SymbolRef S, StringRef M) : Sym(S), Msg(M) {}
+  ~StackHintGeneratorForSymbol() override = default;
+
+  /// Search the call expression for the symbol Sym and dispatch the
+  /// 'getMessageForX()' methods to construct a specific message.
+  std::string getMessage(const ExplodedNode *N) override;
+
+  /// Produces the message of the following form:
+  ///   'Msg via Nth parameter'
+  virtual std::string getMessageForArg(const Expr *ArgE, unsigned ArgIndex);
+
+  virtual std::string getMessageForReturn(const CallExpr *CallExpr) {
+    return Msg;
+  }
+
+  virtual std::string getMessageForSymbolNotFound() {
+    return Msg;
+  }
+};
+
 /// This class provides an interface through which checkers can create
 /// individual bug reports.
 class BugReport {
@@ -313,6 +357,14 @@ protected:
 
   const Stmt *getStmt() const;
 
+  /// If an event occurs in a different frame than the final diagnostic,
+  /// supply a message that will be used to construct an extra hint on the
+  /// returns from all the calls on the stack from this event to the final
+  /// diagnostic.
+  // FIXME: Allow shared_ptr keys in DenseMap?
+  std::map<PathDiagnosticPieceRef, std::unique_ptr<StackHintGenerator>>
+      StackHints;
+
 public:
   PathSensitiveBugReport(const BugType &bt, StringRef desc,
                          const ExplodedNode *errorNode)
@@ -455,6 +507,26 @@ public:
   bool addTrackedCondition(const ExplodedNode *Cond) {
     return TrackedConditions.insert(Cond).second;
   }
+
+  void addCallStackHint(PathDiagnosticPieceRef Piece,
+                        std::unique_ptr<StackHintGenerator> StackHint) {
+    StackHints[Piece] = std::move(StackHint);
+  }
+
+  bool hasCallStackHint(PathDiagnosticPieceRef Piece) const {
+    return StackHints.count(Piece) > 0;
+  }
+
+  /// Produce the hint for the given node. The node contains
+  /// information about the call for which the diagnostic can be generated.
+  std::string
+  getCallStackMessage(PathDiagnosticPieceRef Piece,
+                      const ExplodedNode *N) const {
+    auto I = StackHints.find(Piece);
+    if (I != StackHints.end())
+      return I->second->getMessage(N);
+    return "";
+  }
 };
 
 //===----------------------------------------------------------------------===//

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

@@ -502,65 +502,13 @@ public:
   }
 };
 
-/// Interface for classes constructing Stack hints.
-///
-/// If a PathDiagnosticEvent occurs in a different frame than the final
-/// diagnostic the hints can be used to summarize the effect of the call.
-class StackHintGenerator {
-public:
-  virtual ~StackHintGenerator() = 0;
-
-  /// Construct the Diagnostic message for the given ExplodedNode.
-  virtual std::string getMessage(const ExplodedNode *N) = 0;
-};
-
-/// Constructs a Stack hint for the given symbol.
-///
-/// The class knows how to construct the stack hint message based on
-/// traversing the CallExpr associated with the call and checking if the given
-/// symbol is returned or is one of the arguments.
-/// The hint can be customized by redefining 'getMessageForX()' methods.
-class StackHintGeneratorForSymbol : public StackHintGenerator {
-private:
-  SymbolRef Sym;
-  std::string Msg;
-
-public:
-  StackHintGeneratorForSymbol(SymbolRef S, StringRef M) : Sym(S), Msg(M) {}
-  ~StackHintGeneratorForSymbol() override = default;
-
-  /// Search the call expression for the symbol Sym and dispatch the
-  /// 'getMessageForX()' methods to construct a specific message.
-  std::string getMessage(const ExplodedNode *N) override;
-
-  /// Produces the message of the following form:
-  ///   'Msg via Nth parameter'
-  virtual std::string getMessageForArg(const Expr *ArgE, unsigned ArgIndex);
-
-  virtual std::string getMessageForReturn(const CallExpr *CallExpr) {
-    return Msg;
-  }
-
-  virtual std::string getMessageForSymbolNotFound() {
-    return Msg;
-  }
-};
-
 class PathDiagnosticEventPiece : public PathDiagnosticSpotPiece {
   Optional<bool> IsPrunable;
 
-  /// If the event occurs in a different frame than the final diagnostic,
-  /// supply a message that will be used to construct an extra hint on the
-  /// returns from all the calls on the stack from this event to the final
-  /// diagnostic.
-  std::unique_ptr<StackHintGenerator> CallStackHint;
-
 public:
   PathDiagnosticEventPiece(const PathDiagnosticLocation &pos,
-                           StringRef s, bool addPosRange = true,
-                           StackHintGenerator *stackHint = nullptr)
-      : PathDiagnosticSpotPiece(pos, s, Event, addPosRange),
-        CallStackHint(stackHint) {}
+                           StringRef s, bool addPosRange = true)
+      : PathDiagnosticSpotPiece(pos, s, Event, addPosRange) {}
   ~PathDiagnosticEventPiece() override;
 
   /// Mark the diagnostic piece as being potentially prunable.  This
@@ -577,16 +525,6 @@ public:
     return IsPrunable.hasValue() ? IsPrunable.getValue() : false;
   }
 
-  bool hasCallStackHint() { return (bool)CallStackHint; }
-
-  /// Produce the hint for the given node. The node contains
-  /// information about the call for which the diagnostic can be generated.
-  std::string getCallStackMessage(const ExplodedNode *N) {
-    if (CallStackHint)
-      return CallStackHint->getMessage(N);
-    return {};
-  }
-
   void dump() const override;
 
   static bool classof(const PathDiagnosticPiece *P) {

lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp

@@ -140,8 +140,7 @@ DeleteWithNonVirtualDtorChecker::DeleteBugVisitor::VisitNode(
   OS << "Conversion from derived to base happened here";
   PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
                              N->getLocationContext());
-  return std::make_shared<PathDiagnosticEventPiece>(Pos, OS.str(), true,
-                                                    nullptr);
+  return std::make_shared<PathDiagnosticEventPiece>(Pos, OS.str(), true);
 }
 
 void ento::registerDeleteWithNonVirtualDtorChecker(CheckerManager &mgr) {

lib/StaticAnalyzer/Checkers/DynamicTypeChecker.cpp

@@ -139,8 +139,7 @@ PathDiagnosticPieceRef DynamicTypeChecker::DynamicTypeBugVisitor::VisitNode(
   // Generate the extra diagnostic.
   PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
                              N->getLocationContext());
-  return std::make_shared<PathDiagnosticEventPiece>(Pos, OS.str(), true,
-                                                    nullptr);
+  return std::make_shared<PathDiagnosticEventPiece>(Pos, OS.str(), true);
 }
 
 static bool hasDefinition(const ObjCObjectPointerType *ObjPtr) {

lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp

@@ -972,8 +972,7 @@ PathDiagnosticPieceRef DynamicTypePropagation::GenericsBugVisitor::VisitNode(
   // Generate the extra diagnostic.
   PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
                              N->getLocationContext());
-  return std::make_shared<PathDiagnosticEventPiece>(Pos, OS.str(), true,
-                                                    nullptr);
+  return std::make_shared<PathDiagnosticEventPiece>(Pos, OS.str(), true);
 }
 
 /// Register checkers.

lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp

@@ -299,8 +299,7 @@ PathDiagnosticPieceRef InnerPointerChecker::InnerPointerBRVisitor::VisitNode(
      << "' obtained here";
   PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
                              N->getLocationContext());
-  return std::make_shared<PathDiagnosticEventPiece>(Pos, OS.str(), true,
-                                                    nullptr);
+  return std::make_shared<PathDiagnosticEventPiece>(Pos, OS.str(), true);
 }
 
 void ento::registerInnerPointerChecker(CheckerManager &Mgr) {

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

@@ -2953,15 +2953,15 @@ MallocChecker::MallocBugVisitor::VisitNode(const ExplodedNode *N,
 
   // Find out if this is an interesting point and what is the kind.
   StringRef Msg;
-  StackHintGeneratorForSymbol *StackHint = nullptr;
+  std::unique_ptr<StackHintGeneratorForSymbol> StackHint = nullptr;
   SmallString<256> Buf;
   llvm::raw_svector_ostream OS(Buf);
 
   if (Mode == Normal) {
     if (isAllocated(RS, RSPrev, S)) {
       Msg = "Memory is allocated";
-      StackHint = new StackHintGeneratorForSymbol(Sym,
-                                                  "Returned allocated memory");
+      StackHint = std::make_unique<StackHintGeneratorForSymbol>(
+          Sym, "Returned allocated memory");
     } else if (isReleased(RS, RSPrev, S)) {
       const auto Family = RS->getAllocationFamily();
       switch (Family) {
@@ -2971,8 +2971,8 @@ MallocChecker::MallocBugVisitor::VisitNode(const ExplodedNode *N,
         case AF_CXXNewArray:
         case AF_IfNameIndex:
           Msg = "Memory is released";
-          StackHint = new StackHintGeneratorForSymbol(Sym,
-                                              "Returning; memory was released");
+          StackHint = std::make_unique<StackHintGeneratorForSymbol>(
+              Sym, "Returning; memory was released");
           break;
         case AF_InnerBuffer: {
           const MemRegion *ObjRegion =
@@ -2983,8 +2983,8 @@ MallocChecker::MallocBugVisitor::VisitNode(const ExplodedNode *N,
 
           if (N->getLocation().getKind() == ProgramPoint::PostImplicitCallKind) {
             OS << "deallocated by call to destructor";
-            StackHint = new StackHintGeneratorForSymbol(Sym,
-                                      "Returning; inner buffer was deallocated");
+            StackHint = std::make_unique<StackHintGeneratorForSymbol>(
+                Sym, "Returning; inner buffer was deallocated");
           } else {
             OS << "reallocated by call to '";
             const Stmt *S = RS->getStmt();
@@ -2999,8 +2999,8 @@ MallocChecker::MallocBugVisitor::VisitNode(const ExplodedNode *N,
               OS << (D ? D->getNameAsString() : "unknown");
             }
             OS << "'";
-            StackHint = new StackHintGeneratorForSymbol(Sym,
-                                      "Returning; inner buffer was reallocated");
+            StackHint = std::make_unique<StackHintGeneratorForSymbol>(
+                Sym, "Returning; inner buffer was reallocated");
           }
           Msg = OS.str();
           break;
@@ -3040,12 +3040,12 @@ MallocChecker::MallocBugVisitor::VisitNode(const ExplodedNode *N,
       }
     } else if (isRelinquished(RS, RSPrev, S)) {
       Msg = "Memory ownership is transferred";
-      StackHint = new StackHintGeneratorForSymbol(Sym, "");
+      StackHint = std::make_unique<StackHintGeneratorForSymbol>(Sym, "");
     } else if (isReallocFailedCheck(RS, RSPrev, S)) {
       Mode = ReallocationFailed;
       Msg = "Reallocation failed";
-      StackHint = new StackHintGeneratorForReallocationFailed(Sym,
-                                                       "Reallocation failed");
+      StackHint = std::make_unique<StackHintGeneratorForReallocationFailed>(
+          Sym, "Reallocation failed");
 
       if (SymbolRef sym = findFailedReallocSymbol(state, statePrev)) {
         // Is it possible to fail two reallocs WITHOUT testing in between?
@@ -3064,16 +3064,15 @@ MallocChecker::MallocBugVisitor::VisitNode(const ExplodedNode *N,
     if (!statePrev->get<RegionState>(FailedReallocSymbol)) {
       // We're at the reallocation point.
       Msg = "Attempt to reallocate memory";
-      StackHint = new StackHintGeneratorForSymbol(Sym,
-                                                 "Returned reallocated memory");
+      StackHint = std::make_unique<StackHintGeneratorForSymbol>(
+          Sym, "Returned reallocated memory");
       FailedReallocSymbol = nullptr;
       Mode = Normal;
     }
   }
 
   if (Msg.empty()) {
-    // Silence a memory leak warning by MallocChecker in MallocChecker.cpp :)
-    assert(!StackHint && "Memory leak!");
+    assert(!StackHint);
     return nullptr;
   }
 
@@ -3093,7 +3092,9 @@ MallocChecker::MallocBugVisitor::VisitNode(const ExplodedNode *N,
                                  N->getLocationContext());
   }
 
-  return std::make_shared<PathDiagnosticEventPiece>(Pos, Msg, true, StackHint);
+  auto P = std::make_shared<PathDiagnosticEventPiece>(Pos, Msg, true);
+  BR.addCallStackHint(P, std::move(StackHint));
+  return P;
 }
 
 void MallocChecker::printState(raw_ostream &Out, ProgramStateRef State,

lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

@@ -323,8 +323,7 @@ PathDiagnosticPieceRef NullabilityChecker::NullabilityBugVisitor::VisitNode(
   // Generate the extra diagnostic.
   PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
                              N->getLocationContext());
-  return std::make_shared<PathDiagnosticEventPiece>(Pos, InfoText, true,
-                                                    nullptr);
+  return std::make_shared<PathDiagnosticEventPiece>(Pos, InfoText, true);
 }
 
 /// Returns true when the value stored at the given location has been

lib/StaticAnalyzer/Core/BugReporter.cpp

@@ -239,6 +239,8 @@ public:
   generate(const PathDiagnosticConsumer *PDC) const;
 
 private:
+  void updateStackPiecesWithMessage(PathDiagnosticPieceRef P,
+                                    const CallWithEntryStack &CallStack) const;
   void generatePathDiagnosticsForNode(PathDiagnosticConstruct &C,
                                       PathDiagnosticLocation &PrevLoc) const;
 
@@ -269,6 +271,69 @@ private:
 
 } // namespace
 
+//===----------------------------------------------------------------------===//
+// Base implementation of stack hint generators.
+//===----------------------------------------------------------------------===//
+
+StackHintGenerator::~StackHintGenerator() = default;
+
+std::string StackHintGeneratorForSymbol::getMessage(const ExplodedNode *N){
+  if (!N)
+    return getMessageForSymbolNotFound();
+
+  ProgramPoint P = N->getLocation();
+  CallExitEnd CExit = P.castAs<CallExitEnd>();
+
+  // FIXME: Use CallEvent to abstract this over all calls.
+  const Stmt *CallSite = CExit.getCalleeContext()->getCallSite();
+  const auto *CE = dyn_cast_or_null<CallExpr>(CallSite);
+  if (!CE)
+    return {};
+
+  // Check if one of the parameters are set to the interesting symbol.
+  unsigned ArgIndex = 0;
+  for (CallExpr::const_arg_iterator I = CE->arg_begin(),
+                                    E = CE->arg_end(); I != E; ++I, ++ArgIndex){
+    SVal SV = N->getSVal(*I);
+
+    // Check if the variable corresponding to the symbol is passed by value.
+    SymbolRef AS = SV.getAsLocSymbol();
+    if (AS == Sym) {
+      return getMessageForArg(*I, ArgIndex);
+    }
+
+    // Check if the parameter is a pointer to the symbol.
+    if (Optional<loc::MemRegionVal> Reg = SV.getAs<loc::MemRegionVal>()) {
+      // Do not attempt to dereference void*.
+      if ((*I)->getType()->isVoidPointerType())
+        continue;
+      SVal PSV = N->getState()->getSVal(Reg->getRegion());
+      SymbolRef AS = PSV.getAsLocSymbol();
+      if (AS == Sym) {
+        return getMessageForArg(*I, ArgIndex);
+      }
+    }
+  }
+
+  // Check if we are returning the interesting symbol.
+  SVal SV = N->getSVal(CE);
+  SymbolRef RetSym = SV.getAsLocSymbol();
+  if (RetSym == Sym) {
+    return getMessageForReturn(CE);
+  }
+
+  return getMessageForSymbolNotFound();
+}
+
+std::string StackHintGeneratorForSymbol::getMessageForArg(const Expr *ArgE,
+                                                          unsigned ArgIndex) {
+  // Printed parameters start at 1, not 0.
+  ++ArgIndex;
+
+  return (llvm::Twine(Msg) + " via " + std::to_string(ArgIndex) +
+          llvm::getOrdinalSuffix(ArgIndex) + " parameter").str();
+}
+
 //===----------------------------------------------------------------------===//
 // Helper routines for walking the ExplodedGraph and fetching statements.
 //===----------------------------------------------------------------------===//
@@ -661,7 +726,7 @@ getEnclosingStmtLocation(const Stmt *S, const LocationContext *LC,
 //===----------------------------------------------------------------------===//
 
 /// If the piece contains a special message, add it to all the call pieces on
-/// the active stack. For exampler, my_malloc allocated memory, so MallocChecker
+/// the active stack. For example, my_malloc allocated memory, so MallocChecker
 /// will construct an event at the call to malloc(), and add a stack hint that
 /// an allocated memory was returned. We'll use this hint to construct a message
 /// when returning from the call to my_malloc
@@ -670,22 +735,20 @@ getEnclosingStmtLocation(const Stmt *S, const LocationContext *LC,
 ///   void fishy() {
 ///     void *ptr = my_malloc(); // returned allocated memory
 ///   } // leak
-static void updateStackPiecesWithMessage(PathDiagnosticPiece &P,
-                                         const CallWithEntryStack &CallStack) {
-  if (auto *ep = dyn_cast<PathDiagnosticEventPiece>(&P)) {
-    if (ep->hasCallStackHint())
-      for (const auto &I : CallStack) {
-        PathDiagnosticCallPiece *CP = I.first;
-        const ExplodedNode *N = I.second;
-        std::string stackMsg = ep->getCallStackMessage(N);
-
-        // The last message on the path to final bug is the most important
-        // one. Since we traverse the path backwards, do not add the message
-        // if one has been previously added.
-        if  (!CP->hasCallStackMessage())
-          CP->setCallStackMessage(stackMsg);
-      }
-  }
+void PathDiagnosticBuilder::updateStackPiecesWithMessage(
+    PathDiagnosticPieceRef P, const CallWithEntryStack &CallStack) const {
+  if (R->hasCallStackHint(P))
+    for (const auto &I : CallStack) {
+      PathDiagnosticCallPiece *CP = I.first;
+      const ExplodedNode *N = I.second;
+      std::string stackMsg = R->getCallStackMessage(P, N);
+
+      // The last message on the path to final bug is the most important
+      // one. Since we traverse the path backwards, do not add the message
+      // if one has been previously added.
+      if (!CP->hasCallStackMessage())
+        CP->setCallStackMessage(stackMsg);
+    }
 }
 
 static void CompactMacroExpandedPieces(PathPieces &path,
@@ -1990,7 +2053,7 @@ PathDiagnosticBuilder::generate(const PathDiagnosticConsumer *PDC) const {
 
       if (PDC->shouldAddPathEdges())
         addEdgeToPath(Construct.getActivePath(), PrevLoc, Note->getLocation());
-      updateStackPiecesWithMessage(*Note, Construct.CallStack);
+      updateStackPiecesWithMessage(Note, Construct.CallStack);
       Construct.getActivePath().push_front(Note);
     }
   }

lib/StaticAnalyzer/Core/PathDiagnostic.cpp

@@ -31,7 +31,6 @@
 #include "clang/Basic/SourceManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/FoldingSet.h"
 #include "llvm/ADT/None.h"
@@ -1303,70 +1302,6 @@ void PathDiagnostic::FullProfile(llvm::FoldingSetNodeID &ID) const {
     ID.AddString(*I);
 }
 
-StackHintGenerator::~StackHintGenerator() = default;
-
-std::string StackHintGeneratorForSymbol::getMessage(const ExplodedNode *N){
-  if (!N)
-    return getMessageForSymbolNotFound();
-
-  ProgramPoint P = N->getLocation();
-  CallExitEnd CExit = P.castAs<CallExitEnd>();
-
-  // FIXME: Use CallEvent to abstract this over all calls.
-  const Stmt *CallSite = CExit.getCalleeContext()->getCallSite();
-  const auto *CE = dyn_cast_or_null<CallExpr>(CallSite);
-  if (!CE)
-    return {};
-
-  // Check if one of the parameters are set to the interesting symbol.
-  unsigned ArgIndex = 0;
-  for (CallExpr::const_arg_iterator I = CE->arg_begin(),
-                                    E = CE->arg_end(); I != E; ++I, ++ArgIndex){
-    SVal SV = N->getSVal(*I);
-
-    // Check if the variable corresponding to the symbol is passed by value.
-    SymbolRef AS = SV.getAsLocSymbol();
-    if (AS == Sym) {
-      return getMessageForArg(*I, ArgIndex);
-    }
-
-    // Check if the parameter is a pointer to the symbol.
-    if (Optional<loc::MemRegionVal> Reg = SV.getAs<loc::MemRegionVal>()) {
-      // Do not attempt to dereference void*.
-      if ((*I)->getType()->isVoidPointerType())
-        continue;
-      SVal PSV = N->getState()->getSVal(Reg->getRegion());
-      SymbolRef AS = PSV.getAsLocSymbol();
-      if (AS == Sym) {
-        return getMessageForArg(*I, ArgIndex);
-      }
-    }
-  }
-
-  // Check if we are returning the interesting symbol.
-  SVal SV = N->getSVal(CE);
-  SymbolRef RetSym = SV.getAsLocSymbol();
-  if (RetSym == Sym) {
-    return getMessageForReturn(CE);
-  }
-
-  return getMessageForSymbolNotFound();
-}
-
-std::string StackHintGeneratorForSymbol::getMessageForArg(const Expr *ArgE,
-                                                          unsigned ArgIndex) {
-  // Printed parameters start at 1, not 0.
-  ++ArgIndex;
-
-  SmallString<200> buf;
-  llvm::raw_svector_ostream os(buf);
-
-  os << Msg << " via " << ArgIndex << llvm::getOrdinalSuffix(ArgIndex)
-     << " parameter";
-
-  return os.str();
-}
-
 LLVM_DUMP_METHOD void PathPieces::dump() const {
   unsigned index = 0;
   for (PathPieces::const_iterator I = begin(), E = end(); I != E; ++I) {