[VFS] RedirectingFileSystem only replace path if not already mapped

If the `ExternalFS` has already remapped to an external path then
`RedirectingFileSystem` should not change it to the originally provided
path. This fixes the original path always being used if multiple VFS
overlays were provided and the path wasn't found in the highest (ie.
first in the chain).

For now this is accomplished through the use of a new
`ExposesExternalVFSPath` field on `vfs::Status`. This flag is true when
the `Status` has an external path that's different from its virtual
path, ie. the contained path is the external path. See the plan in
`FileManager::getFileRef` for where this is going - eventually we won't
need `IsVFSMapped` any more and all returned paths should be virtual.

Resolves rdar://90578880 and llvm-project#53306.

Reviewed By: dexonsmith

Differential Revision: https://reviews.llvm.org/D123398

clang/lib/Basic/FileManager.cpp

@@ -287,11 +287,48 @@ FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
     // name to users (in diagnostics) and to tools that don't have access to
     // the VFS (in debug info and dependency '.d' files).
     //
-    // FIXME: This is pretty complicated. It's also inconsistent with how
+    // FIXME: This is pretty complex and has some very complicated interactions
-    // "real" filesystems behave and confuses parts of clang expect to see the
+    // with the rest of clang. It's also inconsistent with how "real"
-    // name-as-accessed on the \a FileEntryRef. Maybe the returned \a
+    // filesystems behave and confuses parts of clang expect to see the
-    // FileEntryRef::getName() could return the accessed name unmodified, but
+    // name-as-accessed on the \a FileEntryRef.
-    // make the external name available via a separate API.
+    //
+    // Further, it isn't *just* external names, but will also give back absolute
+    // paths when a relative path was requested - the check is comparing the
+    // name from the status, which is passed an absolute path resolved from the
+    // current working directory. `clang-apply-replacements` appears to depend
+    // on this behaviour, though it's adjusting the working directory, which is
+    // definitely not supported. Once that's fixed this hack should be able to
+    // be narrowed to only when there's an externally mapped name given back.
+    //
+    // A potential plan to remove this is as follows -
+    //   - Add API to determine if the name has been rewritten by the VFS.
+    //   - Fix `clang-apply-replacements` to pass down the absolute path rather
+    //     than changing the CWD. Narrow this hack down to just externally
+    //     mapped paths.
+    //   - Expose the requested filename. One possibility would be to allow
+    //     redirection-FileEntryRefs to be returned, rather than returning
+    //     the pointed-at-FileEntryRef, and customizing `getName()` to look
+    //     through the indirection.
+    //   - Update callers such as `HeaderSearch::findUsableModuleForHeader()`
+    //     to explicitly use the requested filename rather than just using
+    //     `getName()`.
+    //   - Add a `FileManager::getExternalPath` API for explicitly getting the
+    //     remapped external filename when there is one available. Adopt it in
+    //     callers like diagnostics/deps reporting instead of calling
+    //     `getName()` directly.
+    //   - Switch the meaning of `FileEntryRef::getName()` to get the requested
+    //     name, not the external name. Once that sticks, revert callers that
+    //     want the requested name back to calling `getName()`.
+    //   - Update the VFS to always return the requested name. This could also
+    //     return the external name, or just have an API to request it
+    //     lazily. The latter has the benefit of making accesses of the
+    //     external path easily tracked, but may also require extra work than
+    //     just returning up front.
+    //   - (Optionally) Add an API to VFS to get the external filename lazily
+    //     and update `FileManager::getExternalPath()` to use it instead. This
+    //     has the benefit of making such accesses easily tracked, though isn't
+    //     necessarily required (and could cause extra work than just adding to
+    //     eg. `vfs::Status` up front).
     auto &Redirection =
         *SeenFileEntries
              .insert({Status.getName(), FileEntryRef::MapValue(*UFE, DirInfo)})
@@ -312,12 +349,14 @@ FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
   FileEntryRef ReturnedRef(*NamedFileEnt);
   if (ReusingEntry) { // Already have an entry with this inode, return it.
 
-    // FIXME: this hack ensures that if we look up a file by a virtual path in
+    // FIXME: This hack ensures that `getDir()` will use the path that was
-    // the VFS that the getDir() will have the virtual path, even if we found
+    // used to lookup this file, even if we found a file by different path
-    // the file by a 'real' path first. This is required in order to find a
+    // first. This is required in order to find a module's structure when its
-    // module's structure when its headers/module map are mapped in the VFS.
+    // headers/module map are mapped in the VFS.
-    // We should remove this as soon as we can properly support a file having
+    //
-    // multiple names.
+    // See above for how this will eventually be removed. `IsVFSMapped`
+    // *cannot* be narrowed to `ExposesExternalVFSPath` as crash reproducers
+    // also depend on this logic and they have `use-external-paths: false`.
     if (&DirInfo.getDirEntry() != UFE->Dir && Status.IsVFSMapped)
       UFE->Dir = &DirInfo.getDirEntry();
 

clang/test/VFS/external-names-multi-overlay.c

@@ -0,0 +1,37 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s@NAME_DIR@%{/t:regex_replacement}/A@g" -e "s@EXTERNAL_DIR@%{/t:regex_replacement}/B@g" -e "s@REDIRECT_WITH@fallthrough@g" %t/vfs/base.yaml > %t/vfs/a-b-ft.yaml
+// RUN: sed -e "s@NAME_DIR@%{/t:regex_replacement}/A@g" -e "s@EXTERNAL_DIR@%{/t:regex_replacement}/B@g" -e "s@REDIRECT_WITH@fallback@g" %t/vfs/base.yaml > %t/vfs/a-b-fb.yaml
+
+// Check that the external name is given when multiple overlays are provided
+
+// RUN: %clang_cc1 -Werror -I %t/A -ivfsoverlay %t/vfs/a-b-ft.yaml -ivfsoverlay %t/vfs/empty.yaml -fsyntax-only -E -C %t/main.c 2>&1 | FileCheck --check-prefix=FROM_B %s
+// RUN: %clang_cc1 -Werror -I %t/A -ivfsoverlay %t/vfs/a-b-fb.yaml -ivfsoverlay %t/vfs/empty.yaml -fsyntax-only -E -C %t/main.c 2>&1 | FileCheck --check-prefix=FROM_B %s
+// RUN: %clang_cc1 -Werror -I %t/B -ivfsoverlay %t/vfs/a-b-ft.yaml -ivfsoverlay %t/vfs/empty.yaml -fsyntax-only -E -C %t/main.c 2>&1 | FileCheck --check-prefix=FROM_B %s
+// RUN: %clang_cc1 -Werror -I %t/B -ivfsoverlay %t/vfs/a-b-fb.yaml -ivfsoverlay %t/vfs/empty.yaml -fsyntax-only -E -C %t/main.c 2>&1 | FileCheck --check-prefix=FROM_B %s
+// FROM_B: # 1 "{{.*(/|\\\\)B(/|\\\\)}}Header.h"
+// FROM_B: // Header.h in B
+
+//--- main.c
+#include "Header.h"
+
+//--- B/Header.h
+// Header.h in B
+
+//--- vfs/base.yaml
+{
+  'version': 0,
+  'redirecting-with': 'REDIRECT_WITH',
+  'roots': [
+    { 'name': 'NAME_DIR',
+      'type': 'directory-remap',
+      'external-contents': 'EXTERNAL_DIR'
+    }
+  ]
+}
+
+//--- vfs/empty.yaml
+{
+  'version': 0,
+  'roots': []
+}

llvm/include/llvm/Support/VirtualFileSystem.h

@@ -58,6 +58,17 @@ public:
   // FIXME: remove when files support multiple names
   bool IsVFSMapped = false;
 
+  /// Whether this entity has an external path different from the virtual path,
+  /// and the external path is exposed by leaking it through the abstraction.
+  /// For example, a RedirectingFileSystem will set this for paths where
+  /// UseExternalName is true.
+  ///
+  /// FIXME: Currently the external path is exposed by replacing the virtual
+  /// path in this Status object. Instead, we should leave the path in the
+  /// Status intact (matching the requested virtual path) - see
+  /// FileManager::getFileRef for how how we plan to fix this.
+  bool ExposesExternalVFSPath = false;
+
   Status() = default;
   Status(const llvm::sys::fs::file_status &Status);
   Status(const Twine &Name, llvm::sys::fs::UniqueID UID,

llvm/lib/Support/VirtualFileSystem.cpp

@@ -2163,9 +2163,16 @@ RedirectingFileSystem::lookupPathImpl(
 static Status getRedirectedFileStatus(const Twine &OriginalPath,
                                       bool UseExternalNames,
                                       Status ExternalStatus) {
+  // The path has been mapped by some nested VFS and exposes an external path,
+  // don't override it with the original path.
+  if (ExternalStatus.ExposesExternalVFSPath)
+    return ExternalStatus;
+
   Status S = ExternalStatus;
   if (!UseExternalNames)
     S = Status::copyWithNewName(S, OriginalPath);
+  else
+    S.ExposesExternalVFSPath = true;
   S.IsVFSMapped = true;
   return S;
 }
@@ -2194,11 +2201,13 @@ ErrorOr<Status> RedirectingFileSystem::status(
 ErrorOr<Status>
 RedirectingFileSystem::getExternalStatus(const Twine &CanonicalPath,
                                          const Twine &OriginalPath) const {
-  if (auto Result = ExternalFS->status(CanonicalPath)) {
+  auto Result = ExternalFS->status(CanonicalPath);
-    return Result.get().copyWithNewName(Result.get(), OriginalPath);
+
-  } else {
+  // The path has been mapped by some nested VFS, don't override it with the
-    return Result.getError();
+  // original path.
-  }
+  if (!Result || Result->ExposesExternalVFSPath)
+    return Result;
+  return Status::copyWithNewName(Result.get(), OriginalPath);
 }
 
 ErrorOr<Status> RedirectingFileSystem::status(const Twine &OriginalPath) {
@@ -2268,7 +2277,9 @@ public:
 
 ErrorOr<std::unique_ptr<File>>
 File::getWithPath(ErrorOr<std::unique_ptr<File>> Result, const Twine &P) {
-  if (!Result)
+  // See \c getRedirectedFileStatus - don't update path if it's exposing an
+  // external path.
+  if (!Result || (*Result)->status()->ExposesExternalVFSPath)
     return Result;
 
   ErrorOr<std::unique_ptr<File>> F = std::move(*Result);

llvm/unittests/Support/VirtualFileSystemTest.cpp

@@ -1443,11 +1443,13 @@ TEST_F(VFSFromYAMLTest, MappedFiles) {
   ASSERT_FALSE(S.getError());
   EXPECT_EQ("//root/foo/bar/a", S->getName());
   EXPECT_TRUE(S->IsVFSMapped);
+  EXPECT_TRUE(S->ExposesExternalVFSPath);
 
   ErrorOr<vfs::Status> SLower = O->status("//root/foo/bar/a");
   EXPECT_EQ("//root/foo/bar/a", SLower->getName());
   EXPECT_TRUE(S->equivalent(*SLower));
   EXPECT_FALSE(SLower->IsVFSMapped);
+  EXPECT_FALSE(SLower->ExposesExternalVFSPath);
 
   // file after opening
   auto OpenedF = O->openFileForRead("//root/file1");
@@ -1456,6 +1458,7 @@ TEST_F(VFSFromYAMLTest, MappedFiles) {
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("//root/foo/bar/a", OpenedS->getName());
   EXPECT_TRUE(OpenedS->IsVFSMapped);
+  EXPECT_TRUE(OpenedS->ExposesExternalVFSPath);
 
   // directory
   S = O->status("//root/");
@@ -1468,26 +1471,30 @@ TEST_F(VFSFromYAMLTest, MappedFiles) {
   ASSERT_FALSE(S.getError());
   EXPECT_TRUE(S->isDirectory());
   EXPECT_TRUE(S->IsVFSMapped);
+  EXPECT_TRUE(S->ExposesExternalVFSPath);
   EXPECT_TRUE(S->equivalent(*O->status("//root/foo/bar")));
 
   SLower = O->status("//root/foo/bar");
   EXPECT_EQ("//root/foo/bar", SLower->getName());
   EXPECT_TRUE(S->equivalent(*SLower));
   EXPECT_FALSE(SLower->IsVFSMapped);
+  EXPECT_FALSE(SLower->ExposesExternalVFSPath);
 
   // file in remapped directory
   S = O->status("//root/mappeddir/a");
   ASSERT_FALSE(S.getError());
-  ASSERT_FALSE(S->isDirectory());
+  EXPECT_FALSE(S->isDirectory());
-  ASSERT_TRUE(S->IsVFSMapped);
+  EXPECT_TRUE(S->IsVFSMapped);
-  ASSERT_EQ("//root/foo/bar/a", S->getName());
+  EXPECT_TRUE(S->ExposesExternalVFSPath);
+  EXPECT_EQ("//root/foo/bar/a", S->getName());
 
   // file in remapped directory, with use-external-name=false
   S = O->status("//root/mappeddir2/a");
   ASSERT_FALSE(S.getError());
-  ASSERT_FALSE(S->isDirectory());
+  EXPECT_FALSE(S->isDirectory());
-  ASSERT_TRUE(S->IsVFSMapped);
+  EXPECT_TRUE(S->IsVFSMapped);
-  ASSERT_EQ("//root/mappeddir2/a", S->getName());
+  EXPECT_FALSE(S->ExposesExternalVFSPath);
+  EXPECT_EQ("//root/mappeddir2/a", S->getName());
 
   // file contents in remapped directory
   OpenedF = O->openFileForRead("//root/mappeddir/a");
@@ -1496,6 +1503,7 @@ TEST_F(VFSFromYAMLTest, MappedFiles) {
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("//root/foo/bar/a", OpenedS->getName());
   EXPECT_TRUE(OpenedS->IsVFSMapped);
+  EXPECT_TRUE(OpenedS->ExposesExternalVFSPath);
 
   // file contents in remapped directory, with use-external-name=false
   OpenedF = O->openFileForRead("//root/mappeddir2/a");
@@ -1504,6 +1512,7 @@ TEST_F(VFSFromYAMLTest, MappedFiles) {
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("//root/mappeddir2/a", OpenedS->getName());
   EXPECT_TRUE(OpenedS->IsVFSMapped);
+  EXPECT_FALSE(OpenedS->ExposesExternalVFSPath);
 
   // broken mapping
   EXPECT_EQ(O->status("//root/file2").getError(),
@@ -1536,11 +1545,13 @@ TEST_F(VFSFromYAMLTest, MappedRoot) {
   ASSERT_FALSE(S.getError());
   EXPECT_EQ("//root/foo/bar/a", S->getName());
   EXPECT_TRUE(S->IsVFSMapped);
+  EXPECT_TRUE(S->ExposesExternalVFSPath);
 
   ErrorOr<vfs::Status> SLower = O->status("//root/foo/bar/a");
   EXPECT_EQ("//root/foo/bar/a", SLower->getName());
   EXPECT_TRUE(S->equivalent(*SLower));
   EXPECT_FALSE(SLower->IsVFSMapped);
+  EXPECT_FALSE(SLower->ExposesExternalVFSPath);
 
   // file after opening
   auto OpenedF = O->openFileForRead("//mappedroot/a");
@@ -1549,6 +1560,7 @@ TEST_F(VFSFromYAMLTest, MappedRoot) {
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("//root/foo/bar/a", OpenedS->getName());
   EXPECT_TRUE(OpenedS->IsVFSMapped);
+  EXPECT_TRUE(OpenedS->ExposesExternalVFSPath);
 
   EXPECT_EQ(0, NumDiagnostics);
 }
@@ -1697,11 +1709,13 @@ TEST_F(VFSFromYAMLTest, ReturnsRequestedPathVFSMiss) {
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("a", OpenedS->getName());
   EXPECT_FALSE(OpenedS->IsVFSMapped);
+  EXPECT_FALSE(OpenedS->ExposesExternalVFSPath);
 
   auto DirectS = RemappedFS->status("a");
   ASSERT_FALSE(DirectS.getError());
   EXPECT_EQ("a", DirectS->getName());
   EXPECT_FALSE(DirectS->IsVFSMapped);
+  EXPECT_FALSE(DirectS->ExposesExternalVFSPath);
 
   EXPECT_EQ(0, NumDiagnostics);
 }
@@ -1737,11 +1751,13 @@ TEST_F(VFSFromYAMLTest, ReturnsExternalPathVFSHit) {
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("realname", OpenedS->getName());
   EXPECT_TRUE(OpenedS->IsVFSMapped);
+  EXPECT_TRUE(OpenedS->ExposesExternalVFSPath);
 
   auto DirectS = FS->status("vfsname");
   ASSERT_FALSE(DirectS.getError());
   EXPECT_EQ("realname", DirectS->getName());
   EXPECT_TRUE(DirectS->IsVFSMapped);
+  EXPECT_TRUE(DirectS->ExposesExternalVFSPath);
 
   EXPECT_EQ(0, NumDiagnostics);
 }
@@ -1777,11 +1793,13 @@ TEST_F(VFSFromYAMLTest, ReturnsInternalPathVFSHit) {
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("vfsname", OpenedS->getName());
   EXPECT_TRUE(OpenedS->IsVFSMapped);
+  EXPECT_FALSE(OpenedS->ExposesExternalVFSPath);
 
   auto DirectS = FS->status("vfsname");
   ASSERT_FALSE(DirectS.getError());
   EXPECT_EQ("vfsname", DirectS->getName());
   EXPECT_TRUE(DirectS->IsVFSMapped);
+  EXPECT_FALSE(DirectS->ExposesExternalVFSPath);
 
   EXPECT_EQ(0, NumDiagnostics);
 }