[VFS] OverlayFileSystem should consistently use the last-added FS first

[bnbarham]

Summary

Rather than checking whether a file exists before running an operation
and using the result of the first FS where it exists, always run
operations on the most recently added FS's first. If there's any error
other than missing file, return it immediately.

Also update CWD handling. The previous implementation attempted to
setCWD on all contained FS and then would use the CWD of the first
FS for getCWD, assuming they are all synced. This had a bunch of odd
behaviors:

1. setCWD would fail but all FS up to the failure would have their CWD set. getCWD would then return the just set CWD, even though it "failed"
2. Their CWD could get out of sync, so the CWD used by each FS wouldn't necessarily match the just set CWD

Fix this by keeping a CWD in OverlayFileSystem instead and pass the
already-absoluted path down into the contained FS. This has the downside
that any result needs to have the path overridden back to the original
(possibly relative) path, which is further complicated by certain
implementations needing to return a different path that *shouldn't* be
mapped.

Right now this is handled by only overriding the path if the returned
path was the one that was requested, but it would be nice to come up
with a better solution.

Depends on D121421

[bnbarham]

Moved overlays_range into D121421.

[bnbarham]

clang-tidy/infrastructure/empty-database.cpp is failing because setCurrentWorkingDirectory("") previously returned an error and no longer does.

It would have run on the RealFileSystem with a *process linked* CWD previously, which really just runs a chdir. chdir errors if the path doesn't exist. It now successfully sets on the InMemoryFileSystem since it checks nothing. This raises a few couple interesting questions:

1. The implementation of setCurrentWorkingDirectory is different for the non-process linked case. It first makes the path absolute and then errors if that path doesn't exist. So if that was being used, we also wouldn't error here (it would effectively be a no-op).
2. Should InMemoryFileSystem fail to set the CWD if the directory doesn't exist? I was considering doing this before I noticed this test failing, since it seems inconsistent with the other implementation. Thoughts?

[bnbarham]

Fix failing test

Adds an error to InMemoryFileSystem if attempting to set an empty CWD when no CWD has been set.

[dexonsmith]

This seems like a great thing to do. I have some comments

In D121423#3375818, @bnbarham wrote:

clang-tidy/infrastructure/empty-database.cpp is failing because setCurrentWorkingDirectory("") previously returned an error and no longer does.

It would have run on the RealFileSystem with a *process linked* CWD previously, which really just runs a chdir. chdir errors if the path doesn't exist. It now successfully sets on the InMemoryFileSystem since it checks nothing.

Is this behaviour on an empty string consistent across platforms?

This raises a few couple interesting questions:

1. The implementation of setCurrentWorkingDirectory is different for the non-process linked case. It first makes the path absolute and then errors if that path doesn't exist. So if that was being used, we also wouldn't error here (it would effectively be a no-op).

I feel like there are a few reasonable options.

• Special case empty string in the VFS as meaning the same thing as ..
• Special case empty string in the VFS as always given an error.
• Try to match behaviour of the underlying OS (if they're all the same, that's the same as one of the above).

IMO, cd "" is not a very interesting case, but it'd be nice for it to be consistent.

2. Should InMemoryFileSystem fail to set the CWD if the directory doesn't exist? I was considering doing this before I noticed this test failing, since it seems inconsistent with the other implementation. Thoughts?

My guess is that it doesn't error partly to align with the behaviour in OverlayFileSystem, so updating it as you've done seems fine to me, assuming we get the CWD working correctly in OverlayFS.

However, I'm concerned that the combination of the changes here will make the following do the wrong thing:

overlay:
  in-memory #1: (base)
    /a/x
  in-memory #2:
    /c/a/x

operations:

cd /
cd /c
cat a/x

IIUC the combined effect of your changes, this "should" print /c/a/x from in-memory-v2, but instead it'll print /a/x from in-memory-v1. And I think before this patch, it would have done the right thing.

llvm/lib/Support/VirtualFileSystem.cpp

510

I think this will be easier to reason about if CWD is only updated on the first success inside a given call to OverlayFileSystem::setCurrentWorkingDirectory. (Note that different VFSes could be canonicalizing relative paths differently.)

I also wonder if, instead, we can just keep track of which FS currently has the canonical CWD:

uint32_t IndexForCWD = 0; // Starts as BaseFS.

Calls to getCurrentWorkingDirectory() would forward to overlays_begin()[IndexForCWD].getCurrentWorkingDirectory(). This way, the overlay inherits the CWD canonicalization of the first working FS (usually, BaseFS).

514–517

The out-of-sync-ness here will remain hard to reason about if there is a sequence of cd relative-path calls.

I think if errc::no_such_file_or_directory is NOT returned (if there was success on any of the filesystems) then

• A bit should be stored in its entry in the OverlayFS, which prevents passing subsequent relative paths to it, until it has a valid CWD again.
• Even setCurrentWorkingDirectory() would not be forwarded for a relative CWD unless the filesystem has a valid CWD. It'd be possible to get "permanently lost" after successive relative path moves, such as cd a/ (not found in FS2) followed by cd .. (can't move back!), but I think much easier to reason about. (You could also potentially recover from that, by "trying" to apply ..s in the path and asking a working FS whether the two CWDs are equivalent.)

(I'm a bit scared of changing this to return true in new places until something like that is in place.)

[dexonsmith]

In D121423#3376212, @dexonsmith wrote:

This seems like a great thing to do. I have some comments

[Forgot to finish this thought]

I meant to say that I don't think the current approach is going to work quite correctly; comments inline :).

[dexonsmith]

In D121423#3376212, @dexonsmith wrote:

However, I'm concerned that the combination of the changes here will make the following do the wrong thing:

overlay:
  in-memory #1: (base)
    /a/x
  in-memory #2:
    /c/a/x

operations:

cd /
cd /c
cat a/x

IIUC the combined effect of your changes, this "should" print /c/a/x from in-memory-v2, but instead it'll print /a/x from in-memory-v1. And I think before this patch, it would have done the right thing.

I think my explanation was a bit muddled.

• Before this patch:
  • cd /c would have returned "directory not found". Clients think we're in /.
  • cat a/x will give content aligning with /a/x.
  • This seems correct.
• After this patch:
  • cd /c has no error. Clients think we're in /c.
  • cat a/x will give content aligning with /a/x/.
  • This seems incorrect. Since the cd /c succeeded, we should get /c/a/x.

[bnbarham]

In D121423#3376253, @dexonsmith wrote:

I think my explanation was a bit muddled.

• Before this patch:
  • cd /c would have returned "directory not found". Clients think we're in /.
  • cat a/x will give content aligning with /a/x.
  • This seems correct.
• After this patch:
  • cd /c has no error. Clients think we're in /c.
  • cat a/x will give content aligning with /a/x/.
  • This seems incorrect. Since the cd /c succeeded, we should get /c/a/x.

To be clear, setCWD never failed on InMemoryFileSystem (except with the current change where it fails on empty).

So before this patch:

• cd /c succeeds, both FS get that CWD
• cat a/x looks up /c/a/x in #1 which fails and then gives #2

After this patch:

• Same thing since setCWD never fails when given an actual path

But this *is* a problem. Consider the same thing but with RedirectingFileSystem instead:

overlay
  #1 /a/x -> /real/foo
  #2 /c/a/x -> /real/bar
  #3 real, assume foo and bar exist but /a and /c do not

Before this patch setCWD would immediately fail. So I guess no weirdness but also... still not great since I clearly do have a /c?

After this patch, setCWD fails on #1 and #3 but succeeds on #2, so the overlay now has /c. We then lookup a/x which gives /a/x instead of /c/a/x, which is likely what is expected.

Neither particularly make any sense here. I think the best option is probably what you suggested - store a bit representing whether or not setCWD failed and if we get any relative paths in, immediately return with no_such_file_or_directory if that bit is set.

To alleviate the "getting stuck" maybe we could find the first setCWD that succeeds, getCWD, and then re-run setCWD on all the rest to that CWD. Thoughts?

llvm/lib/Support/VirtualFileSystem.cpp

510

Updating multiple times was definitely unintentional. I did consider keeping track of the first working FS. I decided against it since theoretically you could change the CWD of one of the underlying FS if you had a reference to it. That is:

OverlayFS->setCurrentWorkingDirectory("/foo");
ContainedFS->setCurrentWorkingDirectory("/bar");
OverlayFS->getCurrentWorkingDirectory(); // Should this be /foo or /bar?

Also note that overlays_begin() is the reverse order, ie. BaseFS is the last FS used (not the first). I'd think it'd actually be fairly uncommon for it to be the first working FS.

514–517

Yes, out-of-sync-ness is a real issue. I'll expand on that in my other comment.

Note that the currently failing tests are related to this somehow. They set CWD to "." and then try to find a relative file in the InMemoryFileSystem. That used to work but is now failing. I'm a little confused by that because the particular sequence seems the same in both:

• setCWD to "." // Does nothing to InMemoryFileSystem, RealFileSystem will continue to have the original path
• add "a.cc" to InMemoryFileSystem
• try to read "a.cc", with my changes it asks for the full path (which fails), since it worked previously I can only assume that originally it passed it in as relative, so I need to find why that is

[dexonsmith]

I've thought of another concern: the "recursively call CWD" behaviour can result in CWD being called multiple times on a filesystem instance.

overlay:
  base:
    /a/a/c
  overlay:
    base:
      /a/a/c
operations:
  cd /
  cd a
  pwd   # /a
  cat c # cat /a/a/c

I think that has changed subtly with this patch:

• Before, pwd would give /a/a
• Now, pwd would give /a.

Another case:

overlay:
  base:
    /a/b/c
  overlay:
    base:
      /a/b/c
operations:
  cd /
  cd a
  pwd     # /a
  cat b/c # fail

• Before, cd a would have failed (but a subsequent pwd would give /a)
• Now it'll succeed

I think that's better? but only incidentally, I think, not by design.

To alleviate the "getting stuck" maybe we could find the first setCWD that succeeds, getCWD, and then re-run setCWD on all the rest to that CWD. Thoughts?

That might work. Or, getCWD() might return /bad/path/../.. because it doesn't know how to remove the ...

---

Stepping back, I'm not sure it's sound to be calling cd on the underlying FS. I think this is what led to RedirectingFS maintaining its own CWD and using absolute paths for access.

llvm/lib/Support/VirtualFileSystem.cpp

510

Aha, right, BaseFS is last.

I decided against it since theoretically you could change the CWD of one of the underlying FS if you had a reference to it.

Maybe a side-channel modification of the CWD of the underlying OS is not something that overlay-fs should have to reason about.

[bnbarham]

In D121423#3376347, @dexonsmith wrote:

I've thought of another concern: the "recursively call CWD" behaviour can result in CWD being called multiple times on a filesystem instance.

Ouch. I didn't consider this case. There's certainly no tests for it and I'd be surprised if anyone actually does that.

But in saying that, I think:

To alleviate the "getting stuck" maybe we could find the first setCWD that succeeds, getCWD, and then re-run setCWD on all the rest to that CWD. Thoughts?

Does actually fix that issue right? Ie. the problem is passing a relative path to each FS. But if we avoid that by using the absolute path of the first FS that succeeds then we're all good.

Stepping back, I'm not sure it's sound to be calling cd on the underlying FS. I think this is what led to RedirectingFS maintaining its own CWD and using absolute paths for access.

I would have to check the order of commits here. But I believe OverlayFS stopped being used for -ivfsoverlay before CWD was added, so RedirectingFS necessarily had to pass the absolute path down because there was no other option. That path leads to insanity though (IMO) and the current implementation isn't even correct - given multiple overlays, if the first fails then we *always* end up returning the original path.

What's the alternative to setting the CWD on the underlying FS here? Just removing OverlayFS? I did attempt to fix the bug I just mentioned directly, but it makes RedirectingFS even *more* of a mess and I was struggling quite a bit to reason about everything. I really don't think that's the direction we want to go.

My vote is on:

1. Use the CWD of the first successful FS
2. Set CWD on all FS to the one returned by the first successful FS

Though I believe this will mean the tests are still broken as this really only fixes the relative case, I haven't had a chance to figure out what's going on there yet.

[dexonsmith]

In D121423#3376377, @bnbarham wrote:

In D121423#3376347, @dexonsmith wrote:

I've thought of another concern: the "recursively call CWD" behaviour can result in CWD being called multiple times on a filesystem instance.

Ouch. I didn't consider this case. There's certainly no tests for it and I'd be surprised if anyone actually does that.

But in saying that, I think:

To alleviate the "getting stuck" maybe we could find the first setCWD that succeeds, getCWD, and then re-run setCWD on all the rest to that CWD. Thoughts?

Does actually fix that issue right? Ie. the problem is passing a relative path to each FS. But if we avoid that by using the absolute path of the first FS that succeeds then we're all good.

I think you're right. If the first success is used to convert to absolute, it should be okay. I think worth adding tests for since I do think this sort of thing can happen... TBH, all of the cases I can think of are about old RedirectingFS behaviour, so maybe not, but at least worth figuring out if it works with the changes here and keeping it working to prevent future confusion.

Stepping back, I'm not sure it's sound to be calling cd on the underlying FS. I think this is what led to RedirectingFS maintaining its own CWD and using absolute paths for access.

I would have to check the order of commits here. But I believe OverlayFS stopped being used for -ivfsoverlay before CWD was added, so RedirectingFS necessarily had to pass the absolute path down because there was no other option. That path leads to insanity though (IMO) and the current implementation isn't even correct - given multiple overlays, if the first fails then we *always* end up returning the original path.

What's the alternative to setting the CWD on the underlying FS here? Just removing OverlayFS? I did attempt to fix the bug I just mentioned directly, but it makes RedirectingFS even *more* of a mess and I was struggling quite a bit to reason about everything. I really don't think that's the direction we want to go.

My vote is on:

1. Use the CWD of the first successful FS
2. Set CWD on all FS to the one returned by the first successful FS

Though I believe this will mean the tests are still broken as this really only fixes the relative case, I haven't had a chance to figure out what's going on there yet.

Redirecting FS is definitely hard to reason about right now; still integrating what gets simplified with your change. I had completely missed that, because you're removing the fallthrough behaviour, it no longer needs to query the underlying FS with a relative path... ever. This is awesome!

---

A few remaining thoughts:

• The first successful FS will often be a RedirectingFS. But the BaseFS seems most likely to be a filesystem that is well-equipped to canonicalize the CWD. Since we're calling this on every FS, should we run the cd loop bottom-up instead of top-down (reverse(overlays()))?

• Should we make PhysicalFileSystem better at canonicalizing the CWD? (maybe off-topic for this patch)
  • On Windows, it could just run remove_dots(/*remove_dot_dot=/true). This is trivial.
  • Else, if there are ..s in a new relative path CWD, it can getRealPath() and:
    • if it matches what remove_dot_dot would give, use that
    • else, try getRealPath() on the remove_dot_dot=true path, and if that's the same as the origin getRealPath(), use the remove-dot-dot path
    • else, give up and use the current logic
  • Downside is that getRealPath() can be slow. But will this matter in practice for setCWD() calls?

• The client's view of the OverlayFS *should* be a single (virtual) filesystem. But without top-level CWD modelling, we can't actually provide that. Consider:

overlay:
  base:
    /file
  memory:
    /dir/

operations:
  cd /dir
  cat ../file

This *should* give the content from /file in base. But even with our proposed changes it's not going to work correctly. I think one way to be correct would be:

• Find the "first" FS (like CWD?) where getRealPath(dirname(relativepath)) does not fail
• Then combine that real-path with filename(relativepath), and go through the overlays with that path

... but this would probably be too slow. Not sure if you have ideas on how to fix it; and I don't think it's made worse by this patch (off topic?). Maybe worth adding a FIXME or description somewhere documenting that it doesn't work.

(Regardless, I think your plan should work.)

[bnbarham]

In D121423#3376431, @dexonsmith wrote:

• The client's view of the OverlayFS *should* be a single (virtual) filesystem. But without top-level CWD modelling, we can't actually provide that. Consider:

overlay:
  base:
    /file
  memory:
    /dir/

operations:
  cd /dir
  cat ../file

This *should* give the content from /file in base. But even with our proposed changes it's not going to work correctly. I think one way to be correct would be:

• Find the "first" FS (like CWD?) where getRealPath(dirname(relativepath)) does not fail
• Then combine that real-path with filename(relativepath), and go through the overlays with that path

... but this would probably be too slow. Not sure if you have ideas on how to fix it; and I don't think it's made worse by this patch (off topic?). Maybe worth adding a FIXME or description somewhere documenting that it doesn't work.

(Regardless, I think your plan should work.)

The real problem here is that we can't just resolve the path in OverlayFileSystem and pass that down because that would mean that the returned file/status would have a different path to the one requested. Unfortunately we can't just overwrite it with the original path, since it's possible the underlying FS *has to* return a different path (ie. because of external-names: true).

If we could resolve the path in OverlayFileSystem then we could just do a status + check that it's a directory on each FS and if so, set CWD in OverlayFileSystem and not any underlying FS.

Maybe we should just do that though with one exception - if the returned path is not the one we gave then *don't* replace the path...? External names is a bit of a hack anyway, so hack for a hack 😆?

EDIT: Note that this wouldn't help with symlinks, but the current implementation doesn't handle that anyway. Even doing Duncan's realpath suggestion wouldn't fix that I believe, since later FS may have different mappings.

And FWIW, this is basically how it worked/works in RedirectingFileSystem, minus the equality check. But the lack of that check (or some other way to handle this case) is a bug that results in external-names: true being ignored.

[bnbarham]

In D121423#3380920, @bnbarham wrote:

If we could resolve the path in OverlayFileSystem then we could just do a status + check that it's a directory on each FS and if so, set CWD in OverlayFileSystem and not any underlying FS.

Maybe we should just do that though with one exception - if the returned path is not the one we gave then *don't* replace the path...? External names is a bit of a hack anyway, so hack for a hack 😆?

I went ahead and implemented this. The main problem I can see is that the File returned by RealFileSystem::openFileForRead looks like it's supposed to give the *real path* for File::getName (though this seems to be the case for only RealFile). setPath currently replaces that as well, which would result in different behaviour for a OverlayFileSystem containing a RealFileSystem than just using RealFileSystem itself:

OverlayFileSystem
  RealFileSystem
    /a/b
    /f

cd /a/b
cat ../../f # File::getName + Status::getName == ../../f

RealFileSystem
  /a/b
  /f

cd /a/b
cat ../../f # File::getName == /a/b/f, Status::getName == ../../f

And this is even more of a problem when RedirectingFileSystem with use-external-names: false:

OverlayFileSystem
  RedirectingFileSystem(useExternalNames: false)
    /f2 -> f
  RealFileSystem
    /a/b
    /f

cd /a/b
cat ../../f2

A FileWithFixedStatus is returned that would have /a/b/../../f2 for File::getName and Status::getName. Then OverlayFileSystem would setPath("../../f2") and thus both would now return ../../f2. I'm not sure what we actually *want* to return here though. Maybe that's fine?

[dexonsmith]

In D121423#3383159, @bnbarham wrote:

In D121423#3380920, @bnbarham wrote:

If we could resolve the path in OverlayFileSystem then we could just do a status + check that it's a directory on each FS and if so, set CWD in OverlayFileSystem and not any underlying FS.

Maybe we should just do that though with one exception - if the returned path is not the one we gave then *don't* replace the path...? External names is a bit of a hack anyway, so hack for a hack 😆?

I went ahead and implemented this. The main problem I can see is that the File returned by RealFileSystem::openFileForRead looks like it's supposed to give the *real path* for File::getName (though this seems to be the case for only RealFile).

Do you really mean real path here (system call to realpath, that sees through symlinks), or just absolute path?

Is it only when RealFileSystem has a CWD? If RealFileSystem has a CWD, then in practice it needs to use absolute paths for calls to sys::fs since the process CWD cannot be trusted. I suspect it's just failing to model the working directory for the paths it returns (which is sometimes hard, in the presence of symlinks). Maybe that's fine for now.

Is this behaviour (overlayfilesystem's behaviour when wrapping a RealFileSystem) a regression/change or is it the same as before?

[bnbarham]

In D121423#3383595, @dexonsmith wrote:

Do you really mean real path here (system call to realpath, that sees through symlinks), or just absolute path?

Is it only when RealFileSystem has a CWD? If RealFileSystem has a CWD, then in practice it needs to use absolute paths for calls to sys::fs since the process CWD cannot be trusted. I suspect it's just failing to model the working directory for the paths it returns (which is sometimes hard, in the presence of symlinks). Maybe that's fine for now.

Is this behaviour (overlayfilesystem's behaviour when wrapping a RealFileSystem) a regression/change or is it the same as before?

I mean realpath, see https://llvm.org/doxygen/namespacellvm_1_1sys_1_1fs.html#a3e2cd27a3a2e6c44e9c0fc022952ea54, which is what RealFile::RealName gets set to. Honestly this seems a bit odd to me, there was a commit back in 2018 that renamed getName to getRealName but it ended up being reverted because tests failed and never re-added. I certainly wouldn't expect File::getName to return a realpath (and then only sometimes). Personally I'd like to remove the realpath behavior, or actually rename it to getRealName and make sure all filesystems implement... but I think that's a rabbit hole for a different day.

Anyway, it would be a regression, though one that's easy to fix - just don't set RealName in RealFile::setPath. This is what I've done (I'll put up new changes after lunch). I was more pointing out the RedirectingFileSystem behavior in comparison (which would still be the same since it returns a FileWithFixedStatus that just uses status()->getName()).

[bnbarham]

Pass absolute paths down into contained FS

[bnbarham]

We had some discussions around the issue of needing to replace the returned path with the original, but that not being possible because of use-external-names: true. I'm going to put up a separate patch that should make it possible to detect when an external path was returned and then not replace in that case. We can then use that here.

llvm/include/llvm/Support/VirtualFileSystem.h

 /// system overrides the other(s).
 class OverlayFileSystem : public FileSystem {
   using FileSystemList = SmallVector<IntrusiveRefCntPtr<FileSystem>, 1>;
 
   /// The stack of file systems, implemented as a list in order of
   /// their addition.
   FileSystemList FSList;
 
+  /// The last CWD successfully set on at least one of the filesystems in
+  /// \c FSList.
+  std::string CWD;
+
 public:
   OverlayFileSystem(IntrusiveRefCntPtr<FileSystem> Base);
 
   /// Pushes a file system on top of the stack.
   void pushOverlay(IntrusiveRefCntPtr<FileSystem> FS);
 
   llvm::ErrorOr<Status> status(const Twine &Path) override;
   llvm::ErrorOr<std::unique_ptr<File>>
   openFileForRead(const Twine &Path) override;
   directory_iterator dir_begin(const Twine &Dir, std::error_code &EC) override;
+
+  /// Gets the last successfully set CWD, which starts as the CWD from \c Base.
   llvm::ErrorOr<std::string> getCurrentWorkingDirectory() const override;
+
+  /// Attempts to set the CWD on all contained FS. Only errors if they all
+  /// fail.
   std::error_code setCurrentWorkingDirectory(const Twine &Path) override;
+
   std::error_code isLocal(const Twine &Path, bool &Result) override;
   std::error_code getRealPath(const Twine &Path,
                               SmallVectorImpl<char> &Output) const override;
 
   using iterator = FileSystemList::reverse_iterator;
   using const_iterator = FileSystemList::const_reverse_iterator;
   using reverse_iterator = FileSystemList::iterator;
   using const_reverse_iterator = FileSystemList::const_iterator;

llvm/lib/Support/VirtualFileSystem.cpp

 
 Status::Status(const Twine &Name, UniqueID UID, sys::TimePoint<> MTime,
                uint32_t User, uint32_t Group, uint64_t Size, file_type Type,
                perms Perms)
     : Name(Name.str()), UID(UID), MTime(MTime), User(User), Group(Group),
       Size(Size), Type(Type), Perms(Perms) {}
 
 Status Status::copyWithNewSize(const Status &In, uint64_t NewSize) {
-  return Status(In.getName(), In.getUniqueID(), In.getLastModificationTime(),
-                In.getUser(), In.getGroup(), NewSize, In.getType(),
-                In.getPermissions());
+  Status S = Status(In.getName(), In.getUniqueID(),
+                    In.getLastModificationTime(), In.getUser(), In.getGroup(),
+                    NewSize, In.getType(), In.getPermissions());
+  S.IsVFSMapped = In.IsVFSMapped;
+  return S;
 }
 
 Status Status::copyWithNewName(const Status &In, const Twine &NewName) {
-  return Status(NewName, In.getUniqueID(), In.getLastModificationTime(),
+  Status S = Status(NewName, In.getUniqueID(), In.getLastModificationTime(),
                     In.getUser(), In.getGroup(), In.getSize(), In.getType(),
                     In.getPermissions());
+  S.IsVFSMapped = In.IsVFSMapped;
+  return S;
 }
 
 Status Status::copyWithNewName(const file_status &In, const Twine &NewName) {
   return Status(NewName, In.getUniqueID(), In.getLastModificationTime(),
                 In.getUser(), In.getGroup(), In.getSize(), In.type(),
                 In.permissions());
 }
 

 
 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.
return directory_iterator(
       std::make_shared<RealFSDirIter>(adjustPath(Dir, Storage), EC));
 }
 
 //===-----------------------------------------------------------------------===/
 // OverlayFileSystem implementation
 //===-----------------------------------------------------------------------===/
 
 OverlayFileSystem::OverlayFileSystem(IntrusiveRefCntPtr<FileSystem> BaseFS) {
+  if (auto NewCWD = BaseFS->getCurrentWorkingDirectory())
+    CWD = *NewCWD;
   FSList.push_back(std::move(BaseFS));
 }
 
 void OverlayFileSystem::pushOverlay(IntrusiveRefCntPtr<FileSystem> FS) {
   FSList.push_back(FS);
-  // Synchronize added file systems by duplicating the working directory from
-  // the first one in the list.
-  FS->setCurrentWorkingDirectory(getCurrentWorkingDirectory().get());
 }
 
 ErrorOr<Status> OverlayFileSystem::status(const Twine &Path) {
   // FIXME: handle symlinks that cross file systems
-  for (iterator I = overlays_begin(), E = overlays_end(); I != E; ++I) {
-    ErrorOr<Status> Status = (*I)->status(Path);
-    if (Status || Status.getError() != llvm::errc::no_such_file_or_directory)
+
+  SmallString<256> AbsPath;
+  Path.toVector(AbsPath);
+  if (std::error_code EC = makeAbsolute(AbsPath))
+    return EC;
+
+  for (auto &FS : overlays_range()) {
+    ErrorOr<Status> Status = FS->status(AbsPath);
+    if (!Status) {
+      if (Status.getError() != errc::no_such_file_or_directory)
         return Status;
+      continue;
     }
-  return make_error_code(llvm::errc::no_such_file_or_directory);
+
+    // If the returned status had a different path to the one requested, just
+    // return it - the underlying FS mapped it to a different path, so we can't
+    // replace it. Otherwise, replace it with the originally requested path so
+    // that relative paths remain relative.
+    if (Status->getName() != AbsPath)
+      return Status;
+    return Status::copyWithNewName(*Status, Path);
+  }
+  return errc::no_such_file_or_directory;
 }
 
 ErrorOr<std::unique_ptr<File>>
 OverlayFileSystem::openFileForRead(const llvm::Twine &Path) {
   // FIXME: handle symlinks that cross file systems
-  for (iterator I = overlays_begin(), E = overlays_end(); I != E; ++I) {
-    auto Result = (*I)->openFileForRead(Path);
-    if (Result || Result.getError() != llvm::errc::no_such_file_or_directory)
+
+  SmallString<256> AbsPath;
+  Path.toVector(AbsPath);
+  if (std::error_code EC = makeAbsolute(AbsPath))
+    return EC;
+
+  for (auto &FS : overlays_range()) {
+    ErrorOr<std::unique_ptr<File>> Result = FS->openFileForRead(AbsPath);
+    if (!Result) {
+      if (Result.getError() != errc::no_such_file_or_directory)
         return Result;
+      continue;
+    }
+
+    // See \c OverlayFileSystem::status. Note that \c status() is being used
+    // rather than \c File::getName since the latter may be an already resolved
+    // real path (and hence would necessarily differ from the original).
+    if (ErrorOr<Status> Status = (*Result)->status()) {
+      if (Status->getName() != AbsPath)
+        return Result;
+      return File::getWithPath(std::move(Result), Path);
     }
-  return make_error_code(llvm::errc::no_such_file_or_directory);
+  }
+  return errc::no_such_file_or_directory;
 }
 
-llvm::ErrorOr<std::string>
-OverlayFileSystem::getCurrentWorkingDirectory() const {
-  // All file systems are synchronized, just take the first working directory.
-  return FSList.front()->getCurrentWorkingDirectory();
+ErrorOr<std::string> OverlayFileSystem::getCurrentWorkingDirectory() const {
+  if (CWD.empty())
+    return errc::operation_not_permitted;
+  return CWD;
 }
 
 std::error_code
 OverlayFileSystem::setCurrentWorkingDirectory(const Twine &Path) {
-  for (auto &FS : FSList)
-    if (std::error_code EC = FS->setCurrentWorkingDirectory(Path))
-      return EC;
+  SmallString<256> AbsPath;
+  Path.toVector(AbsPath);
+  if (std::error_code EC = makeAbsolute(AbsPath))
+
+  bool Success = false;
+  for (auto &FS : overlays_range()) {
+    if (auto Status = FS->status(AbsPath)) {
+      if (Status->isDirectory()) {
+        Success = true;
+        CWD = AbsPath.str().str();
+        break;
+      }
+    }
+  }
+
+  // Possible that all the filesystems could have failed with an unrelated
+  // error, but this seems the most reasonable error to return.
+  if (!Success)
+    return errc::no_such_file_or_directory;
   return {};
 }
 
 std::error_code OverlayFileSystem::isLocal(const Twine &Path, bool &Result) {
-  for (auto &FS : FSList)
-    if (FS->exists(Path))
-      return FS->isLocal(Path, Result);
+  SmallString<256> AbsPath;
+  Path.toVector(AbsPath);
+  if (std::error_code EC = makeAbsolute(AbsPath))
+    return EC;
+
+  for (auto &FS : overlays_range()) {
+    std::error_code EC = FS->isLocal(AbsPath, Result);
+    if (!EC || EC != errc::no_such_file_or_directory)
+      return EC;
+  }
   return errc::no_such_file_or_directory;
 }
 
 std::error_code
 OverlayFileSystem::getRealPath(const Twine &Path,
                                SmallVectorImpl<char> &Output) const {
-  for (const auto &FS : FSList)
-    if (FS->exists(Path))
-      return FS->getRealPath(Path, Output);
+  SmallString<256> AbsPath;
+  Path.toVector(AbsPath);
+  if (std::error_code EC = makeAbsolute(AbsPath))
+    return EC;
+
+  for (auto &FS : overlays_range()) {
+    std::error_code EC = FS->getRealPath(AbsPath, Output);
+    if (!EC || EC != errc::no_such_file_or_directory)
+      return EC;
+  }
   return errc::no_such_file_or_directory;
 }
 
 void OverlayFileSystem::printImpl(raw_ostream &OS, PrintType Type,
                                   unsigned IndentLevel) const {
   printIndent(OS, IndentLevel);
   OS << "OverlayFileSystem\n";
   if (Type == PrintType::Summary)
std::error_code InMemoryFileSystem::setCurrentWorkingDirectory(const Twine &P) {
   // Fix up relative paths. This just prepends the current working directory.
   std::error_code EC = makeAbsolute(Path);
   assert(!EC);
   (void)EC;
 
   if (useNormalizedPaths())
     llvm::sys::path::remove_dots(Path, /*remove_dot_dot=*/true);
 
-  if (!Path.empty())
-    WorkingDirectory = std::string(Path.str());
+  if (Path.empty())
+    return errc::no_such_file_or_directory;
+
   return {};
 }
 
 std::error_code
 InMemoryFileSystem::getRealPath(const Twine &Path,
                                 SmallVectorImpl<char> &Output) const {
   auto CWD = getCurrentWorkingDirectory();
   if (!CWD || CWD->empty())

llvm/unittests/Support/VirtualFileSystemTest.cpp

struct DummyFile : public vfs::File {
   explicit DummyFile(vfs::Status S) : S(S) {}
   llvm::ErrorOr<vfs::Status> status() override { return S; }
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
   getBuffer(const Twine &Name, int64_t FileSize, bool RequiresNullTerminator,
             bool IsVolatile) override {
     llvm_unreachable("unimplemented");
   }
   std::error_code close() override { return std::error_code(); }
+  void setPath(const Twine &Path) override { S = S.copyWithNewName(S, Path); }
 };
 
 class DummyFileSystem : public vfs::FileSystem {
   int FSID;   // used to produce UniqueIDs
   int FileID; // used to produce UniqueIDs
   std::string WorkingDirectory;
   std::map<std::string, vfs::Status> FilesAndDirs;
   typedef std::map<std::string, vfs::Status>::const_iterator const_iterator;
const_iterator findEntry(const Twine &Path) const {
     SmallString<128> P;
     Path.toVector(P);
     std::error_code EC = makeAbsolute(P);
     assert(!EC);
     (void)EC;
     return FilesAndDirs.find(std::string(P.str()));
   }
 
-  void addRegularFile(StringRef Path, sys::fs::perms Perms = sys::fs::all_all) {
-    vfs::Status S(Path, UniqueID(FSID, FileID++),
+  void addRegularFile(StringRef Path, sys::fs::perms Perms = sys::fs::all_all,
+                      StringRef StatusPath = StringRef()) {
+    if (StatusPath.empty())
+      StatusPath = Path;
+    vfs::Status S(StatusPath, UniqueID(FSID, FileID++),
                   std::chrono::system_clock::now(), 0, 0, 1024,
                   sys::fs::file_type::regular_file, Perms);
     addEntry(Path, S);
   }
 
   void addDirectory(StringRef Path, sys::fs::perms Perms = sys::fs::all_all) {
     vfs::Status S(Path, UniqueID(FSID, FileID++),
                   std::chrono::system_clock::now(), 0, 0, 0,

 };
 
 /// Replace back-slashes by front-slashes.
 std::string getPosixPath(std::string S) {
   SmallString<128> Result;
   llvm::sys::path::native(S, Result, llvm::sys::path::Style::posix);
   return std::string(Result.str());
 }
+
+#define EXPECT_PATHS(FS, Path, Expected, ExpectedRealPath)                     \
+  checkAllFSPaths(FS, Path, Expected, ExpectedRealPath, __LINE__)
+
+/// Check that the path returned by \c openFileForRead(Path)->getName() and
+/// \c status(Path)->getName() match \p Expected and \c getRealPath(Path)
+/// matches \p ExpectedRealPath.
+static void checkAllFSPaths(vfs::FileSystem &FS, StringRef Path,
+                            StringRef Expected, StringRef ExpectedRealPath,
+                            int Line) {
+  testing::ScopedTrace trace(__FILE__, Line, "EXPECT_PATHS");
+
+  auto F = FS.openFileForRead(Path);
+  ASSERT_FALSE(F.getError());
+
+  auto Name = (*F)->getName();
+  ASSERT_FALSE(Name.getError());
+  EXPECT_EQ(Expected, *Name);
+  auto Status = (*F)->status();
+  ASSERT_FALSE(Status.getError());
+  EXPECT_TRUE(Status->exists());
+  EXPECT_EQ(Expected, Status->getName());
+
+  Status = FS.status(Path);
+  ASSERT_FALSE(Status.getError());
+  EXPECT_EQ(Expected, Status->getName());
+
+  SmallString<256> RealPath;
+  auto EC = FS.getRealPath(Path, RealPath);
+  ASSERT_FALSE(EC);
+  EXPECT_EQ(ExpectedRealPath, RealPath);
+}
+
 } // end anonymous namespace
 
 TEST(VirtualFileSystemTest, StatusQueries) {
   IntrusiveRefCntPtr<DummyFileSystem> D(new DummyFileSystem());
   ErrorOr<vfs::Status> Status((std::error_code()));
 
   D->addRegularFile("/foo");
   Status = D->status("/foo");
TEST(OverlayFileSystemTest, PrintOutput) {
   ASSERT_EQ("OverlayFileSystem\n"
             "  DummyFileSystem (RecursiveContents)\n"
             "  OverlayFileSystem\n"
             "    DummyFileSystem (RecursiveContents)\n"
             "    DummyFileSystem (RecursiveContents)\n",
             Output);
 }
 
+// Base has no CWD, Overlay shouldn't either
+TEST(OverlayFileSystemTest, BaseNoWorkingDir) {
+  auto EmptyFS = makeIntrusiveRefCnt<DummyFileSystem>();
+  auto OverlayFS = makeIntrusiveRefCnt<vfs::OverlayFileSystem>(EmptyFS);
+  ASSERT_TRUE(OverlayFS->getCurrentWorkingDirectory().getError());
+}
+
+// Base has CWD, overlay should too
+TEST(OverlayFileSystemTest, BaseWithWorkingDir) {
+  auto DummyFS1 = makeIntrusiveRefCnt<DummyFileSystem>();
+  DummyFS1->setCurrentWorkingDirectory("/root");
+  auto OverlayFS = makeIntrusiveRefCnt<vfs::OverlayFileSystem>(DummyFS1);
+  auto CWD = OverlayFS->getCurrentWorkingDirectory();
+  ASSERT_FALSE(CWD.getError());
+  EXPECT_EQ("/root", *CWD);
+}
+
+TEST(OverlayFileSystemTest, WorkingDir) {
+  auto EmptyFS = makeIntrusiveRefCnt<DummyFileSystem>();
+  auto DummyFS = makeIntrusiveRefCnt<DummyFileSystem>();
+  DummyFS->addDirectory("/root");
+  DummyFS->addDirectory("/root2");
+  DummyFS->addDirectory("/root2/b");
+  DummyFS->setCurrentWorkingDirectory("/root");
+  auto OverlayFS = makeIntrusiveRefCnt<vfs::OverlayFileSystem>(EmptyFS);
+
+  // OverlayFS should still have no CWD since it hasn't been set yet. The CWD
+  // of DummyFS should not be affected by the push.
+  OverlayFS->pushOverlay(DummyFS);
+  auto CWD = OverlayFS->getCurrentWorkingDirectory();
+  EXPECT_TRUE(CWD.getError());
+  CWD = DummyFS->getCurrentWorkingDirectory();
+  ASSERT_FALSE(CWD.getError());
+  EXPECT_EQ("/root", *CWD);
+
+  // Should be able to setCWD on OverlayFS
+  auto EC = OverlayFS->setCurrentWorkingDirectory("/root2");
+  EXPECT_FALSE(EC);
+  CWD = OverlayFS->getCurrentWorkingDirectory();
+  ASSERT_FALSE(CWD.getError());
+  EXPECT_EQ("/root2", *CWD);
+
+  EC = OverlayFS->setCurrentWorkingDirectory("b");
+  EXPECT_FALSE(EC);
+  CWD = OverlayFS->getCurrentWorkingDirectory();
+  ASSERT_FALSE(CWD.getError());
+  EXPECT_EQ("/root2/b", *CWD);
+
+  // Can't set if the underlying path doesn't exist in any FS
+  EC = OverlayFS->setCurrentWorkingDirectory("/notfound");
+  EXPECT_TRUE(EC);
+  CWD = OverlayFS->getCurrentWorkingDirectory();
+  ASSERT_FALSE(CWD.getError());
+  EXPECT_EQ("/root2/b", *CWD);
+
+  // Shouldn't touch the underlying CWD
+  CWD = DummyFS->getCurrentWorkingDirectory();
+  ASSERT_FALSE(CWD.getError());
+  EXPECT_EQ("/root", *CWD);
+}
+
+// Check CWD works across contained FS
+TEST(OverlayFileSystemTest, BasicRelativePath) {
+  auto FS1 = makeIntrusiveRefCnt<DummyFileSystem>();
+  FS1->addDirectory("/dir");
+  auto FS2 = makeIntrusiveRefCnt<DummyFileSystem>();
+  FS2->addRegularFile("/dir/../f");
+  FS2->addRegularFile("/dir/../f2", sys::fs::all_all, "/another/path/f2");
+  auto OverlayFS = makeIntrusiveRefCnt<vfs::OverlayFileSystem>(FS2);
+  OverlayFS->pushOverlay(FS1);
+
+  auto EC = OverlayFS->setCurrentWorkingDirectory("/dir");
+  EXPECT_FALSE(EC);
+  auto CWD = OverlayFS->getCurrentWorkingDirectory();
+  ASSERT_FALSE(CWD.getError());
+  EXPECT_EQ("/dir", *CWD);
+
+  EXPECT_PATHS(*OverlayFS, "../f", "../f", "/dir/../f");
+  EXPECT_PATHS(*OverlayFS, "../f2", "/another/path/f2", "/dir/../f2");
+}
+
+// Check that we don't inadvertently get the wrong file when a higher FS doesn't
+// contain the set CWD
+TEST(OverlayFileSystemTest, DifferentRootRelativePath) {
+  auto FS1 = makeIntrusiveRefCnt<vfs::InMemoryFileSystem>();
+  FS1->addFile("/a/b", 0, MemoryBuffer::getMemBuffer("/a/b"));
+  FS1->setCurrentWorkingDirectory("/");
+  auto FS2 = makeIntrusiveRefCnt<vfs::InMemoryFileSystem>();
+  FS2->addFile("/c/a/b", 0, MemoryBuffer::getMemBuffer("/c/a/b"));
+  auto OverlayFS = makeIntrusiveRefCnt<vfs::OverlayFileSystem>(FS2);
+  OverlayFS->pushOverlay(FS1);
+
+  auto EC = OverlayFS->setCurrentWorkingDirectory("/c");
+  EXPECT_FALSE(EC);
+  auto CWD = OverlayFS->getCurrentWorkingDirectory();
+  ASSERT_FALSE(CWD.getError());
+  EXPECT_EQ("/c", *CWD);
+
+  // FS1 is on top of FS2, but since our CWD is /c we should be looking for
+  // /c/a/b *not* /a/b and thus the file should be found from FS2
+  auto F = OverlayFS->openFileForRead("a/b");
+  ASSERT_FALSE(F.getError());
+  auto Buffer = (*F)->getBuffer("ignored");
+  ASSERT_FALSE(Buffer.getError());
+  EXPECT_EQ("/c/a/b", (*Buffer)->getBuffer());
+  auto Name = (*F)->getName();
+  ASSERT_FALSE(Name.getError());
+  EXPECT_EQ("a/b", *Name);
+  auto Status = (*F)->status();
+  ASSERT_FALSE(Status.getError());
+  EXPECT_EQ("a/b", Status->getName());
+}
+
 TEST(ProxyFileSystemTest, Basic) {
   IntrusiveRefCntPtr<vfs::InMemoryFileSystem> Base(
       new vfs::InMemoryFileSystem());
   vfs::ProxyFileSystem PFS(Base);
 
   Base->addFile("/a", 0, MemoryBuffer::getMemBuffer("test"));
 
   auto Stat = PFS.status("/a");
TEST_F(InMemoryFileSystemTest, DirectoryIteration) {
   // path for the sake of the comparison.
   ASSERT_EQ("/b/c", getPosixPath(std::string(I->path())));
   I.increment(EC);
   ASSERT_FALSE(EC);
   ASSERT_EQ(vfs::directory_iterator(), I);
 }
 
 TEST_F(InMemoryFileSystemTest, WorkingDirectory) {
+  auto EC = FS.setCurrentWorkingDirectory("");
+  ASSERT_EQ(EC, std::errc::no_such_file_or_directory);
+
   FS.setCurrentWorkingDirectory("/b");
   FS.addFile("c", 0, MemoryBuffer::getMemBuffer(""));
 
   auto Stat = FS.status("/b/c");
   ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" << FS.toString();
   ASSERT_EQ("/b/c", Stat->getName());
   ASSERT_EQ("/b", *FS.getCurrentWorkingDirectory());