llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Jan Svoboda (jansvoboda11) <details> <summary>Changes</summary> This PR makes it so that the base VFS used during a scan is provided to the top-level service, not to each worker/tool individually. This enables resolving a FIXME in the async-scan-modules mode of the scanner, will clean up [downstream code](https://github.com/swiftlang/llvm-project/blob/0eb56baa1d92190ed61c1a13caa1112e0f70b109/clang/tools/libclang/CDependencies.cpp#L619-L622), and will make it more difficult to have mismatching VFSs across workers, which may cause non-deterministic poisoning of `DependencyScanningFilesystemSharedCache`. --- Full diff: https://github.com/llvm/llvm-project/pull/181424.diff 9 Files Affected: - (modified) clang/include/clang/DependencyScanning/DependencyScanningService.h (+5) - (modified) clang/include/clang/DependencyScanning/DependencyScanningWorker.h (+1-3) - (modified) clang/include/clang/Tooling/DependencyScanningTool.h (+2-4) - (modified) clang/lib/DependencyScanning/DependencyScannerImpl.cpp (+1-2) - (modified) clang/lib/DependencyScanning/DependencyScanningService.cpp (+2-1) - (modified) clang/lib/DependencyScanning/DependencyScanningWorker.cpp (+3-2) - (modified) clang/lib/Tooling/DependencyScanningTool.cpp (-6) - (modified) clang/unittests/DependencyScanning/DependencyScanningWorkerTest.cpp (+2-1) - (modified) clang/unittests/Tooling/DependencyScannerTest.cpp (+4-2) ``````````diff diff --git a/clang/include/clang/DependencyScanning/DependencyScanningService.h b/clang/include/clang/DependencyScanning/DependencyScanningService.h index 509b6a579ecb4..20ba86bd097b0 100644 --- a/clang/include/clang/DependencyScanning/DependencyScanningService.h +++ b/clang/include/clang/DependencyScanning/DependencyScanningService.h @@ -80,6 +80,11 @@ enum class ScanningOptimizations { struct DependencyScanningServiceOptions { DependencyScanningServiceOptions(); + /// The function invoked to create each worker's VFS. This function and the + /// VFS itself must be thread-safe whenever using multiple workers + /// concurrently or whenever \c AsyncScanModules is true. + std::function<IntrusiveRefCntPtr<llvm::vfs::FileSystem>()> + MakeVFS; // = [] { return llvm::vfs::createPhysicalFileSystem(); } /// Whether to use optimized dependency directive scan or full preprocessing. ScanningMode Mode = ScanningMode::DependencyDirectivesScan; /// What output format are we expected to produce. diff --git a/clang/include/clang/DependencyScanning/DependencyScanningWorker.h b/clang/include/clang/DependencyScanning/DependencyScanningWorker.h index 2a48f342335de..e1e0c14c7b52c 100644 --- a/clang/include/clang/DependencyScanning/DependencyScanningWorker.h +++ b/clang/include/clang/DependencyScanning/DependencyScanningWorker.h @@ -87,9 +87,7 @@ class DependencyScanningWorker { /// Construct a dependency scanning worker. /// /// @param Service The parent service. Must outlive the worker. - /// @param BaseFS The filesystem for the worker to use. - DependencyScanningWorker(DependencyScanningService &Service, - IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS); + DependencyScanningWorker(DependencyScanningService &Service); ~DependencyScanningWorker(); diff --git a/clang/include/clang/Tooling/DependencyScanningTool.h b/clang/include/clang/Tooling/DependencyScanningTool.h index fe719aceaef1d..96d21866dd7f9 100644 --- a/clang/include/clang/Tooling/DependencyScanningTool.h +++ b/clang/include/clang/Tooling/DependencyScanningTool.h @@ -36,10 +36,8 @@ class DependencyScanningTool { /// Construct a dependency scanning tool. /// /// @param Service The parent service. Must outlive the tool. - /// @param FS The filesystem for the tool to use. Defaults to the physical FS. - DependencyScanningTool(dependencies::DependencyScanningService &Service, - llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS = - llvm::vfs::createPhysicalFileSystem()); + DependencyScanningTool(dependencies::DependencyScanningService &Service) + : Worker(Service) {} /// Print out the dependency information into a string using the dependency /// file format that is specified in the options (-MD is the default) and diff --git a/clang/lib/DependencyScanning/DependencyScannerImpl.cpp b/clang/lib/DependencyScanning/DependencyScannerImpl.cpp index 28fa2571a24dc..3cb63a1a055ce 100644 --- a/clang/lib/DependencyScanning/DependencyScannerImpl.cpp +++ b/clang/lib/DependencyScanning/DependencyScannerImpl.cpp @@ -620,10 +620,9 @@ struct AsyncModuleCompile : PPCallbacks { if (!LockErr && !Owned) return; // We should build the PCM. - // FIXME: Pass the correct BaseFS to the worker FS. IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS = llvm::makeIntrusiveRefCnt<DependencyScanningWorkerFilesystem>( - Service.getSharedCache(), llvm::vfs::getRealFileSystem()); + Service.getSharedCache(), Service.getOpts().MakeVFS()); VFS = createVFSFromCompilerInvocation(CI.getInvocation(), CI.getDiagnostics(), std::move(VFS)); auto DC = std::make_unique<DiagnosticConsumer>(); diff --git a/clang/lib/DependencyScanning/DependencyScanningService.cpp b/clang/lib/DependencyScanning/DependencyScanningService.cpp index 3651b9b20a70f..eb06c86040f0a 100644 --- a/clang/lib/DependencyScanning/DependencyScanningService.cpp +++ b/clang/lib/DependencyScanning/DependencyScanningService.cpp @@ -14,5 +14,6 @@ using namespace clang; using namespace dependencies; DependencyScanningServiceOptions::DependencyScanningServiceOptions() - : BuildSessionTimestamp( + : MakeVFS([] { return llvm::vfs::createPhysicalFileSystem(); }), + BuildSessionTimestamp( llvm::sys::toTimeT(std::chrono::system_clock::now())) {} diff --git a/clang/lib/DependencyScanning/DependencyScanningWorker.cpp b/clang/lib/DependencyScanning/DependencyScanningWorker.cpp index 7d0c93138d78c..60e5103fde6ef 100644 --- a/clang/lib/DependencyScanning/DependencyScanningWorker.cpp +++ b/clang/lib/DependencyScanning/DependencyScanningWorker.cpp @@ -20,8 +20,7 @@ using namespace clang; using namespace dependencies; DependencyScanningWorker::DependencyScanningWorker( - DependencyScanningService &Service, - llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS) + DependencyScanningService &Service) : Service(Service) { PCHContainerOps = std::make_shared<PCHContainerOperations>(); // We need to read object files from PCH built outside the scanner. @@ -30,6 +29,8 @@ DependencyScanningWorker::DependencyScanningWorker( // The scanner itself writes only raw ast files. PCHContainerOps->registerWriter(std::make_unique<RawPCHContainerWriter>()); + auto BaseFS = Service.getOpts().MakeVFS(); + if (Service.getOpts().TraceVFS) BaseFS = llvm::makeIntrusiveRefCnt<llvm::vfs::TracingFileSystem>( std::move(BaseFS)); diff --git a/clang/lib/Tooling/DependencyScanningTool.cpp b/clang/lib/Tooling/DependencyScanningTool.cpp index cc4c88fc42f5a..2ae149e8fb2cf 100644 --- a/clang/lib/Tooling/DependencyScanningTool.cpp +++ b/clang/lib/Tooling/DependencyScanningTool.cpp @@ -14,7 +14,6 @@ #include "clang/Frontend/Utils.h" #include "llvm/ADT/SmallVectorExtras.h" #include "llvm/ADT/iterator.h" -#include "llvm/Support/VirtualFileSystem.h" #include "llvm/TargetParser/Host.h" #include <optional> @@ -22,11 +21,6 @@ using namespace clang; using namespace tooling; using namespace dependencies; -DependencyScanningTool::DependencyScanningTool( - DependencyScanningService &Service, - llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS) - : Worker(Service, std::move(FS)) {} - namespace { /// Prints out all of the gathered dependencies into a string. class MakeDependencyPrinterConsumer : public DependencyConsumer { diff --git a/clang/unittests/DependencyScanning/DependencyScanningWorkerTest.cpp b/clang/unittests/DependencyScanning/DependencyScanningWorkerTest.cpp index ca51fc3f6fbcb..9fbebcbc4e1ca 100644 --- a/clang/unittests/DependencyScanning/DependencyScanningWorkerTest.cpp +++ b/clang/unittests/DependencyScanning/DependencyScanningWorkerTest.cpp @@ -32,9 +32,10 @@ TEST(DependencyScanner, ScanDepsWithDiagConsumer) { VFS->addFile(AsmPath, 0, llvm::MemoryBuffer::getMemBuffer("")); DependencyScanningServiceOptions Opts; + Opts.MakeVFS = [&] { return VFS; }; Opts.Format = ScanningOutputFormat::Make; DependencyScanningService Service(std::move(Opts)); - DependencyScanningWorker Worker(Service, VFS); + DependencyScanningWorker Worker(Service); llvm::DenseSet<ModuleID> AlreadySeen; FullDependencyConsumer DC(AlreadySeen); diff --git a/clang/unittests/Tooling/DependencyScannerTest.cpp b/clang/unittests/Tooling/DependencyScannerTest.cpp index a764a206c4670..79fd5a312d2b9 100644 --- a/clang/unittests/Tooling/DependencyScannerTest.cpp +++ b/clang/unittests/Tooling/DependencyScannerTest.cpp @@ -229,9 +229,10 @@ TEST(DependencyScanner, ScanDepsWithFS) { llvm::MemoryBuffer::getMemBuffer("#include \"header.h\"\n")); DependencyScanningServiceOptions Opts; + Opts.MakeVFS = [&] { return VFS; }; Opts.Format = ScanningOutputFormat::Make; DependencyScanningService Service(std::move(Opts)); - DependencyScanningTool ScanTool(Service, VFS); + DependencyScanningTool ScanTool(Service); TextDiagnosticBuffer DiagConsumer; std::optional<std::string> DepFile = @@ -287,9 +288,10 @@ TEST(DependencyScanner, ScanDepsWithModuleLookup) { auto InterceptFS = llvm::makeIntrusiveRefCnt<InterceptorFS>(VFS); DependencyScanningServiceOptions Opts; + Opts.MakeVFS = [&] { return InterceptFS; }; Opts.Format = ScanningOutputFormat::Make; DependencyScanningService Service(std::move(Opts)); - DependencyScanningTool ScanTool(Service, InterceptFS); + DependencyScanningTool ScanTool(Service); // This will fail with "fatal error: module 'Foo' not found" but it doesn't // matter, the point of the test is to check that files are not read `````````` </details> https://github.com/llvm/llvm-project/pull/181424 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
