[analyzer] Do not run visitors until the fixpoint, run only once.

In the current implementation, we run visitors until the fixed point is
reached.
That is, if a visitor adds another visitor, the currently processed path
is destroyed, all diagnostics is discarded, and it is regenerated again,
until it's no longer modified.
This pattern has a few negative implications:

 - This loop does not even guarantee to terminate.
   E.g. just imagine two visitors bouncing a diagnostics around.
 - Performance-wise, e.g. for sqlite3 all visitors are being re-run at
   least 10 times for some bugs.
   We have already seen a few reports where it leads to timeouts.
 - If we want to add more computationally intense visitors, this will
   become worse.
 - From architectural standpoint, the current layout requires copying
   visitors, which is conceptually wrong, and can be annoying (e.g. no
   unique_ptr on visitors allowed).

The proposed change is a much simpler architecture: the outer loop
processes nodes upwards, and whenever the visitor is added it only
processes current nodes and above, thus guaranteeing termination.

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

llvm-svn: 335666

1 files changed, 6 insertions, 16 deletions

diff --git a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
index 589054bdaf6..78e4b8f2508 100644
--- a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
@@ -1788,8 +1788,7 @@ namespace {
   //===---------===//
   // Bug Reports.  //
   //===---------===//
-
-  class CFRefReportVisitor : public BugReporterVisitorImpl<CFRefReportVisitor> {
+  class CFRefReportVisitor : public BugReporterVisitor {
   protected:
     SymbolRef Sym;
     const SummaryLogTy &SummaryLog;
@@ -1810,7 +1809,7 @@ namespace {
                                                    BugReporterContext &BRC,
                                                    BugReport &BR) override;
 
-    std::unique_ptr<PathDiagnosticPiece> getEndPath(BugReporterContext &BRC,
+    std::shared_ptr<PathDiagnosticPiece> getEndPath(BugReporterContext &BRC,
                                                     const ExplodedNode *N,
                                                     BugReport &BR) override;
   };
@@ -1821,18 +1820,9 @@ namespace {
                            const SummaryLogTy &log)
        : CFRefReportVisitor(sym, GCEnabled, log) {}
 
-    std::unique_ptr<PathDiagnosticPiece> getEndPath(BugReporterContext &BRC,
+    std::shared_ptr<PathDiagnosticPiece> getEndPath(BugReporterContext &BRC,
                                                     const ExplodedNode *N,
                                                     BugReport &BR) override;
-
-    std::unique_ptr<BugReporterVisitor> clone() const override {
-      // The curiously-recurring template pattern only works for one level of
-      // subclassing. Rather than make a new template base for
-      // CFRefReportVisitor, we simply override clone() to do the right thing.
-      // This could be trouble someday if BugReporterVisitorImpl is ever
-      // used for something else besides a convenient implementation of clone().
-      return llvm::make_unique<CFRefLeakReportVisitor>(*this);
-    }
   };
 
   class CFRefReport : public BugReport {
@@ -2365,14 +2355,14 @@ GetAllocationSite(ProgramStateManager& StateMgr, const ExplodedNode *N,
                         InterestingMethodContext);
 }
 
-std::unique_ptr<PathDiagnosticPiece>
+std::shared_ptr<PathDiagnosticPiece>
 CFRefReportVisitor::getEndPath(BugReporterContext &BRC,
                                const ExplodedNode *EndN, BugReport &BR) {
   BR.markInteresting(Sym);
   return BugReporterVisitor::getDefaultEndPath(BRC, EndN, BR);
 }
 
-std::unique_ptr<PathDiagnosticPiece>
+std::shared_ptr<PathDiagnosticPiece>
 CFRefLeakReportVisitor::getEndPath(BugReporterContext &BRC,
                                    const ExplodedNode *EndN, BugReport &BR) {
 
@@ -2459,7 +2449,7 @@ CFRefLeakReportVisitor::getEndPath(BugReporterContext &BRC,
     os << " is not referenced later in this execution path and has a retain "
           "count of +" << RV->getCount();
 
-  return llvm::make_unique<PathDiagnosticEventPiece>(L, os.str());
+  return std::make_shared<PathDiagnosticEventPiece>(L, os.str());
 }
 
 void CFRefLeakReport::deriveParamLocation(CheckerContext &Ctx, SymbolRef sym) {