[VFS] RedirectingFileSystem only replace path if not already mapped

If the `ExternalFS` has already remapped a path then the
`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).

This also renames `IsVFSMapped` to `ExposesExternalVFSPath` and only
sets it if `UseExternalName` is true. This flag then represents that the
`Status` has an external path that's different from its virtual path.
Right now the contained path is still the external path, but further PRs
will change this to *always* be the virtual path. Clients that need the
external can then request it specifically.

Note that even though `ExposesExternalVFSPath` isn't set for all
VFS-mapped paths, `IsVFSMapped` was only being used by a hack in
`FileManager` that was specific to module searching. In that case
`UseExternalNames` is always `true` and so that hack still applies.

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

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

Author: bnbarham

clang/lib/Basic/FileManager.cpp

@@ -270,12 +270,15 @@ FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
   // This occurs when one dir is symlinked to another, for example.
   FileEntry &UFE = UniqueRealFiles[Status.getUniqueID()];
 
+  // FIXME: This should just check `!Status.ExposesExternalVFSPath`, but the
+  // else branch also ends up fixing up relative paths to be the actually
+  // looked up absolute path. This isn't necessarily desired, but does seem to
+  // be relied on in some clients.
   if (Status.getName() == Filename) {
     // The name matches. Set the FileEntry.
     NamedFileEnt->second = FileEntryRef::MapValue(UFE, DirInfo);
   } else {
-    // Name mismatch. We need a redirect. First grab the actual entry we want
-    // to return.
+    // We need a redirect. First grab the actual entry we want to return.
     //
     // This redirection logic intentionally leaks the external name of a
     // redirected file that uses 'use-external-name' in \a
@@ -285,9 +288,11 @@ FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
     //
     // FIXME: This is pretty complicated. It's also inconsistent with how
     // "real" filesystems behave and confuses parts of clang expect to see the
-    // name-as-accessed on the \a FileEntryRef. Maybe the returned \a
-    // FileEntryRef::getName() could return the accessed name unmodified, but
-    // make the external name available via a separate API.
+    // name-as-accessed on the \a FileEntryRef. To remove this we should
+    // implement the FIXME on `ExposesExternalVFSPath`, ie. update the
+    // `FileEntryRef::getName()` path to *always* be the virtual path and have
+    // clients request the external path only when required through a separate
+    // API.
     auto &Redirection =
         *SeenFileEntries
              .insert({Status.getName(), FileEntryRef::MapValue(UFE, DirInfo)})
@@ -308,13 +313,16 @@ FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
   FileEntryRef ReturnedRef(*NamedFileEnt);
   if (UFE.isValid()) { // 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
-    // the VFS that the getDir() will have the virtual path, even if we found
-    // the file by a 'real' path first. This is required in order to find a
-    // module's structure when its headers/module map are mapped in the VFS.
-    // We should remove this as soon as we can properly support a file having
-    // multiple names.
-    if (&DirInfo.getDirEntry() != UFE.Dir && Status.IsVFSMapped)
+    // FIXME: This hack ensures that `getDir()` will use the path that was
+    // used to lookup this file, even if we found a file by different path
+    // first. This is required in order to find a module's structure when its
+    // headers/module map are mapped in the VFS.
+    //
+    // This should be removed once `HeaderSearch` is updated to use `*Ref`s
+    // *and* the redirection hack above is removed. The removal of the latter
+    // is required since otherwise the ref will have the exposed external VFS
+    // path still.
+    if (&DirInfo.getDirEntry() != UFE.Dir && Status.ExposesExternalVFSPath)
       UFE.Dir = &DirInfo.getDirEntry();
 
     // Always update LastRef to the last name by which a file was accessed.

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

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

llvm/include/llvm/Support/VirtualFileSystem.h

@@ -55,8 +55,19 @@ class Status {
   llvm::sys::fs::perms Perms;
 
 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), and just use this
+  /// flag as hint that a new API, FileSystem::getExternalVFSPath(), will not
+  /// return `None`. Clients can then request the external path only where
+  /// needed (e.g. for diagnostics, or to report discovered dependencies to
+  /// external tools).
+  bool ExposesExternalVFSPath = false;
 
   Status() = default;
   Status(const llvm::sys::fs::file_status &Status);

llvm/lib/Support/VirtualFileSystem.cpp

@@ -2163,10 +2163,16 @@ RedirectingFileSystem::lookupPathImpl(
 static Status getRedirectedFileStatus(const Twine &OriginalPath,
                                       bool UseExternalNames,
                                       Status ExternalStatus) {
+  // The path has been mapped by some nested VFS, don't override it with the
+  // original path.
+  if (ExternalStatus.ExposesExternalVFSPath)
+    return ExternalStatus;
+
   Status S = ExternalStatus;
   if (!UseExternalNames)
     S = Status::copyWithNewName(S, OriginalPath);
-  S.IsVFSMapped = true;
+  else
+    S.ExposesExternalVFSPath = true;
   return S;
 }
 
@@ -2268,7 +2274,9 @@ class FileWithFixedStatus : public File {
 
 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 already been
+  // mapped.
+  if (!Result || (*Result)->status()->ExposesExternalVFSPath)
     return Result;
 
   ErrorOr<std::unique_ptr<File>> F = std::move(*Result);

llvm/unittests/Support/VirtualFileSystemTest.cpp

@@ -1442,20 +1442,20 @@ TEST_F(VFSFromYAMLTest, MappedFiles) {
   ErrorOr<vfs::Status> S = O->status("//root/file1");
   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");
   ASSERT_FALSE(OpenedF.getError());
   auto OpenedS = (*OpenedF)->status();
   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/");
@@ -1467,43 +1467,43 @@ TEST_F(VFSFromYAMLTest, MappedFiles) {
   S = O->status("//root/mappeddir");
   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());
-  ASSERT_TRUE(S->IsVFSMapped);
-  ASSERT_EQ("//root/foo/bar/a", S->getName());
+  EXPECT_FALSE(S->isDirectory());
+  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());
-  ASSERT_TRUE(S->IsVFSMapped);
-  ASSERT_EQ("//root/mappeddir2/a", S->getName());
+  EXPECT_FALSE(S->isDirectory());
+  EXPECT_FALSE(S->ExposesExternalVFSPath);
+  EXPECT_EQ("//root/mappeddir2/a", S->getName());
 
   // file contents in remapped directory
   OpenedF = O->openFileForRead("//root/mappeddir/a");
   ASSERT_FALSE(OpenedF.getError());
   OpenedS = (*OpenedF)->status();
   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");
   ASSERT_FALSE(OpenedF.getError());
   OpenedS = (*OpenedF)->status();
   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(),
@@ -1535,20 +1535,20 @@ TEST_F(VFSFromYAMLTest, MappedRoot) {
   ErrorOr<vfs::Status> S = O->status("//mappedroot/a");
   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");
   ASSERT_FALSE(OpenedF.getError());
   auto OpenedS = (*OpenedF)->status();
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("//root/foo/bar/a", OpenedS->getName());
-  EXPECT_TRUE(OpenedS->IsVFSMapped);
+  EXPECT_TRUE(OpenedS->ExposesExternalVFSPath);
 
   EXPECT_EQ(0, NumDiagnostics);
 }
@@ -1696,12 +1696,12 @@ TEST_F(VFSFromYAMLTest, ReturnsRequestedPathVFSMiss) {
   auto OpenedS = (*OpenedF)->status();
   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);
 }
@@ -1736,12 +1736,12 @@ TEST_F(VFSFromYAMLTest, ReturnsExternalPathVFSHit) {
   auto OpenedS = (*OpenedF)->status();
   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);
 }
@@ -1776,12 +1776,12 @@ TEST_F(VFSFromYAMLTest, ReturnsInternalPathVFSHit) {
   auto OpenedS = (*OpenedF)->status();
   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);
 }