[analyzer] Clean up slightly the messed up ownership model of the analyzer (#128368)

steakhal · web-flow · commit 3dc159431be7 · 2025-02-24T11:34:36.000+01:00
Well, yes. It's not pretty. At least after this we would have a bit more unique pointers than before. This is for fixing the memory leak diagnosed by: https://lab.llvm.org/buildbot/#/builders/24/builds/5580 And that caused the revert of #127409. After these uptrs that patch can re-land finally.
diff --git a/clang/include/clang/Analysis/AnalysisDeclContext.h b/clang/include/clang/Analysis/AnalysisDeclContext.h
@@ -451,7 +451,7 @@ class AnalysisDeclContextManager {
       bool synthesizeBodies = false, bool addStaticInitBranches = false,
       bool addCXXNewAllocator = true, bool addRichCXXConstructors = true,
       bool markElidedCXXConstructors = true, bool addVirtualBaseBranches = true,
-      CodeInjector *injector = nullptr);
+      std::unique_ptr<CodeInjector> injector = nullptr);
 
   AnalysisDeclContext *getContext(const Decl *D);
 
diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
@@ -570,7 +570,8 @@ class BugReporterData {
 public:
   virtual ~BugReporterData() = default;
 
-  virtual ArrayRef<PathDiagnosticConsumer*> getPathDiagnosticConsumers() = 0;
+  virtual ArrayRef<std::unique_ptr<PathDiagnosticConsumer>>
+  getPathDiagnosticConsumers() = 0;
   virtual ASTContext &getASTContext() = 0;
   virtual SourceManager &getSourceManager() = 0;
   virtual AnalyzerOptions &getAnalyzerOptions() = 0;
@@ -608,7 +609,8 @@ class BugReporter {
   /// Generate and flush diagnostics for all bug reports.
   void FlushReports();
 
-  ArrayRef<PathDiagnosticConsumer*> getPathDiagnosticConsumers() {
+  ArrayRef<std::unique_ptr<PathDiagnosticConsumer>>
+  getPathDiagnosticConsumers() {
     return D.getPathDiagnosticConsumers();
   }
 
@@ -670,9 +672,10 @@ class BugReporter {
 protected:
   /// Generate the diagnostics for the given bug report.
   virtual std::unique_ptr<DiagnosticForConsumerMapTy>
-  generateDiagnosticForConsumerMap(BugReport *exampleReport,
-                                   ArrayRef<PathDiagnosticConsumer *> consumers,
-                                   ArrayRef<BugReport *> bugReports);
+  generateDiagnosticForConsumerMap(
+      BugReport *exampleReport,
+      ArrayRef<std::unique_ptr<PathDiagnosticConsumer>> consumers,
+      ArrayRef<BugReport *> bugReports);
 };
 
 /// GRBugReporter is used for generating path-sensitive reports.
@@ -684,10 +687,11 @@ class PathSensitiveBugReporter final : public BugReporter {
       SmallVectorImpl<BugReport *> &bugReports) override;
 
   /// Generate the diagnostics for the given bug report.
-  std::unique_ptr<DiagnosticForConsumerMapTy>
-  generateDiagnosticForConsumerMap(BugReport *exampleReport,
-                                   ArrayRef<PathDiagnosticConsumer *> consumers,
-                                   ArrayRef<BugReport *> bugReports) override;
+  std::unique_ptr<DiagnosticForConsumerMapTy> generateDiagnosticForConsumerMap(
+      BugReport *exampleReport,
+      ArrayRef<std::unique_ptr<PathDiagnosticConsumer>> consumers,
+      ArrayRef<BugReport *> bugReports) override;
+
 public:
   PathSensitiveBugReporter(BugReporterData& d, ExprEngine& eng)
       : BugReporter(d), Eng(eng) {}
@@ -706,7 +710,7 @@ class PathSensitiveBugReporter final : public BugReporter {
   /// Iterates through the bug reports within a single equivalence class,
   /// stops at a first non-invalidated report.
   std::unique_ptr<DiagnosticForConsumerMapTy> generatePathDiagnostics(
-      ArrayRef<PathDiagnosticConsumer *> consumers,
+      ArrayRef<std::unique_ptr<PathDiagnosticConsumer>> consumers,
       ArrayRef<PathSensitiveBugReport *> &bugReports);
 
   void emitReport(std::unique_ptr<BugReport> R) override;
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h b/clang/include/clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h
@@ -30,7 +30,8 @@ class CrossTranslationUnitContext;
 namespace ento {
 
 class PathDiagnosticConsumer;
-typedef std::vector<PathDiagnosticConsumer*> PathDiagnosticConsumers;
+using PathDiagnosticConsumers =
+    std::vector<std::unique_ptr<PathDiagnosticConsumer>>;
 
 #define ANALYSIS_DIAGNOSTICS(NAME, CMDFLAG, DESC, CREATEFN)                    \
   void CREATEFN(PathDiagnosticConsumerOptions Diagopts,                        \
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h
@@ -47,11 +47,11 @@ class AnalysisManager : public BugReporterData {
   AnalyzerOptions &options;
 
   AnalysisManager(ASTContext &ctx, Preprocessor &PP,
-                  const PathDiagnosticConsumers &Consumers,
+                  PathDiagnosticConsumers Consumers,
                   StoreManagerCreator storemgr,
                   ConstraintManagerCreator constraintmgr,
                   CheckerManager *checkerMgr, AnalyzerOptions &Options,
-                  CodeInjector *injector = nullptr);
+                  std::unique_ptr<CodeInjector> injector = nullptr);
 
   ~AnalysisManager() override;
 
@@ -91,7 +91,8 @@ class AnalysisManager : public BugReporterData {
     return LangOpts;
   }
 
-  ArrayRef<PathDiagnosticConsumer*> getPathDiagnosticConsumers() override {
+  ArrayRef<std::unique_ptr<PathDiagnosticConsumer>>
+  getPathDiagnosticConsumers() override {
     return PathConsumers;
   }
 
diff --git a/clang/include/clang/StaticAnalyzer/Frontend/AnalysisConsumer.h b/clang/include/clang/StaticAnalyzer/Frontend/AnalysisConsumer.h
@@ -29,7 +29,8 @@ class CheckerRegistry;
 
 class AnalysisASTConsumer : public ASTConsumer {
 public:
-  virtual void AddDiagnosticConsumer(PathDiagnosticConsumer *Consumer) = 0;
+  virtual void
+  AddDiagnosticConsumer(std::unique_ptr<PathDiagnosticConsumer> Consumer) = 0;
 
   /// This method allows registering statically linked custom checkers that are
   /// not a part of the Clang tree. It employs the same mechanism that is used
diff --git a/clang/lib/Analysis/AnalysisDeclContext.cpp b/clang/lib/Analysis/AnalysisDeclContext.cpp
@@ -71,8 +71,8 @@ AnalysisDeclContextManager::AnalysisDeclContextManager(
     bool addLoopExit, bool addScopes, bool synthesizeBodies,
     bool addStaticInitBranch, bool addCXXNewAllocator,
     bool addRichCXXConstructors, bool markElidedCXXConstructors,
-    bool addVirtualBaseBranches, CodeInjector *injector)
-    : Injector(injector), FunctionBodyFarm(ASTCtx, injector),
+    bool addVirtualBaseBranches, std::unique_ptr<CodeInjector> injector)
+    : Injector(std::move(injector)), FunctionBodyFarm(ASTCtx, Injector.get()),
       SynthesizeBodies(synthesizeBodies) {
   cfgBuildOptions.PruneTriviallyFalseEdges = !useUnoptimizedCFG;
   cfgBuildOptions.AddImplicitDtors = addImplicitDtors;
diff --git a/clang/lib/StaticAnalyzer/Core/AnalysisManager.cpp b/clang/lib/StaticAnalyzer/Core/AnalysisManager.cpp
@@ -14,32 +14,28 @@ using namespace ento;
 void AnalysisManager::anchor() { }
 
 AnalysisManager::AnalysisManager(ASTContext &ASTCtx, Preprocessor &PP,
-                                 const PathDiagnosticConsumers &PDC,
+                                 PathDiagnosticConsumers PDC,
                                  StoreManagerCreator storemgr,
                                  ConstraintManagerCreator constraintmgr,
                                  CheckerManager *checkerMgr,
                                  AnalyzerOptions &Options,
-                                 CodeInjector *injector)
+                                 std::unique_ptr<CodeInjector> injector)
     : AnaCtxMgr(
           ASTCtx, Options.UnoptimizedCFG,
           Options.ShouldIncludeImplicitDtorsInCFG,
-          /*addInitializers=*/true,
-          Options.ShouldIncludeTemporaryDtorsInCFG,
+          /*addInitializers=*/true, Options.ShouldIncludeTemporaryDtorsInCFG,
           Options.ShouldIncludeLifetimeInCFG,
           // Adding LoopExit elements to the CFG is a requirement for loop
           // unrolling.
-          Options.ShouldIncludeLoopExitInCFG ||
-            Options.ShouldUnrollLoops,
-          Options.ShouldIncludeScopesInCFG,
-          Options.ShouldSynthesizeBodies,
+          Options.ShouldIncludeLoopExitInCFG || Options.ShouldUnrollLoops,
+          Options.ShouldIncludeScopesInCFG, Options.ShouldSynthesizeBodies,
           Options.ShouldConditionalizeStaticInitializers,
           /*addCXXNewAllocator=*/true,
           Options.ShouldIncludeRichConstructorsInCFG,
           Options.ShouldElideConstructors,
-          /*addVirtualBaseBranches=*/true,
-          injector),
+          /*addVirtualBaseBranches=*/true, std::move(injector)),
       Ctx(ASTCtx), PP(PP), LangOpts(ASTCtx.getLangOpts()),
-      PathConsumers(PDC), CreateStoreMgr(storemgr),
+      PathConsumers(std::move(PDC)), CreateStoreMgr(storemgr),
       CreateConstraintMgr(constraintmgr), CheckerMgr(checkerMgr),
       options(Options) {
   AnaCtxMgr.getCFGBuildOptions().setAllAlwaysAdd();
@@ -50,14 +46,11 @@ AnalysisManager::AnalysisManager(ASTContext &ASTCtx, Preprocessor &PP,
 
 AnalysisManager::~AnalysisManager() {
   FlushDiagnostics();
-  for (PathDiagnosticConsumer *Consumer : PathConsumers) {
-    delete Consumer;
-  }
 }
 
 void AnalysisManager::FlushDiagnostics() {
   PathDiagnosticConsumer::FilesMade filesMade;
-  for (PathDiagnosticConsumer *Consumer : PathConsumers) {
+  for (const auto &Consumer : PathConsumers) {
     Consumer->FlushDiagnostics(&filesMade);
   }
 }
diff --git a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -2958,7 +2958,7 @@ std::optional<PathDiagnosticBuilder> PathDiagnosticBuilder::findValidReport(
 
 std::unique_ptr<DiagnosticForConsumerMapTy>
 PathSensitiveBugReporter::generatePathDiagnostics(
-    ArrayRef<PathDiagnosticConsumer *> consumers,
+    ArrayRef<std::unique_ptr<PathDiagnosticConsumer>> consumers,
     ArrayRef<PathSensitiveBugReport *> &bugReports) {
   assert(!bugReports.empty());
 
@@ -2968,9 +2968,9 @@ PathSensitiveBugReporter::generatePathDiagnostics(
       PathDiagnosticBuilder::findValidReport(bugReports, *this);
 
   if (PDB) {
-    for (PathDiagnosticConsumer *PC : consumers) {
-      if (std::unique_ptr<PathDiagnostic> PD = PDB->generate(PC)) {
-        (*Out)[PC] = std::move(PD);
+    for (const auto &PC : consumers) {
+      if (std::unique_ptr<PathDiagnostic> PD = PDB->generate(PC.get())) {
+        (*Out)[PC.get()] = std::move(PD);
       }
     }
   }
@@ -3164,7 +3164,7 @@ void BugReporter::FlushReport(BugReportEquivClass &EQ) {
       return;
   }
 
-  ArrayRef<PathDiagnosticConsumer*> Consumers = getPathDiagnosticConsumers();
+  ArrayRef Consumers = getPathDiagnosticConsumers();
   std::unique_ptr<DiagnosticForConsumerMapTy> Diagnostics =
       generateDiagnosticForConsumerMap(report, Consumers, bugReports);
 
@@ -3298,12 +3298,13 @@ findExecutedLines(const SourceManager &SM, const ExplodedNode *N) {
 
 std::unique_ptr<DiagnosticForConsumerMapTy>
 BugReporter::generateDiagnosticForConsumerMap(
-    BugReport *exampleReport, ArrayRef<PathDiagnosticConsumer *> consumers,
+    BugReport *exampleReport,
+    ArrayRef<std::unique_ptr<PathDiagnosticConsumer>> consumers,
     ArrayRef<BugReport *> bugReports) {
   auto *basicReport = cast<BasicBugReport>(exampleReport);
   auto Out = std::make_unique<DiagnosticForConsumerMapTy>();
-  for (auto *Consumer : consumers)
-    (*Out)[Consumer] =
+  for (const auto &Consumer : consumers)
+    (*Out)[Consumer.get()] =
         generateDiagnosticForBasicReport(basicReport, AnalysisEntryPoint);
   return Out;
 }
@@ -3371,11 +3372,10 @@ static void resetDiagnosticLocationToMainFile(PathDiagnostic &PD) {
   }
 }
 
-
-
 std::unique_ptr<DiagnosticForConsumerMapTy>
 PathSensitiveBugReporter::generateDiagnosticForConsumerMap(
-    BugReport *exampleReport, ArrayRef<PathDiagnosticConsumer *> consumers,
+    BugReport *exampleReport,
+    ArrayRef<std::unique_ptr<PathDiagnosticConsumer>> consumers,
     ArrayRef<BugReport *> bugReports) {
   std::vector<BasicBugReport *> BasicBugReports;
   std::vector<PathSensitiveBugReport *> PathSensitiveBugReports;
diff --git a/clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp b/clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
@@ -193,7 +193,8 @@ void ento::createHTMLDiagnosticConsumer(
   if (OutputDir.empty())
     return;
 
-  C.push_back(new HTMLDiagnostics(std::move(DiagOpts), OutputDir, PP, true));
+  C.emplace_back(std::make_unique<HTMLDiagnostics>(std::move(DiagOpts),
+                                                   OutputDir, PP, true));
 }
 
 void ento::createHTMLSingleFileDiagnosticConsumer(
@@ -208,7 +209,8 @@ void ento::createHTMLSingleFileDiagnosticConsumer(
   if (OutputDir.empty())
     return;
 
-  C.push_back(new HTMLDiagnostics(std::move(DiagOpts), OutputDir, PP, false));
+  C.emplace_back(std::make_unique<HTMLDiagnostics>(std::move(DiagOpts),
+                                                   OutputDir, PP, false));
 }
 
 void ento::createPlistHTMLDiagnosticConsumer(
diff --git a/clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp b/clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
@@ -541,9 +541,9 @@ void ento::createPlistDiagnosticConsumer(
   if (OutputFile.empty())
     return;
 
-  C.push_back(new PlistDiagnostics(DiagOpts, OutputFile, PP, CTU,
-                                   MacroExpansions,
-                                   /*supportsMultipleFiles=*/false));
+  C.push_back(std::make_unique<PlistDiagnostics>(
+      DiagOpts, OutputFile, PP, CTU, MacroExpansions,
+      /*supportsMultipleFiles=*/false));
   createTextMinimalPathDiagnosticConsumer(std::move(DiagOpts), C, OutputFile,
                                           PP, CTU, MacroExpansions);
 }
@@ -558,9 +558,9 @@ void ento::createPlistMultiFileDiagnosticConsumer(
   if (OutputFile.empty())
     return;
 
-  C.push_back(new PlistDiagnostics(DiagOpts, OutputFile, PP, CTU,
-                                   MacroExpansions,
-                                   /*supportsMultipleFiles=*/true));
+  C.push_back(std::make_unique<PlistDiagnostics>(
+      DiagOpts, OutputFile, PP, CTU, MacroExpansions,
+      /*supportsMultipleFiles=*/true));
   createTextMinimalPathDiagnosticConsumer(std::move(DiagOpts), C, OutputFile,
                                           PP, CTU, MacroExpansions);
 }
diff --git a/clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp b/clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
@@ -23,6 +23,7 @@
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/JSON.h"
 #include "llvm/Support/Path.h"
+#include <memory>
 
 using namespace llvm;
 using namespace clang;
@@ -60,8 +61,8 @@ void ento::createSarifDiagnosticConsumer(
   if (Output.empty())
     return;
 
-  C.push_back(
-      new SarifDiagnostics(Output, PP.getLangOpts(), PP.getSourceManager()));
+  C.push_back(std::make_unique<SarifDiagnostics>(Output, PP.getLangOpts(),
+                                                 PP.getSourceManager()));
   createTextMinimalPathDiagnosticConsumer(std::move(DiagOpts), C, Output, PP,
                                           CTU, MacroExpansions);
 }
diff --git a/clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp b/clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
@@ -90,7 +90,7 @@ class AnalysisConsumer : public AnalysisASTConsumer,
   const std::string OutDir;
   AnalyzerOptions &Opts;
   ArrayRef<std::string> Plugins;
-  CodeInjector *Injector;
+  std::unique_ptr<CodeInjector> Injector;
   cross_tu::CrossTranslationUnitContext CTU;
 
   /// Stores the declarations from the local translation unit.
@@ -123,10 +123,10 @@ class AnalysisConsumer : public AnalysisASTConsumer,
 
   AnalysisConsumer(CompilerInstance &CI, const std::string &outdir,
                    AnalyzerOptions &opts, ArrayRef<std::string> plugins,
-                   CodeInjector *injector)
+                   std::unique_ptr<CodeInjector> injector)
       : RecVisitorMode(0), RecVisitorBR(nullptr), Ctx(nullptr),
-        PP(CI.getPreprocessor()), OutDir(outdir), Opts(opts),
-        Plugins(plugins), Injector(injector), CTU(CI),
+        PP(CI.getPreprocessor()), OutDir(outdir), Opts(opts), Plugins(plugins),
+        Injector(std::move(injector)), CTU(CI),
         MacroExpansions(CI.getLangOpts()) {
     DigestAnalyzerOptions();
     if (Opts.AnalyzerDisplayProgress || Opts.PrintStats ||
@@ -229,9 +229,9 @@ class AnalysisConsumer : public AnalysisASTConsumer,
     checkerMgr = std::make_unique<CheckerManager>(*Ctx, Opts, PP, Plugins,
                                                   CheckerRegistrationFns);
 
-    Mgr = std::make_unique<AnalysisManager>(*Ctx, PP, PathConsumers,
-                                            CreateStoreMgr, CreateConstraintMgr,
-                                            checkerMgr.get(), Opts, Injector);
+    Mgr = std::make_unique<AnalysisManager>(
+        *Ctx, PP, std::move(PathConsumers), CreateStoreMgr, CreateConstraintMgr,
+        checkerMgr.get(), Opts, std::move(Injector));
   }
 
   /// Store the top level decls in the set to be processed later on.
@@ -342,8 +342,9 @@ class AnalysisConsumer : public AnalysisASTConsumer,
     return true;
   }
 
-  void AddDiagnosticConsumer(PathDiagnosticConsumer *Consumer) override {
-    PathConsumers.push_back(Consumer);
+  void AddDiagnosticConsumer(
+      std::unique_ptr<PathDiagnosticConsumer> Consumer) override {
+    PathConsumers.push_back(std::move(Consumer));
   }
 
   void AddCheckerRegistrationFn(std::function<void(CheckerRegistry&)> Fn) override {
@@ -794,5 +795,5 @@ ento::CreateAnalysisConsumer(CompilerInstance &CI) {
   return std::make_unique<AnalysisConsumer>(
       CI, CI.getFrontendOpts().OutputFile, analyzerOpts,
       CI.getFrontendOpts().Plugins,
-      hasModelPath ? new ModelInjector(CI) : nullptr);
+      hasModelPath ? std::make_unique<ModelInjector>(CI) : nullptr);
 }
diff --git a/clang/unittests/StaticAnalyzer/BugReportInterestingnessTest.cpp b/clang/unittests/StaticAnalyzer/BugReportInterestingnessTest.cpp
@@ -19,6 +19,7 @@
 #include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
 #include "clang/Tooling/Tooling.h"
 #include "gtest/gtest.h"
+#include <memory>
 
 using namespace clang;
 using namespace ento;
@@ -114,8 +115,9 @@ class TestAction : public ASTFrontendAction {
                                                  StringRef File) override {
     std::unique_ptr<AnalysisASTConsumer> AnalysisConsumer =
         CreateAnalysisConsumer(Compiler);
-    AnalysisConsumer->AddDiagnosticConsumer(new VerifyPathDiagnosticConsumer(
-        std::move(ExpectedDiags), Compiler.getSourceManager()));
+    AnalysisConsumer->AddDiagnosticConsumer(
+        std::make_unique<VerifyPathDiagnosticConsumer>(
+            std::move(ExpectedDiags), Compiler.getSourceManager()));
     AnalysisConsumer->AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
       Registry.addChecker<InterestingnessTestChecker>("test.Interestingness",
                                                       "Description", "");
diff --git a/clang/unittests/StaticAnalyzer/CheckerRegistration.h b/clang/unittests/StaticAnalyzer/CheckerRegistration.h
@@ -95,10 +95,10 @@ template <AddCheckerFn... Fns> class TestAction : public ASTFrontendAction {
         CreateAnalysisConsumer(Compiler);
     if (OnlyEmitWarnings)
       AnalysisConsumer->AddDiagnosticConsumer(
-          new OnlyWarningsDiagConsumer(DiagsOutput));
+          std::make_unique<OnlyWarningsDiagConsumer>(DiagsOutput));
     else
       AnalysisConsumer->AddDiagnosticConsumer(
-          new PathDiagConsumer(DiagsOutput));
+          std::make_unique<PathDiagConsumer>(DiagsOutput));
     addChecker<Fns...>(*AnalysisConsumer, Compiler.getAnalyzerOpts());
     return std::move(AnalysisConsumer);
   }
diff --git a/clang/unittests/StaticAnalyzer/Reusables.h b/clang/unittests/StaticAnalyzer/Reusables.h
