Revert "[analyzer] Introduce a simplified API for adding custom path notes."

haoNoQ · haoNoQ · commit 4d6fb5789fca · 2019-03-29T23:11:10.000Z
This reverts commit r357323. ASan leaks found by a buildbot :) Differential Revision: https://reviews.llvm.org/D58367 llvm-svn: 357332
diff --git a/clang/include/clang/Analysis/ProgramPoint.h b/clang/include/clang/Analysis/ProgramPoint.h
@@ -42,11 +42,12 @@ class ProgramPointTag {
   virtual ~ProgramPointTag();
   virtual StringRef getTagDescription() const = 0;
 
+protected:
   /// Used to implement 'isKind' in subclasses.
-  const void *getTagKind() const { return TagKind; }
+  const void *getTagKind() { return TagKind; }
 
 private:
-  const void *const TagKind;
+  const void *TagKind;
 };
 
 class SimpleProgramPointTag : public ProgramPointTag {
diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
@@ -592,60 +592,6 @@ class BugReporterContext {
   NodeMapClosure& getNodeResolver() { return NMC; }
 };
 
-
-/// The tag upon which the TagVisitor reacts. Add these in order to display
-/// additional PathDiagnosticEventPieces along the path.
-class NoteTag : public ProgramPointTag {
-public:
-  using Callback =
-      std::function<std::string(BugReporterContext &, BugReport &)>;
-
-private:
-  static int Kind;
-
-  const Callback Cb;
-
-  NoteTag(Callback &&Cb) : ProgramPointTag(&Kind), Cb(std::move(Cb)) {}
-
-public:
-  static bool classof(const ProgramPointTag *T) {
-    return T->getTagKind() == &Kind;
-  }
-
-  Optional<std::string> generateMessage(BugReporterContext &BRC,
-                                        BugReport &R) const {
-    std::string Msg = Cb(BRC, R);
-    if (Msg.empty())
-      return None;
-
-    return std::move(Msg);
-  }
-
-  StringRef getTagDescription() const override {
-    // TODO: Remember a few examples of generated messages
-    // and display them in the ExplodedGraph dump by
-    // returning them from this function.
-    return "Note Tag";
-  }
-
-  // Manage memory for NoteTag objects.
-  class Factory {
-    llvm::BumpPtrAllocator &Alloc;
-
-  public:
-    Factory(llvm::BumpPtrAllocator &Alloc) : Alloc(Alloc) {}
-
-    const NoteTag *makeNoteTag(Callback &&Cb) {
-      // We cannot use make_unique because we cannot access the private
-      // constructor from inside it.
-      NoteTag *Tag = Alloc.Allocate<NoteTag>();
-      return new (Tag) NoteTag(std::move(Cb));
-    }
-  };
-
-  friend class TagVisitor;
-};
-
 } // namespace ento
 
 } // namespace clang
diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
@@ -14,7 +14,6 @@
 #ifndef LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_BUGREPORTERVISITORS_H
 #define LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_BUGREPORTERVISITORS_H
 
-#include "clang/Analysis/ProgramPoint.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
@@ -329,17 +328,6 @@ class FalsePositiveRefutationBRVisitor final : public BugReporterVisitor {
                        BugReport &BR) override;
 };
 
-
-/// The visitor detects NoteTags and displays the event notes they contain.
-class TagVisitor : public BugReporterVisitor {
-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
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
@@ -219,24 +219,6 @@ class CheckerContext {
     Eng.getBugReporter().emitReport(std::move(R));
   }
 
-
-  /// Produce a program point tag that displays an additional path note
-  /// to the user. This is a lightweight alternative to the
-  /// BugReporterVisitor mechanism: instead of visiting the bug report
-  /// node-by-node to restore the sequence of events that led to discovering
-  /// a bug, you can add notes as you add your transitions.
-  const NoteTag *getNoteTag(NoteTag::Callback &&Cb) {
-    return Eng.getNoteTags().makeNoteTag(std::move(Cb));
-  }
-
-  /// A shorthand version of getNoteTag that doesn't require you to accept
-  /// the BugReporterContext arguments when you don't need it.
-  const NoteTag *getNoteTag(std::function<std::string(BugReport &)> &&Cb) {
-    return getNoteTag(
-        [Cb](BugReporterContext &, BugReport &BR) { return Cb(BR); });
-  }
-
-
   /// Returns the word that should be used to refer to the declaration
   /// in the report.
   StringRef getDeclDescription(const Decl *D);
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -22,7 +22,6 @@
 #include "clang/Analysis/ProgramPoint.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
-#include "clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/FunctionSummary.h"
@@ -156,8 +155,6 @@ class ExprEngine : public SubEngine {
   /// The flag, which specifies the mode of inlining for the engine.
   InliningModes HowToInline;
 
-  NoteTag::Factory NoteTags;
-
 public:
   ExprEngine(cross_tu::CrossTranslationUnitContext &CTU, AnalysisManager &mgr,
              SetOfConstDecls *VisitedCalleesIn,
@@ -399,8 +396,6 @@ class ExprEngine : public SubEngine {
   SymbolManager &getSymbolManager() { return SymMgr; }
   MemRegionManager &getRegionManager() { return MRMgr; }
 
-  NoteTag::Factory &getNoteTags() { return NoteTags; }
-
 
   // Functions for external checking of whether we have unfinished work
   bool wasBlocksExhausted() const { return Engine.wasBlocksExhausted(); }
diff --git a/clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
@@ -80,10 +80,43 @@ class MIGChecker : public Checker<check::PostCall, check::PreStmt<ReturnStmt>,
     checkReturnAux(RS, C);
   }
 
+  class Visitor : public BugReporterVisitor {
+  public:
+    void Profile(llvm::FoldingSetNodeID &ID) const {
+      static int X = 0;
+      ID.AddPointer(&X);
+    }
+
+    std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
+        BugReporterContext &BRC, BugReport &R);
+  };
 };
 } // end anonymous namespace
 
-REGISTER_TRAIT_WITH_PROGRAMSTATE(ReleasedParameter, bool)
+// FIXME: It's a 'const ParmVarDecl *' but there's no ready-made GDM traits
+// specialization for this sort of types.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(ReleasedParameter, const void *)
+
+std::shared_ptr<PathDiagnosticPiece>
+MIGChecker::Visitor::VisitNode(const ExplodedNode *N, BugReporterContext &BRC,
+                               BugReport &R) {
+  const auto *NewPVD = static_cast<const ParmVarDecl *>(
+      N->getState()->get<ReleasedParameter>());
+  const auto *OldPVD = static_cast<const ParmVarDecl *>(
+      N->getFirstPred()->getState()->get<ReleasedParameter>());
+  if (OldPVD == NewPVD)
+    return nullptr;
+
+  assert(NewPVD && "What is deallocated cannot be un-deallocated!");
+  SmallString<64> Str;
+  llvm::raw_svector_ostream OS(Str);
+  OS << "Value passed through parameter '" << NewPVD->getName()
+     << "' is deallocated";
+
+  PathDiagnosticLocation Loc =
+      PathDiagnosticLocation::create(N->getLocation(), BRC.getSourceManager());
+  return std::make_shared<PathDiagnosticEventPiece>(Loc, OS.str());
+}
 
 static const ParmVarDecl *getOriginParam(SVal V, CheckerContext &C) {
   SymbolRef Sym = V.getAsSymbol();
@@ -162,16 +195,7 @@ void MIGChecker::checkPostCall(const CallEvent &Call, CheckerContext &C) const {
   if (!PVD)
     return;
 
-  const NoteTag *T = C.getNoteTag([this, PVD](BugReport &BR) -> std::string {
-    if (&BR.getBugType() != &BT)
-      return "";
-    SmallString<64> Str;
-    llvm::raw_svector_ostream OS(Str);
-    OS << "Value passed through parameter '" << PVD->getName()
-       << "\' is deallocated";
-    return OS.str();
-  });
-  C.addTransition(C.getState()->set<ReleasedParameter>(true), T);
+  C.addTransition(C.getState()->set<ReleasedParameter>(PVD));
 }
 
 // Returns true if V can potentially represent a "successful" kern_return_t.
@@ -236,6 +260,7 @@ void MIGChecker::checkReturnAux(const ReturnStmt *RS, CheckerContext &C) const {
 
   R->addRange(RS->getSourceRange());
   bugreporter::trackExpressionValue(N, RS->getRetValue(), *R, false);
+  R->addVisitor(llvm::make_unique<Visitor>());
   C.emitReport(std::move(R));
 }
 
diff --git a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -2612,7 +2612,6 @@ std::pair<BugReport*, std::unique_ptr<VisitorsDiagnosticsTy>> findValidReport(
     R->addVisitor(llvm::make_unique<NilReceiverBRVisitor>());
     R->addVisitor(llvm::make_unique<ConditionBRVisitor>());
     R->addVisitor(llvm::make_unique<CXXSelfAssignmentBRVisitor>());
-    R->addVisitor(llvm::make_unique<TagVisitor>());
 
     BugReporterContext BRC(Reporter, ErrorGraph.BackMap);
 
diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2472,30 +2472,6 @@ FalsePositiveRefutationBRVisitor::VisitNode(const ExplodedNode *N,
   return nullptr;
 }
 
-int NoteTag::Kind = 0;
-
-void TagVisitor::Profile(llvm::FoldingSetNodeID &ID) const {
-  static int Tag = 0;
-  ID.AddPointer(&Tag);
-}
-
-std::shared_ptr<PathDiagnosticPiece>
-TagVisitor::VisitNode(const ExplodedNode *N, BugReporterContext &BRC,
-                      BugReport &R) {
-  ProgramPoint PP = N->getLocation();
-  const NoteTag *T = dyn_cast_or_null<NoteTag>(PP.getTag());
-  if (!T)
-    return nullptr;
-
-  if (Optional<std::string> Msg = T->generateMessage(BRC, R)) {
-    PathDiagnosticLocation Loc =
-        PathDiagnosticLocation::create(PP, BRC.getSourceManager());
-    return std::make_shared<PathDiagnosticEventPiece>(Loc, *Msg);
-  }
-
-  return nullptr;
-}
-
 void FalsePositiveRefutationBRVisitor::Profile(
     llvm::FoldingSetNodeID &ID) const {
   static int Tag = 0;
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -201,9 +201,7 @@ ExprEngine::ExprEngine(cross_tu::CrossTranslationUnitContext &CTU,
       svalBuilder(StateMgr.getSValBuilder()),
       ObjCNoRet(mgr.getASTContext()),
       BR(mgr, *this),
-      VisitedCallees(VisitedCalleesIn),
-      HowToInline(HowToInlineIn),
-      NoteTags(G.getAllocator()) {
+      VisitedCallees(VisitedCalleesIn), HowToInline(HowToInlineIn) {
   unsigned TrimInterval = mgr.options.GraphTrimInterval;
   if (TrimInterval != 0) {
     // Enable eager node reclamation when constructing the ExplodedGraph.
diff --git a/clang/test/Analysis/mig.mm b/clang/test/Analysis/mig.mm
@@ -91,14 +91,6 @@ kern_return_t release_twice(mach_port_name_t port, vm_address_t addr1, vm_addres
                      // expected-note@-1{{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}}
 }
 
-MIG_SERVER_ROUTINE
-kern_return_t no_unrelated_notes(mach_port_name_t port, vm_address_t address, vm_size_t size) {
-  vm_deallocate(port, address, size); // no-note
-  1 / 0; // expected-warning{{Division by zero}}
-         // expected-note@-1{{Division by zero}}
-  return KERN_SUCCESS;
-}
-
 // Make sure we find the bug when the object is destroyed within an
 // automatic destructor.
 MIG_SERVER_ROUTINE
