[analyzer] NFC: Move stack hints to a side map.

[NoQ]

Summary

Stack hints are attached to PathDiagnosticEventPieces in order to improve path notes at the call site for the call in which the event has occured. For example, they are currently used only by MallocChecker in order to produce the fancy "Returning allocated memory"/"Returning; memory was released" note. By the way, in fact nobody else currently uses this functionality - only MallocChecker.

I want to make the PathDiagnostic interface completely ignorant of Static Analyzer specific concepts such as ExplodedNode or SymbolRef, so i moved this interface to BugReporter. Stack hints are now owned by the PathSensitiveBugReport object against which the visitor emits event pieces for which it needs stack hints.

I'm open to discuss a better design here. Eg., i thought about making it part of the visitor interface instead, but i don't immediately see how to do this without breaking the logic of "only add the note at the call site in which the event has happened, not every time allocated memory is returned from anywhere".

[Szelethus]

I'm open to discuss a better design here. Eg., i thought about making it part of the visitor interface instead, but i don't immediately see how to do this without breaking the logic of "only add the note at the call site in which the event has happened, not every time allocated memory is returned from anywhere".

When I stumbled upon this stack hint thingie during the refactoring of BugReporter.cpp, I found it very clunky, and while I still do, I'm none the wiser about how to do it any better.

Side note, now that you had to work with the freshly rewritten file, do you have any feedback on it?

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

333–337 ↗

(On Diff #219451)

How about a simple twine?

clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp

34 ↗

(On Diff #219451)

Nice, how did you catch this? Some compiler warning?

[NoQ]

clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp

34 ↗

(On Diff #219451)

No, it's just that removing these includes is the whole point of the patch. In order to be able to move this file out of static analyzer, it should not include headers from static analyzer. This patch removes one such include.

[NoQ]

In D67381#1665930, @Szelethus wrote:

Side note, now that you had to work with the freshly rewritten file, do you have any feedback on it?

Dunno, i'm a very functional / pure / stateless person. "I debugged, I edited, I forgot". "I suffer from short-term memory loss".

But given that it was fairly easy for me to focus on the task at hand and this whole thing was definitely not traumatizing, i suspect that the file ended up in a fairly good shape and i can't complain.

[Szelethus]

Aren't StackHints basically ancient NoteTags?

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

 // Interface for individual bug reports.
 //===----------------------------------------------------------------------===//
 
 /// A mapping from diagnostic consumers to the diagnostics they should
 /// consume.
 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 {
 public:
   enum class Kind { Basic, PathSensitive };
 
 protected:
   friend class BugReportEquivClass;
protected:
 
   /// Reports with different uniqueing locations are considered to be different
   /// for the purposes of deduplication.
   PathDiagnosticLocation UniqueingLocation;
   const Decl *UniqueingDecl;
 
   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)
       : BugReport(Kind::PathSensitive, bt, desc), ErrorNode(errorNode),
         ErrorNodeRange(getStmt() ? getStmt()->getSourceRange()
                                  : SourceRange()) {}
 
   PathSensitiveBugReport(const BugType &bt, StringRef shortDesc, StringRef desc,
public:
   visitor_range visitors() { return {visitor_begin(), visitor_end()}; }
 
   /// Notes that the condition of the CFGBlock associated with \p Cond is
   /// being tracked.
   /// \returns false if the condition is already being tracked.
   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 "";
+  }
 };
 
 //===----------------------------------------------------------------------===//
 // BugTypes (collections of related reports).
 //===----------------------------------------------------------------------===//
 
 class BugReportEquivClass : public llvm::FoldingSetNode {
   friend class BugReporter;

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

public:
   void Profile(llvm::FoldingSetNodeID &ID) const override;
 
   static bool classof(const PathDiagnosticPiece *P) {
     return P->getKind() == Event || P->getKind() == Macro ||
            P->getKind() == Note || P->getKind() == PopUp;
   }
 };
 
-/// 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
   /// flag may have been previously set, at which point it will not
   /// be reset unless one specifies to do so.
   void setPrunable(bool isPrunable, bool override = false) {
     if (IsPrunable.hasValue() && !override)
      return;
     IsPrunable = isPrunable;
   }
 
   /// Return true if the diagnostic piece is prunable.
   bool isPrunable() const {
     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) {
     return P->getKind() == Event;
   }
 };
 
 class PathDiagnosticCallPiece : public PathDiagnosticPiece {

cfe/trunk/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp

DeleteWithNonVirtualDtorChecker::DeleteBugVisitor::VisitNode(
   // Stop traversal on this path.
   Satisfied = true;
 
   SmallString<256> Buf;
   llvm::raw_svector_ostream OS(Buf);
   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) {
   mgr.registerChecker<DeleteWithNonVirtualDtorChecker>();
 }
 
 bool ento::shouldRegisterDeleteWithNonVirtualDtorChecker(
                                                         const LangOptions &LO) {
   return true;
 }

cfe/trunk/lib/StaticAnalyzer/Checkers/DynamicTypeChecker.cpp

if (const auto *ExplicitCast = dyn_cast<ExplicitCastExpr>(S)) {
     OS << "')";
   } else {
     OS << "this context";
   }
 
   // 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) {
   const ObjCInterfaceDecl *Decl = ObjPtr->getInterfaceDecl();
   if (!Decl)
     return false;
 
   return Decl->getDefinition();

cfe/trunk/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp

if (const auto *ExplicitCast = dyn_cast<ExplicitCastExpr>(S)) {
     OS << "')";
   } else {
     OS << "this context";
   }
 
   // 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.
 void ento::registerObjCGenericsChecker(CheckerManager &mgr) {
   DynamicTypePropagation *checker = mgr.getChecker<DynamicTypePropagation>();
   checker->CheckGenerics = true;
 }
 

cfe/trunk/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp

PathDiagnosticPieceRef InnerPointerChecker::InnerPointerBRVisitor::VisitNode(
   QualType ObjTy = TypedRegion->getValueType();
 
   SmallString<256> Buf;
   llvm::raw_svector_ostream OS(Buf);
   OS << "Pointer to inner buffer of '" << ObjTy.getAsString()
      << "' 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) {
   registerInnerPointerCheckerAux(Mgr);
   Mgr.registerChecker<InnerPointerChecker>();
 }
 
 bool ento::shouldRegisterInnerPointerChecker(const LangOptions &LO) {
   return true;
 }

cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

if (ReleaseDestructorLC) {
     }
   }
 
   // FIXME: We will eventually need to handle non-statement-based events
   // (__attribute__((cleanup))).
 
   // 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) {
         case AF_Alloca:
         case AF_Malloc:
         case AF_CXXNew:
         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 =
               allocation_state::getContainerObjRegion(statePrev, Sym);
           const auto *TypedRegion = cast<TypedValueRegion>(ObjRegion);
           QualType ObjTy = TypedRegion->getValueType();
           OS << "Inner buffer of '" << ObjTy.getAsString() << "' ";
 
           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();
             if (const auto *MemCallE = dyn_cast<CXXMemberCallExpr>(S)) {
               OS << MemCallE->getMethodDecl()->getNameAsString();
             } else if (const auto *OpCallE = dyn_cast<CXXOperatorCallExpr>(S)) {
               OS << OpCallE->getDirectCallee()->getNameAsString();
             } else if (const auto *CallE = dyn_cast<CallExpr>(S)) {
               auto &CEMgr = BRC.getStateManager().getCallEventManager();
               CallEventRef<> Call = CEMgr.getSimpleCall(CallE, state, CurrentLC);
               const auto *D = dyn_cast_or_null<NamedDecl>(Call->getDecl());
               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;
         }
         case AF_None:
           llvm_unreachable("Unhandled allocation family!");
       }
 
if (isAllocated(RS, RSPrev, S)) {
             // released into it. So we're only interested in the innermost
             // destructor.
             FoundAnyDestructor = true;
           }
         }
       }
     } 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?
         assert((!FailedReallocSymbol || FailedReallocSymbol == sym) &&
           "We only support one failed realloc at a time.");
         BR.markInteresting(sym);
         FailedReallocSymbol = sym;
       }
     }
 
   // We are in a special mode if a reallocation failed later in the path.
   } else if (Mode == ReallocationFailed) {
     assert(FailedReallocSymbol && "No symbol to look for.");
 
     // Is this is the first appearance of the reallocated symbol?
     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;
   }
 
   assert(StackHint);
 
   // Generate the extra diagnostic.
   PathDiagnosticLocation Pos;
   if (!S) {
     assert(RS->getAllocationFamily() == AF_InnerBuffer);
     auto PostImplCall = N->getLocation().getAs<PostImplicitCall>();
     if (!PostImplCall)
       return nullptr;
     Pos = PathDiagnosticLocation(PostImplCall->getLocation(),
                                  BRC.getSourceManager());
   } else {
     Pos = PathDiagnosticLocation(S, BRC.getSourceManager(),
                                  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,
                                const char *NL, const char *Sep) const {
 
   RegionStateTy RS = State->get<RegionState>();
 
   if (!RS.isEmpty()) {

cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

PathDiagnosticPieceRef NullabilityChecker::NullabilityBugVisitor::VisitNode(
   std::string InfoText =
       (llvm::Twine("Nullability '") +
        getNullabilityString(TrackedNullab->getValue()) + "' is inferred")
           .str();
 
   // 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
 /// constrained to null after being passed through an object of nonnnull type.
 static bool checkValueAtLValForInvariantViolation(ProgramStateRef State,
                                                   SVal LV, QualType T) {
   if (getNullabilityAnnotation(T) != Nullability::Nonnull)
     return false;

cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp

public:
   /// for plists) and without  (used for HTML and text). When edges are added,
   /// the path is modified to insert artificially generated edges.
   /// Otherwise, more detailed diagnostics is emitted for block edges,
   /// explaining the transitions in words.
   std::unique_ptr<PathDiagnostic>
   generate(const PathDiagnosticConsumer *PDC) const;
 
 private:
+  void updateStackPiecesWithMessage(PathDiagnosticPieceRef P,
+                                    const CallWithEntryStack &CallStack) const;
   void generatePathDiagnosticsForNode(PathDiagnosticConstruct &C,
                                       PathDiagnosticLocation &PrevLoc) const;
 
   void generateMinimalDiagForBlockEdge(PathDiagnosticConstruct &C,
                                        BlockEdge BE) const;
 
   PathDiagnosticPieceRef
   generateDiagForGotoOP(const PathDiagnosticConstruct &C, const Stmt *S,
ExecutionContinues(llvm::raw_string_ostream &os,
                      const PathDiagnosticConstruct &C) const;
 
   const PathSensitiveBugReport *getBugReport() const { return R; }
 };
 
 } // 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.
 //===----------------------------------------------------------------------===//
 
 static const Stmt *GetPreviousStmt(const ExplodedNode *N) {
   for (N = N->getFirstPred(); N; N = N->getFirstPred())
     if (const Stmt *S = PathDiagnosticLocation::getStmt(N))
       return S;
 
getEnclosingStmtLocation(const Stmt *S, const LocationContext *LC,
   return PathDiagnosticLocation(S, SMgr, LC);
 }
 
 //===----------------------------------------------------------------------===//
 // "Minimal" path diagnostic generation algorithm.
 //===----------------------------------------------------------------------===//
 
 /// 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
 ///
 ///   void *my_malloc() { return malloc(sizeof(int)); }
 ///   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);
+void PathDiagnosticBuilder::updateStackPiecesWithMessage(
+    PathDiagnosticPieceRef P, const CallWithEntryStack &CallStack) const {
+  if (R->hasCallStackHint(P))
+      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);
-      }
-  }
-}
+      if (!CP->hasCallStackMessage())
 
 static void CompactMacroExpandedPieces(PathPieces &path,
                                        const SourceManager& SM);
 
 PathDiagnosticPieceRef PathDiagnosticBuilder::generateDiagForSwitchOP(
     const PathDiagnosticConstruct &C, const CFGBlock *Dst,
     PathDiagnosticLocation &Start) const {
 
while (Construct.ascendToPrevNode()) {
     for (const PathDiagnosticPieceRef &Note : VisitorNotes->second) {
       llvm::FoldingSetNodeID ID;
       Note->Profile(ID);
       if (!DeduplicationSet.insert(ID).second)
         continue;
 
       if (PDC->shouldAddPathEdges())
         addEdgeToPath(Construct.getActivePath(), PrevLoc, Note->getLocation());
-      updateStackPiecesWithMessage(*Note, Construct.CallStack);
+      updateStackPiecesWithMessage(Note, Construct.CallStack);
       Construct.getActivePath().push_front(Note);
     }
   }
 
   if (PDC->shouldAddPathEdges()) {
     // Add an edge to the start of the function.
     // We'll prune it out later, but it helps make diagnostics more uniform.
     const StackFrameContext *CalleeLC =

cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp

 #include "clang/Analysis/CFG.h"
 #include "clang/Analysis/ProgramPoint.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 #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"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringExtras.h"

 void PathDiagnostic::FullProfile(llvm::FoldingSetNodeID &ID) const {
   Profile(ID);
   for (const auto &I : path)
     ID.Add(*I);
   for (meta_iterator I = meta_begin(), E = meta_end(); I != E; ++I)
     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) {
     llvm::errs() << "[" << index++ << "]  ";
     (*I)->dump();
     llvm::errs() << "\n";
   }
 }