[clang] Move state out of PreprocessorOptions (1/n) (#86358)

An instance of `PreprocessorOptions` is part of `CompilerInvocation` which is supposed to be a value type. The `DependencyDirectivesForFile` member is problematic, since it holds an owning reference of the scanning VFS. This makes it not a true value type, and it can keep potentially large chunk of memory (the local cache in the scanning VFS) alive for longer than clients might expect. Let's move it into the `Preprocessor` instead.
llvm · Mar 29, 2024 · 407a2f2 · 407a2f2
1 parent 9937952
commit 407a2f2
Show file tree

Hide file tree

Showing 7 changed files with 65 additions and 46 deletions.
diff --git a/clang/include/clang/Frontend/FrontendActions.h b/clang/include/clang/Frontend/FrontendActions.h
@@ -34,12 +34,18 @@ class InitOnlyAction : public FrontendAction {
 
 /// Preprocessor-based frontend action that also loads PCH files.
 class ReadPCHAndPreprocessAction : public FrontendAction {
+  llvm::unique_function<void(CompilerInstance &)> AdjustCI;
+
   void ExecuteAction() override;
 
   std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
                                                  StringRef InFile) override;
 
 public:
+  ReadPCHAndPreprocessAction(
+      llvm::unique_function<void(CompilerInstance &)> AdjustCI)
+      : AdjustCI(std::move(AdjustCI)) {}
+
   bool usesPreprocessorOnly() const override { return false; }
 };
 
@@ -321,11 +327,15 @@ class PrintPreprocessedAction : public PreprocessorFrontendAction {
 
 class GetDependenciesByModuleNameAction : public PreprocessOnlyAction {
   StringRef ModuleName;
+  llvm::unique_function<void(CompilerInstance &)> AdjustCI;
+
   void ExecuteAction() override;
 
 public:
-  GetDependenciesByModuleNameAction(StringRef ModuleName)
-      : ModuleName(ModuleName) {}
+  GetDependenciesByModuleNameAction(
+      StringRef ModuleName,
+      llvm::unique_function<void(CompilerInstance &)> AdjustCI)
+      : ModuleName(ModuleName), AdjustCI(std::move(AdjustCI)) {}
 };
 
 }  // end namespace clang

diff --git a/clang/include/clang/Lex/Preprocessor.h b/clang/include/clang/Lex/Preprocessor.h
@@ -736,6 +736,19 @@ class Preprocessor {
     State ConditionalStackState = Off;
   } PreambleConditionalStack;
 
+  /// Function for getting the dependency preprocessor directives of a file.
+  ///
+  /// These are directives derived from a special form of lexing where the
+  /// source input is scanned for the preprocessor directives that might have an
+  /// effect on the dependencies for a compilation unit.
+  ///
+  /// Enables a client to cache the directives for a file and provide them
+  /// across multiple compiler invocations.
+  /// FIXME: Allow returning an error.
+  using DependencyDirectivesFn = llvm::unique_function<std::optional<
+      ArrayRef<dependency_directives_scan::Directive>>(FileEntryRef)>;
+  DependencyDirectivesFn DependencyDirectivesForFile;
+
   /// The current top of the stack that we're lexing from if
   /// not expanding a macro and we are lexing directly from source code.
   ///
@@ -1270,6 +1283,11 @@ class Preprocessor {
   /// false if it is producing tokens to be consumed by Parse and Sema.
   bool isPreprocessedOutput() const { return PreprocessedOutput; }
 
+  /// Set the function used to get dependency directives for a file.
+  void setDependencyDirectivesFn(DependencyDirectivesFn Fn) {
+    DependencyDirectivesForFile = std::move(Fn);
+  }
+
   /// Return true if we are lexing directly from the specified lexer.
   bool isCurrentLexer(const PreprocessorLexer *L) const {
     return CurPPLexer == L;

diff --git a/clang/include/clang/Lex/PreprocessorOptions.h b/clang/include/clang/Lex/PreprocessorOptions.h
@@ -208,19 +208,6 @@ class PreprocessorOptions {
   /// build it again.
   std::shared_ptr<FailedModulesSet> FailedModules;
 
-  /// Function for getting the dependency preprocessor directives of a file.
-  ///
-  /// These are directives derived from a special form of lexing where the
-  /// source input is scanned for the preprocessor directives that might have an
-  /// effect on the dependencies for a compilation unit.
-  ///
-  /// Enables a client to cache the directives for a file and provide them
-  /// across multiple compiler invocations.
-  /// FIXME: Allow returning an error.
-  std::function<std::optional<ArrayRef<dependency_directives_scan::Directive>>(
-      FileEntryRef)>
-      DependencyDirectivesForFile;
-
   /// Set up preprocessor for RunAnalysis action.
   bool SetUpStaticAnalyzer = false;
 

diff --git a/clang/lib/Frontend/FrontendActions.cpp b/clang/lib/Frontend/FrontendActions.cpp
@@ -69,7 +69,10 @@ void InitOnlyAction::ExecuteAction() {
 
 // Basically PreprocessOnlyAction::ExecuteAction.
 void ReadPCHAndPreprocessAction::ExecuteAction() {
-  Preprocessor &PP = getCompilerInstance().getPreprocessor();
+  CompilerInstance &CI = getCompilerInstance();
+  AdjustCI(CI);
+
+  Preprocessor &PP = CI.getPreprocessor();
 
   // Ignore unknown pragmas.
   PP.IgnorePragmas();
@@ -1188,6 +1191,8 @@ void PrintDependencyDirectivesSourceMinimizerAction::ExecuteAction() {
 
 void GetDependenciesByModuleNameAction::ExecuteAction() {
   CompilerInstance &CI = getCompilerInstance();
+  AdjustCI(CI);
+
   Preprocessor &PP = CI.getPreprocessor();
   SourceManager &SM = PP.getSourceManager();
   FileID MainFileID = SM.getMainFileID();

diff --git a/clang/lib/Lex/PPLexerChange.cpp b/clang/lib/Lex/PPLexerChange.cpp
@@ -93,16 +93,10 @@ bool Preprocessor::EnterSourceFile(FileID FID, ConstSearchDirIterator CurDir,
   }
 
   Lexer *TheLexer = new Lexer(FID, *InputFile, *this, IsFirstIncludeOfFile);
-  if (getPreprocessorOpts().DependencyDirectivesForFile &&
-      FID != PredefinesFileID) {
-    if (OptionalFileEntryRef File = SourceMgr.getFileEntryRefForID(FID)) {
-      if (std::optional<ArrayRef<dependency_directives_scan::Directive>>
-              DepDirectives =
-                  getPreprocessorOpts().DependencyDirectivesForFile(*File)) {
+  if (DependencyDirectivesForFile && FID != PredefinesFileID)
+    if (OptionalFileEntryRef File = SourceMgr.getFileEntryRefForID(FID))
+      if (auto DepDirectives = DependencyDirectivesForFile(*File))
         TheLexer->DepDirectives = *DepDirectives;
-      }
-    }
-  }
 
   EnterSourceFileWithLexer(TheLexer, CurDir);
   return false;

diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -363,20 +363,22 @@ class DependencyScanningAction : public tooling::ToolAction {
               PrebuiltModuleVFSMap, ScanInstance.getDiagnostics()))
         return false;
 
-    // Use the dependency scanning optimized file system if requested to do so.
-    if (DepFS) {
-      llvm::IntrusiveRefCntPtr<DependencyScanningWorkerFilesystem> LocalDepFS =
-          DepFS;
-      ScanInstance.getPreprocessorOpts().DependencyDirectivesForFile =
-          [LocalDepFS = std::move(LocalDepFS)](FileEntryRef File)
-          -> std::optional<ArrayRef<dependency_directives_scan::Directive>> {
-        if (llvm::ErrorOr<EntryRef> Entry =
-                LocalDepFS->getOrCreateFileSystemEntry(File.getName()))
-          if (LocalDepFS->ensureDirectiveTokensArePopulated(*Entry))
-            return Entry->getDirectiveTokens();
-        return std::nullopt;
-      };
-    }
+    auto AdjustCI = [&](CompilerInstance &CI) {
+      // Set up the dependency scanning file system callback if requested.
+      if (DepFS) {
+        auto GetDependencyDirectives = [LocalDepFS = DepFS](FileEntryRef File)
+            -> std::optional<ArrayRef<dependency_directives_scan::Directive>> {
+          if (llvm::ErrorOr<EntryRef> Entry =
+                  LocalDepFS->getOrCreateFileSystemEntry(File.getName()))
+            if (LocalDepFS->ensureDirectiveTokensArePopulated(*Entry))
+              return Entry->getDirectiveTokens();
+          return std::nullopt;
+        };
+
+        CI.getPreprocessor().setDependencyDirectivesFn(
+            std::move(GetDependencyDirectives));
+      }
+    };
 
     // Create the dependency collector that will collect the produced
     // dependencies.
@@ -428,9 +430,11 @@ class DependencyScanningAction : public tooling::ToolAction {
     std::unique_ptr<FrontendAction> Action;
 
     if (ModuleName)
-      Action = std::make_unique<GetDependenciesByModuleNameAction>(*ModuleName);
+      Action = std::make_unique<GetDependenciesByModuleNameAction>(
+          *ModuleName, std::move(AdjustCI));
     else
-      Action = std::make_unique<ReadPCHAndPreprocessAction>();
+      Action =
+          std::make_unique<ReadPCHAndPreprocessAction>(std::move(AdjustCI));
 
     if (ScanInstance.getDiagnostics().hasErrorOccurred())
       return false;

diff --git a/clang/unittests/Lex/PPDependencyDirectivesTest.cpp b/clang/unittests/Lex/PPDependencyDirectivesTest.cpp
@@ -117,11 +117,6 @@ TEST_F(PPDependencyDirectivesTest, MacroGuard) {
   };
 
   auto PPOpts = std::make_shared<PreprocessorOptions>();
-  PPOpts->DependencyDirectivesForFile = [&](FileEntryRef File)
-      -> std::optional<ArrayRef<dependency_directives_scan::Directive>> {
-    return getDependencyDirectives(File);
-  };
-
   TrivialModuleLoader ModLoader;
   HeaderSearch HeaderInfo(std::make_shared<HeaderSearchOptions>(), SourceMgr,
                           Diags, LangOpts, Target.get());
@@ -130,6 +125,12 @@ TEST_F(PPDependencyDirectivesTest, MacroGuard) {
                   /*OwnsHeaderSearch =*/false);
   PP.Initialize(*Target);
 
+  PP.setDependencyDirectivesFn(
+      [&](FileEntryRef File)
+          -> std::optional<ArrayRef<dependency_directives_scan::Directive>> {
+        return getDependencyDirectives(File);
+      });
+
   SmallVector<StringRef> IncludedFiles;
   PP.addPPCallbacks(std::make_unique<IncludeCollector>(PP, IncludedFiles));
   PP.EnterMainSourceFile();