[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 &FileSystemOpts,
+                     IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS = nullptr)
+      : FileManager(FileSystemOpts, FS){};
+  llvm::Expected<FileEntryRef> 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<FrontendAction> ToolAction, const Twine &Code,
     llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS,
@@ -208,7 +231,7 @@
   StringRef FileNameRef = FileName.toNullTerminatedStringRef(FileNameStorage);
 
   llvm::IntrusiveRefCntPtr<FileManager> 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<FileManager> 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 &FileSystemOpts,
               IntrusiveRefCntPtr<llvm::vfs::FileSystem> 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<FileEntryRef> getFileRef(StringRef Filename,
-                                          bool OpenFile = false,
-                                          bool CacheFailure = true);
+  virtual llvm::Expected<FileEntryRef> getFileRef(StringRef Filename,
+                                                  bool OpenFile = false,
+                                                  bool CacheFailure = true);
 
   /// Get the FileEntryRef for stdin, returning an error if stdin cannot be
   /// read.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D124816.426558.patch
Type: text/x-patch
Size: 4172 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220503/e92dedf2/attachment.bin>