[VFS] Simplify RedirectingFileSystem to perform mapping only

[bnbarham]

Summary

RedirectingFileSystem has grown significantly over time. When it was
added it simply performed a path remapping and then used that path in
the filesystem it was passed (ExternalFS). The functionality provided
by -ivfsoverlay was handled by OverlayFileSystem, which would run
operations on the contained filesystems in reverse order.

Since then, "fallthrough" and "redirecting-with" options were added to
the overlay YAML format. As part of these changes, OverlayFileSystem
stopped being used in the handling of -ivfsoverlay and this was
instead handled by nesting RedirectingFileSystem together. The
right-most overlay would use the next-to-the-left
RedirectingFileSystem as its external filesystem, etc.

This allowed:

1. Overlay paths to be dependent on previous -ivfsoverlay, ie. a second -ivfsoverlay could specify a virtual path for its overlay if that path is in the first overlay.
2. Paths from one overlay to affect those in an earlier, ie. if the right-most overlay specified a mapping from A -> B and the left-most a mapping from B -> C, A could now map to C.
3. The real filesystem to be skipped, ie. all specified paths have to be contained in an overlay (which would still map to the real filesystem).

(1) and (3) are now implemented by getVFSFromYAMLs using the
OverlayFileSystem (D121425), which allows removing the
redirecting-with handling from RedirectingFileSystem entirely. With
that removal RedirectingFileSystem is now back to only performing a
path remapping and using that path in the ExternalFS. (2) will no
longer be possible, but this also makes overlays *much* simpler to
reason about.

Depends on D121425

[bnbarham]

Noticed there's a ::assign on SmallVectorImpl, use that instead of clear + append.

[bnbarham]

Ah, I should only be depending on the one rather than multiple. I'll fix that up soon (and for the following as well). That does make more sense, not sure why I thought it would need all of them!

[bnbarham]

Update to single review dependency

[dexonsmith]

This allowed:

• Overlay paths to be dependent on previous -ivfsoverlay, ie. a second -ivfsoverlay could specify a virtual path for its overlay if that path is in the first overlay.
• Paths from one overlay to affect those in an earlier, ie. if the right-most overlay specified a mapping from A -> B and the left-most a mapping from B -> C, A could now map to C.
• The real filesystem to be skipped, ie. all specified paths have to be contained in an overlay (which would still map to the real filesystem).

FileSystem also had a get/set added for the current working directory
to allow for filesystem-specific CWD rather than a process-wide CWD,
further complicating the RedirectingFileSystem implementation.

It's not clear from the description what the plan is for the RedirectingFileSystem's CWD. It sounds like you're suggesting dropping it. If so, what happens if the redirecting filesystem remaps things to a new directory /new/dir that doesn't exist in the underlying filesystem, and the client calls cd /new/dir?

This change reverts RedirectingFileSystem to a single responsibility -
mapping paths and passing that path to the underlying filesystem.

I'm not clear about two things:

• What is "the underlying filesystem"?
• What does RedirectingFileSystem return for something not explicitly listed in the .yaml?

A
later change implements (1) and (3) through the OverlayFileSystem. (2)
is no longer possible, which fixes llvm-project#53306 as a side-effect.

Sorry if I missed a discussions somewhere, but I'm not sure I fully understand the plan for landing these changes. It sounds as if this is going to land, regressing (1) and (3), and then we'll add back (1) and (3) later; is that the plan? That doesn't quite seem right; is there another way to sequence the changes to avoid temporary regressions in tree? (If not, then they probably need to land together.)

[bnbarham]

In D121424#3376299, @dexonsmith wrote:

This allowed:

• Overlay paths to be dependent on previous -ivfsoverlay, ie. a second -ivfsoverlay could specify a virtual path for its overlay if that path is in the first overlay.
• Paths from one overlay to affect those in an earlier, ie. if the right-most overlay specified a mapping from A -> B and the left-most a mapping from B -> C, A could now map to C.
• The real filesystem to be skipped, ie. all specified paths have to be contained in an overlay (which would still map to the real filesystem).

FileSystem also had a get/set added for the current working directory
to allow for filesystem-specific CWD rather than a process-wide CWD,
further complicating the RedirectingFileSystem implementation.

It's not clear from the description what the plan is for the RedirectingFileSystem's CWD. It sounds like you're suggesting dropping it. If so, what happens if the redirecting filesystem remaps things to a new directory /new/dir that doesn't exist in the underlying filesystem, and the client calls cd /new/dir?

Sorry, I was only trying to give some history here. Adding CWD in caused various complications in RedirectingFileSystem due to the added nesting, ie. what path do we pass to the next FS? Hence all the mess with trying to map back to the original path depending on whether use external names is set, but still passing down the canonical name, etc. I'm certainly not suggesting we remove it, it's much easier to reason about with the simplified RedirectinFileSystem.

This change reverts RedirectingFileSystem to a single responsibility -
mapping paths and passing that path to the underlying filesystem.

I'm not clear about two things:

• What is "the underlying filesystem"?
• What does RedirectingFileSystem return for something not explicitly listed in the .yaml?

1. I've been using "external" and "underlying" interchangeably. But I mean ExternalFS, ie. the FS that's passed into RedirectingFileSystem.
2. It returns no_such_file_or_directory, as it once did. If we want an overlay, we should use OverlayFileSystem. Most of this commit message is trying to explain how we got to where we are today in order to provide some context.

A
later change implements (1) and (3) through the OverlayFileSystem. (2)
is no longer possible, which fixes llvm-project#53306 as a side-effect.

Sorry if I missed a discussions somewhere, but I'm not sure I fully understand the plan for landing these changes. It sounds as if this is going to land, regressing (1) and (3), and then we'll add back (1) and (3) later; is that the plan? That doesn't quite seem right; is there another way to sequence the changes to avoid temporary regressions in tree? (If not, then they probably need to land together.)

The plan is to land them together. I just split them to make it easier to review (ie. smaller chunks). Perhaps this has only just added to the confusion.

To be clear:

• D121421 and D121423 and both be landed separately
• D121424, D121425, and D121426 all need to be landed together. I don't see a way to split them up to avoid that, but if you do please let me know.

[dexonsmith]

In D121424#3376321, @bnbarham wrote:

Sorry, I was only trying to give some history here. Adding CWD in caused various complications in RedirectingFileSystem due to the added nesting, ie. what path do we pass to the next FS? Hence all the mess with trying to map back to the original path depending on whether use external names is set, but still passing down the canonical name, etc. I'm certainly not suggesting we remove it, it's much easier to reason about with the simplified RedirectinFileSystem.

Great; I feel like we need the CWD there; the history was useful, I just took the wrong conclusion overall :).

This change reverts RedirectingFileSystem to a single responsibility -
mapping paths and passing that path to the underlying filesystem.

I'm not clear about two things:

• What is "the underlying filesystem"?
• What does RedirectingFileSystem return for something not explicitly listed in the .yaml?

1. I've been using "external" and "underlying" interchangeably. But I mean ExternalFS, ie. the FS that's passed into RedirectingFileSystem.

Either one would have felt ambiguous to me, because I was wondering if you were referring to the sys::fs filesystem and they both kind of point there! Thanks for clarifying.

2. It returns no_such_file_or_directory, as it once did. If we want an overlay, we should use OverlayFileSystem. Most of this commit message is trying to explain how we got to where we are today in order to provide some context.

Okay, makes sense.

The plan is to land them together. I just split them to make it easier to review (ie. smaller chunks). Perhaps this has only just added to the confusion.

Got it!

To be clear:

• D121421 and D121423 and both be landed separately
• D121424, D121425, and D121426 all need to be landed together. I don't see a way to split them up to avoid that, but if you do please let me know.

Got it; I suggest editing each description to make that super clear in those situations ("these will be landed at the same time, split up only for review convenience").

In this case:

• I think there might be a way to re-sequence D121425 ahead of this commit.
  • The new API can be added and unit tested without deleting features from RedirectingFS.
  • There may be some tests that fail until this commit lands; you can either check for the temporary wrong behaviour and fix them in this commit, or shift those tests entirely to this commit if it's too confusing. But it'll help to see what tests suddenly start passing related to the new API.
• Maybe this commit could be made runtime-configurable (OverlayFS constructor, off-by-default, only used in the new API to start, then the old behaviour removed later), to land it ahead of D121426? Maybe that's too complicated; just pointing out the flexibility.
  • Else, I'd suggest merging D121426 with this one. The changes don't touch the same files and it's not a massive patch so I don't think it'll confuse the review much.

[bnbarham]

In D121424#3376381, @dexonsmith wrote:

In this case:

• I think there might be a way to re-sequence D121425 ahead of this commit.
  • The new API can be added and unit tested without deleting features from RedirectingFS.
  • There may be some tests that fail until this commit lands; you can either check for the temporary wrong behaviour and fix them in this commit, or shift those tests entirely to this commit if it's too confusing. But it'll help to see what tests suddenly start passing related to the new API.
• Maybe this commit could be made runtime-configurable (OverlayFS constructor, off-by-default, only used in the new API to start, then the old behaviour removed later), to land it ahead of D121426? Maybe that's too complicated; just pointing out the flexibility.
  • Else, I'd suggest merging D121426 with this one. The changes don't touch the same files and it's not a massive patch so I don't think it'll confuse the review much.

Landing D121425 before this one is definitely *possible*, but I'd really rather this change be atomic - neither really make sense on their own (IMO). The last option you listed said "merging D121426 with this one", but that one is basically just API updates and a few test fixes for the new behavior. Did you mean with D121425 or just all of them together?

[dexonsmith]

In D121424#3376387, @bnbarham wrote:

In D121424#3376381, @dexonsmith wrote:

In this case:

• I think there might be a way to re-sequence D121425 ahead of this commit.
  • The new API can be added and unit tested without deleting features from RedirectingFS.
  • There may be some tests that fail until this commit lands; you can either check for the temporary wrong behaviour and fix them in this commit, or shift those tests entirely to this commit if it's too confusing. But it'll help to see what tests suddenly start passing related to the new API.
• Maybe this commit could be made runtime-configurable (OverlayFS constructor, off-by-default, only used in the new API to start, then the old behaviour removed later), to land it ahead of D121426? Maybe that's too complicated; just pointing out the flexibility.
  • Else, I'd suggest merging D121426 with this one. The changes don't touch the same files and it's not a massive patch so I don't think it'll confuse the review much.

Landing D121425 before this one is definitely *possible*, but I'd really rather this change be atomic - neither really make sense on their own (IMO).

Can you explain more why you don't think it's a good idea in this case?

For complicated changes, it's not uncommon to add the new API (with tests for it), and then do adoption later.

Actually, I wonder if you could even just land these as three separate commits by doing D121424 last:

• D121425: add new API (causes any created RedirectingFS to be created with these options turned off)
• D121426: change clang to use it
• D121424: remove now-unneeded RedirectingFS code and add tests proving that A->B->C case no longer works

That'd be the usual incremental flow, I think. It also gives out-of-tree clients a good commit window to adopt the new API:

• Temporarily revert D121424 on out-of-tree branch to allow merging main.
• Update out-of-tree code to use the new API.
• Reapply D121424 to match main again.

This also has the major benefit of making post-commit review (e.g., reading git log -u) easier to manage.

Maybe it's not worth it here -- maybe it's a lot of work to make these separately-landable and maybe there aren't (known) external uses of RedirectingFS -- but naïvely, based on seeing three Phab reviews and having a better understanding now of what's in each, it seems like it might not be hard to keep them split?

(If there's some temporary regression to worry about, it's better to unify them, but otherwise separate commits would be preferred I think.)

The last option you listed said "merging D121426 with this one", but that one is basically just API updates and a few test fixes for the new behavior. Did you mean with D121425 or just all of them together?

I meant D121425, then D121424 + D121426, because I assumed those last two were intrinsically linked, but maybe they aren't?

[bnbarham]

In D121424#3376440, @dexonsmith wrote:

Can you explain more why you don't think it's a good idea in this case?

For complicated changes, it's not uncommon to add the new API (with tests for it), and then do adoption later.

Actually, I wonder if you could even just land these as three separate commits by doing D121424 last:

• D121425: add new API (causes any created RedirectingFS to be created with these options turned off)
• D121426: change clang to use it
• D121424: remove now-unneeded RedirectingFS code and add tests proving that A->B->C case no longer works

My thinking was that the state after D121425 + D121426 would be such that the redirecting-with options are implemented in both OverlayFS and RedirectingFS, which I was worried would lead to some confusion if we end up needing to revert/looking at history later/etc. I'd also need to split the parsing changes out of this patch, move into D121425, and then add a bit extra to it to still set redirecting-with on RedirectingFS.

Perhaps that isn't as bad as I was imagining though. I'm a little worried it's just Friday night and I'm not thinking this through clearly, so I'll give it the weekend and try it on Monday if I don't think of anything.

add tests proving that A->B->C case no longer works

Are you asking for a test that manually nests RedirectingFileSystem and checks that this case doesn't work? Or just the existing TEST(VFSFromYAMLsTest, NotTransitive) test in D121425? For RedirectingFileSystem all we really want to check is that ExternalFS isn't used as a fallback, which I believe is already tested in the various VFSFromYAMLTest cases. That actually brings me to something I was meaning to bring up...

One thing that I haven't gotten to in these patches is to fix up the VFS tests in general - I only started laying the groundwork for them. We currently have a whole bunch of VFSFromYAMLTest cases that should probably be split up into testing OverlayFS, RedirectingFS, and then the actual getVFSFromYAML(s) API itself. My guess is that ended up getting mixed in together because RedirectingFS became the implementation of -ivfsoverlay and thus getVFSFromYAML *was* RedirectingFileSystem.

I almost did that in this patch, but thought that it would make it more difficult to review (since other than the A -> B -> C chaining, there shouldn't be any functional changes from the -ivfsoverlay perspective). I would like to do that as a follow up though if that makes sense to you.

[dexonsmith]

In D121424#3376840, @bnbarham wrote:

My thinking was that the state after D121425 + D121426 would be such that the redirecting-with options are implemented in both OverlayFS and RedirectingFS, which I was worried would lead to some confusion if we end up needing to revert/looking at history later/etc. I'd also need to split the parsing changes out of this patch, move into D121425, and then add a bit extra to it to still set redirecting-with on RedirectingFS.

Perhaps that isn't as bad as I was imagining though. I'm a little worried it's just Friday night and I'm not thinking this through clearly, so I'll give it the weekend and try it on Monday if I don't think of anything.

Doesn't sound too bad to me, but let me know what you think. In the first commit adding the new API you can add FIXME/TODOs to the old APIs indicating that the functionality is going away to make it clear what the plan is.

One major benefit is that if there is another client (besides clang), and you need to temporarily revert the "delete the old API patch", it's much less revert-churn that reverting the whole shebang (also, it doesn't block forward progress in clang).

add tests proving that A->B->C case no longer works

Are you asking for a test that manually nests RedirectingFileSystem and checks that this case doesn't work? Or just the existing TEST(VFSFromYAMLsTest, NotTransitive) test in D121425? For RedirectingFileSystem all we really want to check is that ExternalFS isn't used as a fallback, which I believe is already tested in the various VFSFromYAMLTest cases. That actually brings me to something I was meaning to bring up...

One thing that I haven't gotten to in these patches is to fix up the VFS tests in general - I only started laying the groundwork for them. We currently have a whole bunch of VFSFromYAMLTest cases that should probably be split up into testing OverlayFS, RedirectingFS, and then the actual getVFSFromYAML(s) API itself. My guess is that ended up getting mixed in together because RedirectingFS became the implementation of -ivfsoverlay and thus getVFSFromYAML *was* RedirectingFileSystem.

I almost did that in this patch, but thought that it would make it more difficult to review (since other than the A -> B -> C chaining, there shouldn't be any functional changes from the -ivfsoverlay perspective). I would like to do that as a follow up though if that makes sense to you.

One historical reason is that RedirectingFileSystem wasn't previously exposed in a header. Would be great to get this cleaned up!

[bnbarham]

Re-order to be after D121425

[bnbarham]

Blocked on the dependents

llvm/include/llvm/Support/VirtualFileSystem.h

 /// FIXME: 'use-external-name' causes behaviour that's inconsistent with how
 /// "real" filesystems behave. Maybe there should be a separate channel for
 /// this information.
 class RedirectingFileSystem : public vfs::FileSystem {
 public:
   enum EntryKind { EK_Directory, EK_DirectoryRemap, EK_File };
   enum NameKind { NK_NotSet, NK_External, NK_Virtual };
 
-  /// The type of redirection to perform.
-  enum class RedirectKind {
-    /// Lookup the redirected path first (ie. the one specified in
-    /// 'external-contents') and if that fails "fallthrough" to a lookup of the
-    /// originally provided path.
-    Fallthrough,
-    /// Lookup the provided path first and if that fails, "fallback" to a
-    /// lookup of the redirected path.
-    Fallback,
-    /// Only lookup the redirected path, do not lookup the originally provided
-    /// path.
-    RedirectOnly
-  };
-
   /// A single file or directory in the VFS.
   class Entry {
     EntryKind Kind;
     std::string Name;
 
   public:
     Entry(EntryKind K, StringRef Name) : Kind(K), Name(Name) {}
     virtual ~Entry() = default;
DirectoryEntry(StringRef Name, std::vector<std::unique_ptr<Entry>> Contents,
                    Status S)
         : Entry(EK_Directory, Name), Contents(std::move(Contents)),
           S(std::move(S)) {}
 
     /// Constructs an empty directory entry.
     DirectoryEntry(StringRef Name, Status S)
         : Entry(EK_Directory, Name), S(std::move(S)) {}
 
-    Status getStatus() { return S; }
+    Status getStatus() const { return S; }
 
     void addContent(std::unique_ptr<Entry> Content) {
       Contents.push_back(std::move(Content));
     }
 
     Entry *getLastContent() const { return Contents.back().get(); }
 
     using iterator = decltype(Contents)::iterator;
private:
   friend class RedirectingFSDirIterImpl;
   friend class RedirectingFileSystemParser;
 
   /// Canonicalize path by removing ".", "..", "./", components. This is
   /// a VFS request, do not bother about symlinks in the path components
   /// but canonicalize in order to perform the correct entry search.
   std::error_code makeCanonical(SmallVectorImpl<char> &Path) const;
 
-  /// Get the File status, or error, from the underlying external file system.
-  /// This returns the status with the originally requested name, while looking
-  /// up the entry using the canonical path.
-  ErrorOr<Status> getExternalStatus(const Twine &CanonicalPath,
-                                    const Twine &OriginalPath) const;
-
   // In a RedirectingFileSystem, keys can be specified in Posix or Windows
   // style (or even a mixture of both), so this comparison helper allows
   // slashes (representing a root) to match backslashes (and vice versa).  Note
   // that, other than the root, path components should not contain slashes or
   // backslashes.
   bool pathComponentMatches(llvm::StringRef lhs, llvm::StringRef rhs) const {
     if ((CaseSensitive ? lhs.equals(rhs) : lhs.equals_insensitive(rhs)))
       return true;
     return (lhs == "/" && rhs == "\\") || (lhs == "\\" && rhs == "/");
   }
 
   /// The root(s) of the virtual file system.
   std::vector<std::unique_ptr<Entry>> Roots;
 
   /// The current working directory of the file system.
   std::string WorkingDirectory;
 
   /// The file system to use for external references.
   IntrusiveRefCntPtr<FileSystem> ExternalFS;
 
-  /// If IsRelativeOverlay is set, this represents the directory
-  /// path that should be prefixed to each 'external-contents' entry
-  /// when reading from YAML files.
-  std::string ExternalContentsPrefixDir;
-
   /// @name Configuration
   /// @{
 
   /// Whether to perform case-sensitive comparisons.
   ///
   /// Currently, case-insensitive matching only works correctly with ASCII.
   bool CaseSensitive = is_style_posix(sys::path::Style::native);
 
-  /// IsRelativeOverlay marks whether a ExternalContentsPrefixDir path must
-  /// be prefixed in every 'external-contents' when reading from YAML files.
-  bool IsRelativeOverlay = false;
-
   /// Whether to use to use the value of 'external-contents' for the
   /// names of files.  This global value is overridable on a per-file basis.
   bool UseExternalNames = true;
 
-  /// Determines the lookups to perform, as well as their order. See
-  /// \c RedirectKind for details.
-  RedirectKind Redirection = RedirectKind::Fallthrough;
   /// @}
 
   RedirectingFileSystem(IntrusiveRefCntPtr<FileSystem> ExternalFS);
 
   /// Looks up the path <tt>[Start, End)</tt> in \p From, possibly recursing
   /// into the contents of \p From if it is a directory. Returns a LookupResult
   /// giving the matched entry and, if that entry is a FileEntry or
   /// DirectoryRemapEntry, the path it redirects to in the external file system.
   ErrorOr<LookupResult> lookupPathImpl(llvm::sys::path::const_iterator Start,
                                        llvm::sys::path::const_iterator End,
                                        Entry *From) const;
 
-  /// Get the status for a path with the provided \c LookupResult.
-  ErrorOr<Status> status(const Twine &CanonicalPath, const Twine &OriginalPath,
-                         const LookupResult &Result);
-
 public:
   /// Looks up \p Path in \c Roots and returns a LookupResult giving the
   /// matched entry and, if the entry was a FileEntry or DirectoryRemapEntry,
   /// the path it redirects to in the external file system.
-  ErrorOr<LookupResult> lookupPath(StringRef Path) const;
+  ErrorOr<LookupResult> lookupPath(StringRef CanonicalPath) const;
 
   /// Parses \p Buffer, which is expected to be in YAML format and
   /// returns a virtual file system representing its contents.
   static std::unique_ptr<RedirectingFileSystem>
   create(std::unique_ptr<MemoryBuffer> Buffer,
          SourceMgr::DiagHandlerTy DiagHandler, StringRef YAMLFilePath,
          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;
 
   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;
 
   std::error_code isLocal(const Twine &Path, bool &Result) override;
 
   std::error_code makeAbsolute(SmallVectorImpl<char> &Path) const override;
 
   directory_iterator dir_begin(const Twine &Dir, std::error_code &EC) override;
 
   void setExternalContentsPrefixDir(StringRef PrefixDir);
 
   StringRef getExternalContentsPrefixDir() const;
 
-  /// Sets the redirection kind to \c Fallthrough if true or \c RedirectOnly
-  /// otherwise. Will removed in the future, use \c setRedirection instead.
-  void setFallthrough(bool Fallthrough);
-
-  void setRedirection(RedirectingFileSystem::RedirectKind Kind);
-
   std::vector<llvm::StringRef> getRoots() const;
 
   void dump(raw_ostream &OS) const override;
   void dumpEntry(raw_ostream &OS, Entry *E, int NumSpaces = 0) const;
 };
 
 /// Collect all pairs of <virtual path, real path> entries from the
 /// \p YAMLFilePath. This is used by the module dependency collector to forward

llvm/lib/Support/VirtualFileSystem.cpp

 
 } // namespace vfs
 } // namespace llvm
 
 //===-----------------------------------------------------------------------===/
 // RedirectingFileSystem implementation
 //===-----------------------------------------------------------------------===/
 
-namespace {
-
 static llvm::sys::path::Style getExistingStyle(llvm::StringRef Path) {
   // Detect the path style in use by checking the first separator.
   llvm::sys::path::Style style = llvm::sys::path::Style::native;
   const size_t n = Path.find_first_of("/\\");
   // Can't distinguish between posix and windows_slash here.
   if (n != static_cast<size_t>(-1))
     style = (Path[n] == '/') ? llvm::sys::path::Style::posix
                              : llvm::sys::path::Style::windows_backslash;
   return style;
 }
 
 /// Removes leading "./" as well as path components like ".." and ".".
 static llvm::SmallString<256> canonicalize(llvm::StringRef Path) {
   // First detect the path style in use by checking the first separator.
   llvm::sys::path::Style style = getExistingStyle(Path);
 
   // Now remove the dots.  Explicitly specifying the path style prevents the
   // direction of the slashes from changing.
   llvm::SmallString<256> result =
       llvm::sys::path::remove_leading_dotslash(Path, style);
   llvm::sys::path::remove_dots(result, /*remove_dot_dot=*/true, style);
   return result;
 }
 
-/// Whether the error and entry specify a file/directory that was not found.
-static bool isFileNotFound(std::error_code EC,
-                           RedirectingFileSystem::Entry *E = nullptr) {
-  if (E && !isa<RedirectingFileSystem::DirectoryRemapEntry>(E))
-    return false;
-  return EC == llvm::errc::no_such_file_or_directory;
-}
-
-} // anonymous namespace
-
-
 RedirectingFileSystem::RedirectingFileSystem(IntrusiveRefCntPtr<FileSystem> FS)
     : ExternalFS(std::move(FS)) {
   if (ExternalFS)
     if (auto ExternalWorkingDirectory =
             ExternalFS->getCurrentWorkingDirectory()) {
       WorkingDirectory = *ExternalWorkingDirectory;
     }
 }

 
 llvm::ErrorOr<std::string>
 RedirectingFileSystem::getCurrentWorkingDirectory() const {
   return WorkingDirectory;
 }
 
 std::error_code
 RedirectingFileSystem::setCurrentWorkingDirectory(const Twine &Path) {
-  // Don't change the working directory if the path doesn't exist.
-  if (!exists(Path))
-    return errc::no_such_file_or_directory;
-
   SmallString<128> AbsolutePath;
   Path.toVector(AbsolutePath);
   if (std::error_code EC = makeAbsolute(AbsolutePath))
     return EC;
+
+  // Don't change the working directory if the path doesn't exist.
+  if (!exists(AbsolutePath))
+    return errc::no_such_file_or_directory;
+
   WorkingDirectory = std::string(AbsolutePath.str());
   return {};
 }
 
 std::error_code RedirectingFileSystem::isLocal(const Twine &Path_,
                                                bool &Result) {
   SmallString<256> Path;
   Path_.toVector(Path);
 
   if (std::error_code EC = makeCanonical(Path))
-    return {};
+    return EC;
 
   return ExternalFS->isLocal(Path, Result);
 }
 
 std::error_code RedirectingFileSystem::makeAbsolute(SmallVectorImpl<char> &Path) const {
   // is_absolute(..., Style::windows_*) accepts paths with both slash types.
   if (llvm::sys::path::is_absolute(Path, llvm::sys::path::Style::posix) ||
       llvm::sys::path::is_absolute(Path,
std::error_code RedirectingFileSystem::makeAbsolute(SmallVectorImpl<char> &Path) const {
   Result.append(Path.data(), Path.size());
   Path.assign(Result.begin(), Result.end());
 
   return {};
 }
 
 directory_iterator RedirectingFileSystem::dir_begin(const Twine &Dir,
                                                     std::error_code &EC) {
-  SmallString<256> Path;
-  Dir.toVector(Path);
-
-  EC = makeCanonical(Path);
+  SmallString<256> CanonicalPath;
+  Dir.toVector(CanonicalPath);
+  EC = makeCanonical(CanonicalPath);
   if (EC)
     return {};
 
-  ErrorOr<RedirectingFileSystem::LookupResult> Result = lookupPath(Path);
+  ErrorOr<RedirectingFileSystem::LookupResult> Result =
+      lookupPath(CanonicalPath);
   if (!Result) {
-    if (Redirection != RedirectKind::RedirectOnly &&
-        isFileNotFound(Result.getError()))
-      return ExternalFS->dir_begin(Path, EC);
-
     EC = Result.getError();
     return {};
   }
 
-  // Use status to make sure the path exists and refers to a directory.
-  ErrorOr<Status> S = status(Path, Dir, *Result);
-  if (!S) {
-    if (Redirection != RedirectKind::RedirectOnly &&
-        isFileNotFound(S.getError(), Result->E))
-      return ExternalFS->dir_begin(Dir, EC);
+  // Create the appropriate directory iterator based on whether we found a
+  // DirectoryRemapEntry or DirectoryEntry.
+  Optional<StringRef> ExtRedirect = Result->getExternalRedirect();
+  if (!ExtRedirect) {
+    // DirectoryEntry - just make an iterator for it
+    auto DE = cast<DirectoryEntry>(Result->E);
+    return directory_iterator(std::make_shared<RedirectingFSDirIterImpl>(
+        CanonicalPath, DE->contents_begin(), DE->contents_end(), EC));
+  }
 
+  // Have a DirectoryRemapEntry - need to check that the external directory
+  // actually exists and then possibly remap names to the virtual path depending
+  // on 'use-external-names'.
+  ErrorOr<Status> S = ExternalFS->status(*ExtRedirect);
+  if (!S) {
     EC = S.getError();
     return {};
   }
-
   if (!S->isDirectory()) {
     EC = errc::not_a_directory;
     return {};
   }
 
-  // Create the appropriate directory iterator based on whether we found a
-  // DirectoryRemapEntry or DirectoryEntry.
-  directory_iterator RedirectIter;
-  std::error_code RedirectEC;
-  if (auto ExtRedirect = Result->getExternalRedirect()) {
-    auto RE = cast<RedirectingFileSystem::RemapEntry>(Result->E);
-    RedirectIter = ExternalFS->dir_begin(*ExtRedirect, RedirectEC);
-
-    if (!RE->useExternalName(UseExternalNames)) {
-      // Update the paths in the results to use the virtual directory's path.
-      RedirectIter =
-          directory_iterator(std::make_shared<RedirectingFSDirRemapIterImpl>(
-              std::string(Path), RedirectIter));
-    }
-  } else {
-    auto DE = cast<DirectoryEntry>(Result->E);
-    RedirectIter =
-        directory_iterator(std::make_shared<RedirectingFSDirIterImpl>(
-            Path, DE->contents_begin(), DE->contents_end(), RedirectEC));
-  }
-
-  if (RedirectEC) {
-    if (RedirectEC != errc::no_such_file_or_directory) {
-      EC = RedirectEC;
-      return {};
-    }
-    RedirectIter = {};
-  }
-
-  if (Redirection == RedirectKind::RedirectOnly) {
-    EC = RedirectEC;
-    return RedirectIter;
-  }
+  directory_iterator RedirectIter = ExternalFS->dir_begin(*ExtRedirect, EC);
+            std::string(CanonicalPath), RedirectIter));
 
-  std::error_code ExternalEC;
-  directory_iterator ExternalIter = ExternalFS->dir_begin(Path, ExternalEC);
-  if (ExternalEC) {
-    if (ExternalEC != errc::no_such_file_or_directory) {
-      EC = ExternalEC;
-      return {};
-    }
-    ExternalIter = {};
-  }
-
-  SmallVector<directory_iterator, 2> Iters;
-  switch (Redirection) {
-  case RedirectKind::Fallthrough:
-    Iters.push_back(ExternalIter);
-    Iters.push_back(RedirectIter);
-    break;
-  case RedirectKind::Fallback:
-    Iters.push_back(RedirectIter);
-    Iters.push_back(ExternalIter);
-    break;
-  default:
-    llvm_unreachable("unhandled RedirectKind");
-  }
-
-  directory_iterator Combined{
-      std::make_shared<CombiningDirIterImpl>(Iters, EC)};
-  if (EC)
-    return {};
-  return Combined;
-}
-
-void RedirectingFileSystem::setExternalContentsPrefixDir(StringRef PrefixDir) {
-  ExternalContentsPrefixDir = PrefixDir.str();
-}
-
-StringRef RedirectingFileSystem::getExternalContentsPrefixDir() const {
-  return ExternalContentsPrefixDir;
-}
-
-void RedirectingFileSystem::setFallthrough(bool Fallthrough) {
-  if (Fallthrough) {
-    Redirection = RedirectingFileSystem::RedirectKind::Fallthrough;
-  } else {
-    Redirection = RedirectingFileSystem::RedirectKind::RedirectOnly;
-  }
-}
-
-void RedirectingFileSystem::setRedirection(
-    RedirectingFileSystem::RedirectKind Kind) {
-  Redirection = Kind;
-}
-
 std::vector<StringRef> RedirectingFileSystem::getRoots() const {
   std::vector<StringRef> R;
   for (const auto &Root : Roots)
     R.push_back(Root->getName());
   return R;
 }
 
 void RedirectingFileSystem::dump(raw_ostream &OS) const {
case EK_DirectoryRemap: {
     break;
   }
   case EK_File:
     OS << "\n";
     break;
   }
 }
 
+namespace {
+
+struct YAMLParseResult {
+  /// The way in which this VFS should be added to an overlay. Note that the
+  /// order is right to left - given [A, B, C], any FS operations run on C
+  /// *first* and if failing, B, followed by A.
+  enum class RedirectKind {
+    /// Allow "falling through" to the next (ie. to the left) VFS.
+    Fallthrough,
+    /// Run any operations on the next VFS *first*, using this VFS as a
+    /// "fallback".
+    Fallback,
+    /// Do not perform any more operations on further VFS.
+    RedirectOnly
+  };
+
+  std::unique_ptr<RedirectingFileSystem> FS;
+  RedirectKind Redirection;
+};
+
+} // namespace
+
 /// A helper class to hold the common YAML parsing state.
 class llvm::vfs::RedirectingFileSystemParser {
   yaml::Stream &Stream;
 
   void error(yaml::Node *N, const Twine &Msg) { Stream.printError(N, Msg); }
 
   // false on error
   bool parseScalarString(yaml::Node *N, StringRef &Result,
if (Value.equals_insensitive("true") || Value.equals_insensitive("on") ||
       Result = false;
       return true;
     }
 
     error(N, "expected boolean value");
     return false;
   }
 
-  Optional<RedirectingFileSystem::RedirectKind>
-  parseRedirectKind(yaml::Node *N) {
+  Optional<YAMLParseResult::RedirectKind> parseRedirectKind(yaml::Node *N) {
     SmallString<12> Storage;
     StringRef Value;
     if (!parseScalarString(N, Value, Storage))
       return None;
 
     if (Value.equals_insensitive("fallthrough")) {
-      return RedirectingFileSystem::RedirectKind::Fallthrough;
+      return YAMLParseResult::RedirectKind::Fallthrough;
     } else if (Value.equals_insensitive("fallback")) {
-      return RedirectingFileSystem::RedirectKind::Fallback;
+      return YAMLParseResult::RedirectKind::Fallback;
     } else if (Value.equals_insensitive("redirect-only")) {
-      return RedirectingFileSystem::RedirectKind::RedirectOnly;
+      return YAMLParseResult::RedirectKind::RedirectOnly;
     }
     return None;
   }
 
   struct KeyStatus {
     bool Required;
     bool Seen = false;
 
for (const auto &I : Keys) {
         return false;
       }
     }
     return true;
   }
 
 public:
   static RedirectingFileSystem::Entry *
-  lookupOrCreateEntry(RedirectingFileSystem *FS, StringRef Name,
+  lookupOrCreateEntry(RedirectingFileSystem &FS, StringRef Name,
                       RedirectingFileSystem::Entry *ParentEntry = nullptr) {
     if (!ParentEntry) { // Look for a existent root
-      for (const auto &Root : FS->Roots) {
+      for (const auto &Root : FS.Roots) {
         if (Name.equals(Root->getName())) {
           ParentEntry = Root.get();
           return ParentEntry;
         }
       }
     } else { // Advance to the next component
       auto *DE = dyn_cast<RedirectingFileSystem::DirectoryEntry>(ParentEntry);
       for (std::unique_ptr<RedirectingFileSystem::Entry> &Content :
            llvm::make_range(DE->contents_begin(), DE->contents_end())) {
         auto *DirContent =
             dyn_cast<RedirectingFileSystem::DirectoryEntry>(Content.get());
         if (DirContent && Name.equals(Content->getName()))
           return DirContent;
       }
     }
 
     // ... or create a new one
     std::unique_ptr<RedirectingFileSystem::Entry> E =
         std::make_unique<RedirectingFileSystem::DirectoryEntry>(
             Name, Status("", getNextVirtualUniqueID(),
                          std::chrono::system_clock::now(), 0, 0, 0,
                          file_type::directory_file, sys::fs::all_all));
 
     if (!ParentEntry) { // Add a new root to the overlay
-      FS->Roots.push_back(std::move(E));
-      ParentEntry = FS->Roots.back().get();
+      FS.Roots.push_back(std::move(E));
+      ParentEntry = FS.Roots.back().get();
       return ParentEntry;
     }
 
     auto *DE = cast<RedirectingFileSystem::DirectoryEntry>(ParentEntry);
     DE->addContent(std::move(E));
     return DE->getLastContent();
   }
 
 private:
-  void uniqueOverlayTree(RedirectingFileSystem *FS,
+  void uniqueOverlayTree(RedirectingFileSystem &FS,
                          RedirectingFileSystem::Entry *SrcE,
                          RedirectingFileSystem::Entry *NewParentE = nullptr) {
     StringRef Name = SrcE->getName();
     switch (SrcE->getKind()) {
     case RedirectingFileSystem::EK_Directory: {
       auto *DE = cast<RedirectingFileSystem::DirectoryEntry>(SrcE);
       // Empty directories could be present in the YAML as a way to
       // describe a file for a current directory after some of its subdir
case RedirectingFileSystem::EK_File: {
       DE->addContent(std::make_unique<RedirectingFileSystem::FileEntry>(
           Name, FE->getExternalContentsPath(), FE->getUseName()));
       break;
     }
     }
   }
 
   std::unique_ptr<RedirectingFileSystem::Entry>
-  parseEntry(yaml::Node *N, RedirectingFileSystem *FS, bool IsRootEntry) {
+  parseEntry(yaml::Node *N, bool IsRelativeOverlay,
+             StringRef ExternalContentsPrefixDir, bool IsRootEntry) {
     auto *M = dyn_cast<yaml::MappingNode>(N);
     if (!M) {
       error(N, "expected mapping node for file or directory entry");
       return nullptr;
     }
 
     KeyStatusPair Fields[] = {
         KeyStatusPair("name", true),
for (auto &I : *M) {
         if (!Contents) {
           // FIXME: this is only for directories, what about files?
           error(I.getValue(), "expected array");
           return nullptr;
         }
 
         for (auto &I : *Contents) {
           if (std::unique_ptr<RedirectingFileSystem::Entry> E =
-                  parseEntry(&I, FS, /*IsRootEntry*/ false))
+                  parseEntry(&I, IsRelativeOverlay, ExternalContentsPrefixDir,
+                             /*IsRootEntry=*/false))
             EntryArrayContents.push_back(std::move(E));
           else
             return nullptr;
         }
       } else if (Key == "external-contents") {
         if (ContentsField != CF_NotSet) {
           error(I.getKey(),
                 "entry already has 'contents' or 'external-contents'");
           return nullptr;
         }
         ContentsField = CF_External;
         if (!parseScalarString(I.getValue(), Value, Buffer))
           return nullptr;
 
         SmallString<256> FullPath;
-        if (FS->IsRelativeOverlay) {
-          FullPath = FS->getExternalContentsPrefixDir();
+        if (IsRelativeOverlay) {
+          FullPath.append(ExternalContentsPrefixDir);
           assert(!FullPath.empty() &&
                  "External contents prefix directory must exist");
           llvm::sys::path::append(FullPath, Value);
         } else {
           FullPath = Value;
         }
 
         // Guarantee that old YAML files containing paths with ".." and "."
for (sys::path::reverse_iterator I = sys::path::rbegin(Parent, path_style),
                  0, 0, 0, file_type::directory_file, sys::fs::all_all));
     }
     return Result;
   }
 
 public:
   RedirectingFileSystemParser(yaml::Stream &S) : Stream(S) {}
 
-  // false on error
-  bool parse(yaml::Node *Root, RedirectingFileSystem *FS) {
+  static Optional<YAMLParseResult>
+  parse(MemoryBufferRef Buffer, SourceMgr &SM,
+        IntrusiveRefCntPtr<FileSystem> ExternalFS) {
+    yaml::Stream Stream(Buffer, SM);
+    yaml::document_iterator DI = Stream.begin();
+    yaml::Node *Root = DI->getRoot();
+    if (DI == Stream.end() || !Root) {
+      SM.PrintMessage(SMLoc(), SourceMgr::DK_Error, "expected root node");
+      return None;
+    }
+
+    SmallString<256> ExternalContentsPrefixDir;
+    if (!Buffer.getBufferIdentifier().empty()) {
+      // Use the YAML path from -ivfsoverlay to compute the dir to be prefixed
+      // to each 'external-contents' path.
+      //
+      // Example:
+      //    -ivfsoverlay dummy.cache/vfs/vfs.yaml
+      // yields:
+      //  FS->ExternalContentsPrefixDir => /<absolute_path_to>/dummy.cache/vfs
+      //
+      ExternalContentsPrefixDir =
+          sys::path::parent_path(Buffer.getBufferIdentifier());
+      std::error_code EC = ExternalFS->makeAbsolute(ExternalContentsPrefixDir);
+      assert(!EC && "Overlay dir final path must be absolute");
+      (void)EC;
+    }
+
+    RedirectingFileSystemParser P(Stream);
+    return P.parse(Root, ExternalContentsPrefixDir, std::move(ExternalFS));
+  }
+
+  Optional<YAMLParseResult> parse(yaml::Node *Root,
+                                  StringRef ExternalContentsPrefixDir,
+                                  IntrusiveRefCntPtr<FileSystem> ExternalFS) {
     auto *Top = dyn_cast<yaml::MappingNode>(Root);
     if (!Top) {
       error(Root, "expected mapping node");
-      return false;
+      return None;
     }
 
     KeyStatusPair Fields[] = {
         KeyStatusPair("version", true),
         KeyStatusPair("case-sensitive", false),
         KeyStatusPair("use-external-names", false),
         KeyStatusPair("overlay-relative", false),
         KeyStatusPair("fallthrough", false),
         KeyStatusPair("redirecting-with", false),
         KeyStatusPair("roots", true),
     };
 
     DenseMap<StringRef, KeyStatus> Keys(std::begin(Fields), std::end(Fields));
     std::vector<std::unique_ptr<RedirectingFileSystem::Entry>> RootEntries;
 
+    YAMLParseResult Result{std::unique_ptr<RedirectingFileSystem>(
+                               new RedirectingFileSystem(ExternalFS)),
+                           YAMLParseResult::RedirectKind::Fallthrough};
+    bool IsRelativeOverlay = false;
+
     // Parse configuration and 'roots'
     for (auto &I : *Top) {
       SmallString<10> KeyBuffer;
       StringRef Key;
       if (!parseScalarString(I.getKey(), Key, KeyBuffer))
-        return false;
+        return None;
 
       if (!checkDuplicateOrUnknownKey(I.getKey(), Key, Keys))
-        return false;
+        return None;
 
       if (Key == "roots") {
         auto *Roots = dyn_cast<yaml::SequenceNode>(I.getValue());
         if (!Roots) {
           error(I.getValue(), "expected array");
-          return false;
+          return None;
         }
 
         for (auto &I : *Roots) {
           if (std::unique_ptr<RedirectingFileSystem::Entry> E =
-                  parseEntry(&I, FS, /*IsRootEntry*/ true))
+                  parseEntry(&I, IsRelativeOverlay, ExternalContentsPrefixDir,
+                             /*IsRootEntry=*/true))
             RootEntries.push_back(std::move(E));
           else
-            return false;
+            return None;
         }
       } else if (Key == "version") {
         StringRef VersionString;
         SmallString<4> Storage;
         if (!parseScalarString(I.getValue(), VersionString, Storage))
-          return false;
+          return None;
         int Version;
         if (VersionString.getAsInteger<int>(10, Version)) {
           error(I.getValue(), "expected integer");
-          return false;
+          return None;
         }
         if (Version < 0) {
           error(I.getValue(), "invalid version number");
-          return false;
+          return None;
         }
         if (Version != 0) {
           error(I.getValue(), "version mismatch, expected 0");
-          return false;
+          return None;
         }
       } else if (Key == "case-sensitive") {
-        if (!parseScalarBool(I.getValue(), FS->CaseSensitive))
-          return false;
+        if (!parseScalarBool(I.getValue(), Result.FS->CaseSensitive))
+          return None;
       } else if (Key == "overlay-relative") {
-        if (!parseScalarBool(I.getValue(), FS->IsRelativeOverlay))
-          return false;
+        if (!parseScalarBool(I.getValue(), IsRelativeOverlay))
+          return None;
       } else if (Key == "use-external-names") {
-        if (!parseScalarBool(I.getValue(), FS->UseExternalNames))
-          return false;
+        if (!parseScalarBool(I.getValue(), Result.FS->UseExternalNames))
+          return None;
       } else if (Key == "fallthrough") {
         if (Keys["redirecting-with"].Seen) {
           error(I.getValue(),
                 "'fallthrough' and 'redirecting-with' are mutually exclusive");
-          return false;
+          return None;
         }
 
         bool ShouldFallthrough = false;
         if (!parseScalarBool(I.getValue(), ShouldFallthrough))
-          return false;
+          return None;
 
         if (ShouldFallthrough) {
-          FS->Redirection = RedirectingFileSystem::RedirectKind::Fallthrough;
+          Result.Redirection = YAMLParseResult::RedirectKind::Fallthrough;
         } else {
-          FS->Redirection = RedirectingFileSystem::RedirectKind::RedirectOnly;
+          Result.Redirection = YAMLParseResult::RedirectKind::RedirectOnly;
         }
       } else if (Key == "redirecting-with") {
         if (Keys["fallthrough"].Seen) {
           error(I.getValue(),
                 "'fallthrough' and 'redirecting-with' are mutually exclusive");
-          return false;
+          return None;
         }
 
         if (auto Kind = parseRedirectKind(I.getValue())) {
-          FS->Redirection = *Kind;
+          Result.Redirection = *Kind;
         } else {
           error(I.getValue(), "expected valid redirect kind");
-          return false;
+          return None;
         }
       } else {
         llvm_unreachable("key missing from Keys");
       }
     }
 
     if (Stream.failed())
-      return false;
+      return None;
 
     if (!checkMissingKeys(Top, Keys))
-      return false;
+      return None;
 
     // Now that we sucessefully parsed the YAML file, canonicalize the internal
     // representation to a proper directory tree so that we can search faster
     // inside the VFS.
     for (auto &E : RootEntries)
-      uniqueOverlayTree(FS, E.get());
+      uniqueOverlayTree(*Result.FS, E.get());
 
-    return true;
+    return Result;
   }
 };
 
 std::unique_ptr<RedirectingFileSystem>
 RedirectingFileSystem::create(std::unique_ptr<MemoryBuffer> Buffer,
                               SourceMgr::DiagHandlerTy DiagHandler,
                               StringRef YAMLFilePath, void *DiagContext,
                               IntrusiveRefCntPtr<FileSystem> ExternalFS) {
   SourceMgr SM;
-  yaml::Stream Stream(Buffer->getMemBufferRef(), SM);
-
   SM.setDiagHandler(DiagHandler, DiagContext);
-  yaml::document_iterator DI = Stream.begin();
-  yaml::Node *Root = DI->getRoot();
-  if (DI == Stream.end() || !Root) {
-    SM.PrintMessage(SMLoc(), SourceMgr::DK_Error, "expected root node");
-    return nullptr;
-  }
-
-  RedirectingFileSystemParser P(Stream);
-
-  std::unique_ptr<RedirectingFileSystem> FS(
-      new RedirectingFileSystem(ExternalFS));
 
-  if (!YAMLFilePath.empty()) {
-    // Use the YAML path from -ivfsoverlay to compute the dir to be prefixed
-    // to each 'external-contents' path.
-    //
-    // Example:
-    //    -ivfsoverlay dummy.cache/vfs/vfs.yaml
-    // yields:
-    //  FS->ExternalContentsPrefixDir => /<absolute_path_to>/dummy.cache/vfs
-    //
-    SmallString<256> OverlayAbsDir = sys::path::parent_path(YAMLFilePath);
-    std::error_code EC = llvm::sys::fs::make_absolute(OverlayAbsDir);
-    assert(!EC && "Overlay dir final path must be absolute");
-    (void)EC;
-    FS->setExternalContentsPrefixDir(OverlayAbsDir);
-  }
-
-  if (!P.parse(Root, FS.get()))
-    return nullptr;
-
-  return FS;
+  if (Optional<YAMLParseResult> Result = RedirectingFileSystemParser::parse(
+          Buffer->getMemBufferRef(), SM, std::move(ExternalFS)))
+    return std::move(Result->FS);
 }
 
 std::unique_ptr<RedirectingFileSystem> RedirectingFileSystem::create(
     ArrayRef<std::pair<std::string, std::string>> RemappedFiles,
     bool UseExternalNames, FileSystem &ExternalFS) {
   std::unique_ptr<RedirectingFileSystem> FS(
       new RedirectingFileSystem(&ExternalFS));
   FS->UseExternalNames = UseExternalNames;
if (ToEntry)
       continue;
 
     // Add parent directories.
     RedirectingFileSystem::Entry *Parent = nullptr;
     StringRef FromDirectory = llvm::sys::path::parent_path(From);
     for (auto I = llvm::sys::path::begin(FromDirectory),
               E = llvm::sys::path::end(FromDirectory);
          I != E; ++I) {
-      Parent = RedirectingFileSystemParser::lookupOrCreateEntry(FS.get(), *I,
-                                                                Parent);
+      Parent =
+          RedirectingFileSystemParser::lookupOrCreateEntry(*FS, *I, Parent);
     }
     assert(Parent && "File without a directory?");
     {
       auto EC = ExternalFS.makeAbsolute(To);
       (void)EC;
       assert(!EC && "Could not make absolute path");
     }
 
RedirectingFileSystem::makeCanonical(SmallVectorImpl<char> &Path) const {
   if (CanonicalPath.empty())
     return make_error_code(llvm::errc::invalid_argument);
 
   Path.assign(CanonicalPath.begin(), CanonicalPath.end());
   return {};
 }
 
 ErrorOr<RedirectingFileSystem::LookupResult>
-RedirectingFileSystem::lookupPath(StringRef Path) const {
-  sys::path::const_iterator Start = sys::path::begin(Path);
-  sys::path::const_iterator End = sys::path::end(Path);
+RedirectingFileSystem::lookupPath(StringRef CanonicalPath) const {
+  sys::path::const_iterator Start = sys::path::begin(CanonicalPath);
+  sys::path::const_iterator End = sys::path::end(CanonicalPath);
   for (const auto &Root : Roots) {
     ErrorOr<RedirectingFileSystem::LookupResult> Result =
         lookupPathImpl(Start, End, Root.get());
     if (Result || Result.getError() != llvm::errc::no_such_file_or_directory)
       return Result;
   }
   return make_error_code(llvm::errc::no_such_file_or_directory);
 }
ErrorOr<RedirectingFileSystem::LookupResult> Result =
         lookupPathImpl(Start, End, DirEntry.get());
     if (Result || Result.getError() != llvm::errc::no_such_file_or_directory)
       return Result;
   }
 
   return make_error_code(llvm::errc::no_such_file_or_directory);
 }
 
-static Status getRedirectedFileStatus(const Twine &OriginalPath,
-                                      bool UseExternalNames,
-                                      Status ExternalStatus) {
-  Status S = ExternalStatus;
-  if (!UseExternalNames)
-    S = Status::copyWithNewName(S, OriginalPath);
-  S.IsVFSMapped = true;
-  return S;
-}
-
-ErrorOr<Status> RedirectingFileSystem::status(
-    const Twine &CanonicalPath, const Twine &OriginalPath,
-    const RedirectingFileSystem::LookupResult &Result) {
-  if (Optional<StringRef> ExtRedirect = Result.getExternalRedirect()) {
-    SmallString<256> CanonicalRemappedPath((*ExtRedirect).str());
-    if (std::error_code EC = makeCanonical(CanonicalRemappedPath))
-      return EC;
-
-    ErrorOr<Status> S = ExternalFS->status(CanonicalRemappedPath);
-    if (!S)
-      return S;
-    S = Status::copyWithNewName(*S, *ExtRedirect);
-    auto *RE = cast<RedirectingFileSystem::RemapEntry>(Result.E);
-    return getRedirectedFileStatus(OriginalPath,
-                                   RE->useExternalName(UseExternalNames), *S);
-  }
-
-  auto *DE = cast<RedirectingFileSystem::DirectoryEntry>(Result.E);
-  return Status::copyWithNewName(DE->getStatus(), CanonicalPath);
-}
-
-ErrorOr<Status>
-RedirectingFileSystem::getExternalStatus(const Twine &CanonicalPath,
-                                         const Twine &OriginalPath) const {
-  if (auto Result = ExternalFS->status(CanonicalPath)) {
-    return Result.get().copyWithNewName(Result.get(), OriginalPath);
-  } else {
-    return Result.getError();
-  }
-}
-
 ErrorOr<Status> RedirectingFileSystem::status(const Twine &OriginalPath) {
   SmallString<256> CanonicalPath;
   OriginalPath.toVector(CanonicalPath);
-
   if (std::error_code EC = makeCanonical(CanonicalPath))
     return EC;
 
-  if (Redirection == RedirectKind::Fallback) {
-    // Attempt to find the original file first, only falling back to the
-    // mapped file if that fails.
-    ErrorOr<Status> S = getExternalStatus(CanonicalPath, OriginalPath);
-    if (S)
-      return S;
-  }
-
   ErrorOr<RedirectingFileSystem::LookupResult> Result =
       lookupPath(CanonicalPath);
-  if (!Result) {
-    // Was not able to map file, fallthrough to using the original path if
-    // that was the specified redirection type.
-    if (Redirection == RedirectKind::Fallthrough &&
-        isFileNotFound(Result.getError()))
-      return getExternalStatus(CanonicalPath, OriginalPath);
+  if (!Result)
     return Result.getError();
-  }
 
-  ErrorOr<Status> S = status(CanonicalPath, OriginalPath, *Result);
-  if (!S && Redirection == RedirectKind::Fallthrough &&
-      isFileNotFound(S.getError(), Result->E)) {
-    // Mapped the file but it wasn't found in the underlying filesystem,
-    // fallthrough to using the original path if that was the specified
-    // redirection type.
-    return getExternalStatus(CanonicalPath, OriginalPath);
+  Optional<StringRef> ExtRedirect = Result->getExternalRedirect();
+  if (!ExtRedirect) {
+    auto *DE = cast<RedirectingFileSystem::DirectoryEntry>(Result->E);
+    Status S = DE->getStatus();
+    if (!UseExternalNames)
+      S = Status::copyWithNewName(S, OriginalPath);
+    else
+      S = Status::copyWithNewName(S, CanonicalPath);
+    S.IsVFSMapped = true;
+    return S;
   }
 
+  ErrorOr<Status> S = ExternalFS->status(*ExtRedirect);
+  if (!S)
+    return S;
+
+  auto *RE = cast<RedirectingFileSystem::RemapEntry>(Result->E);
+  if (!RE->useExternalName(UseExternalNames))
+    S = Status::copyWithNewName(*S, OriginalPath);
+  S->IsVFSMapped = true;
   return S;
 }
 
 namespace {
 
 /// Provide a file wrapper with an overriden status.
 class FileWithFixedStatus : public File {
   std::unique_ptr<File> InnerFile;
   Status S;
 
 public:
   FileWithFixedStatus(std::unique_ptr<File> InnerFile, Status S)
       : InnerFile(std::move(InnerFile)), S(std::move(S)) {}
 
   ErrorOr<Status> status() override { return S; }
-  ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
 
+  ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
   getBuffer(const Twine &Name, int64_t FileSize, bool RequiresNullTerminator,
             bool IsVolatile) override {
     return InnerFile->getBuffer(Name, FileSize, RequiresNullTerminator,
                                 IsVolatile);
   }
 
   std::error_code close() override { return InnerFile->close(); }
 
if (Name && Name.get() != P.str())
     F->get()->setPath(P);
   return F;
 }
 
 ErrorOr<std::unique_ptr<File>>
 RedirectingFileSystem::openFileForRead(const Twine &OriginalPath) {
   SmallString<256> CanonicalPath;
   OriginalPath.toVector(CanonicalPath);
-
   if (std::error_code EC = makeCanonical(CanonicalPath))
     return EC;
 
-  if (Redirection == RedirectKind::Fallback) {
-    // Attempt to find the original file first, only falling back to the
-    // mapped file if that fails.
-    auto F = File::getWithPath(ExternalFS->openFileForRead(CanonicalPath),
-                               OriginalPath);
-    if (F)
-      return F;
-  }
-
   ErrorOr<RedirectingFileSystem::LookupResult> Result =
       lookupPath(CanonicalPath);
-  if (!Result) {
-    // Was not able to map file, fallthrough to using the original path if
-    // that was the specified redirection type.
-    if (Redirection == RedirectKind::Fallthrough &&
-        isFileNotFound(Result.getError()))
-      return File::getWithPath(ExternalFS->openFileForRead(CanonicalPath),
-                               OriginalPath);
+  if (!Result)
     return Result.getError();
-  }
 
-  if (!Result->getExternalRedirect()) // FIXME: errc::not_a_file?
+  Optional<StringRef> ExtRedirect = Result->getExternalRedirect();
+  if (!ExtRedirect)
     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);
-  if (!ExternalFile) {
-    if (Redirection == RedirectKind::Fallthrough &&
-        isFileNotFound(ExternalFile.getError(), Result->E)) {
-      // Mapped the file but it wasn't found in the underlying filesystem,
-      // fallthrough to using the original path if that was the specified
-      // redirection type.
-      return File::getWithPath(ExternalFS->openFileForRead(CanonicalPath),
-                               OriginalPath);
-    }
+  ErrorOr<std::unique_ptr<File>> ExternalFile =
+      ExternalFS->openFileForRead(*ExtRedirect);
+  if (!RE->useExternalName(UseExternalNames))
+    ExternalFile = File::getWithPath(std::move(ExternalFile), OriginalPath);
+  if (!ExternalFile)
     return ExternalFile;
-  }
 
-  auto ExternalStatus = (*ExternalFile)->status();
+  ErrorOr<Status> ExternalStatus = (*ExternalFile)->status();
   if (!ExternalStatus)
     return ExternalStatus.getError();
-
-  // Otherwise, the file was successfully remapped. Mark it as such. Also
-  // replace the underlying path if the external name is being used.
-  Status S = getRedirectedFileStatus(
-      OriginalPath, RE->useExternalName(UseExternalNames), *ExternalStatus);
-  return std::unique_ptr<File>(
-      std::make_unique<FileWithFixedStatus>(std::move(*ExternalFile), S));
+  ExternalStatus->IsVFSMapped = true;
+  return std::make_unique<FileWithFixedStatus>(std::move(*ExternalFile),
+                                               *ExternalStatus);
 }
 
 std::error_code
 RedirectingFileSystem::getRealPath(const Twine &OriginalPath,
                                    SmallVectorImpl<char> &Output) const {
   SmallString<256> CanonicalPath;
   OriginalPath.toVector(CanonicalPath);
-
   if (std::error_code EC = makeCanonical(CanonicalPath))
     return EC;
 
-  if (Redirection == RedirectKind::Fallback) {
-    // Attempt to find the original file first, only falling back to the
-    // mapped file if that fails.
-    std::error_code EC = ExternalFS->getRealPath(CanonicalPath, Output);
-    if (!EC)
-      return EC;
-  }
-
   ErrorOr<RedirectingFileSystem::LookupResult> Result =
       lookupPath(CanonicalPath);
-  if (!Result) {
-    // Was not able to map file, fallthrough to using the original path if
-    // that was the specified redirection type.
-    if (Redirection == RedirectKind::Fallthrough &&
-        isFileNotFound(Result.getError()))
-      return ExternalFS->getRealPath(CanonicalPath, Output);
+  if (!Result)
     return Result.getError();
-  }
 
-  // If we found FileEntry or DirectoryRemapEntry, look up the mapped
-  // path in the external file system.
-  if (auto ExtRedirect = Result->getExternalRedirect()) {
-    auto P = ExternalFS->getRealPath(*ExtRedirect, Output);
-    if (P && Redirection == RedirectKind::Fallthrough &&
-        isFileNotFound(P, Result->E)) {
-      // Mapped the file but it wasn't found in the underlying filesystem,
-      // fallthrough to using the original path if that was the specified
-      // redirection type.
-      return ExternalFS->getRealPath(CanonicalPath, Output);
-    }
-    return P;
-  }
-
-  // If we found a DirectoryEntry, still fallthrough to the original path if
-  // allowed, because directories don't have a single external contents path.
-  if (Redirection == RedirectKind::Fallthrough)
-    return ExternalFS->getRealPath(CanonicalPath, Output);
-  return llvm::errc::invalid_argument;
+  if (Optional<StringRef> ExternalRedirect = Result->getExternalRedirect())
+    return ExternalFS->getRealPath(*ExternalRedirect, Output);
+
+  // Otherwise we have a DirectoryEntry, just use its path
+  Output.assign(CanonicalPath);
+  return {};
 }
 
 std::unique_ptr<FileSystem>
 vfs::getVFSFromYAML(std::unique_ptr<MemoryBuffer> Buffer,
                     SourceMgr::DiagHandlerTy DiagHandler,
                     StringRef YAMLFilePath, void *DiagContext,
                     IntrusiveRefCntPtr<FileSystem> ExternalFS) {
   return RedirectingFileSystem::create(std::move(Buffer), DiagHandler,

llvm/unittests/Support/VirtualFileSystemTest.cpp

TEST(VFSFromRemappedFilesTest, LastMappingWins) {
 
   auto BufferKeepA = RemappedFSKeepName->getBufferForFile("//root/a");
   auto BufferExternalA = RemappedFSExternalName->getBufferForFile("//root/a");
   ASSERT_TRUE(BufferKeepA);
   ASSERT_TRUE(BufferExternalA);
   EXPECT_EQ("contents of c", (*BufferKeepA)->getBuffer());
   EXPECT_EQ("contents of c", (*BufferExternalA)->getBuffer());
 }
+
+// Check that a mapped directory and its parents all return a status with the
+// correct path
+TEST(RedirectingFileSystemTest, DirectoryStatus) {
+  auto Real = makeIntrusiveRefCnt<DummyFileSystem>();
+  Real->addDirectory("/real");
+
+  SmallVector<std::pair<std::string, std::string>> Mappings = {
+      {"/a/b/c", "/real"}};
+  auto FS = vfs::RedirectingFileSystem::create(Mappings, true, *Real);
+  ASSERT_TRUE(FS);
+
+  auto S = FS->status("/a");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("/a", S->getName());
+
+  S = FS->status("/a/b");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("/a/b", S->getName());
+
+  S = FS->status("/a/b/c");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("/real", S->getName());
+
+  FS->setCurrentWorkingDirectory("/a");
+
+  S = FS->status("b");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("/a/b", S->getName());
+
+  S = FS->status("b/c");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("/real", S->getName());
+
+  FS = vfs::RedirectingFileSystem::create(Mappings, false, *Real);
+  ASSERT_TRUE(FS);
+
+  // External names is false, use the passed in path instead
+  FS->setCurrentWorkingDirectory("/a");
+
+  S = FS->status("b");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("b", S->getName());
+
+  S = FS->status("b/c");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("b/c", S->getName());
+}