[analyzer] [RetainCountChecker] [NFC] Remove SummaryLog

The complicated machinery for passing the summary log around is actually
only used for one thing! To figure out whether the "dealloc" message was
sent.

Since I have tried to extend it for other uses and failed (it's actually
very hard to use), I think it's much better to simply use a tag and
remove the summary log altogether.

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

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

4 files changed, 46 insertions, 103 deletions

diff --git a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
index 4e6fd8490d..28873a465f 100644
--- a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
+++ b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
@@ -414,42 +414,6 @@ void RetainCountChecker::checkPostCall(const CallEvent &Call,
   checkSummary(*Summ, Call, C);
 }
 
-void RetainCountChecker::checkEndAnalysis(ExplodedGraph &G, BugReporter &BR,
-                                          ExprEngine &Eng) const {
-  // FIXME: This is a hack to make sure the summary log gets cleared between
-  // analyses of different code bodies.
-  //
-  // Why is this necessary? Because a checker's lifetime is tied to a
-  // translation unit, but an ExplodedGraph's lifetime is just a code body.
-  // Once in a blue moon, a new ExplodedNode will have the same address as an
-  // old one with an associated summary, and the bug report visitor gets very
-  // confused. (To make things worse, the summary lifetime is currently also
-  // tied to a code body, so we get a crash instead of incorrect results.)
-  //
-  // Why is this a bad solution? Because if the lifetime of the ExplodedGraph
-  // changes, things will start going wrong again. Really the lifetime of this
-  // log needs to be tied to either the specific nodes in it or the entire
-  // ExplodedGraph, not to a specific part of the code being analyzed.
-  //
-  // (Also, having stateful local data means that the same checker can't be
-  // used from multiple threads, but a lot of checkers have incorrect
-  // assumptions about that anyway. So that wasn't a priority at the time of
-  // this fix.)
-  //
-  // This happens at the end of analysis, but bug reports are emitted /after/
-  // this point. So we can't just clear the summary log now. Instead, we mark
-  // that the next time we access the summary log, it should be cleared.
-
-  // If we never reset the summary log during /this/ code body analysis,
-  // there were no new summaries. There might still have been summaries from
-  // the /last/ analysis, so clear them out to make sure the bug report
-  // visitors don't get confused.
-  if (ShouldResetSummaryLog)
-    SummaryLog.clear();
-
-  ShouldResetSummaryLog = !SummaryLog.empty();
-}
-
 CFRefBug *
 RetainCountChecker::getLeakWithinFunctionBug(const LangOptions &LOpts) const {
   if (!leakWithinFunction)
@@ -609,6 +573,11 @@ void RetainCountChecker::checkSummary(const RetainSummary &Summ,
   SourceRange ErrorRange;
   SymbolRef ErrorSym = nullptr;
 
+  // Helper tag for providing diagnostics: indicate whether dealloc was sent
+  // at this location.
+  static CheckerProgramPointTag DeallocSentTag(this, DeallocTagDescription);
+  bool DeallocSent = false;
+
   for (unsigned idx = 0, e = CallOrMsg.getNumArgs(); idx != e; ++idx) {
     SVal V = CallOrMsg.getArgSVal(idx);
 
@@ -627,6 +596,8 @@ void RetainCountChecker::checkSummary(const RetainSummary &Summ,
           ErrorRange = CallOrMsg.getArgSourceRange(idx);
           ErrorSym = Sym;
           break;
+        } else if (Effect.getKind() == Dealloc) {
+          DeallocSent = true;
         }
       }
     }
@@ -644,6 +615,8 @@ void RetainCountChecker::checkSummary(const RetainSummary &Summ,
           if (hasErr) {
             ErrorRange = MsgInvocation->getOriginExpr()->getReceiverRange();
             ErrorSym = Sym;
+          } else if (Summ.getReceiverEffect().getKind() == Dealloc) {
+            DeallocSent = true;
           }
         }
       }
@@ -688,24 +661,10 @@ void RetainCountChecker::checkSummary(const RetainSummary &Summ,
       state = setRefBinding(state, Sym, *updatedRefVal);
   }
 
-  // This check is actually necessary; otherwise the statement builder thinks
-  // we've hit a previously-found path.
-  // Normally addTransition takes care of this, but we want the node pointer.
-  ExplodedNode *NewNode;
-  if (state == C.getState()) {
-    NewNode = C.getPredecessor();
+  if (DeallocSent) {
+    C.addTransition(state, C.getPredecessor(), &DeallocSentTag);
   } else {
-    NewNode = C.addTransition(state);
-  }
-
-  // Annotate the node with summary we used.
-  if (NewNode) {
-    // FIXME: This is ugly. See checkEndAnalysis for why it's necessary.
-    if (ShouldResetSummaryLog) {
-      SummaryLog.clear();
-      ShouldResetSummaryLog = false;
-    }
-    SummaryLog[NewNode] = &Summ;
+    C.addTransition(state);
   }
 }
 
@@ -744,7 +703,7 @@ ProgramStateRef RetainCountChecker::updateSymbol(ProgramStateRef state,
       llvm_unreachable("Applies to pointer-to-pointer parameters, which should "
                        "not have ref state.");
 
-    case Dealloc:
+    case Dealloc: // NB. we only need to add a note in a non-error case.
       switch (V.getKind()) {
         default:
           llvm_unreachable("Invalid RefVal state for an explicit dealloc.");
@@ -880,7 +839,7 @@ void RetainCountChecker::processNonLeakError(ProgramStateRef St,
 
   assert(BT);
   auto report = llvm::make_unique<CFRefReport>(
-      *BT, C.getASTContext().getLangOpts(), SummaryLog, N, Sym);
+      *BT, C.getASTContext().getLangOpts(), N, Sym);
   report->addRange(ErrorRange);
   C.emitReport(std::move(report));
 }
@@ -1084,7 +1043,7 @@ ExplodedNode * RetainCountChecker::checkReturnWithRetEffect(const ReturnStmt *S,
         if (N) {
           const LangOptions &LOpts = C.getASTContext().getLangOpts();
           auto R = llvm::make_unique<CFRefLeakReport>(
-              *getLeakAtReturnBug(LOpts), LOpts, SummaryLog, N, Sym, C);
+              *getLeakAtReturnBug(LOpts), LOpts, N, Sym, C);
           C.emitReport(std::move(R));
         }
         return N;
@@ -1112,8 +1071,7 @@ ExplodedNode * RetainCountChecker::checkReturnWithRetEffect(const ReturnStmt *S,
             returnNotOwnedForOwned.reset(new ReturnedNotOwnedForOwned(this));
 
           auto R = llvm::make_unique<CFRefReport>(
-              *returnNotOwnedForOwned, C.getASTContext().getLangOpts(),
-              SummaryLog, N, Sym);
+              *returnNotOwnedForOwned, C.getASTContext().getLangOpts(), N, Sym);
           C.emitReport(std::move(R));
         }
         return N;
@@ -1316,8 +1274,8 @@ RetainCountChecker::handleAutoreleaseCounts(ProgramStateRef state,
       overAutorelease.reset(new OverAutorelease(this));
 
     const LangOptions &LOpts = Ctx.getASTContext().getLangOpts();
-    auto R = llvm::make_unique<CFRefReport>(*overAutorelease, LOpts, SummaryLog,
-                                            N, Sym, os.str());
+    auto R = llvm::make_unique<CFRefReport>(*overAutorelease, LOpts, N, Sym,
+                                            os.str());
     Ctx.emitReport(std::move(R));
   }
 
@@ -1369,8 +1327,8 @@ RetainCountChecker::processLeaks(ProgramStateRef state,
                           : getLeakAtReturnBug(LOpts);
       assert(BT && "BugType not initialized.");
 
-      Ctx.emitReport(llvm::make_unique<CFRefLeakReport>(
-          *BT, LOpts, SummaryLog, N, *I, Ctx));
+      Ctx.emitReport(
+          llvm::make_unique<CFRefLeakReport>(*BT, LOpts, N, *I, Ctx));
     }
   }
 
diff --git a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h
index d8bbe6fcdd..0c233f72dd 100644
--- a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h
+++ b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h
@@ -239,7 +239,6 @@ public:
 class RetainCountChecker
   : public Checker< check::Bind,
                     check::DeadSymbols,
-                    check::EndAnalysis,
                     check::BeginFunction,
                     check::EndFunction,
                     check::PostStmt<BlockExpr>,
@@ -263,11 +262,8 @@ class RetainCountChecker
   mutable SymbolTagMap DeadSymbolTags;
 
   mutable std::unique_ptr<RetainSummaryManager> Summaries;
-  mutable SummaryLogTy SummaryLog;
-
-  mutable bool ShouldResetSummaryLog;
-
 public:
+  static constexpr const char *DeallocTagDescription = "DeallocSent";
 
   /// Track Objective-C and CoreFoundation objects.
   bool TrackObjCAndCFObjects = false;
@@ -275,13 +271,10 @@ public:
   /// Track sublcasses of OSObject.
   bool TrackOSObjects = false;
 
-  RetainCountChecker() : ShouldResetSummaryLog(false) {}
+  RetainCountChecker() {}
 
   ~RetainCountChecker() override { DeleteContainerSeconds(DeadSymbolTags); }
 
-  void checkEndAnalysis(ExplodedGraph &G, BugReporter &BR,
-                        ExprEngine &Eng) const;
-
   CFRefBug *getLeakWithinFunctionBug(const LangOptions &LOpts) const;
 
   CFRefBug *getLeakAtReturnBug(const LangOptions &LOpts) const;
diff --git a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
index 5a687ef8d1..0f01d9a995 100644
--- a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
+++ b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
@@ -43,14 +43,12 @@ static std::string getPrettyTypeName(QualType QT) {
 /// return whether the note should be generated.
 static bool shouldGenerateNote(llvm::raw_string_ostream &os,
                                const RefVal *PrevT, const RefVal &CurrV,
-                               SmallVector<ArgEffect, 2> &AEffects) {
+                               bool DeallocSent) {
   // Get the previous type state.
   RefVal PrevV = *PrevT;
 
   // Specially handle -dealloc.
-  if (std::find_if(AEffects.begin(), AEffects.end(), [](ArgEffect &E) {
-        return E.getKind() == Dealloc;
-      }) != AEffects.end()) {
+  if (DeallocSent) {
     // Determine if the object's reference count was pushed to zero.
     assert(!PrevV.hasSameState(CurrV) && "The state should have changed.");
     // We may not have transitioned to 'release' if we hit an error.
@@ -194,11 +192,9 @@ namespace retaincountchecker {
 class CFRefReportVisitor : public BugReporterVisitor {
 protected:
   SymbolRef Sym;
-  const SummaryLogTy &SummaryLog;
 
 public:
-  CFRefReportVisitor(SymbolRef sym, const SummaryLogTy &log)
-      : Sym(sym), SummaryLog(log) {}
+  CFRefReportVisitor(SymbolRef sym) : Sym(sym) {}
 
   void Profile(llvm::FoldingSetNodeID &ID) const override {
     static int x = 0;
@@ -217,9 +213,7 @@ public:
 
 class CFRefLeakReportVisitor : public CFRefReportVisitor {
 public:
-  CFRefLeakReportVisitor(SymbolRef sym,
-                         const SummaryLogTy &log)
-     : CFRefReportVisitor(sym, log) {}
+  CFRefLeakReportVisitor(SymbolRef sym) : CFRefReportVisitor(sym) {}
 
   std::shared_ptr<PathDiagnosticPiece> getEndPath(BugReporterContext &BRC,
                                                   const ExplodedNode *N,
@@ -311,6 +305,7 @@ annotateConsumedSummaryMismatch(const ExplodedNode *N,
 std::shared_ptr<PathDiagnosticPiece>
 CFRefReportVisitor::VisitNode(const ExplodedNode *N,
                               BugReporterContext &BRC, BugReport &BR) {
+
   const SourceManager &SM = BRC.getSourceManager();
   CallEventManager &CEMgr = BRC.getStateManager().getCallEventManager();
   if (auto CE = N->getLocationAs<CallExitBegin>())
@@ -383,9 +378,11 @@ CFRefReportVisitor::VisitNode(const ExplodedNode *N,
 
   // Gather up the effects that were performed on the object at this
   // program point
-  SmallVector<ArgEffect, 2> AEffects;
-  const ExplodedNode *OrigNode = BRC.getNodeResolver().getOriginalNode(N);
-  if (const RetainSummary *Summ = SummaryLog.lookup(OrigNode)) {
+  bool DeallocSent = false;
+
+  if (N->getLocation().getTag() &&
+      N->getLocation().getTag()->getTagDescription().contains(
+          RetainCountChecker::DeallocTagDescription)) {
     // We only have summaries attached to nodes after evaluating CallExpr and
     // ObjCMessageExprs.
     const Stmt *S = N->getLocation().castAs<StmtPoint>().getStmt();
@@ -403,20 +400,20 @@ CFRefReportVisitor::VisitNode(const ExplodedNode *N,
           continue;
 
         // We have an argument.  Get the effect!
-        AEffects.push_back(Summ->getArg(i));
+        DeallocSent = true;
       }
     } else if (const ObjCMessageExpr *ME = dyn_cast<ObjCMessageExpr>(S)) {
       if (const Expr *receiver = ME->getInstanceReceiver()) {
         if (CurrSt->getSValAsScalarOrLoc(receiver, LCtx)
               .getAsLocSymbol() == Sym) {
           // The symbol we are tracking is the receiver.
-          AEffects.push_back(Summ->getReceiverEffect());
+          DeallocSent = true;
         }
       }
     }
   }
 
-  if (!shouldGenerateNote(os, PrevT, CurrV, AEffects))
+  if (!shouldGenerateNote(os, PrevT, CurrV, DeallocSent))
     return nullptr;
 
   if (os.str().empty())
@@ -639,20 +636,18 @@ CFRefLeakReportVisitor::getEndPath(BugReporterContext &BRC,
   return std::make_shared<PathDiagnosticEventPiece>(L, os.str());
 }
 
-CFRefReport::CFRefReport(CFRefBug &D, const LangOptions &LOpts,
-                         const SummaryLogTy &Log, ExplodedNode *n,
+CFRefReport::CFRefReport(CFRefBug &D, const LangOptions &LOpts, ExplodedNode *n,
                          SymbolRef sym, bool registerVisitor)
     : BugReport(D, D.getDescription(), n), Sym(sym) {
   if (registerVisitor)
-    addVisitor(llvm::make_unique<CFRefReportVisitor>(sym, Log));
+    addVisitor(llvm::make_unique<CFRefReportVisitor>(sym));
 }
 
-CFRefReport::CFRefReport(CFRefBug &D, const LangOptions &LOpts,
-                         const SummaryLogTy &Log, ExplodedNode *n,
+CFRefReport::CFRefReport(CFRefBug &D, const LangOptions &LOpts, ExplodedNode *n,
                          SymbolRef sym, StringRef endText)
     : BugReport(D, D.getDescription(), endText, n) {
 
-  addVisitor(llvm::make_unique<CFRefReportVisitor>(sym, Log));
+  addVisitor(llvm::make_unique<CFRefReportVisitor>(sym));
 }
 
 void CFRefLeakReport::deriveParamLocation(CheckerContext &Ctx, SymbolRef sym) {
@@ -665,7 +660,8 @@ void CFRefLeakReport::deriveParamLocation(CheckerContext &Ctx, SymbolRef sym) {
   if (Region) {
     const Decl *PDecl = Region->getDecl();
     if (PDecl && isa<ParmVarDecl>(PDecl)) {
-      PathDiagnosticLocation ParamLocation = PathDiagnosticLocation::create(PDecl, SMgr);
+      PathDiagnosticLocation ParamLocation =
+          PathDiagnosticLocation::create(PDecl, SMgr);
       Location = ParamLocation;
       UniqueingLocation = ParamLocation;
       UniqueingDecl = Ctx.getLocationContext()->getDecl();
@@ -733,10 +729,9 @@ void CFRefLeakReport::createDescription(CheckerContext &Ctx) {
 }
 
 CFRefLeakReport::CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts,
-                                 const SummaryLogTy &Log,
                                  ExplodedNode *n, SymbolRef sym,
                                  CheckerContext &Ctx)
-  : CFRefReport(D, LOpts, Log, n, sym, false) {
+  : CFRefReport(D, LOpts, n, sym, false) {
 
   deriveAllocLocation(Ctx, sym);
   if (!AllocBinding)
@@ -744,5 +739,5 @@ CFRefLeakReport::CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts,
 
   createDescription(Ctx);
 
-  addVisitor(llvm::make_unique<CFRefLeakReportVisitor>(sym, Log));
+  addVisitor(llvm::make_unique<CFRefLeakReportVisitor>(sym));
 }
diff --git a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
index a30f62ac34..f27027ab62 100644
--- a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
+++ b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
@@ -37,20 +37,17 @@ public:
   virtual bool isLeak() const { return false; }
 };
 
-typedef ::llvm::DenseMap<const ExplodedNode *, const RetainSummary *>
-  SummaryLogTy;
-
 class CFRefReport : public BugReport {
 protected:
   SymbolRef Sym;
 
 public:
   CFRefReport(CFRefBug &D, const LangOptions &LOpts,
-              const SummaryLogTy &Log, ExplodedNode *n, SymbolRef sym,
+              ExplodedNode *n, SymbolRef sym,
               bool registerVisitor = true);
 
   CFRefReport(CFRefBug &D, const LangOptions &LOpts,
-              const SummaryLogTy &Log, ExplodedNode *n, SymbolRef sym,
+              ExplodedNode *n, SymbolRef sym,
               StringRef endText);
 
   llvm::iterator_range<ranges_iterator> getRanges() override {
@@ -75,7 +72,7 @@ class CFRefLeakReport : public CFRefReport {
 
 public:
   CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts,
-                  const SummaryLogTy &Log, ExplodedNode *n, SymbolRef sym,
+                  ExplodedNode *n, SymbolRef sym,
                   CheckerContext &Ctx);
 
   PathDiagnosticLocation getLocation(const SourceManager &SM) const override {