[clang][deps] Make dependency directives getter thread-safe (#136178)

This PR fixes two issues in one go:
1. The dependency directives getter (a `std::function`) was being stored
in `PreprocessorOptions`. This goes against the principle where the
options classes are supposed to be value-objects representing the `-cc1`
command line arguments. This is fixed by moving the getter directly to
`CompilerInstance` and propagating it explicitly.
2. The getter was capturing the `ScanInstance` VFS. That's fine in
synchronous implicit module builds where the same VFS instance is used
throughout, but breaks down once you try to build modules asynchronously
(which forces the use of separate VFS instances). This is fixed by
explicitly passing a `FileManager` into the getter and extracting the
right instance of the scanning VFS out of it.

Author: jansvoboda11

clang/include/clang/Frontend/CompilerInstance.h

@@ -16,6 +16,7 @@
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PCHContainerOperations.h"
 #include "clang/Frontend/Utils.h"
+#include "clang/Lex/DependencyDirectivesScanner.h"
 #include "clang/Lex/HeaderSearchOptions.h"
 #include "clang/Lex/ModuleLoader.h"
 #include "llvm/ADT/ArrayRef.h"
@@ -99,6 +100,9 @@ class CompilerInstance : public ModuleLoader {
   /// The cache of PCM files.
   IntrusiveRefCntPtr<ModuleCache> ModCache;
 
+  /// Functor for getting the dependency preprocessor directives of a file.
+  std::unique_ptr<DependencyDirectivesGetter> GetDependencyDirectives;
+
   /// The preprocessor.
   std::shared_ptr<Preprocessor> PP;
 
@@ -697,6 +701,11 @@ class CompilerInstance : public ModuleLoader {
   /// and replace any existing one with it.
   void createPreprocessor(TranslationUnitKind TUKind);
 
+  void setDependencyDirectivesGetter(
+      std::unique_ptr<DependencyDirectivesGetter> Getter) {
+    GetDependencyDirectives = std::move(Getter);
+  }
+
   std::string getSpecificModuleCachePath(StringRef ModuleHash);
   std::string getSpecificModuleCachePath() {
     return getSpecificModuleCachePath(getInvocation().getModuleHash());

clang/include/clang/Lex/DependencyDirectivesScanner.h

@@ -21,6 +21,7 @@
 #include "llvm/ADT/ArrayRef.h"
 
 namespace clang {
+class FileManager;
 
 namespace tok {
 enum TokenKind : unsigned short;
@@ -135,6 +136,19 @@ void printDependencyDirectivesAsSource(
     ArrayRef<dependency_directives_scan::Directive> Directives,
     llvm::raw_ostream &OS);
 
+/// Functor that returns the dependency directives for a given file.
+class DependencyDirectivesGetter {
+public:
+  /// Clone the getter for a new \c FileManager instance.
+  virtual std::unique_ptr<DependencyDirectivesGetter>
+  cloneFor(FileManager &FileMgr) = 0;
+
+  /// Get the dependency directives for the given file.
+  virtual std::optional<ArrayRef<dependency_directives_scan::Directive>>
+  operator()(FileEntryRef File) = 0;
+
+  virtual ~DependencyDirectivesGetter() = default;
+};
 } // end namespace clang
 
 #endif // LLVM_CLANG_LEX_DEPENDENCYDIRECTIVESSCANNER_H

clang/include/clang/Lex/Preprocessor.h

@@ -140,6 +140,12 @@ class Preprocessor {
   friend class VariadicMacroScopeGuard;
 
   llvm::unique_function<void(const clang::Token &)> OnToken;
+  /// Functor 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.
+  DependencyDirectivesGetter *GetDependencyDirectives = nullptr;
   const PreprocessorOptions &PPOpts;
   DiagnosticsEngine        *Diags;
   const LangOptions &LangOpts;
@@ -1326,6 +1332,10 @@ class Preprocessor {
     OnToken = std::move(F);
   }
 
+  void setDependencyDirectivesGetter(DependencyDirectivesGetter &Get) {
+    GetDependencyDirectives = &Get;
+  }
+
   void setPreprocessToken(bool Preprocess) { PreprocessToken = Preprocess; }
 
   bool isMacroDefined(StringRef Id) {

clang/include/clang/Lex/PreprocessorOptions.h

@@ -189,19 +189,6 @@ class PreprocessorOptions {
   /// with support for lifetime-qualified pointers.
   ObjCXXARCStandardLibraryKind ObjCXXARCStandardLibrary = ARCXX_nolib;
 
-  /// 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;
 

clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h

@@ -379,6 +379,16 @@ class DependencyScanningWorkerFilesystem
   /// false if not (i.e. this entry is not a file or its scan fails).
   bool ensureDirectiveTokensArePopulated(EntryRef Entry);
 
+  /// \returns The scanned preprocessor directive tokens of the file that are
+  /// used to speed up preprocessing, if available.
+  std::optional<ArrayRef<dependency_directives_scan::Directive>>
+  getDirectiveTokens(const Twine &Path) {
+    if (llvm::ErrorOr<EntryRef> Entry = getOrCreateFileSystemEntry(Path.str()))
+      if (ensureDirectiveTokensArePopulated(*Entry))
+        return Entry->getDirectiveTokens();
+    return std::nullopt;
+  }
+
   /// Check whether \p Path exists. By default checks cached result of \c
   /// status(), and falls back on FS if unable to do so.
   bool exists(const Twine &Path) override;

clang/lib/Frontend/CompilerInstance.cpp

@@ -536,6 +536,9 @@ void CompilerInstance::createPreprocessor(TranslationUnitKind TUKind) {
                            /*ShowAllHeaders=*/true, /*OutputPath=*/"",
                            /*ShowDepth=*/true, /*MSStyle=*/true);
   }
+
+  if (GetDependencyDirectives)
+    PP->setDependencyDirectivesGetter(*GetDependencyDirectives);
 }
 
 std::string CompilerInstance::getSpecificModuleCachePath(StringRef ModuleHash) {
@@ -1246,6 +1249,10 @@ std::unique_ptr<CompilerInstance> CompilerInstance::cloneForModuleCompileImpl(
   // Make a copy for the new instance.
   Instance.FailedModules = FailedModules;
 
+  if (GetDependencyDirectives)
+    Instance.GetDependencyDirectives =
+        GetDependencyDirectives->cloneFor(Instance.getFileManager());
+
   // If we're collecting module dependencies, we need to share a collector
   // between all of the module CompilerInstances. Other than that, we don't
   // want to produce any dependency output from the module build.

clang/lib/Lex/PPLexerChange.cpp

@@ -92,16 +92,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)) {
-        TheLexer->DepDirectives = *DepDirectives;
-      }
-    }
-  }
+  if (GetDependencyDirectives && FID != PredefinesFileID)
+    if (OptionalFileEntryRef File = SourceMgr.getFileEntryRefForID(FID))
+      if (auto MaybeDepDirectives = (*GetDependencyDirectives)(*File))
+        TheLexer->DepDirectives = *MaybeDepDirectives;
 
   EnterSourceFileWithLexer(TheLexer, CurDir);
   return false;

clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp

@@ -349,6 +349,32 @@ static void canonicalizeDefines(PreprocessorOptions &PPOpts) {
   std::swap(PPOpts.Macros, NewMacros);
 }
 
+class ScanningDependencyDirectivesGetter : public DependencyDirectivesGetter {
+  DependencyScanningWorkerFilesystem *DepFS;
+
+public:
+  ScanningDependencyDirectivesGetter(FileManager &FileMgr) : DepFS(nullptr) {
+    FileMgr.getVirtualFileSystem().visit([&](llvm::vfs::FileSystem &FS) {
+      auto *DFS = llvm::dyn_cast<DependencyScanningWorkerFilesystem>(&FS);
+      if (DFS) {
+        assert(!DepFS && "Found multiple scanning VFSs");
+        DepFS = DFS;
+      }
+    });
+    assert(DepFS && "Did not find scanning VFS");
+  }
+
+  std::unique_ptr<DependencyDirectivesGetter>
+  cloneFor(FileManager &FileMgr) override {
+    return std::make_unique<ScanningDependencyDirectivesGetter>(FileMgr);
+  }
+
+  std::optional<ArrayRef<dependency_directives_scan::Directive>>
+  operator()(FileEntryRef File) override {
+    return DepFS->getDirectiveTokens(File.getName());
+  }
+};
+
 /// A clang tool that runs the preprocessor in a mode that's optimized for
 /// dependency scanning for the given compiler invocation.
 class DependencyScanningAction : public tooling::ToolAction {
@@ -416,6 +442,9 @@ class DependencyScanningAction : public tooling::ToolAction {
         ScanInstance.getInvocation(), ScanInstance.getDiagnostics(),
         DriverFileMgr->getVirtualFileSystemPtr());
 
+    // Create a new FileManager to match the invocation's FileSystemOptions.
+    auto *FileMgr = ScanInstance.createFileManager(FS);
+
     // Use the dependency scanning optimized file system if requested to do so.
     if (DepFS) {
       StringRef ModulesCachePath =
@@ -425,19 +454,10 @@ class DependencyScanningAction : public tooling::ToolAction {
       if (!ModulesCachePath.empty())
         DepFS->setBypassedPathPrefix(ModulesCachePath);
 
-      ScanInstance.getPreprocessorOpts().DependencyDirectivesForFile =
-          [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;
-      };
+      ScanInstance.setDependencyDirectivesGetter(
+          std::make_unique<ScanningDependencyDirectivesGetter>(*FileMgr));
     }
 
-    // Create a new FileManager to match the invocation's FileSystemOptions.
-    auto *FileMgr = ScanInstance.createFileManager(FS);
     ScanInstance.createSourceManager(*FileMgr);
 
     // Create a collection of stable directories derived from the ScanInstance

clang/unittests/Lex/PPDependencyDirectivesTest.cpp

@@ -103,25 +103,33 @@ TEST_F(PPDependencyDirectivesTest, MacroGuard) {
     SmallVector<dependency_directives_scan::Token> Tokens;
     SmallVector<dependency_directives_scan::Directive> Directives;
   };
-  SmallVector<std::unique_ptr<DepDirectives>> DepDirectivesObjects;
-
-  auto getDependencyDirectives = [&](FileEntryRef File)
-      -> std::optional<ArrayRef<dependency_directives_scan::Directive>> {
-    DepDirectivesObjects.push_back(std::make_unique<DepDirectives>());
-    StringRef Input = (*FileMgr.getBufferForFile(File))->getBuffer();
-    bool Err = scanSourceForDependencyDirectives(
-        Input, DepDirectivesObjects.back()->Tokens,
-        DepDirectivesObjects.back()->Directives);
-    EXPECT_FALSE(Err);
-    return llvm::ArrayRef(DepDirectivesObjects.back()->Directives);
-  };
 
-  PreprocessorOptions PPOpts;
-  PPOpts.DependencyDirectivesForFile = [&](FileEntryRef File)
-      -> std::optional<ArrayRef<dependency_directives_scan::Directive>> {
-    return getDependencyDirectives(File);
+  class TestDependencyDirectivesGetter : public DependencyDirectivesGetter {
+    FileManager &FileMgr;
+    SmallVector<std::unique_ptr<DepDirectives>> DepDirectivesObjects;
+
+  public:
+    TestDependencyDirectivesGetter(FileManager &FileMgr) : FileMgr(FileMgr) {}
+
+    std::unique_ptr<DependencyDirectivesGetter>
+    cloneFor(FileManager &FileMgr) override {
+      return std::make_unique<TestDependencyDirectivesGetter>(FileMgr);
+    }
+
+    std::optional<ArrayRef<dependency_directives_scan::Directive>>
+    operator()(FileEntryRef File) override {
+      DepDirectivesObjects.push_back(std::make_unique<DepDirectives>());
+      StringRef Input = (*FileMgr.getBufferForFile(File))->getBuffer();
+      bool Err = scanSourceForDependencyDirectives(
+          Input, DepDirectivesObjects.back()->Tokens,
+          DepDirectivesObjects.back()->Directives);
+      EXPECT_FALSE(Err);
+      return DepDirectivesObjects.back()->Directives;
+    }
   };
+  TestDependencyDirectivesGetter GetDependencyDirectives(FileMgr);
 
+  PreprocessorOptions PPOpts;
   HeaderSearchOptions HSOpts;
   TrivialModuleLoader ModLoader;
   HeaderSearch HeaderInfo(HSOpts, SourceMgr, Diags, LangOpts, Target.get());
@@ -130,6 +138,8 @@ TEST_F(PPDependencyDirectivesTest, MacroGuard) {
                   /*OwnsHeaderSearch =*/false);
   PP.Initialize(*Target);
 
+  PP.setDependencyDirectivesGetter(GetDependencyDirectives);
+
   SmallVector<StringRef> IncludedFiles;
   PP.addPPCallbacks(std::make_unique<IncludeCollector>(PP, IncludedFiles));
   PP.EnterMainSourceFile();