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

[Kale]

Summary

ClangTool using single FileManager might confuse two header files with the same relative path in different CWD(current working directory).

As the cache of previously opened FileEntry in FileManager is indexed by the file name, which is fine to be used in single compiler invocation, ClangTool, however, needs to parse more than one compile command, and the CWD is not assured to be consistent, thus the previous cached FileEntry which uses relative path as index would be invalid.

This patch tries to solve it by using a container named ToolFileManager called, which actually stores different FileManager for different CWD.

All file managers stored in ToolFileManager will be invalidated from where using FileManager::setVirtualFileSystem(), because of possibly unsound file status caches.

The unit test is written by Hao Zhang <[e-mail]> in his original but unfinished commit.

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

[Kale]

Use different FileManager for each CWD instead. Also add a unit test.

[Kale]

Fix path in compile_commands.json for windows.

[Kale]

Format all overriding functions

[dexonsmith]

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.

[dexonsmith]

In D124816#3494243, @dexonsmith wrote:

• 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.

More precisely, I think calling FileManager::setVirtualFileSystem() is fundamentally unsound unless the new VFS behaves identically for every path the FileManager has accessed so far.

[Kale]

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.

[Kale]

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

[Kale]

Fix tests & runInvocation in clang-tools-extra.

[dexonsmith]

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

clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp

169–170 ↗

(On Diff #427822)

Should WorkingDirectory be an argument to getOrCreateFileManager() here?

171–172 ↗

(On Diff #427822)

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

202–203 ↗

(On Diff #427822)

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 -ivfsoverlays 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.

clang/lib/Tooling/Tooling.cpp

445–446

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?

[Kale]

clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp

171–172 ↗

(On Diff #427822)

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.

202–203 ↗

(On Diff #427822)

That makes sense. Will prepend a patch to explicitly create a new FileManager if DepFS is used.

clang/lib/Tooling/Tooling.cpp

445–446

Use BaseFS as the condition to judge whether to invalidate previous file managers? Sounds worth a try. Will amend later.

[Kale]

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

clang/include/clang/Basic/FileManager.h

 
 public:
   /// Construct a file manager, optionally with a custom VFS.
   ///
   /// \param FS if non-null, the VFS to use.  Otherwise uses
   /// llvm::vfs::getRealFileSystem().
   FileManager(const FileSystemOptions &FileSystemOpts,
               IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS = nullptr);
-  ~FileManager();
+  virtual ~FileManager();
 
   /// Installs the provided FileSystemStatCache object within
   /// the FileManager.
   ///
   /// Ownership of this object is transferred to the FileManager.
   ///
   /// \param statCache the new stat cache to install. Ownership of this
   /// object is transferred to the FileManager.
   void setStatCache(std::unique_ptr<FileSystemStatCache> statCache);
 
   /// Removes the FileSystemStatCache object from the manager.
   void clearStatCache();
 
   /// Returns the number of unique real file entries cached by the file manager.
-  size_t getNumUniqueRealFiles() const { return UniqueRealFiles.size(); }
+  virtual size_t getNumUniqueRealFiles() const {
+    return UniqueRealFiles.size();
+  }
 
   /// Lookup, cache, and verify the specified directory (real or
   /// virtual).
   ///
   /// This returns a \c std::error_code if there was an error reading the
   /// directory. On success, returns the reference to the directory entry
   /// together with the exact path that was used to access a file by a
   /// particular call to getDirectoryRef.
   ///
   /// \param CacheFailure If true and the file does not exist, we'll cache
   /// the failure to find this file.
-  llvm::Expected<DirectoryEntryRef> getDirectoryRef(StringRef DirName,
-                                                    bool CacheFailure = true);
+  virtual llvm::Expected<DirectoryEntryRef>
+  getDirectoryRef(StringRef DirName, bool CacheFailure = true);
 
   /// Get a \c DirectoryEntryRef if it exists, without doing anything on error.
   llvm::Optional<DirectoryEntryRef>
   getOptionalDirectoryRef(StringRef DirName, bool CacheFailure = true) {
     return llvm::expectedToOptional(getDirectoryRef(DirName, CacheFailure));
   }
 
   /// Lookup, cache, and verify the specified directory (real or
public:
   ///
   /// This returns a \c std::error_code if there was an error loading the file,
   /// or a \c FileEntryRef otherwise.
   ///
   /// \param OpenFile if true and the file exists, it will be opened.
   ///
   /// \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,
+  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.
   ///
   /// This reads and caches stdin before returning. Subsequent calls return the
   /// same file entry, and a reference to the cached input is returned by calls
   /// to getBufferForFile.
   llvm::Expected<FileEntryRef> getSTDIN();
public:
   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.
   ///
   /// The file itself is not accessed.
-  FileEntryRef getVirtualFileRef(StringRef Filename, off_t Size,
+  virtual FileEntryRef getVirtualFileRef(StringRef Filename, off_t Size,
                                          time_t ModificationTime);
 
   const FileEntry *getVirtualFile(StringRef Filename, off_t Size,
                                   time_t ModificationTime);
 
   /// Retrieve a FileEntry that bypasses VFE, which is expected to be a virtual
   /// file entry, to access the real file.  The returned FileEntry will have
   /// the same filename as FE but a different identity and its own stat.
   ///
   /// This should be used only for rare error recovery paths because it
   /// bypasses all mapping and uniquing, blindly creating a new FileEntry.
   /// There is no attempt to deduplicate these; if you bypass the same file
   /// twice, you get two new file entries.
-  llvm::Optional<FileEntryRef> getBypassFile(FileEntryRef VFE);
+  virtual llvm::Optional<FileEntryRef> getBypassFile(FileEntryRef VFE);
 
   /// Open the specified file as a MemoryBuffer, returning a new
   /// MemoryBuffer if successful, otherwise returning null.
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
   getBufferForFile(const FileEntry *Entry, bool isVolatile = false,
                    bool RequiresNullTerminator = true);
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
   getBufferForFile(StringRef Filename, bool isVolatile = false,
public:
 
   /// Makes \c Path absolute taking into account FileSystemOptions and the
   /// working directory option.
   /// \returns true if \c Path changed to absolute.
   bool makeAbsolutePath(SmallVectorImpl<char> &Path) const;
 
   /// Produce an array mapping from the unique IDs assigned to each
   /// file to the corresponding FileEntry pointer.
-  void GetUniqueIDMapping(
-                    SmallVectorImpl<const FileEntry *> &UIDToFiles) const;
+  virtual void
+  GetUniqueIDMapping(SmallVectorImpl<const FileEntry *> &UIDToFiles) const;
 
   /// Retrieve the canonical name for a given directory.
   ///
   /// This is a very expensive operation, despite its results being cached,
   /// and should only be used when the physical layout of the file system is
   /// required, which is (almost) never.
-  StringRef getCanonicalName(const DirectoryEntry *Dir);
+  virtual StringRef getCanonicalName(const DirectoryEntry *Dir);
 
   /// Retrieve the canonical name for a given file.
   ///
   /// This is a very expensive operation, despite its results being cached,
   /// and should only be used when the physical layout of the file system is
   /// required, which is (almost) never.
-  StringRef getCanonicalName(const FileEntry *File);
+  virtual StringRef getCanonicalName(const FileEntry *File);
 
-  void PrintStats() const;
+  virtual void PrintStats() const;
 };
 
 } // end namespace clang
 
 #endif // LLVM_CLANG_BASIC_FILEMANAGER_H

clang/include/clang/Tooling/Tooling.h

 class Compilation;
 
 } // namespace driver
 
 namespace tooling {
 
 class CompilationDatabase;
 
+class ToolingFileManager : public FileManager {
+  // Use a FileManager for each different CWD
+  mutable std::map<std::string, FileManager *> fileManagers;
+
+public:
+  ToolingFileManager(const FileSystemOptions &FileSystemOpts,
+                     IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS = nullptr)
+      : FileManager(FileSystemOpts, FS){};
+
+  size_t getNumUniqueRealFiles() const override {
+    return getRealFileManager()->getNumUniqueRealFiles();
+  }
+
+  llvm::Expected<DirectoryEntryRef>
+  getDirectoryRef(StringRef DirName, bool CacheFailure = true) override {
+    auto realFileManager = getRealFileManager();
+    return realFileManager->getDirectoryRef(DirName, CacheFailure);
+  }
+
+  llvm::Expected<FileEntryRef> getFileRef(StringRef Filename, bool openFile,
+                                          bool CacheFailure) override {
+    auto realFileManager = getRealFileManager();
+    return realFileManager->getFileRef(Filename, openFile, CacheFailure);
+  }
+
+  FileEntryRef getVirtualFileRef(StringRef Filename, off_t Size,
+                                 time_t ModificationTime) override {
+    return getRealFileManager()->getVirtualFileRef(Filename, Size,
+                                                   ModificationTime);
+  }
+
+  llvm::Optional<FileEntryRef> getBypassFile(FileEntryRef VFE) override {
+    return getRealFileManager()->getBypassFile(VFE);
+  }
+
+  void GetUniqueIDMapping(
+      SmallVectorImpl<const FileEntry *> &UIDToFiles) const override {
+    getRealFileManager()->GetUniqueIDMapping(UIDToFiles);
+  }
+
+  StringRef getCanonicalName(const DirectoryEntry *Dir) override {
+    return getRealFileManager()->getCanonicalName(Dir);
+  }
+
+  StringRef getCanonicalName(const FileEntry *File) override {
+    return getRealFileManager()->getCanonicalName(File);
+  }
+
+  void PrintStats() const override { getRealFileManager()->PrintStats(); }
+
+  FileManager *getRealFileManager() const {
+    auto Success = getVirtualFileSystem().getCurrentWorkingDirectory();
+    if (!Success)
+      llvm::report_fatal_error("Cannot get current working directory!\n");
+    std::string Cwd = Success.get();
+    if (!fileManagers.count(Cwd)) {
+      fileManagers[Cwd] =
+          new FileManager(getFileSystemOpts(), &getVirtualFileSystem());
+    }
+    return fileManagers[Cwd];
+  }
+};
+
 /// Retrieves the flags of the `-cc1` job in `Compilation` that has only source
 /// files as its inputs.
 /// Returns nullptr if there are no such jobs or multiple of them. Note that
 /// offloading jobs are ignored.
 const llvm::opt::ArgStringList *
 getCC1Arguments(DiagnosticsEngine *Diagnostics,
                 driver::Compilation *Compilation);
 

clang/lib/Tooling/Tooling.cpp

bool runToolOnCodeWithArgs(
     llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS,
     const std::vector<std::string> &Args, const Twine &FileName,
     const Twine &ToolName,
     std::shared_ptr<PCHContainerOperations> PCHContainerOps) {
   SmallString<16> FileNameStorage;
   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),
       std::move(ToolAction), Files.get(), std::move(PCHContainerOps));
   return Invocation.run();
 }
 
 bool runToolOnCodeWithArgs(
ClangTool::ClangTool(const CompilationDatabase &Compilations,
                      std::shared_ptr<PCHContainerOperations> PCHContainerOps,
                      IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS,
                      IntrusiveRefCntPtr<FileManager> Files)
     : Compilations(Compilations), SourcePaths(SourcePaths),
       PCHContainerOps(std::move(PCHContainerOps)),
       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());
   appendArgumentsAdjuster(getClangStripDependencyFileAdjuster());
   if (Files)
     Files->setVirtualFileSystem(OverlayFileSystem);
 }
 
 ClangTool::~ClangTool() = default;
 
 void ClangTool::mapVirtualFile(StringRef FilePath, StringRef Content) {
   MappedFileContents.push_back(std::make_pair(FilePath, Content));
 }
 
std::unique_ptr<ASTUnit> buildASTFromCodeWithArgs(
   std::vector<std::unique_ptr<ASTUnit>> ASTs;
   ASTBuilderAction Action(ASTs);
   llvm::IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem> OverlayFileSystem(
       new llvm::vfs::OverlayFileSystem(llvm::vfs::getRealFileSystem()));
   llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> InMemoryFileSystem(
       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),
       &Action, Files.get(), std::move(PCHContainerOps));
   Invocation.setDiagnosticConsumer(DiagConsumer);
 
   InMemoryFileSystem->addFile(FileName, 0,
                               llvm::MemoryBuffer::getMemBufferCopy(Code));

clang/test/Tooling/multiple-source-include-different-header-with-same-relative-path.cpp

+// 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"}]' > %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