[clang] Processing real directories added as virtual ones

[ivanmurashko]

The FileManager might create a virtual directory that can be used later as a search path. This is the case when we use remapping, as demonstrated in the suggested LIT test.

We might encounter a false cache miss and add the same directory twice into FileManager::SeenDirEntries if the added record is a real directory that is present on a disk:

• Once as a virtual directory
• And once as a real one

This isn't a problem if the added directories have the same name, as in this case, we will get a cache hit. However, it could lead to compilation errors if the directory names are different but point to the same folder. For example, one might use an absolute name and another a relative one. For instance, the implicit-module-remap.cpp LIT test will fail with the following message:

/.../implicit-module-remap.cpp.tmp/test.cpp:1:2: fatal error: module 'a' was built in directory 
'/.../implicit-module-remap.cpp.tmp' but now resides in directory '.'
    1 | #include "a.h"
      |  ^
1 error generated.

The suggested fix checks if the added virtual directory is present on the disk and handles it as a real one if that is the case.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-modules

Author: Ivan Murashko (ivanmurashko)

Changes

The FileManager might create a virtual directory that can be used later as a search path. If the added record is a real directory that is present on a disk, we might encounter a false cache miss and add the same directory twice:

• Once as a virtual directory
• And once as a real one

This isn't a problem if the added directories have the same name. However, it could lead to compilation errors if the directory names are different. For example, one might use an absolute name and another a relative one. For instance, the implicit-module-remap.cpp LIT test will fail with the following message:

/.../implicit-module-remap.cpp.tmp/test.cpp:1:2: fatal error: module 'a' was built in directory 
'/.../implicit-module-remap.cpp.tmp' but now resides in directory '.'
    1 | #include "a.h"
      |  ^
1 error generated.

The suggested fix checks if the added virtual directory is present on the disk and handles it as a real one if that is the case.

---

Full diff: https://github.com/llvm/llvm-project/pull/91645.diff

3 Files Affected:

• (modified) clang/include/clang/Basic/FileManager.h (+2)
• (modified) clang/lib/Basic/FileManager.cpp (+33-15)
• (added) clang/test/Modules/implicit-module-remap.cpp (+21)

diff --git a/clang/include/clang/Basic/FileManager.h b/clang/include/clang/Basic/FileManager.h
index 8b4206e52cd48..e1f33d57a8980 100644
--- a/clang/include/clang/Basic/FileManager.h
+++ b/clang/include/clang/Basic/FileManager.h
@@ -299,6 +299,8 @@ class FileManager : public RefCountedBase<FileManager> {
   getBufferForFileImpl(StringRef Filename, int64_t FileSize, bool isVolatile,
                        bool RequiresNullTerminator) const;
 
+  DirectoryEntry *&getRealDirEntry(const llvm::vfs::Status &Status);
+
 public:
   /// Get the 'stat' information for the given \p Path.
   ///
diff --git a/clang/lib/Basic/FileManager.cpp b/clang/lib/Basic/FileManager.cpp
index 143c04309d075..1dc51deb82987 100644
--- a/clang/lib/Basic/FileManager.cpp
+++ b/clang/lib/Basic/FileManager.cpp
@@ -82,6 +82,22 @@ getDirectoryFromFile(FileManager &FileMgr, StringRef Filename,
   return FileMgr.getDirectoryRef(DirName, CacheFailure);
 }
 
+DirectoryEntry *&FileManager::getRealDirEntry(const llvm::vfs::Status &Status) {
+  assert(Status.isDirectory() && "The directory should exist!");
+  // See if we have already opened a directory with the
+  // same inode (this occurs on Unix-like systems when one dir is
+  // symlinked to another, for example) or the same path (on
+  // Windows).
+  DirectoryEntry *&UDE = UniqueRealDirs[Status.getUniqueID()];
+
+  if (!UDE) {
+    // We don't have this directory yet, add it.  We use the string
+    // key from the SeenDirEntries map as the string.
+    UDE = new (DirsAlloc.Allocate()) DirectoryEntry();
+  }
+  return UDE;
+}
+
 /// Add all ancestors of the given path (pointing to either a file or
 /// a directory) as virtual directories.
 void FileManager::addAncestorsAsVirtualDirs(StringRef Path) {
@@ -99,10 +115,21 @@ void FileManager::addAncestorsAsVirtualDirs(StringRef Path) {
   if (NamedDirEnt.second)
     return;
 
-  // Add the virtual directory to the cache.
-  auto *UDE = new (DirsAlloc.Allocate()) DirectoryEntry();
-  NamedDirEnt.second = *UDE;
-  VirtualDirectoryEntries.push_back(UDE);
+  // Check to see if the directory exists.
+  llvm::vfs::Status Status;
+  auto statError =
+      getStatValue(DirName, Status, false, nullptr /*directory lookup*/);
+  if (statError) {
+    // There's no real directory at the given path.
+    // Add the virtual directory to the cache.
+    auto *UDE = new (DirsAlloc.Allocate()) DirectoryEntry();
+    NamedDirEnt.second = *UDE;
+    VirtualDirectoryEntries.push_back(UDE);
+  } else {
+    // There is the real directory
+    DirectoryEntry *&UDE = getRealDirEntry(Status);
+    NamedDirEnt.second = *UDE;
+  }
 
   // Recursively add the other ancestors.
   addAncestorsAsVirtualDirs(DirName);
@@ -162,17 +189,8 @@ FileManager::getDirectoryRef(StringRef DirName, bool CacheFailure) {
     return llvm::errorCodeToError(statError);
   }
 
-  // It exists.  See if we have already opened a directory with the
-  // same inode (this occurs on Unix-like systems when one dir is
-  // symlinked to another, for example) or the same path (on
-  // Windows).
-  DirectoryEntry *&UDE = UniqueRealDirs[Status.getUniqueID()];
-
-  if (!UDE) {
-    // We don't have this directory yet, add it.  We use the string
-    // key from the SeenDirEntries map as the string.
-    UDE = new (DirsAlloc.Allocate()) DirectoryEntry();
-  }
+  // It exists.
+  DirectoryEntry *&UDE = getRealDirEntry(Status);
   NamedDirEnt.second = *UDE;
 
   return DirectoryEntryRef(NamedDirEnt);
diff --git a/clang/test/Modules/implicit-module-remap.cpp b/clang/test/Modules/implicit-module-remap.cpp
new file mode 100644
index 0000000000000..47927b9694016
--- /dev/null
+++ b/clang/test/Modules/implicit-module-remap.cpp
@@ -0,0 +1,21 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -fmodules -fmodule-map-file=module.modulemap -fmodules-cache-path=%t -remap-file "test.cpp;%t/test.cpp"  %t/test.cpp
+
+//--- a.h
+#define FOO
+
+//--- module.modulemap
+module a {
+  header "a.h"
+}
+
+//--- test.cpp
+#include "a.h"
+
+#ifndef FOO
+#error foo
+#endif
+

[ivanmurashko]

Hi @sam-mccall,

The patch touches some fairly old code that hasn't been modified for a long time. You are one of the few people who worked on it recently, around two years ago. I would appreciate any comments and a review of the suggested patch.

[dexonsmith]

@benlangmuir and @jansvoboda11 might also be good reviewers for this.

[ivanmurashko]

Friendly ping
@sam-mccall , @jansvoboda11 , could you look at the diff. It fixes a critical issue for anyone who wants to use Clang modules with file remapping, for instance at clangd.

[benlangmuir]

LGTM