[clang][deps] Don't share in-memory VFS across scans

[jansvoboda11]

Summary

The dependency scanning worker may create an in-memory VFS and inject that into its FileManager. (Implemented in D109485.) This VFS currently persists between scans, which is not correct: in-memory files created in one scan could affect subsequent scans. This patch changes things so that the in-memory VFS is local to DependencyScanningWorker::computeDependencies().

To test this, one would first scan for dependencies of a named module and then run second scan (on the same worker) for something unrelated, which would pick up the in-memory file created in the first scan. This set up is impossible to achieve with clang-scan-deps. We could set this up in ToolingTests, but the scanner needs to access the physical filesystem to store .pcm files. AFAIK we don't have a good way to propagate something like %t into unit tests to make that feasible. (This could be achieved with something like the virtualized OutputManager.)

[Bigcheese]

lgtm

[DavidSpickett]

Hi @jansvoboda11 , we (Linaro) happen to be one of the few running a bot with threading disabled (for reasons that aren't important here). It has been red a lot lately so you won't have seen the report from it.

https://lab.llvm.org/buildbot/#/builders/178/builds/3045

This does not fail on our other bots and I think LLVM_ENABLE_THREADS=OFF is what breaks it.

From a working build:

$ /home/david.spickett/build-llvm-arm/bin/clang-scan-deps -compilation-database /home/david.spickett/build-llvm-arm-broken/tools/clang/test/ClangScanDeps/Output/modules-full-by-mod-name.cpp.tmp.cdb -j 4 -format experimental-full -mode preprocess-dependency-directives -module-name=header1
Building DependencyScanningWorker
Building DependencyScanningWorker
Building DependencyScanningWorker
Building DependencyScanningWorker
BaseFS was not null
BaseFS was not null
BaseFS moved
BaseFS moved

From a build with no threading:

$ /home/david.spickett/build-llvm-arm-broken/bin/clang-scan-deps -compilation-database /home/david.spickett/build-llvm-arm-broken/tools/clang/test/ClangScanDeps/Output/modules-full-by-mod-name.cpp.tmp.cdb -j 4 -format experimental-full -mode preprocess-dependency-directives -module-name=header1
Building DependencyScanningWorker
BaseFS was not null
BaseFS moved
clang-scan-deps: /home/david.spickett/llvm-project/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:393: bool clang::tooling::dependencies::DependencyScanningWorker::computeDependencies(llvm::StringRef, const std::vector<std::string> &, clang::tooling::dependencies::DependencyConsumer &, clang::DiagnosticConsumer &, llvm::Optional<StringRef>): Assertion `BaseFS' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.      Program arguments: /home/david.spickett/build-llvm-arm-broken/bin/clang-scan-deps -compilation-database /home/david.spickett/build-llvm-arm-broken/tools/clang/test/ClangScanDeps/Output/modules-full-by-mod-name.cpp.tmp.cdb -j 4 -format experimental-full -mode preprocess-dependency-directives -module-name=header1
Aborted (core dumped)

I think I narrowed it down to the move you added (I put a comment on it). When threading is enabled 4 workers are made and so no one tries to use BaseFs after it has been moved from. When only one worker is made, it attempts to use after move BaseFs.

In fact, if you give -j 1 to the threaded version, it also crashes:

$ /home/david.spickett/build-llvm-arm/bin/clang-scan-deps -compilation-database /home/david.spickett/build-llvm-arm-broken/tools/clang/test/Cla
ngScanDeps/Output/modules-full-by-mod-name.cpp.tmp.cdb -j 1 -format experimental-full -mode preprocess-dependency-directives -module-name=header1
Building DependencyScanningWorker
BaseFS was not null
BaseFS moved
clang-scan-deps: /home/david.spickett/llvm-project/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:393: bool clang::tooling::dependencies::DependencyScanningWorker::computeDependencies(llvm::StringRef, const std::vector<std::string> &, clang::tooling::dependencies::DependencyConsumer &, clang::DiagnosticConsumer &, llvm::Optional<StringRef>): Assertion `BaseFS' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Aborted (core dumped)

I think the same sort of thing would happen if you said -j N+1 when you had N cores? One worker would be run twice? I'm not up on all the details.

So yeah, to reproduce build without threading or add -j 1.

Do you have an idea how this might be handled? If you are moving you clearly want to transfer ownership, but I'm not sure if the BaseFs can just be rebuilt each time. I suppose if N workers can each have a BaseFs, a single worker could recreate it as needed.

Let me know what you think there.

clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp

401

This move seems like a problem when only one worker is made.

clang/include/clang/Basic/FileManager.h

return llvm::expectedToOptional(
         getFileRef(Filename, OpenFile, CacheFailure));
   }
 
   /// Returns the current file system options
   FileSystemOptions &getFileSystemOpts() { return FileSystemOpts; }
   const FileSystemOptions &getFileSystemOpts() const { return FileSystemOpts; }
 
   llvm::vfs::FileSystem &getVirtualFileSystem() const { return *FS; }
+  llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
+  getVirtualFileSystemPtr() const {
+    return FS;
+  }
 
   void setVirtualFileSystem(IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS) {
     this->FS = std::move(FS);
   }
 
   /// Retrieve a file entry for a "virtual" file that acts as
   /// if there were a file with the given name on disk.
   ///

clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h

llvm::Error computeDependencies(StringRef WorkingDirectory,
                                   const std::vector<std::string> &CommandLine,
                                   DependencyConsumer &Consumer,
                                   llvm::Optional<StringRef> ModuleName = None);
 
   bool shouldEagerLoadModules() const { return EagerLoadModules; }
 
 private:
   std::shared_ptr<PCHContainerOperations> PCHContainerOps;
-
-  /// The physical filesystem overlaid by `InMemoryFS`.
-  llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> RealFS;
-  /// The in-memory filesystem laid on top the physical filesystem in `RealFS`.
-  llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> InMemoryFS;
-  /// The file system that is used by each worker when scanning for
-  /// dependencies. This filesystem persists across multiple compiler
-  /// invocations.
+  /// The file system to be used during the scan.
+  /// This is either \c FS passed in the constructor (when performing canonical
+  /// preprocessing), or \c DepFS (when performing dependency directives scan).
+  llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS;
+  /// When performing dependency directives scan, this is the caching (and
+  /// dependency-directives-extracting) filesystem overlaid on top of \c FS
+  /// (passed in the constructor).
   llvm::IntrusiveRefCntPtr<DependencyScanningWorkerFilesystem> DepFS;
   ScanningOutputFormat Format;
   /// Whether to optimize the modules' command-line arguments.
   bool OptimizeArgs;
   /// Whether to set up command-lines to load PCM files eagerly.
   bool EagerLoadModules;
 };
 
 } // end namespace dependencies
 } // end namespace tooling
 } // end namespace clang
 
 #endif // LLVM_CLANG_TOOLING_DEPENDENCYSCANNING_DEPENDENCYSCANNINGWORKER_H

clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp

if (!ScanInstance.getPreprocessorOpts().ImplicitPCHInclude.empty())
           ScanInstance.getHeaderSearchOpts().PrebuiltModuleFiles,
           PrebuiltModulesInputFiles, /*VisitInputFiles=*/DepFS != nullptr);
 
     // Use the dependency scanning optimized file system if requested to do so.
     if (DepFS) {
       // Support for virtual file system overlays on top of the caching
       // filesystem.
       FileMgr->setVirtualFileSystem(createVFSFromCompilerInvocation(
-          ScanInstance.getInvocation(), ScanInstance.getDiagnostics(), DepFS));
+          ScanInstance.getInvocation(), ScanInstance.getDiagnostics(),
+          FileMgr->getVirtualFileSystemPtr()));
 
       llvm::IntrusiveRefCntPtr<DependencyScanningWorkerFilesystem> LocalDepFS =
           DepFS;
       ScanInstance.getPreprocessorOpts().DependencyDirectivesForFile =
           [LocalDepFS = std::move(LocalDepFS)](FileEntryRef File)
           -> Optional<ArrayRef<dependency_directives_scan::Directive>> {
         if (llvm::ErrorOr<EntryRef> Entry =
                 LocalDepFS->getOrCreateFileSystemEntry(File.getName()))
DependencyScanningWorker::DependencyScanningWorker(
   PCHContainerOps = std::make_shared<PCHContainerOperations>();
   PCHContainerOps->registerReader(
       std::make_unique<ObjectFilePCHContainerReader>());
   // We don't need to write object files, but the current PCH implementation
   // requires the writer to be registered as well.
   PCHContainerOps->registerWriter(
       std::make_unique<ObjectFilePCHContainerWriter>());
 
-  auto OverlayFS =
-      llvm::makeIntrusiveRefCnt<llvm::vfs::OverlayFileSystem>(std::move(FS));
-  InMemoryFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
-  OverlayFS->pushOverlay(InMemoryFS);
-  RealFS = OverlayFS;
-
-  if (Service.getMode() == ScanningMode::DependencyDirectivesScan)
-    DepFS = new DependencyScanningWorkerFilesystem(Service.getSharedCache(),
-                                                   RealFS);
+  switch (Service.getMode()) {
+  case ScanningMode::DependencyDirectivesScan:
+    DepFS =
+        new DependencyScanningWorkerFilesystem(Service.getSharedCache(), FS);
+    BaseFS = DepFS;
+    break;
+  case ScanningMode::CanonicalPreprocessing:
+    DepFS = nullptr;
+    BaseFS = FS;
+    break;
+  }
 }
 
 llvm::Error DependencyScanningWorker::computeDependencies(
     StringRef WorkingDirectory, const std::vector<std::string> &CommandLine,
     DependencyConsumer &Consumer, llvm::Optional<StringRef> ModuleName) {
   std::vector<const char *> CLI;
   for (const std::string &Arg : CommandLine)
     CLI.push_back(Arg.c_str());
static bool forEachDriverJob(
   return true;
 }
 
 bool DependencyScanningWorker::computeDependencies(
     StringRef WorkingDirectory, const std::vector<std::string> &CommandLine,
     DependencyConsumer &Consumer, DiagnosticConsumer &DC,
     llvm::Optional<StringRef> ModuleName) {
   // Reset what might have been modified in the previous worker invocation.
-  RealFS->setCurrentWorkingDirectory(WorkingDirectory);
-
-  FileSystemOptions FSOpts;
-  FSOpts.WorkingDir = WorkingDirectory.str();
-  auto FileMgr = llvm::makeIntrusiveRefCnt<FileManager>(FSOpts, RealFS);
+  BaseFS->setCurrentWorkingDirectory(WorkingDirectory);
 
   Optional<std::vector<std::string>> ModifiedCommandLine;
+  llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> ModifiedFS;
   if (ModuleName) {
     ModifiedCommandLine = CommandLine;
-    InMemoryFS->addFile(*ModuleName, 0, llvm::MemoryBuffer::getMemBuffer(""));
     ModifiedCommandLine->emplace_back(*ModuleName);
+
+    auto OverlayFS = llvm::makeIntrusiveRefCnt<llvm::vfs::OverlayFileSystem>(
+        std::move(BaseFS));
+    auto InMemoryFS =
+        llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
+    InMemoryFS->setCurrentWorkingDirectory(WorkingDirectory);
+    InMemoryFS->addFile(*ModuleName, 0, llvm::MemoryBuffer::getMemBuffer(""));
+    OverlayFS->pushOverlay(InMemoryFS);
+    ModifiedFS = OverlayFS;
   }
 
   const std::vector<std::string> &FinalCommandLine =
       ModifiedCommandLine ? *ModifiedCommandLine : CommandLine;
 
+  FileSystemOptions FSOpts;
+  FSOpts.WorkingDir = WorkingDirectory.str();
+  auto FileMgr = llvm::makeIntrusiveRefCnt<FileManager>(
+      FSOpts, ModifiedFS ? ModifiedFS : BaseFS);
+
   std::vector<const char *> FinalCCommandLine(CommandLine.size(), nullptr);
   llvm::transform(CommandLine, FinalCCommandLine.begin(),
                   [](const std::string &Str) { return Str.c_str(); });
 
   auto DiagOpts = CreateAndPopulateDiagOpts(FinalCCommandLine);
   sanitizeDiagOpts(*DiagOpts);
   IntrusiveRefCntPtr<DiagnosticsEngine> Diags =
       CompilerInstance::createDiagnostics(DiagOpts.release(), &DC,