[VFS] Use original path when falling back to external FS

[keith]

Summary

This is a follow up to 0be9ca7c0f9a733f846bb6bc4e8e36d46b518162 to make
paths in the case of falling back to the external file system use the
original format, preserving relative paths, and allow the external
filesystem to canonicalize them if needed.

[keith]

Some of the other checks were invalid given the current tests, these seem to be enough to solve this issue without breaking existing tests

[varungandhi-apple]

cc @JDevlieghere

[dexonsmith]

I think @vsapsai was looking at this as well.

This mostly LGTM, but I think this can be pretty easily unit tested. Please add something to the tests in llvm/unittests/Support/VirtualFileSystemTest.cpp. Also a nit inline.

llvm/lib/Support/VirtualFileSystem.cpp

2075–2076

I suggest giving this a real name if it's being used for anything. Maybe PathAsWritten?

[vsapsai]

Was looking at an issue caused by over-eager RedirectingFileSystem path canonicalization and this patch fixes it. The repro we have is more complicated (involves 3 modules), so I don't think it is worth including with this change.

llvm/lib/Support/VirtualFileSystem.cpp

2075–2076

Another idea is to keep this variable Path and rename old Path to CanonicalPath.

[keith]

Update variable name and add unit test

[JDevlieghere]

I'm sure I'm missing something, but after rereading the patch several times I still don't see the functional change. It just looks like it's renaming every instance of Path to CanonicalPath and Path_ to Path?

[keith]

In D109128#2982780, @JDevlieghere wrote:

I'm sure I'm missing something, but after rereading the patch several times I still don't see the functional change. It just looks like it's renaming every instance of Path to CanonicalPath and Path_ to Path?

The functional change is not changing line 2016 and 2029 to use the newly named CanonicalPath when they did before. This was a bit more clear before I changed that variable name (but this is overall better IMO)

[keith]

I think I might have to apply this same logic to the other functions changed in the original commit as well, this seems to solve this isolated case, but I think there are some more cases that aren't currently fixed by this

[keith]

I'm realizing more now that these changes break the original intent of rG0be9ca7c0f9a733f846bb6bc4e8e36d46b518162 as well, given that it seems like the core canonicalization of the path before passing it down was the intent, but that's also the part that appears to be causing the issue. @JDevlieghere I would definitely appreciate some advice here, I'm still trying to see if I can satisfy the new test case without breaking the tests you changed, but I think they might fundamentally be at odds

[keith]

Handle all cases by passing relative path to ExternalFS first

[JDevlieghere]

Keith and I discussed this offline. My suggestion was to do the following:

1. Check the overlay for the canonicalized path
2. Check the fall-through for the canonicalized path
3. Check the fall-through for the original path

If I understand correctly, this patch does that, but swaps 2 and 3. What's the motivation for trying the non-canonical path first? If we treat the current working directory as a property directory (which is what the canonicalization is all about), then it seems like we should exhaust those options first.

[keith]

In D109128#2997588, @JDevlieghere wrote:

If I understand correctly, this patch does that, but swaps 2 and 3. What's the motivation for trying the non-canonical path first? If we treat the current working directory as a property directory (which is what the canonicalization is all about), then it seems like we should exhaust those options first.

Using the canonical path first is the root of the problem, using that first the behavior to return the absolute path virtually always, for example in my new test these assertions

EXPECT_EQ("a", Name.get());
EXPECT_EQ("a", OpenedS->getName());

fail and would have to be converted to:

EXPECT_EQ("//root/foo/a", Name.get());
EXPECT_EQ("//root/foo/a", OpenedS->getName());

[dexonsmith]

In D109128#2997588, @JDevlieghere wrote:

Keith and I discussed this offline. My suggestion was to do the following:

1. Check the overlay for the canonicalized path
2. Check the fall-through for the canonicalized path
3. Check the fall-through for the original path

I'm not sure it's correct to do (3) at all...

In D109128#2997787, @keith wrote:

In D109128#2997588, @JDevlieghere wrote:

If I understand correctly, this patch does that, but swaps 2 and 3. What's the motivation for trying the non-canonical path first? If we treat the current working directory as a property directory (which is what the canonicalization is all about), then it seems like we should exhaust those options first.

Using the canonical path first is the root of the problem, using that first the behavior to return the absolute path virtually always, for example in my new test these assertions

I think the problem is that we've been conflating two separate things:

• The path used internally to find the filesystem object.
• The path reported back to the user.

I assert that the path used internally to find the filesystem object should always/only be the canonical path.

1. Check the overlay for the canonicalized path.
2. Check the fall-through for the canonicalized path.

But the path reported back to the user should almost always be the original path used for lookup. ONLY when the path was found in the overlay with "use-external-names" should it be modified at all.

I think this could be implemented by with the help of adding the following APIs to vfs::File:

class File {
public:
  // Get the same file wrapped with a different reported path.
  static std::unique_ptr<File> getWithPath(std::unique_ptr<File> F,
                                           const Twine &P);

protected:
  // API for mutating the path. Override to avoid needing a wrapper
  // object for File::getWithPath.
  virtual bool setPath(const Twine &Path) { return false; }
};

then getWithPath can be:

namespace {
class WrapperFile : public File { /* Change the observed path */ };
}

std::unique_ptr<File> File::getWithPath(std::unique_ptr<File> F, const Twine &P) {
  if (F->setPath(P))
    return F;
  return std::make_unique<WrapperFile>(F, P);
}

Most the in-tree vfs::File derived classes can implement setPath so there shouldn't be many extra heap objects in practice.

@JDevlieghere / @keith, WDYT?

llvm/lib/Support/VirtualFileSystem.cpp

2075–2079

With the new comment, I think it'd be useful to have SmallString versions of path the original and canonical paths. In which case:

SmallString<256> Path;
Path_.toVector(Path);
SmallString<256> CanonicalPath = Path;

(with the original Path_ for the parameter) seems cleanest?

2100–2104

I think the correct logic here is something like:

if (auto Result = ExternalFS->openFileForRead(CanonicalPath))
  return Result->getPath() == CanonicalPath && Path != CanonicalPath
      ? File::getWithPath(*Result, Path)
      : *Result;
else
  return Result.getError();

• Ensures soundness in terms of getting the right file by always using the canonical path.
• If the fallthrough filesystem changes the name (e.g., if it's another redirecting filesystem with use-external-names) then passes that through.
• If the fallthrough filesystem reports back the name it was given, then passes back the name this function was given (matches stat behaviour) -- by induction, if all underlying filesystems are following a similar rule, then only if use-external-names=true was hit would the path be changed at all.

2119–2121

Can this logic be shared, with the above, maybe put in a lambda?

2130

I think Path should be sent in here, not CanonicalPath. Only if use-external-names is set should the observed path be canonicalized here... and in that case, the argument will be ignored anyway.

[dexonsmith]

(BTW, does this problem affect OverlayFileSystem as well?)

[JDevlieghere]

Yes, Keith and I came to the same conclusion yesterday. I was worried about tracking both paths at all times, but I like your suggestion of only changing the path when requested.

[dexonsmith]

In D109128#3000157, @JDevlieghere wrote:

Yes, Keith and I came to the same conclusion yesterday. I was worried about tracking both paths at all times, but I like your suggestion of only changing the path when requested.

Another idea that could be useful would be to add a type:

struct CanonicalizedPath {
  StringRef OriginalPath;
  StringRef CanonicalPath;
};

and add an overload for vfs::FileSystem::openFileForRead that takes this type.

• The default implementation in vfs::FileSystem could call openFileForRead(.Canonical) and then do a File::getWithPath(.Original) (as with my idea above, just when File::getPath matches .Canonical and .Canonical!=.Original).
• CanonicalizedPath-aware derived classes would invert the relationship. openFileForRead(FilesystemLookupPath) would use CanonicalPath for lookup, use OriginalPath for creating File, and pass through (a potentially updated!) CanonicalizedPath to base filesystems... and implement openFileForRead(Twine) by calling canonicalizePath.

Seems like a bit more code, but maybe not much, and the resulting logic might(?) be easier to reason about. (maybe a different name than openFileForRead would be better for clarity, rather than an overload?)

(Probably similar to your idea about tracking both paths at all times, but I'm not sure that needs to be mutually exclusive)

[vsapsai]

In D109128#2999846, @dexonsmith wrote:

In D109128#2997588, @JDevlieghere wrote:

Keith and I discussed this offline. My suggestion was to do the following:

1. Check the overlay for the canonicalized path
2. Check the fall-through for the canonicalized path
3. Check the fall-through for the original path

I'm not sure it's correct to do (3) at all...

I might have missed that but what use case are we addressing by avoiding the usage of the original path for the fall-through file system? Because we have not only a problem with names reported back but also canonicalization can break symlinks with relative paths. For example, for the layout

|- packages
|  |- a
|  |  `- include
|  `- b
|     `- include
`- include -> packages/a/include

-I ./include/../../b/include doesn't work if you have VFS, even an empty one.

[dexonsmith]

In D109128#3000374, @vsapsai wrote:

In D109128#2999846, @dexonsmith wrote:

In D109128#2997588, @JDevlieghere wrote:

Keith and I discussed this offline. My suggestion was to do the following:

1. Check the overlay for the canonicalized path
2. Check the fall-through for the canonicalized path
3. Check the fall-through for the original path

I'm not sure it's correct to do (3) at all...

I might have missed that but what use case are we addressing by avoiding the usage of the original path for the fall-through file system?

See rG0be9ca7c0f9a733f846bb6bc4e8e36d46b518162.

• RedirectingFileSystem has a virtual working directory.
• Prior to that commit, the external FS's working directory and RedirectingFileSystem's could get out of sync.
• Starting with that commit, RedirectingFileSystem consistently applies the virtual working directory.

Because we have not only a problem with names reported back but also canonicalization can break symlinks with relative paths.

Maybe this should be using makeAbsolute instead of makeCanonical (the latter calls the former)? (Note BTW that the absolute path logic is all pretty iffy on Windows, since each drive should have its own shadow virtual current directory, but that's got to be outside the scope here...)

In terms of canonicalization and symlinks, I think you're right that remove_dot_dot=true isn't really safe to use.

[keith]

Update to remap file paths after fetching them from the external FS

[keith]

Ok I've updated the diff here based on @dexonsmith's original suggestion, and some offline discussion with @JDevlieghere

The new logic remaps the files and statuses, in the fallback to the external FS case, to use the originally requested path. I opted not to use the second suggestion with passing the struct containing both paths through because of the churn that would have on the API to maintain the current public signatures while also supporting that. This approach is analogous to how we're currently remapping the statuses for use-external-names as well. Please let me know what you think!

Note there's some churn from me renaming Path -> CanonicalPath and Path_ -> OriginalPath, let me know if you'd prefer I extract this into a separate diff that we land first without any logic changes.

[JDevlieghere]

llvm/lib/Support/VirtualFileSystem.cpp

1182–1183

I'm pretty sure there was a reason we stopped doing this. There should be some discussion about that in my original patch.

2036

Can we abstract this in a function similar to getRedirectedFileStatus, something like getOrginialFileStatus or getNonCanonicalizedFileStatus and have a comment explaining what it does and why?

[keith]

llvm/lib/Support/VirtualFileSystem.cpp

1182–1183

So it sounds like it was related to this:

[fallthrough] ... but not for relative paths that would get resolved incorrectly at the lower layer (for example, in case of the RealFileSystem, because the strictly virtual path does not exist).

But if I remove that 2 of my new tests ReturnsInternalPathVFSHit and ReturnsExternalPathVFSHit do not pass. I think the behavior of them is what we want, what do you think?

[keith]

Extract fallback status logic to another function

[keith]

Fix format

[keith]

Improve lit test

[dexonsmith]

llvm/lib/Support/VirtualFileSystem.cpp

1182–1183

We stopped doing this because it puts ExternalFS in an unknown state since Path may not exist there. Future calls with relative paths could do very strange things.

E.g., here's a simple testcase that demonstrates something going very wrong:

• external FS has file /1/a
• redirecting FS has file /2/b (and passes through to external FS)
• execute: cd /1 && cd /2 && stat a

The correct result is for the stat to fail because /2/a doesn't exist. But your change above will instead find /1/a in ExternalFS.

Another example:

• external FS has file /1/a and /1/nest/c
• redirecting FS has file /2/b
• execute: cd /1/nest && cd /2 && cd .. && stat a

External FS will have CWD of /1, redirecting will have CWD of /, and stat a will erroneously give the result for /1/a instead of /a.

(Probably it'd be good to add tests for cases like this...)

To safely call ExternalFS->setCurrentWorkingDirectory(), you'll need extra state (and checks anywhere ExternalFS is used) tracking whether it has a valid working directory. If it does not, then it should not be queried, or it should only be sent absolute paths, or (etc.); and there should also be a way to re-establish a valid working directory.

[keith]

Remove cd of ExternalFS

llvm/lib/Support/VirtualFileSystem.cpp

1182–1183

Ok I definitely understand the use case now. This is probably something we should add tests for I guess, since I didn't seem to break anything with all my changes here.

I've updated the logic here, the core issue my new tests were failing without this is because the redirect from the VFS that is returned is not canonicalized itself, meaning when you asked for vfsname you would get vfsmappedname back, instead of //absolute/path/to/vfsmappedname. Since we stopped doing this cd, that wasn't enough. With my new change here we're now canonicalizing the return path as well, which is canonicalized against the working directory of the VFS itself.

The one thing I'm unusure about here, is why this isn't being done by the values returned from the VFS instead. I've added a new test here VFSFromYAMLTest.RelativePathHitWithoutCWD that illustrates the behavior I'm talking about. Requesting the absolute file path still fails because my canonicalization of the remapped path is incorrect, and it should be based on the directory's root instead of the VFS's PWD.

[keith]

Fix test paths on windows

Otherwise this resulted in a json string with non-escaped backslashes.

[dexonsmith]

llvm/lib/Support/VirtualFileSystem.cpp

1182–1183

This is probably something we should add tests for I guess, since I didn't seem to break anything with all my changes here.

It'd be awesome if you could do that if you're up for it, while your head is in this... (maybe in parallel with or after this patch?)

The one thing I'm unusure about here, is why this isn't being done by the values returned from the VFS instead. I've added a new test here VFSFromYAMLTest.RelativePathHitWithoutCWD that illustrates the behavior I'm talking about. Requesting the absolute file path still fails because my canonicalization of the remapped path is incorrect, and it should be based on the directory's root instead of the VFS's PWD.

I'm sorry, I'm not quite following; not sure if I'm thinking slowly today or if it's just that this stuff is complicated :). Can you add an inline comment in context to the testcase in question (just on Phab) that explains which behaviour/lines of code/etc. are unexpected/why/etc?

[keith]

llvm/unittests/Support/VirtualFileSystemTest.cpp

1738

So this test case is actually failing. The difference between it and the others is I don't call FS->setCurrentWorkingDirectory("//root/foo"). This results in us (with my most recent change here) performing this logic:

1. Fetch the absolute //root/foo/vfsname
2. This results in realname being returned
3. We attempt to canonicalize realname, but we have no pwd, so this doesn't result in a valid path
4. everything fails past this

It seems to me, without having a ton of context here, that the value returned from the VFS lookup should actually be //root/foo/realname, since otherwise we could likely hit one of the same issues as those discussed above where if you actually had this situation:

• mkdir /root/foo /root/bar
• touch /root/foo/realname /root/bar/realname
• cd /root/bar
• lookup /root/foo/vfsname, get back realname
• canonicalize realname to the wrong path /root/bar/realname

You'd end up with the wrong file, but I don't think this is actually new behavior with my change?

@dexonsmith wdyt?

[keith]

Remove broken test for now

Turns out this fails on main as well, so I'll punt this to another change

[keith]

@dexonsmith turns out the test I was adding is broken on main today too. If it's alright with you I will punt that to another diff to not increase the scope of this one.

[keith]

@dexonsmith can you take another look?

[keith]

@JDevlieghere can you take another pass?

[dexonsmith]

In D109128#3097060, @keith wrote:

@dexonsmith turns out the test I was adding is broken on main today too. If it's alright with you I will punt that to another diff to not increase the scope of this one.

Yes, SGTM.

In D109128#3113058, @keith wrote:

@dexonsmith can you take another look?

LGTM! Thanks for seeing this through. (And thanks for your patience; this (and the pings) just got buried somehow in my inbox.)

llvm/unittests/Support/VirtualFileSystemTest.cpp

1738

I agree with your analysis of the correct behaviour. Until the first call to setCurrentWorkingDirectory(), it seems like the working directory should implicitly be the one in the underlying FS (not sure whether it should be captured on construction, or just used going forward).

[keith]

Rebase

clang/test/VFS/relative-path-errors.c

+// RUN: mkdir -p %t
+// RUN: cd %t
+// RUN: cp %s main.c
+// RUN: not %clang_cc1 main.c 2>&1 | FileCheck %s
+// RUN: echo '{"roots": [],"version": 0}' > %t.yaml
+// RUN: not %clang_cc1 -ivfsoverlay %t.yaml main.c 2>&1 | FileCheck %s
+// RUN: echo '{"version": 0,"roots":[{"type":"directory","name":"%/t","contents":[{"type":"file","name":"vfsname", "external-contents":"main.c"}]}]}' > %t.yaml
+// RUN: not %clang_cc1 -ivfsoverlay %t.yaml vfsname 2>&1 | FileCheck %s
+
+// CHECK: {{^}}main.c:[[# @LINE + 1]]:1: error:
+foobarbaz

llvm/include/llvm/Support/VirtualFileSystem.h

public:
 
   /// Get the contents of the file as a \p MemoryBuffer.
   virtual llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
   getBuffer(const Twine &Name, int64_t FileSize = -1,
             bool RequiresNullTerminator = true, bool IsVolatile = false) = 0;
 
   /// Closes the file.
   virtual std::error_code close() = 0;
+
+  // Get the same file with a different path.
+  static ErrorOr<std::unique_ptr<File>>
+  getWithPath(ErrorOr<std::unique_ptr<File>> Result, const Twine &P);
+
+protected:
+  // Set the file's underlying path.
+  virtual void setPath(const Twine &Path) {}
 };
 
 /// A member of a directory, yielded by a directory_iterator.
 /// Only information available on most platforms is included.
 class directory_entry {
   std::string Path;
   llvm::sys::fs::file_type Type = llvm::sys::fs::file_type::type_unknown;
 
private:
   /// 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;
 
   /// Whether to fall back to the external file system when an operation fails
   /// with the given error code on a path associated with the provided Entry.
   bool shouldFallBackToExternalFS(std::error_code EC, Entry *E = nullptr) 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;
private:
   /// 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 &Path, const LookupResult &Result);
+  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;
 
   /// Parses \p Buffer, which is expected to be in YAML format and

llvm/lib/Support/VirtualFileSystem.cpp

public:
 
   ErrorOr<Status> status() override;
   ErrorOr<std::string> getName() override;
   ErrorOr<std::unique_ptr<MemoryBuffer>> getBuffer(const Twine &Name,
                                                    int64_t FileSize,
                                                    bool RequiresNullTerminator,
                                                    bool IsVolatile) override;
   std::error_code close() override;
+  void setPath(const Twine &Path) override;
 };
 
 } // namespace
 
 RealFile::~RealFile() { close(); }
 
 ErrorOr<Status> RealFile::status() {
   assert(FD != kInvalidFile && "cannot stat closed file");

 }
 
 std::error_code RealFile::close() {
   std::error_code EC = sys::fs::closeFile(FD);
   FD = kInvalidFile;
   return EC;
 }
 
+void RealFile::setPath(const Twine &Path) {
+  RealName = Path.str();
+  if (auto Status = status())
+    S = Status.get().copyWithNewName(Status.get(), Path);
+}
+
 namespace {
 
 /// A file system according to your operating system.
 /// This may be linked to the process's working directory, or maintain its own.
 ///
 /// Currently, its own working directory is emulated by storing the path and
 /// sending absolute paths to llvm::sys::fs:: functions.
 /// A more principled approach would be to push this down a level, modelling
public:
   getBuffer(const Twine &Name, int64_t FileSize, bool RequiresNullTerminator,
             bool IsVolatile) override {
     llvm::MemoryBuffer *Buf = Node.getBuffer();
     return llvm::MemoryBuffer::getMemBuffer(
         Buf->getBuffer(), Buf->getBufferIdentifier(), RequiresNullTerminator);
   }
 
   std::error_code close() override { return {}; }
+
+  void setPath(const Twine &Path) override { RequestedName = Path.str(); }
 };
 } // namespace
 
 class InMemoryDirectory : public InMemoryNode {
   Status Stat;
   llvm::StringMap<std::unique_ptr<InMemoryNode>> Entries;
 
 public:

 }
 
 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;
   WorkingDirectory = std::string(AbsolutePath.str());
   return {};
 }
 
 std::error_code RedirectingFileSystem::isLocal(const Twine &Path_,
                                                bool &Result) {
directory_iterator RedirectingFileSystem::dir_begin(const Twine &Dir,
   if (!Result) {
     EC = Result.getError();
     if (shouldFallBackToExternalFS(EC))
       return ExternalFS->dir_begin(Path, EC);
     return {};
   }
 
   // Use status to make sure the path exists and refers to a directory.
-  ErrorOr<Status> S = status(Path, *Result);
+  ErrorOr<Status> S = status(Path, Dir, *Result);
   if (!S) {
     if (shouldFallBackToExternalFS(S.getError(), Result->E))
       return ExternalFS->dir_begin(Dir, EC);
     EC = S.getError();
     return {};
   }
   if (!S->isDirectory()) {
     EC = std::error_code(static_cast<int>(errc::not_a_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 &Path, bool UseExternalNames,
+static Status getRedirectedFileStatus(const Twine &OriginalPath,
+                                      bool UseExternalNames,
                                       Status ExternalStatus) {
   Status S = ExternalStatus;
   if (!UseExternalNames)
-    S = Status::copyWithNewName(S, Path);
+    S = Status::copyWithNewName(S, OriginalPath);
   S.IsVFSMapped = true;
   return S;
 }
 
 ErrorOr<Status> RedirectingFileSystem::status(
-    const Twine &Path, const RedirectingFileSystem::LookupResult &Result) {
+    const Twine &CanonicalPath, const Twine &OriginalPath,
+    const RedirectingFileSystem::LookupResult &Result) {
   if (Optional<StringRef> ExtRedirect = Result.getExternalRedirect()) {
-    ErrorOr<Status> S = ExternalFS->status(*ExtRedirect);
+    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(Path, RE->useExternalName(UseExternalNames),
-                                   *S);
+    return getRedirectedFileStatus(OriginalPath,
+                                   RE->useExternalName(UseExternalNames), *S);
   }
 
   auto *DE = cast<RedirectingFileSystem::DirectoryEntry>(Result.E);
-  return Status::copyWithNewName(DE->getStatus(), Path);
+  return Status::copyWithNewName(DE->getStatus(), CanonicalPath);
 }
 
-ErrorOr<Status> RedirectingFileSystem::status(const Twine &Path_) {
-  SmallString<256> Path;
-  Path_.toVector(Path);
+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();
+  }
+}
 
-  if (std::error_code EC = makeCanonical(Path))
+ErrorOr<Status> RedirectingFileSystem::status(const Twine &OriginalPath) {
+  SmallString<256> CanonicalPath;
+  OriginalPath.toVector(CanonicalPath);
+
+  if (std::error_code EC = makeCanonical(CanonicalPath))
     return EC;
 
-  ErrorOr<RedirectingFileSystem::LookupResult> Result = lookupPath(Path);
+  ErrorOr<RedirectingFileSystem::LookupResult> Result =
+      lookupPath(CanonicalPath);
   if (!Result) {
-    if (shouldFallBackToExternalFS(Result.getError()))
-      return ExternalFS->status(Path);
+    if (shouldFallBackToExternalFS(Result.getError())) {
+      return getExternalStatus(CanonicalPath, OriginalPath);
+    }
     return Result.getError();
   }
 
-  ErrorOr<Status> S = status(Path, *Result);
-  if (!S && shouldFallBackToExternalFS(S.getError(), Result->E))
-    S = ExternalFS->status(Path);
+  ErrorOr<Status> S = status(CanonicalPath, OriginalPath, *Result);
+  if (!S && shouldFallBackToExternalFS(S.getError(), Result->E)) {
+    return getExternalStatus(CanonicalPath, OriginalPath);
+  }
+
   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>>
 
   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(); }
+
+  void setPath(const Twine &Path) override { S = S.copyWithNewName(S, Path); }
 };
 
 } // namespace
 
 ErrorOr<std::unique_ptr<File>>
-RedirectingFileSystem::openFileForRead(const Twine &Path_) {
-  SmallString<256> Path;
-  Path_.toVector(Path);
+File::getWithPath(ErrorOr<std::unique_ptr<File>> Result, const Twine &P) {
+  if (!Result)
+    return Result;
 
-  if (std::error_code EC = makeCanonical(Path))
+  auto F = std::move(*Result);
+  auto Name = F->getName();
+  if (Name && Name.get() != P.str())
+    F->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;
 
-  ErrorOr<RedirectingFileSystem::LookupResult> Result = lookupPath(Path);
+  ErrorOr<RedirectingFileSystem::LookupResult> Result =
+      lookupPath(CanonicalPath);
   if (!Result) {
     if (shouldFallBackToExternalFS(Result.getError()))
-      return ExternalFS->openFileForRead(Path);
+      return File::getWithPath(ExternalFS->openFileForRead(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 = ExternalFS->openFileForRead(ExtRedirect);
+  auto ExternalFile = File::getWithPath(
+      ExternalFS->openFileForRead(CanonicalRemappedPath), ExtRedirect);
   if (!ExternalFile) {
     if (shouldFallBackToExternalFS(ExternalFile.getError(), Result->E))
-      return ExternalFS->openFileForRead(Path);
+      return File::getWithPath(ExternalFS->openFileForRead(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(
-      Path, RE->useExternalName(UseExternalNames), *ExternalStatus);
+      OriginalPath, RE->useExternalName(UseExternalNames), *ExternalStatus);
   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

TEST_F(VFSFromYAMLTest, RemappedDirectoryOverlayNoFallthrough) {
 
   Lower->addRegularFile("//root/foo/b");
   checkContents(FS->dir_begin("//root/bar", EC),
                 {"//root/foo/a", "//root/foo/b"});
 
   EXPECT_EQ(0, NumDiagnostics);
 }
 
+TEST_F(VFSFromYAMLTest, ReturnsRequestedPathVFSMiss) {
+  IntrusiveRefCntPtr<vfs::InMemoryFileSystem> BaseFS(
+      new vfs::InMemoryFileSystem);
+  BaseFS->addFile("//root/foo/a", 0,
+                  MemoryBuffer::getMemBuffer("contents of a"));
+  ASSERT_FALSE(BaseFS->setCurrentWorkingDirectory("//root/foo"));
+  auto RemappedFS = vfs::RedirectingFileSystem::create(
+      {}, /*UseExternalNames=*/false, *BaseFS);
+
+  auto OpenedF = RemappedFS->openFileForRead("a");
+  ASSERT_FALSE(OpenedF.getError());
+  llvm::ErrorOr<std::string> Name = (*OpenedF)->getName();
+  ASSERT_FALSE(Name.getError());
+  EXPECT_EQ("a", Name.get());
+
+  auto OpenedS = (*OpenedF)->status();
+  ASSERT_FALSE(OpenedS.getError());
+  EXPECT_EQ("a", OpenedS->getName());
+  EXPECT_FALSE(OpenedS->IsVFSMapped);
+
+  auto DirectS = RemappedFS->status("a");
+  ASSERT_FALSE(DirectS.getError());
+  EXPECT_EQ("a", DirectS->getName());
+  EXPECT_FALSE(DirectS->IsVFSMapped);
+
+  EXPECT_EQ(0, NumDiagnostics);
+}
+
+TEST_F(VFSFromYAMLTest, ReturnsExternalPathVFSHit) {
+  IntrusiveRefCntPtr<vfs::InMemoryFileSystem> BaseFS(
+      new vfs::InMemoryFileSystem);
+  BaseFS->addFile("//root/foo/realname", 0,
+                  MemoryBuffer::getMemBuffer("contents of a"));
+  auto FS =
+      getFromYAMLString("{ 'use-external-names': true,\n"
+                        "  'roots': [\n"
+                        "{\n"
+                        "  'type': 'directory',\n"
+                        "  'name': '//root/foo',\n"
+                        "  'contents': [ {\n"
+                        "                  'type': 'file',\n"
+                        "                  'name': 'vfsname',\n"
+                        "                  'external-contents': 'realname'\n"
+                        "                }\n"
+                        "              ]\n"
+                        "}]}",
+                        BaseFS);
+  ASSERT_FALSE(FS->setCurrentWorkingDirectory("//root/foo"));
+
+  auto OpenedF = FS->openFileForRead("vfsname");
+  ASSERT_FALSE(OpenedF.getError());
+  llvm::ErrorOr<std::string> Name = (*OpenedF)->getName();
+  ASSERT_FALSE(Name.getError());
+  EXPECT_EQ("realname", Name.get());
+
+  auto OpenedS = (*OpenedF)->status();
+  ASSERT_FALSE(OpenedS.getError());
+  EXPECT_EQ("realname", OpenedS->getName());
+  EXPECT_TRUE(OpenedS->IsVFSMapped);
+
+  auto DirectS = FS->status("vfsname");
+  ASSERT_FALSE(DirectS.getError());
+  EXPECT_EQ("realname", DirectS->getName());
+  EXPECT_TRUE(DirectS->IsVFSMapped);
+
+  EXPECT_EQ(0, NumDiagnostics);
+}
+
+TEST_F(VFSFromYAMLTest, ReturnsInternalPathVFSHit) {
+  IntrusiveRefCntPtr<vfs::InMemoryFileSystem> BaseFS(
+      new vfs::InMemoryFileSystem);
+  BaseFS->addFile("//root/foo/realname", 0,
+                  MemoryBuffer::getMemBuffer("contents of a"));
+  auto FS =
+      getFromYAMLString("{ 'use-external-names': false,\n"
+                        "  'roots': [\n"
+                        "{\n"
+                        "  'type': 'directory',\n"
+                        "  'name': '//root/foo',\n"
+                        "  'contents': [ {\n"
+                        "                  'type': 'file',\n"
+                        "                  'name': 'vfsname',\n"
+                        "                  'external-contents': 'realname'\n"
+                        "                }\n"
+                        "              ]\n"
+                        "}]}",
+                        BaseFS);
+  ASSERT_FALSE(FS->setCurrentWorkingDirectory("//root/foo"));
+
+  auto OpenedF = FS->openFileForRead("vfsname");
+  ASSERT_FALSE(OpenedF.getError());
+  llvm::ErrorOr<std::string> Name = (*OpenedF)->getName();
+  ASSERT_FALSE(Name.getError());
+  EXPECT_EQ("vfsname", Name.get());
+
+  auto OpenedS = (*OpenedF)->status();
+  ASSERT_FALSE(OpenedS.getError());
+  EXPECT_EQ("vfsname", OpenedS->getName());
+  EXPECT_TRUE(OpenedS->IsVFSMapped);
+
+  auto DirectS = FS->status("vfsname");
+  ASSERT_FALSE(DirectS.getError());
+  EXPECT_EQ("vfsname", DirectS->getName());
+  EXPECT_TRUE(DirectS->IsVFSMapped);
+
+  EXPECT_EQ(0, NumDiagnostics);
+}
+
 TEST_F(VFSFromYAMLTest, CaseInsensitive) {
   IntrusiveRefCntPtr<DummyFileSystem> Lower(new DummyFileSystem());
   Lower->addRegularFile("//root/foo/bar/a");
   IntrusiveRefCntPtr<vfs::FileSystem> FS = getFromYAMLString(
       "{ 'case-sensitive': 'false',\n"
       "  'roots': [\n"
       "{\n"
       "  'type': 'directory',\n"
       "  'name': '//root/',\n"