[PATCH] D125859: [clang][deps] stop reusing FileManager in DependencyScanningAction

[Shi Chen via Phabricator via cfe-commits]

Kale created this revision.
Herald added a project: All.
Kale requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

DepFS already provides necessary caching, so explicitly stop reusing the 
FileMgr for possibly cache conflict.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D125859

Files:
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp


Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -7,6 +7,7 @@
 
//===--===//
 
 #include "clang/Tooling/DependencyScanning/DependencyScanningWorker.h"
+#include "clang/Basic/FileManager.h"
 #include "clang/CodeGen/ObjectFilePCHContainerOperations.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
@@ -197,10 +198,14 @@
   for (const auto  : ScanInstance.getHeaderSearchOpts().VFSOverlayFiles)
 DepFS->disableMinimization(F);
 
-  // Support for virtual file system overlays on top of the caching
-  // filesystem.
-  FileMgr->setVirtualFileSystem(createVFSFromCompilerInvocation(
-  ScanInstance.getInvocation(), ScanInstance.getDiagnostics(), DepFS));
+  // DepFS already provides caching, so we don't need to reuse FileManager.
+  // The provided FileManger is abandoned for possibly invalid caches 
caused
+  // by the change of VFS.
+  auto VFS = createVFSFromCompilerInvocation(
+  ScanInstance.getInvocation(), ScanInstance.getDiagnostics(), DepFS);
+  FileMgr = new FileManager(FileMgr->getFileSystemOpts(), VFS);
+  ScanInstance.setFileManager(FileMgr);
+  ScanInstance.createSourceManager(*FileMgr);
 
   // Pass the skip mappings which should speed up excluded conditional 
block
   // skipping in the preprocessor.


Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "clang/Tooling/DependencyScanning/DependencyScanningWorker.h"
+#include "clang/Basic/FileManager.h"
 #include "clang/CodeGen/ObjectFilePCHContainerOperations.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
@@ -197,10 +198,14 @@
   for (const auto  : ScanInstance.getHeaderSearchOpts().VFSOverlayFiles)
 DepFS->disableMinimization(F);
 
-  // Support for virtual file system overlays on top of the caching
-  // filesystem.
-  FileMgr->setVirtualFileSystem(createVFSFromCompilerInvocation(
-  ScanInstance.getInvocation(), ScanInstance.getDiagnostics(), DepFS));
+  // DepFS already provides caching, so we don't need to reuse FileManager.
+  // The provided FileManger is abandoned for possibly invalid caches caused
+  // by the change of VFS.
+  auto VFS = createVFSFromCompilerInvocation(
+  ScanInstance.getInvocation(), ScanInstance.getDiagnostics(), DepFS);
+  FileMgr = new FileManager(FileMgr->getFileSystemOpts(), VFS);
+  ScanInstance.setFileManager(FileMgr);
+  ScanInstance.createSourceManager(*FileMgr);
 
   // Pass the skip mappings which should speed up excluded conditional block
   // skipping in the preprocessor.
___
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 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

[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(
 

[PATCH] D124816: [Tooling] use different FileManager for each CWD

[Shi Chen via Phabricator via cfe-commits]

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

Reimplement ToolFileManager as a container of filemanagers which ToolInvocation 
takes by parameter. Also invalidate all file managers when using 
FileManager::setVirtualFileSystem().


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124816

Files:
  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/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(
 Compiler.getInvocation().getDependencyOutputOpts(), Deps));
 
Index: clang/test/Tooling/multiple-source-include-different-header-with-same-relative-path.cpp
===
--- /dev/null
+++ clang/test/Tooling/multiple-source-include-different-header-with-same-relative-path.cpp
@@ -0,0 +1,42 @@
+// This test case presents a circumstance that the information related one file
+// is misused by another.
+//
+// The path tree of the test directory is as follors.
+//   .
+//   ├── a
+//   │   ├── a.c
+//   │   └── config.h
+//  

[PATCH] D124816: [Tooling] use different FileManager for each CWD

[Shi Chen via Phabricator via cfe-commits]

Kale added a comment.

In D124816#3494243 , @dexonsmith 
wrote:

> Two high-level comments:
>
> - Making these functions `virtual` makes it harder to reason about possible 
> implementations, but I don't think that's necessary for what you're doing 
> here. Why not expose `ToolFileManager` as a container of filemanagers which 
> ToolInvocation takes by parameter, then calls `getOrCreateFileManager()` on 
> at time of need passing relevant arguments (e.g., CWD)?
>   - This could also create/manage the relevant VFS instance, to which the 
> FileManager is fundamentally tied. See also below.
> - I don't think this goes far enough. The FileManager keeps state for any 
> accessed file, which can be wrong any time the VFS changes at all, not just 
> if the `CWD` is different. E.g., different `-ivfsoverlay` options are passed, 
> or someone is reading from an `InMemoryFileSystem` vs. from disk.
>   - AFAICT, every call to `FileManager::setVirtualFileSystem()` is 
> fundamentally unsound unless the new FS is equivalent to the old one or the 
> filemanager hasn't been used yet.

Thanks for your careful and useful suggestions. With your explanation, I 
realized that the change of VFS of course should be considered as well. I'll 
revise this patch and tries to cope with it.


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: [Tooling] use different FileManager for each CWD

[Shi Chen via Phabricator via cfe-commits]

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

Format all overriding functions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124816

Files:
  clang/include/clang/Basic/FileManager.h
  clang/include/clang/Tooling/Tooling.h
  clang/lib/Tooling/Tooling.cpp
  
clang/test/Tooling/multiple-source-include-different-header-with-same-relative-path.cpp

Index: clang/test/Tooling/multiple-source-include-different-header-with-same-relative-path.cpp
===
--- /dev/null
+++ clang/test/Tooling/multiple-source-include-different-header-with-same-relative-path.cpp
@@ -0,0 +1,42 @@
+// This test case presents a circumstance that the information related one file
+// is misused by another.
+//
+// The path tree of the test directory is as follors.
+//   .
+//   ├── a
+//   │   ├── a.c
+//   │   └── config.h
+//   ├── b
+//   │   ├── b.c
+//   │   └── config.h
+//   └── compile_commands.json
+//
+// Both source files (a/a.c and b/b.c) includes the config.h file of their own
+// directory with `#include "config.h"`. However, the two config.h files are
+// different. File a/config.h is longer than b/config.h. Both a/a.c and b/b.c
+// are compiled in their own directory, which is recorded in the compilation
+// database.
+//
+// When using ClangTool to parse these two source files one by one, since the
+// file name of both header files are the same, the FileManager will confuse
+// with them and using the file entry of a/config.h for b/config.h. And the
+// wrong file length will lead to a buffer overflow when reading the file.
+//
+// In this test case, to avoid buffer overflow, we use the leading '\0' in an
+// empty buffer to trigger the problem. We set a/config.h as an empty line
+// comment, and leave b/config.h empty. Firstly, a/config.h is read and cached,
+// then when reading b/config.h, if the size of a/config.h is used, the first
+// two chars are read and the first one must be a '\0'.
+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t/a
+// RUN: mkdir -p %t/b
+// RUN: echo '#include "config.h"' > %t/a/a.c
+// RUN: echo '#include "config.h"' > %t/b/b.c
+// RUN: echo '//' > %t/a/config.h
+// RUN: echo ''   > %t/b/config.h
+// RUN: echo '[{"arguments": ["cc", "-c", "-o", "a.o", "a.c"], "directory": "%t/a", "file": "a.c"}, {"arguments": ["cc", "-c", "-o", "b.o", "b.c"], "directory": "%t/b", "file": "b.c"}]' | sed -e 's/\\//g' > %t/compile_commands.json
+
+// The following two test RUNs should have no output.
+// RUN: cd %t && clang-check a/a.c b/b.c 2>&1 | count 0
+// RUN: cd %t && clang-check b/b.c a/a.c 2>&1 | count 0
Index: clang/lib/Tooling/Tooling.cpp
===
--- clang/lib/Tooling/Tooling.cpp
+++ clang/lib/Tooling/Tooling.cpp
@@ -208,7 +208,7 @@
   StringRef FileNameRef = FileName.toNullTerminatedStringRef(FileNameStorage);
 
   llvm::IntrusiveRefCntPtr Files(
-  new FileManager(FileSystemOptions(), VFS));
+  new ToolingFileManager(FileSystemOptions(), VFS));
   ArgumentsAdjuster Adjuster = getClangStripDependencyFileAdjuster();
   ToolInvocation Invocation(
   getSyntaxOnlyToolArgs(ToolName, Adjuster(Args, FileNameRef), FileNameRef),
@@ -436,7 +436,8 @@
   OverlayFileSystem(new llvm::vfs::OverlayFileSystem(std::move(BaseFS))),
   InMemoryFileSystem(new llvm::vfs::InMemoryFileSystem),
   Files(Files ? Files
-  : new FileManager(FileSystemOptions(), OverlayFileSystem)) {
+  : new ToolingFileManager(FileSystemOptions(),
+   OverlayFileSystem)) {
   OverlayFileSystem->pushOverlay(InMemoryFileSystem);
   appendArgumentsAdjuster(getClangStripOutputAdjuster());
   appendArgumentsAdjuster(getClangSyntaxOnlyAdjuster());
@@ -656,7 +657,7 @@
   new llvm::vfs::InMemoryFileSystem);
   OverlayFileSystem->pushOverlay(InMemoryFileSystem);
   llvm::IntrusiveRefCntPtr Files(
-  new FileManager(FileSystemOptions(), OverlayFileSystem));
+  new ToolingFileManager(FileSystemOptions(), OverlayFileSystem));
 
   ToolInvocation Invocation(
   getSyntaxOnlyToolArgs(ToolName, Adjuster(Args, FileName), FileName),
Index: clang/include/clang/Tooling/Tooling.h
===
--- clang/include/clang/Tooling/Tooling.h
+++ clang/include/clang/Tooling/Tooling.h
@@ -65,6 +65,67 @@
 
 class CompilationDatabase;
 
+class ToolingFileManager : public FileManager {
+  // Use a FileManager for each different CWD
+  mutable std::map fileManagers;
+
+public:
+  ToolingFileManager(const FileSystemOptions ,
+ IntrusiveRefCntPtr FS = nullptr)
+  : FileManager(FileSystemOpts, FS){};
+
+  size_t getNumUniqueRealFiles() const override {
+return getRealFileManager()->getNumUniqueRealFiles();
+  }
+
+  

[PATCH] D124816: [Tooling] use different FileManager for each CWD

[Shi Chen via Phabricator via cfe-commits]

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

Fix path in compile_commands.json for windows.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124816

Files:
  clang/include/clang/Basic/FileManager.h
  clang/include/clang/Tooling/Tooling.h
  clang/lib/Tooling/Tooling.cpp
  
clang/test/Tooling/multiple-source-include-different-header-with-same-relative-path.cpp

Index: clang/test/Tooling/multiple-source-include-different-header-with-same-relative-path.cpp
===
--- /dev/null
+++ clang/test/Tooling/multiple-source-include-different-header-with-same-relative-path.cpp
@@ -0,0 +1,42 @@
+// This test case presents a circumstance that the information related one file
+// is misused by another.
+//
+// The path tree of the test directory is as follors.
+//   .
+//   ├── a
+//   │   ├── a.c
+//   │   └── config.h
+//   ├── b
+//   │   ├── b.c
+//   │   └── config.h
+//   └── compile_commands.json
+//
+// Both source files (a/a.c and b/b.c) includes the config.h file of their own
+// directory with `#include "config.h"`. However, the two config.h files are
+// different. File a/config.h is longer than b/config.h. Both a/a.c and b/b.c
+// are compiled in their own directory, which is recorded in the compilation
+// database.
+//
+// When using ClangTool to parse these two source files one by one, since the
+// file name of both header files are the same, the FileManager will confuse
+// with them and using the file entry of a/config.h for b/config.h. And the
+// wrong file length will lead to a buffer overflow when reading the file.
+//
+// In this test case, to avoid buffer overflow, we use the leading '\0' in an
+// empty buffer to trigger the problem. We set a/config.h as an empty line
+// comment, and leave b/config.h empty. Firstly, a/config.h is read and cached,
+// then when reading b/config.h, if the size of a/config.h is used, the first
+// two chars are read and the first one must be a '\0'.
+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t/a
+// RUN: mkdir -p %t/b
+// RUN: echo '#include "config.h"' > %t/a/a.c
+// RUN: echo '#include "config.h"' > %t/b/b.c
+// RUN: echo '//' > %t/a/config.h
+// RUN: echo ''   > %t/b/config.h
+// RUN: echo '[{"arguments": ["cc", "-c", "-o", "a.o", "a.c"], "directory": "%t/a", "file": "a.c"}, {"arguments": ["cc", "-c", "-o", "b.o", "b.c"], "directory": "%t/b", "file": "b.c"}]' | sed -e 's/\\//g' > %t/compile_commands.json
+
+// The following two test RUNs should have no output.
+// RUN: cd %t && clang-check a/a.c b/b.c 2>&1 | count 0
+// RUN: cd %t && clang-check b/b.c a/a.c 2>&1 | count 0
Index: clang/lib/Tooling/Tooling.cpp
===
--- clang/lib/Tooling/Tooling.cpp
+++ clang/lib/Tooling/Tooling.cpp
@@ -208,7 +208,7 @@
   StringRef FileNameRef = FileName.toNullTerminatedStringRef(FileNameStorage);
 
   llvm::IntrusiveRefCntPtr Files(
-  new FileManager(FileSystemOptions(), VFS));
+  new ToolingFileManager(FileSystemOptions(), VFS));
   ArgumentsAdjuster Adjuster = getClangStripDependencyFileAdjuster();
   ToolInvocation Invocation(
   getSyntaxOnlyToolArgs(ToolName, Adjuster(Args, FileNameRef), FileNameRef),
@@ -436,7 +436,8 @@
   OverlayFileSystem(new llvm::vfs::OverlayFileSystem(std::move(BaseFS))),
   InMemoryFileSystem(new llvm::vfs::InMemoryFileSystem),
   Files(Files ? Files
-  : new FileManager(FileSystemOptions(), OverlayFileSystem)) {
+  : new ToolingFileManager(FileSystemOptions(),
+   OverlayFileSystem)) {
   OverlayFileSystem->pushOverlay(InMemoryFileSystem);
   appendArgumentsAdjuster(getClangStripOutputAdjuster());
   appendArgumentsAdjuster(getClangSyntaxOnlyAdjuster());
@@ -656,7 +657,7 @@
   new llvm::vfs::InMemoryFileSystem);
   OverlayFileSystem->pushOverlay(InMemoryFileSystem);
   llvm::IntrusiveRefCntPtr Files(
-  new FileManager(FileSystemOptions(), OverlayFileSystem));
+  new ToolingFileManager(FileSystemOptions(), OverlayFileSystem));
 
   ToolInvocation Invocation(
   getSyntaxOnlyToolArgs(ToolName, Adjuster(Args, FileName), FileName),
Index: clang/include/clang/Tooling/Tooling.h
===
--- clang/include/clang/Tooling/Tooling.h
+++ clang/include/clang/Tooling/Tooling.h
@@ -65,6 +65,69 @@
 
 class CompilationDatabase;
 
+class ToolingFileManager : public FileManager {
+  // Use a FileManager for each different CWD
+  mutable std::map fileManagers;
+
+public:
+  ToolingFileManager(const FileSystemOptions ,
+ IntrusiveRefCntPtr FS = nullptr)
+  : FileManager(FileSystemOpts, FS){};
+
+  size_t getNumUniqueRealFiles() const override {
+return getRealFileManager()->getNumUniqueRealFiles();

[PATCH] D124816: [Tooling] add ToolingFileManager to force using abs path

[Shi Chen via Phabricator via cfe-commits]

Kale created this revision.
Herald added a subscriber: dexonsmith.
Herald added a project: All.
Kale requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

ClangTool will make FileManager mix up two header files with the same relative 
path in different absolute paths.

As the cache of previously opened FileEntry in FileManager is indexed by the 
file name, when relative paths are used as the index, wrong FileEntry may be 
used for the file with the same relative path. With ClangTool, as the current 
working directory will change when parsing multiple files, files with the same 
relative paths but different absolute paths will be mixed up by the FileManager.

This patch tries to solve it by using a modified FileManager which always uses 
absolute path

Refer-to: https://github.com/llvm/llvm-project/issues/54410
Refer-to: https://reviews.llvm.org/D92160


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124816

Files:
  clang/include/clang/Basic/FileManager.h
  clang/lib/Tooling/Tooling.cpp


Index: clang/lib/Tooling/Tooling.cpp
===
--- clang/lib/Tooling/Tooling.cpp
+++ clang/lib/Tooling/Tooling.cpp
@@ -198,6 +198,29 @@
 namespace clang {
 namespace tooling {
 
+class ToolingFileManager : public FileManager {
+public:
+  ToolingFileManager(const FileSystemOptions ,
+ IntrusiveRefCntPtr FS = nullptr)
+  : FileManager(FileSystemOpts, FS){};
+  llvm::Expected getFileRef(StringRef Filename, bool openFile,
+  bool CacheFailure) override {
+// This is a hack to detect whether the code is running in tests.
+// FIXME: Use an option to control might be better?
+if (getenv("GTEST_TOTAL_SHARDS") != nullptr) {
+  // Enable to use relative path in tests for convenience.
+  return FileManager::getFileRef(Filename, openFile, CacheFailure);
+} else {
+  // The original File Manager is designed for immutable CWD, it is fine
+  // for single compiler invocation, but not always correct for libtooling
+  // who sometimes needs to parse multiple compiler invocations in 
different
+  // CWD, which would be confused by the same relative path.
+  return FileManager::getFileRef(getAbsolutePath(Filename), openFile,
+ CacheFailure);
+}
+  }
+};
+
 bool runToolOnCodeWithArgs(
 std::unique_ptr ToolAction, const Twine ,
 llvm::IntrusiveRefCntPtr VFS,
@@ -208,7 +231,7 @@
   StringRef FileNameRef = FileName.toNullTerminatedStringRef(FileNameStorage);
 
   llvm::IntrusiveRefCntPtr Files(
-  new FileManager(FileSystemOptions(), VFS));
+  new ToolingFileManager(FileSystemOptions(), VFS));
   ArgumentsAdjuster Adjuster = getClangStripDependencyFileAdjuster();
   ToolInvocation Invocation(
   getSyntaxOnlyToolArgs(ToolName, Adjuster(Args, FileNameRef), 
FileNameRef),
@@ -436,7 +459,8 @@
   OverlayFileSystem(new llvm::vfs::OverlayFileSystem(std::move(BaseFS))),
   InMemoryFileSystem(new llvm::vfs::InMemoryFileSystem),
   Files(Files ? Files
-  : new FileManager(FileSystemOptions(), OverlayFileSystem)) {
+  : new ToolingFileManager(FileSystemOptions(),
+   OverlayFileSystem)) {
   OverlayFileSystem->pushOverlay(InMemoryFileSystem);
   appendArgumentsAdjuster(getClangStripOutputAdjuster());
   appendArgumentsAdjuster(getClangSyntaxOnlyAdjuster());
@@ -656,7 +680,7 @@
   new llvm::vfs::InMemoryFileSystem);
   OverlayFileSystem->pushOverlay(InMemoryFileSystem);
   llvm::IntrusiveRefCntPtr Files(
-  new FileManager(FileSystemOptions(), OverlayFileSystem));
+  new ToolingFileManager(FileSystemOptions(), OverlayFileSystem));
 
   ToolInvocation Invocation(
   getSyntaxOnlyToolArgs(ToolName, Adjuster(Args, FileName), FileName),
Index: clang/include/clang/Basic/FileManager.h
===
--- clang/include/clang/Basic/FileManager.h
+++ clang/include/clang/Basic/FileManager.h
@@ -135,7 +135,7 @@
   /// llvm::vfs::getRealFileSystem().
   FileManager(const FileSystemOptions ,
   IntrusiveRefCntPtr FS = nullptr);
-  ~FileManager();
+  virtual ~FileManager();
 
   /// Installs the provided FileSystemStatCache object within
   /// the FileManager.
@@ -218,9 +218,9 @@
   ///
   /// \param CacheFailure If true and the file does not exist, we'll cache
   /// the failure to find this file.
-  llvm::Expected getFileRef(StringRef Filename,
-  bool OpenFile = false,
-  bool CacheFailure = true);
+  virtual llvm::Expected getFileRef(StringRef Filename,
+  bool OpenFile = false,
+  bool CacheFailure = true);
 
   /// Get the FileEntryRef