[analyzer] Remove BugReporter.BugTypes.

[NoQ]

Summary

I'm slowly cleaning up BugReporter as a preparation for porting clang-tidy to use it exclusively as discussed in http://lists.llvm.org/pipermail/cfe-dev/2019-August/063092.html . Basically, we'll need to make a BugReport and a BugReporter that both can be compiled with CLANG_ENABLE_STATIC_ANALYZER=OFF: no "error nodes", no nothing. So we'll most likely make a clear separation between a basic bug report(er) and a path-sensitive bug report(er), having them inherit from common bug report(er) classes.

While figuring out which field/method goes where, i already made a couple of trivial commits (rC369319, rC369320). This one, however, is relatively interesting.

The BugTypes field is basically unused, and it definitely doesn't need to be implemented as an immutable set (if at all). But once i removed it i started noticing a bunch of null dereferences in generateDiagnosticForConsumerMap():

3060	  std::unique_ptr<DiagnosticForConsumerMapTy> Out =
3061	      generatePathDiagnostics(consumers, bugReports);
3062	 
3063	  if (Out->empty())   // <== crash: Out is null!
3064	    return Out;

This was fun to debug because it's obvious by looking at PathSensitiveBugReporter::generatePathDiagnostics() that it never returns null:

2643 std::unique_ptr<DiagnosticForConsumerMapTy>
2644 PathSensitiveBugReporter::generatePathDiagnostics(...) {
...
2649   auto Out = llvm::make_unique<DiagnosticForConsumerMapTy>();
...
2658   return Out;
2659 }

The debugger was also sure that we're dealing with a path-sensitive bug reporter here. The answer turned out to be related to this tiny discussion. Or, rather, here's a report that's worth a thousand words:

report-f2e935.html537 KBDownload

So i decided to avoid the destructor minefield entirely, even if it means calling flush manually.

Also, ugh, why is CloneChecker subscribed to check::EndOfTranslationUnit which is a path-sensitive callback? :/

[Szelethus]

So we'll most likely make a clear separation between a basic bug report(er) and a path-sensitive bug report(er), having them inherit from common bug report(er) classes.

That sounds terrific! Clang-tidy this or that, the analyzer itself would benefit from this greatly, we do have syntactic only checks.

[xazax.hun]

LG!

[NoQ]

Committed as rC369451 but forgot to add a phabricator link.

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

 /// and flush the corresponding diagnostics.
 ///
 /// The base class is used for generating path-insensitive
 class BugReporter {
 public:
   enum Kind { BasicBRKind, PathSensitiveBRKind };
 
 private:
-  using BugTypesTy = llvm::ImmutableSet<BugType *>;
-
-  BugTypesTy::Factory F;
-  BugTypesTy BugTypes;
-
   const Kind kind;
   BugReporterData& D;
 
   /// Generate and flush the diagnostics for the given bug report.
   void FlushReport(BugReportEquivClass& EQ);
 
   /// Generate the diagnostics for the given bug report.
   std::unique_ptr<DiagnosticForConsumerMapTy>
   generateDiagnosticForConsumerMap(BugReport *exampleReport,
                                    ArrayRef<PathDiagnosticConsumer *> consumers,
                                    ArrayRef<BugReport *> bugReports);
 
   /// The set of bug reports tracked by the BugReporter.
   llvm::FoldingSet<BugReportEquivClass> EQClasses;
 
   /// A vector of BugReports for tracking the allocated pointers and cleanup.
   std::vector<BugReportEquivClass *> EQClassesVector;
 
 protected:
   BugReporter(BugReporterData& d, Kind k)
-      : BugTypes(F.getEmptySet()), kind(k), D(d) {}
+      : kind(k), D(d) {}
 
 public:
-  BugReporter(BugReporterData& d)
-      : BugTypes(F.getEmptySet()), kind(BasicBRKind), D(d) {}
+  BugReporter(BugReporterData &d) : kind(BasicBRKind), D(d) {}
   virtual ~BugReporter();
 
   /// Generate and flush diagnostics for all bug reports.
   void FlushReports();
 
   Kind getKind() const { return kind; }
 
   DiagnosticsEngine& getDiagnostic() {
     return D.getDiagnostic();
   }
 
   ArrayRef<PathDiagnosticConsumer*> getPathDiagnosticConsumers() {
     return D.getPathDiagnosticConsumers();
   }
 
-  /// Iterator over the set of BugTypes tracked by the BugReporter.
-  using iterator = BugTypesTy::iterator;
-  iterator begin() { return BugTypes.begin(); }
-  iterator end() { return BugTypes.end(); }
-
   /// Iterator over the set of BugReports tracked by the BugReporter.
   using EQClasses_iterator = llvm::FoldingSet<BugReportEquivClass>::iterator;
   EQClasses_iterator EQClasses_begin() { return EQClasses.begin(); }
   EQClasses_iterator EQClasses_end() { return EQClasses.end(); }
 
   ASTContext &getContext() { return D.getASTContext(); }
 
   const SourceManager &getSourceManager() { return D.getSourceManager(); }
 
   const AnalyzerOptions &getAnalyzerOptions() { return D.getAnalyzerOptions(); }
 
   virtual std::unique_ptr<DiagnosticForConsumerMapTy>
   generatePathDiagnostics(ArrayRef<PathDiagnosticConsumer *> consumers,
                           ArrayRef<BugReport *> &bugReports) {
     return {};
   }
 
-  void Register(const BugType *BT);
-
   /// Add the given report to the set of reports tracked by BugReporter.
   ///
   /// The reports are usually generated by the checkers. Further, they are
   /// folded based on the profile value, which is done to coalesce similar
   /// reports.
   void emitReport(std::unique_ptr<BugReport> R);
 
   void EmitBasicReport(const Decl *DeclWithIssue, const CheckerBase *Checker,

 /// GRBugReporter is used for generating path-sensitive reports.
 class PathSensitiveBugReporter : public BugReporter {
   ExprEngine& Eng;
 
 public:
   PathSensitiveBugReporter(BugReporterData& d, ExprEngine& eng)
       : BugReporter(d, PathSensitiveBRKind), Eng(eng) {}
 
-  ~PathSensitiveBugReporter() override = default;
-
   /// getGraph - Get the exploded graph created by the analysis engine
   ///  for the analyzed method or function.
   const ExplodedGraph &getGraph() const;
 
   /// getStateManager - Return the state manager used by the analysis
   ///  engine.
   ProgramStateManager &getStateManager();
 

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

ProgramStateManager &PathSensitiveBugReporter::getStateManager() {
   return Eng.getStateManager();
 }
 
 ProgramStateManager &PathSensitiveBugReporter::getStateManager() const {
   return Eng.getStateManager();
 }
 
 BugReporter::~BugReporter() {
-  FlushReports();
+  // Make sure reports are flushed.
+  assert(StrBugTypes.empty() &&
+         "Destroying BugReporter before diagnostics are emitted!");
 
   // Free the bug reports we are tracking.
   for (const auto I : EQClassesVector)
     delete I;
 }
 
 void BugReporter::FlushReports() {
-  if (BugTypes.isEmpty())
-    return;
-
   // We need to flush reports in deterministic order to ensure the order
   // of the reports is consistent between runs.
   for (const auto EQ : EQClassesVector)
     FlushReport(*EQ);
 
   // BugReporter owns and deletes only BugTypes created implicitly through
   // EmitBasicReport.
   // FIXME: There are leaks from checkers that assume that the BugTypes they
   // create will be destroyed by the BugReporter.
   llvm::DeleteContainerSeconds(StrBugTypes);
-
-  // Remove all references to the BugType objects.
-  BugTypes = F.getEmptySet();
 }
 
 //===----------------------------------------------------------------------===//
 // PathDiagnostics generation.
 //===----------------------------------------------------------------------===//
 
 namespace {
 
PathSensitiveBugReporter::generatePathDiagnostics(
 
   if (PDB)
     for (PathDiagnosticConsumer *PC : consumers)
       (*Out)[PC] = PDB->generate(PC);
 
   return Out;
 }
 
-void BugReporter::Register(const BugType *BT) {
-  BugTypes = F.add(BugTypes, BT);
-}
-
 void BugReporter::emitReport(std::unique_ptr<BugReport> R) {
   if (const ExplodedNode *E = R->getErrorNode()) {
     // An error node must either be a sink or have a tag, otherwise
     // it could get reclaimed before the path diagnostic is created.
     assert((E->isSink() || E->getLocation().getTag()) &&
             "Error node must either be a sink or have a tag");
 
     const AnalysisDeclContext *DeclCtx =
void BugReporter::emitReport(std::unique_ptr<BugReport> R) {
   if (!ValidSourceLoc)
     return;
 
   // Compute the bug report's hash to determine its equivalence class.
   llvm::FoldingSetNodeID ID;
   R->Profile(ID);
 
   // Lookup the equivance class.  If there isn't one, create it.
-  const BugType& BT = R->getBugType();
-  Register(&BT);
   void *InsertPos;
   BugReportEquivClass* EQ = EQClasses.FindNodeOrInsertPos(ID, InsertPos);
 
   if (!EQ) {
     EQ = new BugReportEquivClass(std::move(R));
     EQClasses.InsertNode(EQ, InsertPos);
     EQClassesVector.push_back(EQ);
   } else

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

void AnalysisConsumer::runAnalysisOnTranslationUnit(ASTContext &C) {
   }
 
   if (Mgr->shouldInlineCall())
     HandleDeclsCallGraph(LocalTUDeclsSize);
 
   // After all decls handled, run checkers on the entire TranslationUnit.
   checkerMgr->runCheckersOnEndOfTranslationUnit(TU, *Mgr, BR);
 
+  BR.FlushReports();
   RecVisitorBR = nullptr;
 }
 
 void AnalysisConsumer::reportAnalyzerProgress(StringRef S) {
   if (Opts->AnalyzerDisplayProgress)
     llvm::errs() << S;
 }
 
void AnalysisConsumer::HandleCode(Decl *D, AnalysisMode Mode,
 
   if (Mode & AM_Syntax) {
     if (SyntaxCheckTimer)
       SyntaxCheckTimer->startTimer();
     checkerMgr->runCheckersOnASTBody(D, *Mgr, BR);
     if (SyntaxCheckTimer)
       SyntaxCheckTimer->stopTimer();
   }
+
+  BR.FlushReports();
+
   if ((Mode & AM_Path) && checkerMgr->hasPathSensitiveCheckers()) {
     RunPathSensitiveChecks(D, IMode, VisitedCallees);
     if (IMode != ExprEngine::Inline_Minimal)
       NumFunctionsAnalyzed++;
   }
 }
 
 //===----------------------------------------------------------------------===//