[DNM][VFS] Do not overwrite the path when nesting RedirectingFileSystems

[bnbarham]

Summary

86e2af8043c7728710a711b623f27425801a36c3 aimed to preserve the original
relative path when falling back to the external filesystem. But when
there's nested RedirectingFileSystems this results in the outer-most
VFS overwriting the path of a lower VFS.

For example, take a directory remapping overlay of A -> B and another
of B -> C where both have use-external-names set. If A/foo is
requested this will map A/foo to B/foo to C/foo but when this
result returns to the A -> B VFS, it will then remap the Status and
File to B/foo instead.

This is only a partial fix - it fixes openFileForRead, but status
and getRealPath would need equivalent changes. I'm putting up this
partial PR to gather opinions on whether this seems reasonable first.

[bnbarham]

Also have bug up at https://github.com/llvm/llvm-project/issues/53306.

[dexonsmith]

@keith, any thoughts on this? IIRC, you'd found a few other bugs while you were in there which you'd left for later; not sure if this is related to them or not.

clang/test/VFS/directory.c

2

This makes it hard to read what has changed. Probably better to commit an NFC patch (updating the testcase with no behaviour change) and then rebase this patch on top.

llvm/unittests/Support/VirtualFileSystemTest.cpp

2714–2716

Remapping in two directions seems like an extra layer of complexity; is that the case where this came up, or was it something more straightforward?

[bnbarham]

clang/test/VFS/directory.c

2

Yep, to be clear this is very much a WIP PR - I mostly put it up to get some feedback. I expect there to be a fair bit to clean up.

The only thing actually *changed* here is the addition of checking the path that was written out as well. The rest was to help me understand what was actually going on originally, I'll probably just revert and just add the path if we do end up wanting this (unless people prefer split-file).

llvm/unittests/Support/VirtualFileSystemTest.cpp

2714–2716

It shows up in A -> B -> C (as in the non-unit test) so this one could test that instead. I did this when originally reproducing the issue and only figured out after that it was just due to nesting in general.

The problem is nested RedirectingFileSystems, where the outermost now overwrites the paths written by the nested FS's.

[dexonsmith]

clang/test/VFS/directory.c

2

Just that it's hard to read the WIP PR with the noise of the unrelated change; if the testcase is easier to read with split-file seems like it'd be nice to land the improvement, but up to you.

[bnbarham]

Modified directory.c to make the test difference clearer.

[bnbarham]

clang/test/VFS/directory.c

2

Updated to remove the (large amount of) noise.

[keith]

In D117730#3262622, @dexonsmith wrote:

@keith, any thoughts on this? IIRC, you'd found a few other bugs while you were in there which you'd left for later; not sure if this is related to them or not.

This issue wasn't one of the ones on my radar, but in general this change seems safe to me

[bnbarham]

Rather than trying to fix nested RedirectingFileSystems, I've instead put up a bunch of patches to simplify RedirectingFileSystem. This does mean we can't "chain" remappings any more, but that seems like a good change to me. See:
https://reviews.llvm.org/D121421
https://reviews.llvm.org/D121423
https://reviews.llvm.org/D121424
https://reviews.llvm.org/D121425
https://reviews.llvm.org/D121426

clang/test/VFS/directory.c

 // RUN: rm -rf %t
-// RUN: mkdir -p %t/Underlying
-// RUN: mkdir -p %t/Overlay
-// RUN: mkdir -p %t/Middle
-// RUN: echo '// B.h in Underlying' > %t/Underlying/B.h
-// RUN: echo '#ifdef NESTED' >> %t/Underlying/B.h
-// RUN: echo '#include "C.h"' >> %t/Underlying/B.h
-// RUN: echo '#endif' >> %t/Underlying/B.h
-// RUN: echo '// C.h in Underlying' > %t/Underlying/C.h
-// RUN: echo '// C.h in Middle' > %t/Middle/C.h
-// RUN: echo '// C.h in Overlay' > %t/Overlay/C.h
-
-// 1) Underlying -> Overlay (C.h found, B.h falling back to Underlying)
-// RUN: sed -e "s@INPUT_DIR@%{/t:regex_replacement}/Overlay@g" -e "s@OUT_DIR@%{/t:regex_replacement}/Underlying@g" %S/Inputs/vfsoverlay-directory.yaml > %t/vfs.yaml
-// RUN: %clang_cc1 -Werror -I %t/Underlying -ivfsoverlay %t/vfs.yaml -fsyntax-only -E -C %s 2>&1 | FileCheck --check-prefix=DIRECT %s
-// RUN: %clang_cc1 -Werror -I %t/Underlying -ivfsoverlay %t/vfs.yaml -fsyntax-only -DNESTED -E -C %s 2>&1 | FileCheck --check-prefix=DIRECT %s
-// RUN: sed -e "s@INPUT_DIR@Overlay@g" -e "s@OUT_DIR@%{/t:regex_replacement}/Underlying@g" %S/Inputs/vfsoverlay-directory-relative.yaml > %t/vfs-relative.yaml
-// RUN: %clang_cc1 -Werror -I %t/Underlying -ivfsoverlay %t/vfs-relative.yaml -fsyntax-only -E -C %s 2>&1 | FileCheck --check-prefix=DIRECT %s
-
-// DIRECT: {{^}}// B.h in Underlying
-// DIRECT: {{^}}// C.h in Overlay
-
-// 2) Underlying -> Middle -> Overlay (C.h found, B.h falling back to Underlying)
-// RUN: sed -e "s@INPUT_DIR@%{/t:regex_replacement}/Overlay@g" -e "s@OUT_DIR@%{/t:regex_replacement}/Middle@g" %S/Inputs/vfsoverlay-directory.yaml > %t/vfs.yaml
-// RUN: sed -e "s@INPUT_DIR@%{/t:regex_replacement}/Middle@g" -e "s@OUT_DIR@%{/t:regex_replacement}/Underlying@g" %S/Inputs/vfsoverlay-directory.yaml > %t/vfs2.yaml
-// RUN: %clang_cc1 -Werror -I %t/Underlying -ivfsoverlay %t/vfs.yaml -ivfsoverlay %t/vfs2.yaml -fsyntax-only -E -C %s 2>&1 | FileCheck --check-prefix=DIRECT %s
-// RUN: %clang_cc1 -Werror -I %t/Underlying -ivfsoverlay %t/vfs.yaml -ivfsoverlay %t/vfs2.yaml -DNESTED -fsyntax-only -E -C %s 2>&1 | FileCheck --check-prefix=DIRECT %s
-
-// Same as direct above
-
-// 3) Underlying -> Middle -> Overlay (C.h falling back to Middle, B.h falling back to Underlying)
-// RUN: rm -f %t/Overlay/C.h
-// RUN: %clang_cc1 -Werror -I %t/Underlying -ivfsoverlay %t/vfs.yaml -ivfsoverlay %t/vfs2.yaml -fsyntax-only -E -C %s 2>&1 | FileCheck --check-prefix=FALLBACK %s
+// RUN: split-file %s %t
 
-// FALLBACK: {{^}}// B.h in Underlying
-// FALLBACK: {{^}}// C.h in Middle
+// Underlying -> Overlay
+// RUN: sed -e "s@EXTERNAL_DIR@%{/t:regex_replacement}/Overlay@g" -e "s@NAME_DIR@%{/t:regex_replacement}/Underlying@g" %t/vfs/base.yaml > %t/vfs/underlying-to-overlay.yaml
+// RUN: sed -e "s@EXTERNAL_DIR@../Overlay@g" -e "s@NAME_DIR@%{/t:regex_replacement}/Underlying@g" %t/vfs/base-relative.yaml > %t/vfs/underlying-to-relative-overlay.yaml
 
-// 3) Underlying -> Middle -> Overlay (C.h falling back to Underlying, B.h falling back to Underlying)
+// Middle -> Overlay
+// RUN: sed -e "s@EXTERNAL_DIR@%{/t:regex_replacement}/Overlay@g" -e "s@NAME_DIR@%{/t:regex_replacement}/Middle@g" %t/vfs/base.yaml > %t/vfs/middle-to-overlay.yaml
+
+// Underlying -> Middle
+// RUN: sed -e "s@EXTERNAL_DIR@%{/t:regex_replacement}/Middle@g" -e "s@NAME_DIR@%{/t:regex_replacement}/Underlying@g" %t/vfs/base.yaml > %t/vfs/underlying-to-middle.yaml
+
+// 1) Underlying -> Overlay (C.h found in Overlay, B.h falls back to Underlying)
+// RUN: %clang_cc1 -Werror -I %t/Underlying -ivfsoverlay %t/vfs/underlying-to-overlay.yaml -fsyntax-only -E -C %t/main.c 2>&1 | FileCheck --check-prefix=U_TO_O %s
+// RUN: %clang_cc1 -Werror -I %t/Underlying -ivfsoverlay %t/vfs/underlying-to-overlay.yaml -fsyntax-only -DNESTED -E -C %t/main.c 2>&1 | FileCheck --check-prefix=U_TO_O %s
+// RUN: %clang_cc1 -Werror -I %t/Underlying -ivfsoverlay %t/vfs/underlying-to-relative-overlay.yaml -fsyntax-only -E -C %t/main.c 2>&1 | FileCheck --check-prefix=U_TO_O %s
+
+// U_TO_O: # 1 "{{.*(/|\\\\)Underlying(/|\\\\)}}B.h"
+// U_TO_O-NEXT: // B.h in Underlying
+// U_TO_O: # 1 "{{.*(/|\\\\)Overlay(/|\\\\)}}C.h"
+// U_TO_O-NEXT: // C.h in Overlay
+
+// 2) Underlying -> Middle -> Overlay (C.h found in Overlay, B.h falls back to Underlying)
+// RUN: %clang_cc1 -Werror -I %t/Underlying -ivfsoverlay %t/vfs/middle-to-overlay.yaml -ivfsoverlay %t/vfs/underlying-to-middle.yaml -fsyntax-only -E -C %t/main.c 2>&1 | FileCheck --check-prefix=U_TO_O %s
+// RUN: %clang_cc1 -Werror -I %t/Underlying -ivfsoverlay %t/vfs/middle-to-overlay.yaml -ivfsoverlay %t/vfs/underlying-to-middle.yaml -DNESTED -fsyntax-only -E -C %t/main.c 2>&1 | FileCheck --check-prefix=U_TO_O %s
+
+// Ends up same as U_TO_O above since Middle is mapped to Overlay
+
+// 3) Underlying -> Middle -> Overlay but with the Overlay/C.h removed (C.h falls back to Middle, B.h falls back to Underlying)
+// RUN: rm %t/Overlay/C.h
+// RUN: %clang_cc1 -Werror -I %t/Underlying -ivfsoverlay %t/vfs/middle-to-overlay.yaml -ivfsoverlay %t/vfs/underlying-to-middle.yaml -fsyntax-only -E -C %t/main.c 2>&1 | FileCheck --check-prefix=U_TO_M %s
+
+// U_TO_M: # 1 "{{.*(/|\\\\)Underlying(/|\\\\)}}B.h"
+// U_TO_M-NEXT: // B.h in Underlying
+// U_TO_M: # 1 "{{.*(/|\\\\)Middle(/|\\\\)}}C.h"
+// U_TO_M-NEXT: // C.h in Middle
+
+// 4) Underlying -> Middle -> Overlay but with Middle/C.h also removed (C.h and B.h fall back to Underlying)
 // RUN: rm -f %t/Middle/C.h
-// RUN: %clang_cc1 -Werror -I %t/Underlying -ivfsoverlay %t/vfs.yaml -ivfsoverlay %t/vfs2.yaml -fsyntax-only -E -C %s 2>&1 | FileCheck --check-prefix=FALLBACK2 %s
+// RUN: %clang_cc1 -Werror -I %t/Underlying -ivfsoverlay %t/vfs/middle-to-overlay.yaml -ivfsoverlay %t/vfs/underlying-to-middle.yaml -fsyntax-only -E -C %t/main.c 2>&1 | FileCheck --check-prefix=U_ONLY %s
 
-// FALLBACK2: {{^}}// B.h in Underlying
-// FALLBACK2: {{^}}// C.h in Underlying
+// U_ONLY: # 1 "{{.*(/|\\\\)Underlying(/|\\\\)}}B.h"
+// U_ONLY-NEXT: // B.h in Underlying
+// U_ONLY: # 1 "{{.*(/|\\\\)Underlying(/|\\\\)}}C.h"
+// U_ONLY-NEXT: // C.h in Underlying
 
+//--- main.c
 #include "B.h"
 #ifndef NESTED
 #include "C.h"
 #endif
+
+//--- Underlying/B.h
+// B.h in Underlying
+#ifdef NESTED
+#include "C.h"
+#endif
+
+//--- Underlying/C.h
+// C.h in Underlying
+
+//--- Middle/C.h
+// C.h in Middle
+
+//--- Overlay/C.h
+// C.h in Overlay
+
+//--- vfs/base.yaml
+{
+  'version': 0,
+  'fallthrough': true,
+  'roots': [
+    { 'name': 'NAME_DIR',
+      'type': 'directory-remap',
+      'external-contents': 'EXTERNAL_DIR'
+    }
+  ]
+}
+
+//--- vfs/base-relative.yaml
+{
+  'version': 0,
+  'fallthrough': true,
+  'overlay-relative': true,
+  'roots': [
+    { 'name': 'NAME_DIR',
+      'type': 'directory-remap',
+      'external-contents': 'EXTERNAL_DIR'
+    }
+  ]
+}

llvm/include/llvm/Support/VirtualFileSystem.h

 class FileSystem : public llvm::ThreadSafeRefCountedBase<FileSystem> {
 public:
   virtual ~FileSystem();
 
   /// Get the status of the entry at \p Path, if one exists.
   virtual llvm::ErrorOr<Status> status(const Twine &Path) = 0;
 
   /// Get a \p File object for the file at \p Path, if one exists.
+  llvm::ErrorOr<std::unique_ptr<File>> openFileForRead(const Twine &Path) {
+    return openFileForReadImpl(Path, Path);
+  }
+
+  /// Get a \p File object for the file at \p LookupPath. In most cases the
+  /// filesystem should ensure that the returned file has a path of
+  /// \p OriginalPath, but this is not a guarantee. In some cases the path will
+  /// have to differ, eg. if it has been remapped and is intentionally being
+  /// leaked.
   virtual llvm::ErrorOr<std::unique_ptr<File>>
-  openFileForRead(const Twine &Path) = 0;
+  openFileForReadImpl(const Twine &LookupPath, const Twine &OriginalPath) = 0;
 
   /// This is a convenience method that opens a file, gets its content and then
   /// closes the file.
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
   getBufferForFile(const Twine &Name, int64_t FileSize = -1,
                    bool RequiresNullTerminator = true, bool IsVolatile = false);
 
   /// Get a directory_iterator for \p Dir.

 
 public:
   OverlayFileSystem(IntrusiveRefCntPtr<FileSystem> Base);
 
   /// Pushes a file system on top of the stack.
   void pushOverlay(IntrusiveRefCntPtr<FileSystem> FS);
 
   llvm::ErrorOr<Status> status(const Twine &Path) override;
   llvm::ErrorOr<std::unique_ptr<File>>
-  openFileForRead(const Twine &Path) override;
+  ErrorOr<std::unique_ptr<File>> openFileForReadImpl(
+      const Twine &LookupPath, const Twine &OriginalPath) override;
   directory_iterator dir_begin(const Twine &Dir, std::error_code &EC) override;
   llvm::ErrorOr<std::string> getCurrentWorkingDirectory() const override;
   std::error_code setCurrentWorkingDirectory(const Twine &Path) override;
   std::error_code isLocal(const Twine &Path, bool &Result) override;
   std::error_code getRealPath(const Twine &Path,
                               SmallVectorImpl<char> &Output) const override;
 
   using iterator = FileSystemList::reverse_iterator;

 class ProxyFileSystem : public FileSystem {
 public:
   explicit ProxyFileSystem(IntrusiveRefCntPtr<FileSystem> FS)
       : FS(std::move(FS)) {}
 
   llvm::ErrorOr<Status> status(const Twine &Path) override {
     return FS->status(Path);
   }
-  llvm::ErrorOr<std::unique_ptr<File>>
-  openFileForRead(const Twine &Path) override {
-    return FS->openFileForRead(Path);
+  llvm::ErrorOr<std::unique_ptr<File>> openFileForReadImpl(
+      const Twine &LookupPath, const Twine &OriginalPath) override {
+    return FS->openFileForReadImpl(LookupPath, OriginalPath);
   }
   directory_iterator dir_begin(const Twine &Dir, std::error_code &EC) override {
     return FS->dir_begin(Dir, EC);
   }
   llvm::ErrorOr<std::string> getCurrentWorkingDirectory() const override {
     return FS->getCurrentWorkingDirectory();
   }
   std::error_code setCurrentWorkingDirectory(const Twine &Path) override {
bool addFileNoOwn(const Twine &Path, time_t ModificationTime,
                     Optional<llvm::sys::fs::perms> Perms = None);
 
   std::string toString() const;
 
   /// Return true if this file system normalizes . and .. in paths.
   bool useNormalizedPaths() const { return UseNormalizedPaths; }
 
   llvm::ErrorOr<Status> status(const Twine &Path) override;
-  llvm::ErrorOr<std::unique_ptr<File>>
-  openFileForRead(const Twine &Path) override;
+  llvm::ErrorOr<std::unique_ptr<File>> openFileForReadImpl(
+      const Twine &LookupPath, const Twine &OriginalPath) override;
   directory_iterator dir_begin(const Twine &Dir, std::error_code &EC) override;
 
   llvm::ErrorOr<std::string> getCurrentWorkingDirectory() const override {
     return WorkingDirectory;
   }
   /// Canonicalizes \p Path by combining with the current working
   /// directory and normalizing the path (e.g. remove dots). If the current
   /// working directory is not set, this returns errc::operation_not_permitted.
create(std::unique_ptr<MemoryBuffer> Buffer,
          void *DiagContext, IntrusiveRefCntPtr<FileSystem> ExternalFS);
 
   /// Redirect each of the remapped files from first to second.
   static std::unique_ptr<RedirectingFileSystem>
   create(ArrayRef<std::pair<std::string, std::string>> RemappedFiles,
          bool UseExternalNames, FileSystem &ExternalFS);
 
   ErrorOr<Status> status(const Twine &Path) override;
-  ErrorOr<std::unique_ptr<File>> openFileForRead(const Twine &Path) override;
+  llvm::ErrorOr<std::unique_ptr<File>> openFileForReadImpl(
+      const Twine &LookupPath, const Twine &OriginalPath) override;
 
   std::error_code getRealPath(const Twine &Path,
                               SmallVectorImpl<char> &Output) const override;
 
   llvm::ErrorOr<std::string> getCurrentWorkingDirectory() const override;
 
   std::error_code setCurrentWorkingDirectory(const Twine &Path) override;
 

llvm/lib/Support/FileCollector.cpp

public:
 
   llvm::ErrorOr<llvm::vfs::Status> status(const Twine &Path) override {
     auto Result = FS->status(Path);
     if (Result && Result->exists())
       Collector->addFile(Path);
     return Result;
   }
 
-  llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>
-  openFileForRead(const Twine &Path) override {
-    auto Result = FS->openFileForRead(Path);
+  llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>> openFileForReadImpl(
+      const Twine &LookupPath, const Twine &OriginalPath) override {
+    auto Result = FS->openFileForReadImpl(LookupPath, OriginalPath);
     if (Result && *Result)
-      Collector->addFile(Path);
+      Collector->addFile(LookupPath);
     return Result;
   }
 
   llvm::vfs::directory_iterator dir_begin(const llvm::Twine &Dir,
                                           std::error_code &EC) override {
     return Collector->addDirectoryImpl(Dir, FS, EC);
   }
 

llvm/lib/Support/VirtualFileSystem.cpp

if (!LinkCWDToProcess) {
       if (llvm::sys::fs::real_path(PWD, RealPWD))
         WD = {PWD, PWD};
       else
         WD = {PWD, RealPWD};
     }
   }
 
   ErrorOr<Status> status(const Twine &Path) override;
-  ErrorOr<std::unique_ptr<File>> openFileForRead(const Twine &Path) override;
+  ErrorOr<std::unique_ptr<File>> openFileForReadImpl(
+      const Twine &LookupPath, const Twine &OriginalPath) override;
   directory_iterator dir_begin(const Twine &Dir, std::error_code &EC) override;
 
   llvm::ErrorOr<std::string> getCurrentWorkingDirectory() const override;
   std::error_code setCurrentWorkingDirectory(const Twine &Path) override;
   std::error_code isLocal(const Twine &Path, bool &Result) override;
   std::error_code getRealPath(const Twine &Path,
                               SmallVectorImpl<char> &Output) const override;
 
ErrorOr<Status> RealFileSystem::status(const Twine &Path) {
   SmallString<256> Storage;
   sys::fs::file_status RealStatus;
   if (std::error_code EC =
           sys::fs::status(adjustPath(Path, Storage), RealStatus))
     return EC;
   return Status::copyWithNewName(RealStatus, Path);
 }
 
+
 ErrorOr<std::unique_ptr<File>>
-RealFileSystem::openFileForRead(const Twine &Name) {
+RealFileSystem::openFileForReadImpl(const Twine &LookupPath,
+                                    const Twine &OriginalPath) {
   SmallString<256> RealName, Storage;
   Expected<file_t> FDOrErr = sys::fs::openNativeFileForRead(
-      adjustPath(Name, Storage), sys::fs::OF_None, &RealName);
+      adjustPath(LookupPath, Storage), sys::fs::OF_None, &RealName);
   if (!FDOrErr)
     return errorToErrorCode(FDOrErr.takeError());
   return std::unique_ptr<File>(
-      new RealFile(*FDOrErr, Name.str(), RealName.str()));
+      new RealFile(*FDOrErr, OriginalPath.str(), RealName.str()));
 }
 
 llvm::ErrorOr<std::string> RealFileSystem::getCurrentWorkingDirectory() const {
   if (WD)
     return std::string(WD->Specified.str());
 
   SmallString<128> Dir;
   if (std::error_code EC = llvm::sys::fs::current_path(Dir))
for (iterator I = overlays_begin(), E = overlays_end(); I != E; ++I) {
     ErrorOr<Status> Status = (*I)->status(Path);
     if (Status || Status.getError() != llvm::errc::no_such_file_or_directory)
       return Status;
   }
   return make_error_code(llvm::errc::no_such_file_or_directory);
 }
 
 ErrorOr<std::unique_ptr<File>>
-OverlayFileSystem::openFileForRead(const llvm::Twine &Path) {
+OverlayFileSystem::openFileForReadImpl(const Twine &LookupPath,
+                                       const Twine &OriginalPath) {
   // FIXME: handle symlinks that cross file systems
   for (iterator I = overlays_begin(), E = overlays_end(); I != E; ++I) {
-    auto Result = (*I)->openFileForRead(Path);
+    auto Result = (*I)->openFileForReadImpl(LookupPath, OriginalPath);
     if (Result || Result.getError() != llvm::errc::no_such_file_or_directory)
       return Result;
   }
   return make_error_code(llvm::errc::no_such_file_or_directory);
 }
 
 llvm::ErrorOr<std::string>
 OverlayFileSystem::getCurrentWorkingDirectory() const {

 llvm::ErrorOr<Status> InMemoryFileSystem::status(const Twine &Path) {
   auto Node = lookupInMemoryNode(*this, Root.get(), Path);
   if (Node)
     return detail::getNodeStatus(*Node, Path);
   return Node.getError();
 }
 
 llvm::ErrorOr<std::unique_ptr<File>>
-InMemoryFileSystem::openFileForRead(const Twine &Path) {
-  auto Node = lookupInMemoryNode(*this, Root.get(), Path);
+InMemoryFileSystem::openFileForReadImpl(const Twine &LookupPath,
+                                        const Twine &OriginalPath) {
+  auto Node = lookupInMemoryNode(*this, Root.get(), LookupPath);
   if (!Node)
     return Node.getError();
 
   // When we have a file provide a heap-allocated wrapper for the memory buffer
   // to match the ownership semantics for File.
   if (auto *F = dyn_cast<detail::InMemoryFile>(*Node))
     return std::unique_ptr<File>(
-        new detail::InMemoryFileAdaptor(*F, Path.str()));
+        new detail::InMemoryFileAdaptor(*F, OriginalPath.str()));
 
   // FIXME: errc::not_a_file?
   return make_error_code(llvm::errc::invalid_argument);
 }
 
 namespace {
 
 /// Adaptor from InMemoryDir::iterator to directory_iterator.
File::getWithPath(ErrorOr<std::unique_ptr<File>> Result, const Twine &P) {
   ErrorOr<std::unique_ptr<File>> F = std::move(*Result);
   auto Name = F->get()->getName();
   if (Name && Name.get() != P.str())
     F->get()->setPath(P);
   return F;
 }
 
 ErrorOr<std::unique_ptr<File>>
-RedirectingFileSystem::openFileForRead(const Twine &OriginalPath) {
+RedirectingFileSystem::openFileForReadImpl(const Twine &LookupPath,
+                                           const Twine &OriginalPath) {
   SmallString<256> CanonicalPath;
-  OriginalPath.toVector(CanonicalPath);
+  LookupPath.toVector(CanonicalPath);
 
   if (std::error_code EC = makeCanonical(CanonicalPath))
     return EC;
 
   ErrorOr<RedirectingFileSystem::LookupResult> Result =
       lookupPath(CanonicalPath);
   if (!Result) {
     if (shouldFallBackToExternalFS(Result.getError()))
-      return File::getWithPath(ExternalFS->openFileForRead(CanonicalPath),
-                               OriginalPath);
+      return ExternalFS->openFileForReadImpl(CanonicalPath, OriginalPath);
 
     return Result.getError();
   }
 
   if (!Result->getExternalRedirect()) // FIXME: errc::not_a_file?
     return make_error_code(llvm::errc::invalid_argument);
 
   StringRef ExtRedirect = *Result->getExternalRedirect();
   SmallString<256> CanonicalRemappedPath(ExtRedirect.str());
   if (std::error_code EC = makeCanonical(CanonicalRemappedPath))
     return EC;
 
   auto *RE = cast<RedirectingFileSystem::RemapEntry>(Result->E);
 
-  auto ExternalFile = File::getWithPath(
-      ExternalFS->openFileForRead(CanonicalRemappedPath), ExtRedirect);
+  auto ExternalFile = ExternalFS->openFileForReadImpl(
+      CanonicalRemappedPath,
+      RE->useExternalName(UseExternalNames) ? ExtRedirect : OriginalPath);
   if (!ExternalFile) {
     if (shouldFallBackToExternalFS(ExternalFile.getError(), Result->E))
-      return File::getWithPath(ExternalFS->openFileForRead(CanonicalPath),
-                               OriginalPath);
+      return ExternalFS->openFileForReadImpl(CanonicalPath, OriginalPath);
     return ExternalFile;
   }
 
   auto ExternalStatus = (*ExternalFile)->status();
   if (!ExternalStatus)
     return ExternalStatus.getError();
 
-  // FIXME: Update the status with the name and VFSMapped.
-  Status S = getRedirectedFileStatus(
-      OriginalPath, RE->useExternalName(UseExternalNames), *ExternalStatus);
+  Status S = *ExternalStatus;
+  S.IsVFSMapped = true;
   return std::unique_ptr<File>(
       std::make_unique<FileWithFixedStatus>(std::move(*ExternalFile), S));
 }
 
 std::error_code
 RedirectingFileSystem::getRealPath(const Twine &Path_,
                                    SmallVectorImpl<char> &Output) const {
   SmallString<256> Path;

llvm/unittests/Support/VirtualFileSystemTest.cpp

public:
   DummyFileSystem() : FSID(getNextFSID()), FileID(0) {}
 
   ErrorOr<vfs::Status> status(const Twine &Path) override {
     auto I = findEntry(Path);
     if (I == FilesAndDirs.end())
       return make_error_code(llvm::errc::no_such_file_or_directory);
     return I->second;
   }
-  ErrorOr<std::unique_ptr<vfs::File>>
-  openFileForRead(const Twine &Path) override {
-    auto S = status(Path);
-    if (S)
-      return std::unique_ptr<vfs::File>(new DummyFile{*S});
+  ErrorOr<std::unique_ptr<vfs::File>> openFileForReadImpl(
+      const Twine &LookupPath, const Twine &OriginalPath) override {
+    auto S = status(LookupPath);
+    if (S) {
+      auto FixedS = vfs::Status::copyWithNewName(*S, OriginalPath);
+      return std::unique_ptr<vfs::File>(new DummyFile{FixedS});
+    }
     return S.getError();
   }
   llvm::ErrorOr<std::string> getCurrentWorkingDirectory() const override {
     return WorkingDirectory;
   }
   std::error_code setCurrentWorkingDirectory(const Twine &Path) override {
     WorkingDirectory = Path.str();
     return std::error_code();
TEST_F(VFSFromYAMLTest, YAMLVFSWriterTestHandleDirs) {
   IntrusiveRefCntPtr<vfs::FileSystem> FS = getFromYAMLRawString(Buffer, Lower);
   ASSERT_TRUE(FS.get() != nullptr);
 
   EXPECT_FALSE(FS->exists(_a.path("a")));
   EXPECT_FALSE(FS->exists(_b.path("b")));
   EXPECT_FALSE(FS->exists(_c.path("c")));
 }
 
+TEST_F(VFSFromYAMLTest, MultipleRemappedDirectories) {
+  // Test the interaction of two overlays where one maps back to the other,
+  // ie. `a` -> `b` and then `b` -> `a`. This should always use `a` if it
+  // exists and fallback to `b` otherwise.
+
+  IntrusiveRefCntPtr<DummyFileSystem> Both(new DummyFileSystem());
+  Both->addDirectory("//root/a");
+  Both->addRegularFile("//root/a/f");
+  Both->addDirectory("//root/b");
+  Both->addRegularFile("//root/b/f");
+
+  IntrusiveRefCntPtr<DummyFileSystem> AOnly(new DummyFileSystem());
+  AOnly->addDirectory("//root/a");
+  AOnly->addRegularFile("//root/a/f");
+
+  IntrusiveRefCntPtr<DummyFileSystem> BOnly(new DummyFileSystem());
+  BOnly->addDirectory("//root/b");
+  BOnly->addRegularFile("//root/b/f");
+
+  auto AToBStr = "{ 'roots': [\n"
+                 "  {\n"
+                 "    'type': 'directory-remap',\n"
+                 "    'name': '//root/a',\n"
+                 "    'external-contents': '//root/b'\n"
+                 "  }"
+                 "]}";
+  auto BToAStr = "{ 'roots': [\n"
+                 "  {\n"
+                 "    'type': 'directory-remap',\n"
+                 "    'name': '//root/b',\n"
+                 "    'external-contents': '//root/a'\n"
+                 "  }"
+                 "]}";
+
+  auto ExpectPath = [&](vfs::FileSystem &FS, StringRef Expected) {
+    auto AF = FS.openFileForRead("//root/a/f");
+    ASSERT_FALSE(AF.getError());
+    auto AFName = (*AF)->getName();
+    ASSERT_FALSE(AFName.getError());
+    EXPECT_EQ(Expected.str(), AFName.get());
+
+    auto AS = FS.status("//root/a/f");
+    ASSERT_FALSE(AS.getError());
+    EXPECT_EQ(Expected.str(), AS->getName());
+
+    auto BF = FS.openFileForRead("//root/b/f");
+    ASSERT_FALSE(BF.getError());
+    auto BName = (*BF)->getName();
+    ASSERT_FALSE(BName.getError());
+    EXPECT_EQ(Expected.str(), BName.get());
+
+    auto BS = FS.status("//root/b/f");
+    ASSERT_FALSE(BS.getError());
+    EXPECT_EQ(Expected.str(), BS->getName());
+  };
+
+  IntrusiveRefCntPtr<vfs::FileSystem> BothFS =
+      getFromYAMLString(AToBStr, getFromYAMLString(BToAStr, Both));
+  ASSERT_TRUE(BothFS.get() != nullptr);
+  ExpectPath(*BothFS, "//root/a/f");
+
+  IntrusiveRefCntPtr<vfs::FileSystem> OnlyAFS =
+      getFromYAMLString(AToBStr, getFromYAMLString(BToAStr, AOnly));
+  ASSERT_TRUE(OnlyAFS.get() != nullptr);
+  ExpectPath(*OnlyAFS, "//root/a/f");
+
+  IntrusiveRefCntPtr<vfs::FileSystem> OnlyBFS =
+      getFromYAMLString(AToBStr, getFromYAMLString(BToAStr, BOnly));
+  ASSERT_TRUE(OnlyBFS.get() != nullptr);
+  ExpectPath(*OnlyBFS, "//root/b/f");
+
+  EXPECT_EQ(0, NumDiagnostics);
+}
+
 TEST(VFSFromRemappedFilesTest, Basic) {
   IntrusiveRefCntPtr<vfs::InMemoryFileSystem> BaseFS =
       new vfs::InMemoryFileSystem;
   BaseFS->addFile("//root/b", 0, MemoryBuffer::getMemBuffer("contents of b"));
   BaseFS->addFile("//root/c", 0, MemoryBuffer::getMemBuffer("contents of c"));
 
   std::vector<std::pair<std::string, std::string>> RemappedFiles = {
       {"//root/a/a", "//root/b"},