[PATCH] D124816: [LibTooling] use ToolFileManager to store file managers for each CWD

[Shi Chen via Phabricator via cfe-commits]

Kale updated this revision to Diff 430268.
Kale added a comment.

1. Use BaseFS as the condition to judge whether to invalidate previous file 
managers
2. Use WorkingDir as a parameter for getOrCreateFileManager


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124816/new/

https://reviews.llvm.org/D124816

Files:
  clang-tools-extra/clang-include-fixer/IncludeFixer.cpp
  clang-tools-extra/clang-include-fixer/IncludeFixer.h
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clangd/indexer/IndexerMain.cpp
  clang-tools-extra/clangd/unittests/IndexActionTests.cpp
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
  clang-tools-extra/unittests/clang-include-fixer/IncludeFixerTest.cpp
  
clang-tools-extra/unittests/clang-include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
  clang-tools-extra/unittests/clang-tidy/ClangTidyTest.h
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h
  clang/include/clang/Tooling/Tooling.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/lib/Tooling/Tooling.cpp
  
clang/test/Tooling/multiple-source-include-different-header-with-same-relative-path.cpp
  clang/tools/clang-fuzzer/handle-cxx/handle_cxx.cpp
  clang/unittests/Tooling/DependencyScannerTest.cpp
  clang/unittests/Tooling/ToolingTest.cpp

Index: clang/unittests/Tooling/ToolingTest.cpp
===
--- clang/unittests/Tooling/ToolingTest.cpp
+++ clang/unittests/Tooling/ToolingTest.cpp
@@ -179,8 +179,8 @@
   llvm::IntrusiveRefCntPtr InMemoryFileSystem(
   new llvm::vfs::InMemoryFileSystem);
   OverlayFileSystem->pushOverlay(InMemoryFileSystem);
-  llvm::IntrusiveRefCntPtr Files(
-  new FileManager(FileSystemOptions(), OverlayFileSystem));
+  llvm::IntrusiveRefCntPtr Files(
+  new ToolFileManager(FileSystemOptions(), OverlayFileSystem));
   std::vector Args;
   Args.push_back("tool-executable");
   Args.push_back("-Idef");
@@ -205,8 +205,8 @@
   llvm::IntrusiveRefCntPtr InMemoryFileSystem(
   new llvm::vfs::InMemoryFileSystem);
   OverlayFileSystem->pushOverlay(InMemoryFileSystem);
-  llvm::IntrusiveRefCntPtr Files(
-  new FileManager(FileSystemOptions(), OverlayFileSystem));
+  llvm::IntrusiveRefCntPtr Files(
+  new ToolFileManager(FileSystemOptions(), OverlayFileSystem));
   std::vector Args;
   Args.push_back("tool-executable");
   Args.push_back("-Idef");
@@ -231,8 +231,8 @@
   llvm::IntrusiveRefCntPtr InMemoryFileSystem(
   new llvm::vfs::InMemoryFileSystem);
   OverlayFileSystem->pushOverlay(InMemoryFileSystem);
-  llvm::IntrusiveRefCntPtr Files(
-  new FileManager(FileSystemOptions(), OverlayFileSystem));
+  llvm::IntrusiveRefCntPtr Files(
+  new ToolFileManager(FileSystemOptions(), OverlayFileSystem));
 
   std::vector Args;
   Args.push_back("tool-executable");
@@ -260,8 +260,8 @@
   llvm::IntrusiveRefCntPtr InMemoryFileSystem(
   new llvm::vfs::InMemoryFileSystem);
   OverlayFileSystem->pushOverlay(InMemoryFileSystem);
-  llvm::IntrusiveRefCntPtr Files(
-  new FileManager(FileSystemOptions(), OverlayFileSystem));
+  llvm::IntrusiveRefCntPtr Files(
+  new ToolFileManager(FileSystemOptions(), OverlayFileSystem));
 
   std::vector Args;
   Args.push_back("tool-executable");
@@ -306,8 +306,8 @@
   llvm::IntrusiveRefCntPtr InMemoryFileSystem(
   new llvm::vfs::InMemoryFileSystem);
   OverlayFileSystem->pushOverlay(InMemoryFileSystem);
-  llvm::IntrusiveRefCntPtr Files(
-  new FileManager(FileSystemOptions(), OverlayFileSystem));
+  llvm::IntrusiveRefCntPtr Files(
+  new ToolFileManager(FileSystemOptions(), OverlayFileSystem));
   std::vector Args;
   Args.push_back("tool-executable");
   // Note: intentional error; user probably meant -ferror-limit=0.
Index: clang/unittests/Tooling/DependencyScannerTest.cpp
===
--- clang/unittests/Tooling/DependencyScannerTest.cpp
+++ clang/unittests/Tooling/DependencyScannerTest.cpp
@@ -51,18 +51,18 @@
   TestDependencyScanningAction(std::vector ) : Deps(Deps) {}
 
   bool runInvocation(std::shared_ptr Invocation,
- FileManager *FileMgr,
+ ToolFileManager *FileMgr,
  std::shared_ptr PCHContainerOps,
  DiagnosticConsumer *DiagConsumer) override {
 CompilerInstance Compiler(std::move(PCHContainerOps));
 Compiler.setInvocation(std::move(Invocation));
-Compiler.setFileManager(FileMgr);
+Compiler.setFileManager(FileMgr->getOrCreateFileManager().get());
 
 Compiler.createDiagnostics(DiagConsumer, /*ShouldOwnClient=*/false);
 if (!Compiler.hasDiagnostics())
   return false;
 
-Compiler.createSourceManager(*FileMgr);
+Compiler.createSourceManager(*FileMgr->getOrCreateFileManager());
 Compiler.addDependencyCollector(std::make_shared(
 

[PATCH] D124816: [LibTooling] use ToolFileManager to store file managers for each CWD

[Shi Chen via Phabricator via cfe-commits]

Kale added inline comments.



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:171-172
 FileMgr->getFileSystemOpts().WorkingDir = std::string(WorkingDirectory);
 ScanInstance.setFileManager(FileMgr);
 ScanInstance.createSourceManager(*FileMgr);
 

dexonsmith wrote:
> Do these need to be moved lower, after the FileMgr/VFS might change?
Seems `visitPrebuiltModule` in line 179 uses the FileManager set by 
`ScanInstance.setFileManager(FileMgr);`, perhaps it's better to keep it here, 
and redo this after `createVFSFromCompilerInvocation`.



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:202-203
   // filesystem.
-  FileMgr->setVirtualFileSystem(createVFSFromCompilerInvocation(
+  ToolFileMgr->setVirtualFileSystem(createVFSFromCompilerInvocation(
   ScanInstance.getInvocation(), ScanInstance.getDiagnostics(), DepFS));
 

dexonsmith wrote:
> I don't see how calling this on `ToolFileMgr` threads through to updating the 
> `FileMgr` in use, which has already been sent to `ScanInstance`.
> 
> Note that `createVFSFromCompilerInvocation()` *always* returns a new VFS, 
> even if it's equivalent to the one used before (same DepFS options and same 
> `-ivfsoverlay`s from `ScanInstance.getInvocation()`). Threading it through 
> would mean that `FileMgr` is never reused.
> 
> IMO, never-reuse-the-filemanager is the right/correct behaviour for 
> clang-scan-deps, since DepFS provides caching for things that can be cached 
> soundly, but probably better to explicitly stop reusing the FileMgr (in a 
> separate patch ahead of time) rather than leaving code that looks like it 
> might get reused.
That makes sense. Will prepend a patch to explicitly create a new FileManager 
if DepFS is used.



Comment at: clang/lib/Tooling/Tooling.cpp:447-448
   appendArgumentsAdjuster(getClangStripDependencyFileAdjuster());
   if (Files)
 Files->setVirtualFileSystem(OverlayFileSystem);
 }

dexonsmith wrote:
> This will *never* reuse the file manager, since it's *always* a new VFS.
> - Maybe that's correct, since we want to inject different things into 
> InMemoryFS?
> - Or, maybe that's overly pessimistic, and we should pass BaseFS into the 
> filemanager if `appendArgumentsAdjuster()` never added anything to InMemoryFS?
Use BaseFS as the condition to judge whether to invalidate previous file 
managers? Sounds worth a try. Will amend later.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124816/new/

https://reviews.llvm.org/D124816

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124816: [LibTooling] use ToolFileManager to store file managers for each CWD

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

This looks mostly correct to me, but perhaps pessimistic enough it'll regress 
performance unnecessarily. A few comments inline.




Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:169-170
 ScanInstance.getFrontendOpts().UseGlobalModuleIndex = false;
-
+auto FileMgr = ToolFileMgr->getOrCreateFileManager().get();
 FileMgr->getFileSystemOpts().WorkingDir = std::string(WorkingDirectory);
 ScanInstance.setFileManager(FileMgr);

Should `WorkingDirectory` be an argument to `getOrCreateFileManager()` here?



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:171-172
 FileMgr->getFileSystemOpts().WorkingDir = std::string(WorkingDirectory);
 ScanInstance.setFileManager(FileMgr);
 ScanInstance.createSourceManager(*FileMgr);
 

Do these need to be moved lower, after the FileMgr/VFS might change?



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:202-203
   // filesystem.
-  FileMgr->setVirtualFileSystem(createVFSFromCompilerInvocation(
+  ToolFileMgr->setVirtualFileSystem(createVFSFromCompilerInvocation(
   ScanInstance.getInvocation(), ScanInstance.getDiagnostics(), DepFS));
 

I don't see how calling this on `ToolFileMgr` threads through to updating the 
`FileMgr` in use, which has already been sent to `ScanInstance`.

Note that `createVFSFromCompilerInvocation()` *always* returns a new VFS, even 
if it's equivalent to the one used before (same DepFS options and same 
`-ivfsoverlay`s from `ScanInstance.getInvocation()`). Threading it through 
would mean that `FileMgr` is never reused.

IMO, never-reuse-the-filemanager is the right/correct behaviour for 
clang-scan-deps, since DepFS provides caching for things that can be cached 
soundly, but probably better to explicitly stop reusing the FileMgr (in a 
separate patch ahead of time) rather than leaving code that looks like it might 
get reused.



Comment at: clang/lib/Tooling/Tooling.cpp:447-448
   appendArgumentsAdjuster(getClangStripDependencyFileAdjuster());
   if (Files)
 Files->setVirtualFileSystem(OverlayFileSystem);
 }

This will *never* reuse the file manager, since it's *always* a new VFS.
- Maybe that's correct, since we want to inject different things into 
InMemoryFS?
- Or, maybe that's overly pessimistic, and we should pass BaseFS into the 
filemanager if `appendArgumentsAdjuster()` never added anything to InMemoryFS?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124816/new/

https://reviews.llvm.org/D124816

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124816: [LibTooling] use ToolFileManager to store file managers for each CWD

[Shi Chen via Phabricator via cfe-commits]

Kale updated this revision to Diff 427822.
Kale added a comment.
Herald added subscribers: carlosgalvezp, usaxena95, kadircet, arphaman.
Herald added a project: clang-tools-extra.

Fix tests & runInvocation in clang-tools-extra.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124816/new/

https://reviews.llvm.org/D124816

Files:
  clang-tools-extra/clang-include-fixer/IncludeFixer.cpp
  clang-tools-extra/clang-include-fixer/IncludeFixer.h
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clangd/indexer/IndexerMain.cpp
  clang-tools-extra/clangd/unittests/IndexActionTests.cpp
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
  clang-tools-extra/unittests/clang-include-fixer/IncludeFixerTest.cpp
  
clang-tools-extra/unittests/clang-include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
  clang-tools-extra/unittests/clang-tidy/ClangTidyTest.h
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h
  clang/include/clang/Tooling/Tooling.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/lib/Tooling/Tooling.cpp
  
clang/test/Tooling/multiple-source-include-different-header-with-same-relative-path.cpp
  clang/tools/clang-fuzzer/handle-cxx/handle_cxx.cpp
  clang/unittests/Tooling/DependencyScannerTest.cpp
  clang/unittests/Tooling/ToolingTest.cpp

Index: clang/unittests/Tooling/ToolingTest.cpp
===
--- clang/unittests/Tooling/ToolingTest.cpp
+++ clang/unittests/Tooling/ToolingTest.cpp
@@ -179,8 +179,8 @@
   llvm::IntrusiveRefCntPtr InMemoryFileSystem(
   new llvm::vfs::InMemoryFileSystem);
   OverlayFileSystem->pushOverlay(InMemoryFileSystem);
-  llvm::IntrusiveRefCntPtr Files(
-  new FileManager(FileSystemOptions(), OverlayFileSystem));
+  llvm::IntrusiveRefCntPtr Files(
+  new ToolFileManager(FileSystemOptions(), OverlayFileSystem));
   std::vector Args;
   Args.push_back("tool-executable");
   Args.push_back("-Idef");
@@ -205,8 +205,8 @@
   llvm::IntrusiveRefCntPtr InMemoryFileSystem(
   new llvm::vfs::InMemoryFileSystem);
   OverlayFileSystem->pushOverlay(InMemoryFileSystem);
-  llvm::IntrusiveRefCntPtr Files(
-  new FileManager(FileSystemOptions(), OverlayFileSystem));
+  llvm::IntrusiveRefCntPtr Files(
+  new ToolFileManager(FileSystemOptions(), OverlayFileSystem));
   std::vector Args;
   Args.push_back("tool-executable");
   Args.push_back("-Idef");
@@ -231,8 +231,8 @@
   llvm::IntrusiveRefCntPtr InMemoryFileSystem(
   new llvm::vfs::InMemoryFileSystem);
   OverlayFileSystem->pushOverlay(InMemoryFileSystem);
-  llvm::IntrusiveRefCntPtr Files(
-  new FileManager(FileSystemOptions(), OverlayFileSystem));
+  llvm::IntrusiveRefCntPtr Files(
+  new ToolFileManager(FileSystemOptions(), OverlayFileSystem));
 
   std::vector Args;
   Args.push_back("tool-executable");
@@ -260,8 +260,8 @@
   llvm::IntrusiveRefCntPtr InMemoryFileSystem(
   new llvm::vfs::InMemoryFileSystem);
   OverlayFileSystem->pushOverlay(InMemoryFileSystem);
-  llvm::IntrusiveRefCntPtr Files(
-  new FileManager(FileSystemOptions(), OverlayFileSystem));
+  llvm::IntrusiveRefCntPtr Files(
+  new ToolFileManager(FileSystemOptions(), OverlayFileSystem));
 
   std::vector Args;
   Args.push_back("tool-executable");
@@ -306,8 +306,8 @@
   llvm::IntrusiveRefCntPtr InMemoryFileSystem(
   new llvm::vfs::InMemoryFileSystem);
   OverlayFileSystem->pushOverlay(InMemoryFileSystem);
-  llvm::IntrusiveRefCntPtr Files(
-  new FileManager(FileSystemOptions(), OverlayFileSystem));
+  llvm::IntrusiveRefCntPtr Files(
+  new ToolFileManager(FileSystemOptions(), OverlayFileSystem));
   std::vector Args;
   Args.push_back("tool-executable");
   // Note: intentional error; user probably meant -ferror-limit=0.
Index: clang/unittests/Tooling/DependencyScannerTest.cpp
===
--- clang/unittests/Tooling/DependencyScannerTest.cpp
+++ clang/unittests/Tooling/DependencyScannerTest.cpp
@@ -51,18 +51,18 @@
   TestDependencyScanningAction(std::vector ) : Deps(Deps) {}
 
   bool runInvocation(std::shared_ptr Invocation,
- FileManager *FileMgr,
+ ToolFileManager *FileMgr,
  std::shared_ptr PCHContainerOps,
  DiagnosticConsumer *DiagConsumer) override {
 CompilerInstance Compiler(std::move(PCHContainerOps));
 Compiler.setInvocation(std::move(Invocation));
-Compiler.setFileManager(FileMgr);
+Compiler.setFileManager(FileMgr->getOrCreateFileManager().get());
 
 Compiler.createDiagnostics(DiagConsumer, /*ShouldOwnClient=*/false);
 if (!Compiler.hasDiagnostics())
   return false;
 
-Compiler.createSourceManager(*FileMgr);
+Compiler.createSourceManager(*FileMgr->getOrCreateFileManager());
 Compiler.addDependencyCollector(std::make_shared(