[PATCH] D122549: [VFS] RedirectingFileSystem only replace path if not already mapped

[Ben Barham via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG3fda0edc51fd: [VFS] RedirectingFileSystem only replace path 
if not already mapped (authored by bnbarham).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122549/new/

https://reviews.llvm.org/D122549

Files:
  clang/lib/Basic/FileManager.cpp
  clang/test/VFS/external-names-multi-overlay.c
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp
  llvm/unittests/Support/VirtualFileSystemTest.cpp

Index: llvm/unittests/Support/VirtualFileSystemTest.cpp
===
--- llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -1442,12 +1442,12 @@
   ErrorOr S = O->status("//root/file1");
   ASSERT_FALSE(S.getError());
   EXPECT_EQ("//root/foo/bar/a", S->getName());
-  EXPECT_TRUE(S->IsVFSMapped);
+  EXPECT_TRUE(S->ExposesExternalVFSPath);
 
   ErrorOr SLower = O->status("//root/foo/bar/a");
   EXPECT_EQ("//root/foo/bar/a", SLower->getName());
   EXPECT_TRUE(S->equivalent(*SLower));
-  EXPECT_FALSE(SLower->IsVFSMapped);
+  EXPECT_FALSE(SLower->ExposesExternalVFSPath);
 
   // file after opening
   auto OpenedF = O->openFileForRead("//root/file1");
@@ -1455,7 +1455,7 @@
   auto OpenedS = (*OpenedF)->status();
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("//root/foo/bar/a", OpenedS->getName());
-  EXPECT_TRUE(OpenedS->IsVFSMapped);
+  EXPECT_TRUE(OpenedS->ExposesExternalVFSPath);
 
   // directory
   S = O->status("//root/");
@@ -1467,27 +1467,27 @@
   S = O->status("//root/mappeddir");
   ASSERT_FALSE(S.getError());
   EXPECT_TRUE(S->isDirectory());
-  EXPECT_TRUE(S->IsVFSMapped);
+  EXPECT_TRUE(S->ExposesExternalVFSPath);
   EXPECT_TRUE(S->equivalent(*O->status("//root/foo/bar")));
 
   SLower = O->status("//root/foo/bar");
   EXPECT_EQ("//root/foo/bar", SLower->getName());
   EXPECT_TRUE(S->equivalent(*SLower));
-  EXPECT_FALSE(SLower->IsVFSMapped);
+  EXPECT_FALSE(SLower->ExposesExternalVFSPath);
 
   // file in remapped directory
   S = O->status("//root/mappeddir/a");
   ASSERT_FALSE(S.getError());
-  ASSERT_FALSE(S->isDirectory());
-  ASSERT_TRUE(S->IsVFSMapped);
-  ASSERT_EQ("//root/foo/bar/a", S->getName());
+  EXPECT_FALSE(S->isDirectory());
+  EXPECT_TRUE(S->ExposesExternalVFSPath);
+  EXPECT_EQ("//root/foo/bar/a", S->getName());
 
   // file in remapped directory, with use-external-name=false
   S = O->status("//root/mappeddir2/a");
   ASSERT_FALSE(S.getError());
-  ASSERT_FALSE(S->isDirectory());
-  ASSERT_TRUE(S->IsVFSMapped);
-  ASSERT_EQ("//root/mappeddir2/a", S->getName());
+  EXPECT_FALSE(S->isDirectory());
+  EXPECT_FALSE(S->ExposesExternalVFSPath);
+  EXPECT_EQ("//root/mappeddir2/a", S->getName());
 
   // file contents in remapped directory
   OpenedF = O->openFileForRead("//root/mappeddir/a");
@@ -1495,7 +1495,7 @@
   OpenedS = (*OpenedF)->status();
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("//root/foo/bar/a", OpenedS->getName());
-  EXPECT_TRUE(OpenedS->IsVFSMapped);
+  EXPECT_TRUE(OpenedS->ExposesExternalVFSPath);
 
   // file contents in remapped directory, with use-external-name=false
   OpenedF = O->openFileForRead("//root/mappeddir2/a");
@@ -1503,7 +1503,7 @@
   OpenedS = (*OpenedF)->status();
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("//root/mappeddir2/a", OpenedS->getName());
-  EXPECT_TRUE(OpenedS->IsVFSMapped);
+  EXPECT_FALSE(OpenedS->ExposesExternalVFSPath);
 
   // broken mapping
   EXPECT_EQ(O->status("//root/file2").getError(),
@@ -1535,12 +1535,12 @@
   ErrorOr S = O->status("//mappedroot/a");
   ASSERT_FALSE(S.getError());
   EXPECT_EQ("//root/foo/bar/a", S->getName());
-  EXPECT_TRUE(S->IsVFSMapped);
+  EXPECT_TRUE(S->ExposesExternalVFSPath);
 
   ErrorOr SLower = O->status("//root/foo/bar/a");
   EXPECT_EQ("//root/foo/bar/a", SLower->getName());
   EXPECT_TRUE(S->equivalent(*SLower));
-  EXPECT_FALSE(SLower->IsVFSMapped);
+  EXPECT_FALSE(SLower->ExposesExternalVFSPath);
 
   // file after opening
   auto OpenedF = O->openFileForRead("//mappedroot/a");
@@ -1548,7 +1548,7 @@
   auto OpenedS = (*OpenedF)->status();
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("//root/foo/bar/a", OpenedS->getName());
-  EXPECT_TRUE(OpenedS->IsVFSMapped);
+  EXPECT_TRUE(OpenedS->ExposesExternalVFSPath);
 
   EXPECT_EQ(0, NumDiagnostics);
 }
@@ -1696,12 +1696,12 @@
   auto OpenedS = (*OpenedF)->status();
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("a", OpenedS->getName());
-  EXPECT_FALSE(OpenedS->IsVFSMapped);
+  EXPECT_FALSE(OpenedS->ExposesExternalVFSPath);
 
   auto DirectS = RemappedFS->status("a");
   ASSERT_FALSE(DirectS.getError());
   EXPECT_EQ("a", DirectS->getName());
-  EXPECT_FALSE(DirectS->IsVFSMapped);
+  EXPECT_FALSE(DirectS->ExposesExternalVFSPath);
 
   EXPECT_EQ(0, NumDiagnostics);
 }
@@ -1736,12 +1736,12 @@
   auto 

[PATCH] D122549: [VFS] RedirectingFileSystem only replace path if not already mapped

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122549/new/

https://reviews.llvm.org/D122549

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122549: [VFS] RedirectingFileSystem only replace path if not already mapped

[Ben Barham via Phabricator via cfe-commits]

bnbarham updated this revision to Diff 418959.
bnbarham added a comment.

Keep using the redirection hack for the time being


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122549/new/

https://reviews.llvm.org/D122549

Files:
  clang/lib/Basic/FileManager.cpp
  clang/test/VFS/external-names-multi-overlay.c
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp
  llvm/unittests/Support/VirtualFileSystemTest.cpp

Index: llvm/unittests/Support/VirtualFileSystemTest.cpp
===
--- llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -1442,12 +1442,12 @@
   ErrorOr S = O->status("//root/file1");
   ASSERT_FALSE(S.getError());
   EXPECT_EQ("//root/foo/bar/a", S->getName());
-  EXPECT_TRUE(S->IsVFSMapped);
+  EXPECT_TRUE(S->ExposesExternalVFSPath);
 
   ErrorOr SLower = O->status("//root/foo/bar/a");
   EXPECT_EQ("//root/foo/bar/a", SLower->getName());
   EXPECT_TRUE(S->equivalent(*SLower));
-  EXPECT_FALSE(SLower->IsVFSMapped);
+  EXPECT_FALSE(SLower->ExposesExternalVFSPath);
 
   // file after opening
   auto OpenedF = O->openFileForRead("//root/file1");
@@ -1455,7 +1455,7 @@
   auto OpenedS = (*OpenedF)->status();
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("//root/foo/bar/a", OpenedS->getName());
-  EXPECT_TRUE(OpenedS->IsVFSMapped);
+  EXPECT_TRUE(OpenedS->ExposesExternalVFSPath);
 
   // directory
   S = O->status("//root/");
@@ -1467,27 +1467,27 @@
   S = O->status("//root/mappeddir");
   ASSERT_FALSE(S.getError());
   EXPECT_TRUE(S->isDirectory());
-  EXPECT_TRUE(S->IsVFSMapped);
+  EXPECT_TRUE(S->ExposesExternalVFSPath);
   EXPECT_TRUE(S->equivalent(*O->status("//root/foo/bar")));
 
   SLower = O->status("//root/foo/bar");
   EXPECT_EQ("//root/foo/bar", SLower->getName());
   EXPECT_TRUE(S->equivalent(*SLower));
-  EXPECT_FALSE(SLower->IsVFSMapped);
+  EXPECT_FALSE(SLower->ExposesExternalVFSPath);
 
   // file in remapped directory
   S = O->status("//root/mappeddir/a");
   ASSERT_FALSE(S.getError());
-  ASSERT_FALSE(S->isDirectory());
-  ASSERT_TRUE(S->IsVFSMapped);
-  ASSERT_EQ("//root/foo/bar/a", S->getName());
+  EXPECT_FALSE(S->isDirectory());
+  EXPECT_TRUE(S->ExposesExternalVFSPath);
+  EXPECT_EQ("//root/foo/bar/a", S->getName());
 
   // file in remapped directory, with use-external-name=false
   S = O->status("//root/mappeddir2/a");
   ASSERT_FALSE(S.getError());
-  ASSERT_FALSE(S->isDirectory());
-  ASSERT_TRUE(S->IsVFSMapped);
-  ASSERT_EQ("//root/mappeddir2/a", S->getName());
+  EXPECT_FALSE(S->isDirectory());
+  EXPECT_FALSE(S->ExposesExternalVFSPath);
+  EXPECT_EQ("//root/mappeddir2/a", S->getName());
 
   // file contents in remapped directory
   OpenedF = O->openFileForRead("//root/mappeddir/a");
@@ -1495,7 +1495,7 @@
   OpenedS = (*OpenedF)->status();
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("//root/foo/bar/a", OpenedS->getName());
-  EXPECT_TRUE(OpenedS->IsVFSMapped);
+  EXPECT_TRUE(OpenedS->ExposesExternalVFSPath);
 
   // file contents in remapped directory, with use-external-name=false
   OpenedF = O->openFileForRead("//root/mappeddir2/a");
@@ -1503,7 +1503,7 @@
   OpenedS = (*OpenedF)->status();
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("//root/mappeddir2/a", OpenedS->getName());
-  EXPECT_TRUE(OpenedS->IsVFSMapped);
+  EXPECT_FALSE(OpenedS->ExposesExternalVFSPath);
 
   // broken mapping
   EXPECT_EQ(O->status("//root/file2").getError(),
@@ -1535,12 +1535,12 @@
   ErrorOr S = O->status("//mappedroot/a");
   ASSERT_FALSE(S.getError());
   EXPECT_EQ("//root/foo/bar/a", S->getName());
-  EXPECT_TRUE(S->IsVFSMapped);
+  EXPECT_TRUE(S->ExposesExternalVFSPath);
 
   ErrorOr SLower = O->status("//root/foo/bar/a");
   EXPECT_EQ("//root/foo/bar/a", SLower->getName());
   EXPECT_TRUE(S->equivalent(*SLower));
-  EXPECT_FALSE(SLower->IsVFSMapped);
+  EXPECT_FALSE(SLower->ExposesExternalVFSPath);
 
   // file after opening
   auto OpenedF = O->openFileForRead("//mappedroot/a");
@@ -1548,7 +1548,7 @@
   auto OpenedS = (*OpenedF)->status();
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("//root/foo/bar/a", OpenedS->getName());
-  EXPECT_TRUE(OpenedS->IsVFSMapped);
+  EXPECT_TRUE(OpenedS->ExposesExternalVFSPath);
 
   EXPECT_EQ(0, NumDiagnostics);
 }
@@ -1696,12 +1696,12 @@
   auto OpenedS = (*OpenedF)->status();
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("a", OpenedS->getName());
-  EXPECT_FALSE(OpenedS->IsVFSMapped);
+  EXPECT_FALSE(OpenedS->ExposesExternalVFSPath);
 
   auto DirectS = RemappedFS->status("a");
   ASSERT_FALSE(DirectS.getError());
   EXPECT_EQ("a", DirectS->getName());
-  EXPECT_FALSE(DirectS->IsVFSMapped);
+  EXPECT_FALSE(DirectS->ExposesExternalVFSPath);
 
   EXPECT_EQ(0, NumDiagnostics);
 }
@@ -1736,12 +1736,12 @@
   auto OpenedS = (*OpenedF)->status();
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("realname", OpenedS->getName());
-  

[PATCH] D122549: [VFS] RedirectingFileSystem only replace path if not already mapped

[Ben Barham via Phabricator via cfe-commits]

bnbarham marked an inline comment as not done.
bnbarham added inline comments.



Comment at: clang/lib/Basic/FileManager.cpp:273
 
-  if (Status.getName() == Filename) {
+  if (!Status.IsVFSMapped) {
 // The name matches. Set the FileEntry.

dexonsmith wrote:
> bnbarham wrote:
> > bnbarham wrote:
> > > dexonsmith wrote:
> > > > An incremental patch you could try would be:
> > > > ```
> > > > lang=c++
> > > > if (Status.getName() == Filename || !Status.IsVFSMapped)
> > > > ```
> > > > ... dropping all the other changes.
> > > > 
> > > > This limits the redirection hack to only apply when a RedirectingFS is 
> > > > involved (leaving until later the fine-tuning of when `IsVFSMapped` 
> > > > gets set). If this smaller change still causes a test failure, it might 
> > > > be easier to reason about why / how to fix it / be sure that the fix is 
> > > > sound.
> > > > 
> > > > Eventually we might want something like:
> > > > ```
> > > > lang=c++
> > > > if (!Status.IsVFSMapped) {
> > > >   assert(Status.getName() == Filename);
> > > > ```
> > > > I imagine that's not going to succeed yet due to the CWD behaviour in 
> > > > the VFSes, but as a speculative patch it might help track down whatever 
> > > > the problem is.
> > > That assertion currently won't succeed for any relative path, since 
> > > `getStatValue` does the absolute pathing for relative paths. So 
> > > `Status.getName()` is guaranteed to be different to `Filename` in that 
> > > case.
> > > 
> > > Possibly even more odd is that the failing test passes locally on MacOSX. 
> > > I'll try the suggestion above and see if they fail again.
> > Actually, thinking about this a bit more - the failing test doesn't use an 
> > overlay at all, so changing to the above wouldn't help at all.
> The test was added in 8188484daa4195a2c8b5253765036fa2c6da7263 / 
> https://reviews.llvm.org/D112647, with a change to do:
> ```
> +auto PrevWorkingDir = SM.getFileManager().getFileSystemOpts().WorkingDir;
> +if (BuildDir)
> +  SM.getFileManager().getFileSystemOpts().WorkingDir = 
> std::move(*BuildDir);
>  if (auto Entry = SM.getFileManager().getFile(R.getFilePath())) {
>if (SourceTU) {
>  auto  = DiagReplacements[*Entry];
> @@ -170,18 +175,19 @@ groupReplacements(const TUReplacements , const 
> TUDiagnostics ,
>errs() << "Described file '" << R.getFilePath()
>   << "' doesn't exist. Ignoring...\n";
>  }
> +SM.getFileManager().getFileSystemOpts().WorkingDir = PrevWorkingDir;
> ```
> 
> This looks incredibly suspicious. Changing the working directory mid-stream 
> on the FileManager isn't sound. Consider:
> ```
> /dir1/file1.h
> /dir2/file1.h
> /dir2/file2.h
> ```
> if you do:
> ```
> FM.WorkingDir = "/dir1";
> FM.getFile("file1.h"); // returns /dir1/file1.h
> FM.WorkingDir = "/dir2";
> FM.getFile("file1.h"); // returns /dir1/file1.h !!!
> FM.getFile("file2.h"); // returns /dir2/file2.h
> ```
> 
> That doesn't explain why it's not reproducing locally for you, though.
I ended up building in a docker container and tracked it down.

The underlying problem is two fold:
1. What you've said (shouldn't change WorkingDir)
2. clang-apply-replacements stores the FileEntry and uses that to read the file 
later. Because relative paths are absolute pathed inside `getStatValue`, the 
path in the FileEntryRef remains relative (without the redirection hack).

The reason I'm not seeing this test fail on MacOS is down to the iteration 
order of files. On MacOS we process `file2.yaml` and then `file1.yaml`. On 
others it looks like we're processing `file1.yaml` and then `file2.yaml`.

If `file1.yaml` is processed last, its last lookup is an absolute path and thus 
everything works fine. But if `file2.yaml` is processed last, its last lookup 
is relative and then the file can't be found.

This would normally be fine since the relative path would be resolved using the 
working directory, but because that's continually changed...  

Another quirk here is that if `getBufferForFile(FileEntry)` was used and 
`getFile` originally passed `openFile=true` then this would work, because it 
would be cached. And wouldn't work if `openFile=false` was passed, which is the 
default case.

TLDR: I don't think we should modify the redirection-hack condition in this 
patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122549/new/

https://reviews.llvm.org/D122549

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122549: [VFS] RedirectingFileSystem only replace path if not already mapped

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/lib/Basic/FileManager.cpp:273
 
-  if (Status.getName() == Filename) {
+  if (!Status.IsVFSMapped) {
 // The name matches. Set the FileEntry.

bnbarham wrote:
> bnbarham wrote:
> > dexonsmith wrote:
> > > An incremental patch you could try would be:
> > > ```
> > > lang=c++
> > > if (Status.getName() == Filename || !Status.IsVFSMapped)
> > > ```
> > > ... dropping all the other changes.
> > > 
> > > This limits the redirection hack to only apply when a RedirectingFS is 
> > > involved (leaving until later the fine-tuning of when `IsVFSMapped` gets 
> > > set). If this smaller change still causes a test failure, it might be 
> > > easier to reason about why / how to fix it / be sure that the fix is 
> > > sound.
> > > 
> > > Eventually we might want something like:
> > > ```
> > > lang=c++
> > > if (!Status.IsVFSMapped) {
> > >   assert(Status.getName() == Filename);
> > > ```
> > > I imagine that's not going to succeed yet due to the CWD behaviour in the 
> > > VFSes, but as a speculative patch it might help track down whatever the 
> > > problem is.
> > That assertion currently won't succeed for any relative path, since 
> > `getStatValue` does the absolute pathing for relative paths. So 
> > `Status.getName()` is guaranteed to be different to `Filename` in that case.
> > 
> > Possibly even more odd is that the failing test passes locally on MacOSX. 
> > I'll try the suggestion above and see if they fail again.
> Actually, thinking about this a bit more - the failing test doesn't use an 
> overlay at all, so changing to the above wouldn't help at all.
The test was added in 8188484daa4195a2c8b5253765036fa2c6da7263 / 
https://reviews.llvm.org/D112647, with a change to do:
```
+auto PrevWorkingDir = SM.getFileManager().getFileSystemOpts().WorkingDir;
+if (BuildDir)
+  SM.getFileManager().getFileSystemOpts().WorkingDir = 
std::move(*BuildDir);
 if (auto Entry = SM.getFileManager().getFile(R.getFilePath())) {
   if (SourceTU) {
 auto  = DiagReplacements[*Entry];
@@ -170,18 +175,19 @@ groupReplacements(const TUReplacements , const 
TUDiagnostics ,
   errs() << "Described file '" << R.getFilePath()
  << "' doesn't exist. Ignoring...\n";
 }
+SM.getFileManager().getFileSystemOpts().WorkingDir = PrevWorkingDir;
```

This looks incredibly suspicious. Changing the working directory mid-stream on 
the FileManager isn't sound. Consider:
```
/dir1/file1.h
/dir2/file1.h
/dir2/file2.h
```
if you do:
```
FM.WorkingDir = "/dir1";
FM.getFile("file1.h"); // returns /dir1/file1.h
FM.WorkingDir = "/dir2";
FM.getFile("file1.h"); // returns /dir1/file1.h !!!
FM.getFile("file2.h"); // returns /dir2/file2.h
```

That doesn't explain why it's not reproducing locally for you, though.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122549/new/

https://reviews.llvm.org/D122549

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122549: [VFS] RedirectingFileSystem only replace path if not already mapped

[Ben Barham via Phabricator via cfe-commits]

bnbarham added inline comments.



Comment at: clang/lib/Basic/FileManager.cpp:273
 
-  if (Status.getName() == Filename) {
+  if (!Status.IsVFSMapped) {
 // The name matches. Set the FileEntry.

bnbarham wrote:
> dexonsmith wrote:
> > An incremental patch you could try would be:
> > ```
> > lang=c++
> > if (Status.getName() == Filename || !Status.IsVFSMapped)
> > ```
> > ... dropping all the other changes.
> > 
> > This limits the redirection hack to only apply when a RedirectingFS is 
> > involved (leaving until later the fine-tuning of when `IsVFSMapped` gets 
> > set). If this smaller change still causes a test failure, it might be 
> > easier to reason about why / how to fix it / be sure that the fix is sound.
> > 
> > Eventually we might want something like:
> > ```
> > lang=c++
> > if (!Status.IsVFSMapped) {
> >   assert(Status.getName() == Filename);
> > ```
> > I imagine that's not going to succeed yet due to the CWD behaviour in the 
> > VFSes, but as a speculative patch it might help track down whatever the 
> > problem is.
> That assertion currently won't succeed for any relative path, since 
> `getStatValue` does the absolute pathing for relative paths. So 
> `Status.getName()` is guaranteed to be different to `Filename` in that case.
> 
> Possibly even more odd is that the failing test passes locally on MacOSX. 
> I'll try the suggestion above and see if they fail again.
Actually, thinking about this a bit more - the failing test doesn't use an 
overlay at all, so changing to the above wouldn't help at all.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122549/new/

https://reviews.llvm.org/D122549

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122549: [VFS] RedirectingFileSystem only replace path if not already mapped

[Ben Barham via Phabricator via cfe-commits]

bnbarham marked 4 inline comments as done.
bnbarham added inline comments.



Comment at: clang/lib/Basic/FileManager.cpp:273
 
-  if (Status.getName() == Filename) {
+  if (!Status.IsVFSMapped) {
 // The name matches. Set the FileEntry.

dexonsmith wrote:
> An incremental patch you could try would be:
> ```
> lang=c++
> if (Status.getName() == Filename || !Status.IsVFSMapped)
> ```
> ... dropping all the other changes.
> 
> This limits the redirection hack to only apply when a RedirectingFS is 
> involved (leaving until later the fine-tuning of when `IsVFSMapped` gets 
> set). If this smaller change still causes a test failure, it might be easier 
> to reason about why / how to fix it / be sure that the fix is sound.
> 
> Eventually we might want something like:
> ```
> lang=c++
> if (!Status.IsVFSMapped) {
>   assert(Status.getName() == Filename);
> ```
> I imagine that's not going to succeed yet due to the CWD behaviour in the 
> VFSes, but as a speculative patch it might help track down whatever the 
> problem is.
That assertion currently won't succeed for any relative path, since 
`getStatValue` does the absolute pathing for relative paths. So 
`Status.getName()` is guaranteed to be different to `Filename` in that case.

Possibly even more odd is that the failing test passes locally on MacOSX. I'll 
try the suggestion above and see if they fail again.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122549/new/

https://reviews.llvm.org/D122549

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122549: [VFS] RedirectingFileSystem only replace path if not already mapped

[Ben Barham via Phabricator via cfe-commits]

bnbarham updated this revision to Diff 418721.
bnbarham edited the summary of this revision.
bnbarham added a comment.

Rename `IsVFSMapped` as suggested by Duncan. Update comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122549/new/

https://reviews.llvm.org/D122549

Files:
  clang/lib/Basic/FileManager.cpp
  clang/test/VFS/external-names-multi-overlay.c
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp
  llvm/unittests/Support/VirtualFileSystemTest.cpp

Index: llvm/unittests/Support/VirtualFileSystemTest.cpp
===
--- llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -1442,12 +1442,12 @@
   ErrorOr S = O->status("//root/file1");
   ASSERT_FALSE(S.getError());
   EXPECT_EQ("//root/foo/bar/a", S->getName());
-  EXPECT_TRUE(S->IsVFSMapped);
+  EXPECT_TRUE(S->ExposesExternalVFSPath);
 
   ErrorOr SLower = O->status("//root/foo/bar/a");
   EXPECT_EQ("//root/foo/bar/a", SLower->getName());
   EXPECT_TRUE(S->equivalent(*SLower));
-  EXPECT_FALSE(SLower->IsVFSMapped);
+  EXPECT_FALSE(SLower->ExposesExternalVFSPath);
 
   // file after opening
   auto OpenedF = O->openFileForRead("//root/file1");
@@ -1455,7 +1455,7 @@
   auto OpenedS = (*OpenedF)->status();
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("//root/foo/bar/a", OpenedS->getName());
-  EXPECT_TRUE(OpenedS->IsVFSMapped);
+  EXPECT_TRUE(OpenedS->ExposesExternalVFSPath);
 
   // directory
   S = O->status("//root/");
@@ -1467,27 +1467,27 @@
   S = O->status("//root/mappeddir");
   ASSERT_FALSE(S.getError());
   EXPECT_TRUE(S->isDirectory());
-  EXPECT_TRUE(S->IsVFSMapped);
+  EXPECT_TRUE(S->ExposesExternalVFSPath);
   EXPECT_TRUE(S->equivalent(*O->status("//root/foo/bar")));
 
   SLower = O->status("//root/foo/bar");
   EXPECT_EQ("//root/foo/bar", SLower->getName());
   EXPECT_TRUE(S->equivalent(*SLower));
-  EXPECT_FALSE(SLower->IsVFSMapped);
+  EXPECT_FALSE(SLower->ExposesExternalVFSPath);
 
   // file in remapped directory
   S = O->status("//root/mappeddir/a");
   ASSERT_FALSE(S.getError());
-  ASSERT_FALSE(S->isDirectory());
-  ASSERT_TRUE(S->IsVFSMapped);
-  ASSERT_EQ("//root/foo/bar/a", S->getName());
+  EXPECT_FALSE(S->isDirectory());
+  EXPECT_TRUE(S->ExposesExternalVFSPath);
+  EXPECT_EQ("//root/foo/bar/a", S->getName());
 
   // file in remapped directory, with use-external-name=false
   S = O->status("//root/mappeddir2/a");
   ASSERT_FALSE(S.getError());
-  ASSERT_FALSE(S->isDirectory());
-  ASSERT_TRUE(S->IsVFSMapped);
-  ASSERT_EQ("//root/mappeddir2/a", S->getName());
+  EXPECT_FALSE(S->isDirectory());
+  EXPECT_FALSE(S->ExposesExternalVFSPath);
+  EXPECT_EQ("//root/mappeddir2/a", S->getName());
 
   // file contents in remapped directory
   OpenedF = O->openFileForRead("//root/mappeddir/a");
@@ -1495,7 +1495,7 @@
   OpenedS = (*OpenedF)->status();
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("//root/foo/bar/a", OpenedS->getName());
-  EXPECT_TRUE(OpenedS->IsVFSMapped);
+  EXPECT_TRUE(OpenedS->ExposesExternalVFSPath);
 
   // file contents in remapped directory, with use-external-name=false
   OpenedF = O->openFileForRead("//root/mappeddir2/a");
@@ -1503,7 +1503,7 @@
   OpenedS = (*OpenedF)->status();
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("//root/mappeddir2/a", OpenedS->getName());
-  EXPECT_TRUE(OpenedS->IsVFSMapped);
+  EXPECT_FALSE(OpenedS->ExposesExternalVFSPath);
 
   // broken mapping
   EXPECT_EQ(O->status("//root/file2").getError(),
@@ -1535,12 +1535,12 @@
   ErrorOr S = O->status("//mappedroot/a");
   ASSERT_FALSE(S.getError());
   EXPECT_EQ("//root/foo/bar/a", S->getName());
-  EXPECT_TRUE(S->IsVFSMapped);
+  EXPECT_TRUE(S->ExposesExternalVFSPath);
 
   ErrorOr SLower = O->status("//root/foo/bar/a");
   EXPECT_EQ("//root/foo/bar/a", SLower->getName());
   EXPECT_TRUE(S->equivalent(*SLower));
-  EXPECT_FALSE(SLower->IsVFSMapped);
+  EXPECT_FALSE(SLower->ExposesExternalVFSPath);
 
   // file after opening
   auto OpenedF = O->openFileForRead("//mappedroot/a");
@@ -1548,7 +1548,7 @@
   auto OpenedS = (*OpenedF)->status();
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("//root/foo/bar/a", OpenedS->getName());
-  EXPECT_TRUE(OpenedS->IsVFSMapped);
+  EXPECT_TRUE(OpenedS->ExposesExternalVFSPath);
 
   EXPECT_EQ(0, NumDiagnostics);
 }
@@ -1696,12 +1696,12 @@
   auto OpenedS = (*OpenedF)->status();
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("a", OpenedS->getName());
-  EXPECT_FALSE(OpenedS->IsVFSMapped);
+  EXPECT_FALSE(OpenedS->ExposesExternalVFSPath);
 
   auto DirectS = RemappedFS->status("a");
   ASSERT_FALSE(DirectS.getError());
   EXPECT_EQ("a", DirectS->getName());
-  EXPECT_FALSE(DirectS->IsVFSMapped);
+  EXPECT_FALSE(DirectS->ExposesExternalVFSPath);
 
   EXPECT_EQ(0, NumDiagnostics);
 }
@@ -1736,12 +1736,12 @@
   auto OpenedS = (*OpenedF)->status();
   ASSERT_FALSE(OpenedS.getError());
   

[PATCH] D122549: [VFS] RedirectingFileSystem only replace path if not already mapped

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/lib/Basic/FileManager.cpp:273
 
-  if (Status.getName() == Filename) {
+  if (!Status.IsVFSMapped) {
 // The name matches. Set the FileEntry.

An incremental patch you could try would be:
```
lang=c++
if (Status.getName() == Filename || !Status.IsVFSMapped)
```
... dropping all the other changes.

This limits the redirection hack to only apply when a RedirectingFS is involved 
(leaving until later the fine-tuning of when `IsVFSMapped` gets set). If this 
smaller change still causes a test failure, it might be easier to reason about 
why / how to fix it / be sure that the fix is sound.

Eventually we might want something like:
```
lang=c++
if (!Status.IsVFSMapped) {
  assert(Status.getName() == Filename);
```
I imagine that's not going to succeed yet due to the CWD behaviour in the 
VFSes, but as a speculative patch it might help track down whatever the problem 
is.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122549/new/

https://reviews.llvm.org/D122549

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122549: [VFS] RedirectingFileSystem only replace path if not already mapped

[Ben Barham via Phabricator via cfe-commits]

bnbarham added a comment.

In D122549#3412064 , @dexonsmith 
wrote:

> In D122549#3412021 , @bnbarham 
> wrote:
>
>> `clang-apply-replacements/relative-paths.cpp` is failing, I haven't looked 
>> into it but my guess would be that it's from the `Status.getName() == 
>> Filename` -> `!Status.IsVFSMapped` change. That seems very odd to me.
>
> Is it just failing on Windows? I wonder (rather speculatively...) whether 
> https://reviews.llvm.org/D121733 would help.

No, also debian. Not sure why it isn't saying debian failed in the review.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122549/new/

https://reviews.llvm.org/D122549

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122549: [VFS] RedirectingFileSystem only replace path if not already mapped

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D122549#3412021 , @bnbarham wrote:

> `clang-apply-replacements/relative-paths.cpp` is failing, I haven't looked 
> into it but my guess would be that it's from the `Status.getName() == 
> Filename` -> `!Status.IsVFSMapped` change. That seems very odd to me.

Is it just failing on Windows? I wonder (rather speculatively...) whether 
https://reviews.llvm.org/D121733 would help.




Comment at: clang/lib/Basic/FileManager.cpp:316-318
+// case above is removed. That one can be removed once we always return
+// virtual paths and anywhere that needs external paths specifically
+// requests them.

bnbarham wrote:
> dexonsmith wrote:
> > It's not obvious to me that the redirection case above depends on this 
> > logic sticking around.
> > - If that's what you mean, can you explain in more detail why the 
> > redirection above depends on this `UFE.Dir` logic?
> > - If you're just talking about `IsVFSMapped` having to stick around, I 
> > think that part should be covered by the comment for the redirection.
> I actually meant the reverse - the UFE.Dir logic being removed depends on the 
> redirection logic above being removed. That's because for this to be removed, 
> anywhere that cares about the requested path needs to be changed to use Refs. 
> But even if they change to use Refs, the redirection logic above would itself 
> replace the path with the external one. So it needs to be removed as well.
Okay, I suggest just making the link between them more clear.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122549/new/

https://reviews.llvm.org/D122549

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122549: [VFS] RedirectingFileSystem only replace path if not already mapped

[Ben Barham via Phabricator via cfe-commits]

bnbarham added a comment.

`clang-apply-replacements/relative-paths.cpp` is failing, I haven't looked into 
it but my guess would be that it's from the `Status.getName() == Filename` -> 
`!Status.IsVFSMapped` change. That seems very odd to me.




Comment at: clang/lib/Basic/FileManager.cpp:287-289
 // name-as-accessed on the \a FileEntryRef. Maybe the returned \a
 // FileEntryRef::getName() could return the accessed name unmodified, but
 // make the external name available via a separate API.

dexonsmith wrote:
> I'd suggest updating this text to match the new FIXME next to `IsVFSMapped` 
> (instead of "maybe", list the plan).
Will do



Comment at: clang/lib/Basic/FileManager.cpp:316-318
+// case above is removed. That one can be removed once we always return
+// virtual paths and anywhere that needs external paths specifically
+// requests them.

dexonsmith wrote:
> It's not obvious to me that the redirection case above depends on this logic 
> sticking around.
> - If that's what you mean, can you explain in more detail why the redirection 
> above depends on this `UFE.Dir` logic?
> - If you're just talking about `IsVFSMapped` having to stick around, I think 
> that part should be covered by the comment for the redirection.
I actually meant the reverse - the UFE.Dir logic being removed depends on the 
redirection logic above being removed. That's because for this to be removed, 
anywhere that cares about the requested path needs to be changed to use Refs. 
But even if they change to use Refs, the redirection logic above would itself 
replace the path with the external one. So it needs to be removed as well.



Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:58-63
+  /// Whether the path in this status has been mapped by a VFS to another path.
   bool IsVFSMapped = false;
+  // Note: Currently used by a hack in \c FileManager and internally in
+  // \c RedirectingFileSystem. We should change this to "HasVFSMapping" and
+  // *always* return the virtual path, only providing the mapped/external path
+  // when requested.

dexonsmith wrote:
> I think "mapped to another path" has a broader definition in my mind than 
> you're using here. IMO, all things in a redirecting filesystem map from one 
> path to another. The question is whether the mapping is leaked/available due 
> to use-external-names.
> 
> I think this comment be more precise, and the note should be a FIXME that's 
> part of the main comment, not something that comes later.
> 
> I also think it good to change the field name ~now (or in an immediately 
> following NFC patch) to reflect the new meaning. I like "exposes" or "leaks" 
> because that is clear that this indicates whether information about the 
> mapping is available / exposed in the abstraction, whereas "mapped" and 
> "mapping" sound to me like they're just talking about whether something was 
> redirected. And I like the word "external" because it ties back to 
> use-external-names.
> 
> Suggested text (feel free to change entirely, but I think it covers a couple 
> of important points):
> ```
> /// Whether this entity has an external path different from the virtual path, 
> and
> /// the external path is exposed by leaking it through the abstraction. For 
> example,
> /// a RedirectingFileSystem will set this for paths where UseExternalName is 
> true.
> ///
> /// FIXME: Currently the external path is exposed by replacing the virtual 
> path
> /// in this Status object. Instead, we should leave the path in the Status 
> intact
> /// (matching the requested virtual path), and just use this flag as hint 
> that a
> /// new API, FileSystem::getExternalVFSPath(), will not return `None`. 
> Clients can
> /// then request the external path only where needed (e.g., for diagnostics, 
> or to
> /// report discovered dependencies to external tools).
> bool ExposesExternalVFSPath = false;
> ```
> 
> 
That's fair. I suppose I was thinking that without `use-external-names: true` 
then nothing should be aware that it is "mapped" and thus "mapped" == 
"external". But I can see how that could be confusing.

I also thought that renaming now would be a little odd since eg. 
"HasExternalPath" seems a little different to "the actual path is currently the 
external path" (which is what it is right now). But with the "exposes" naming I 
think this could still make sense (especially with the FIXME you have). So I'll 
rename  


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122549/new/

https://reviews.llvm.org/D122549

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122549: [VFS] RedirectingFileSystem only replace path if not already mapped

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

This looks nice!




Comment at: clang/lib/Basic/FileManager.cpp:287-289
 // name-as-accessed on the \a FileEntryRef. Maybe the returned \a
 // FileEntryRef::getName() could return the accessed name unmodified, but
 // make the external name available via a separate API.

I'd suggest updating this text to match the new FIXME next to `IsVFSMapped` 
(instead of "maybe", list the plan).



Comment at: clang/lib/Basic/FileManager.cpp:316-318
+// case above is removed. That one can be removed once we always return
+// virtual paths and anywhere that needs external paths specifically
+// requests them.

It's not obvious to me that the redirection case above depends on this logic 
sticking around.
- If that's what you mean, can you explain in more detail why the redirection 
above depends on this `UFE.Dir` logic?
- If you're just talking about `IsVFSMapped` having to stick around, I think 
that part should be covered by the comment for the redirection.



Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:58-63
+  /// Whether the path in this status has been mapped by a VFS to another path.
   bool IsVFSMapped = false;
+  // Note: Currently used by a hack in \c FileManager and internally in
+  // \c RedirectingFileSystem. We should change this to "HasVFSMapping" and
+  // *always* return the virtual path, only providing the mapped/external path
+  // when requested.

I think "mapped to another path" has a broader definition in my mind than 
you're using here. IMO, all things in a redirecting filesystem map from one 
path to another. The question is whether the mapping is leaked/available due to 
use-external-names.

I think this comment be more precise, and the note should be a FIXME that's 
part of the main comment, not something that comes later.

I also think it good to change the field name ~now (or in an immediately 
following NFC patch) to reflect the new meaning. I like "exposes" or "leaks" 
because that is clear that this indicates whether information about the mapping 
is available / exposed in the abstraction, whereas "mapped" and "mapping" sound 
to me like they're just talking about whether something was redirected. And I 
like the word "external" because it ties back to use-external-names.

Suggested text (feel free to change entirely, but I think it covers a couple of 
important points):
```
/// Whether this entity has an external path different from the virtual path, 
and
/// the external path is exposed by leaking it through the abstraction. For 
example,
/// a RedirectingFileSystem will set this for paths where UseExternalName is 
true.
///
/// FIXME: Currently the external path is exposed by replacing the virtual path
/// in this Status object. Instead, we should leave the path in the Status 
intact
/// (matching the requested virtual path), and just use this flag as hint that a
/// new API, FileSystem::getExternalVFSPath(), will not return `None`. Clients 
can
/// then request the external path only where needed (e.g., for diagnostics, or 
to
/// report discovered dependencies to external tools).
bool ExposesExternalVFSPath = false;
```




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122549/new/

https://reviews.llvm.org/D122549

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122549: [VFS] RedirectingFileSystem only replace path if not already mapped

[Ben Barham via Phabricator via cfe-commits]

bnbarham created this revision.
bnbarham added reviewers: dexonsmith, keith, JDevlieghere, vsapsai, sammccall.
Herald added a subscriber: hiraditya.
Herald added a project: All.
bnbarham requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

If the `ExternalFS` has already remapped a path then the
`RedirectingFileSystem` should not change it to the originally provided
path. This fixes the original path always being used if multiple VFS
overlays were provided and the path wasn't found in the highest (ie.
first in the chain).

This also changes `IsVFSMapped` to mean the returned path is a *mapped*
path, rather than just from a virtual filesystem. But it was only being used
by a hack in `FileManager` for module searching, where `external-names: true`
is always used.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122549

Files:
  clang/lib/Basic/FileManager.cpp
  clang/test/VFS/external-names-multi-overlay.c
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp
  llvm/unittests/Support/VirtualFileSystemTest.cpp

Index: llvm/unittests/Support/VirtualFileSystemTest.cpp
===
--- llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -1478,16 +1478,16 @@
   // file in remapped directory
   S = O->status("//root/mappeddir/a");
   ASSERT_FALSE(S.getError());
-  ASSERT_FALSE(S->isDirectory());
-  ASSERT_TRUE(S->IsVFSMapped);
-  ASSERT_EQ("//root/foo/bar/a", S->getName());
+  EXPECT_FALSE(S->isDirectory());
+  EXPECT_TRUE(S->IsVFSMapped);
+  EXPECT_EQ("//root/foo/bar/a", S->getName());
 
   // file in remapped directory, with use-external-name=false
   S = O->status("//root/mappeddir2/a");
   ASSERT_FALSE(S.getError());
-  ASSERT_FALSE(S->isDirectory());
-  ASSERT_TRUE(S->IsVFSMapped);
-  ASSERT_EQ("//root/mappeddir2/a", S->getName());
+  EXPECT_FALSE(S->isDirectory());
+  EXPECT_FALSE(S->IsVFSMapped);
+  EXPECT_EQ("//root/mappeddir2/a", S->getName());
 
   // file contents in remapped directory
   OpenedF = O->openFileForRead("//root/mappeddir/a");
@@ -1503,7 +1503,7 @@
   OpenedS = (*OpenedF)->status();
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("//root/mappeddir2/a", OpenedS->getName());
-  EXPECT_TRUE(OpenedS->IsVFSMapped);
+  EXPECT_FALSE(OpenedS->IsVFSMapped);
 
   // broken mapping
   EXPECT_EQ(O->status("//root/file2").getError(),
@@ -1776,12 +1776,12 @@
   auto OpenedS = (*OpenedF)->status();
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("vfsname", OpenedS->getName());
-  EXPECT_TRUE(OpenedS->IsVFSMapped);
+  EXPECT_FALSE(OpenedS->IsVFSMapped);
 
   auto DirectS = FS->status("vfsname");
   ASSERT_FALSE(DirectS.getError());
   EXPECT_EQ("vfsname", DirectS->getName());
-  EXPECT_TRUE(DirectS->IsVFSMapped);
+  EXPECT_FALSE(DirectS->IsVFSMapped);
 
   EXPECT_EQ(0, NumDiagnostics);
 }
Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -2163,10 +2163,16 @@
 static Status getRedirectedFileStatus(const Twine ,
   bool UseExternalNames,
   Status ExternalStatus) {
+  // The path has been mapped by some nested VFS, don't override it with the
+  // original path.
+  if (ExternalStatus.IsVFSMapped)
+return ExternalStatus;
+
   Status S = ExternalStatus;
   if (!UseExternalNames)
 S = Status::copyWithNewName(S, OriginalPath);
-  S.IsVFSMapped = true;
+  else
+S.IsVFSMapped = true;
   return S;
 }
 
@@ -2268,7 +2274,9 @@
 
 ErrorOr>
 File::getWithPath(ErrorOr> Result, const Twine ) {
-  if (!Result)
+  // See \c getRedirectedFileStatus - don't update path if it's already been
+  // mapped.
+  if (!Result || (*Result)->status()->IsVFSMapped)
 return Result;
 
   ErrorOr> F = std::move(*Result);
Index: llvm/include/llvm/Support/VirtualFileSystem.h
===
--- llvm/include/llvm/Support/VirtualFileSystem.h
+++ llvm/include/llvm/Support/VirtualFileSystem.h
@@ -55,8 +55,12 @@
   llvm::sys::fs::perms Perms;
 
 public:
-  // FIXME: remove when files support multiple names
+  /// Whether the path in this status has been mapped by a VFS to another path.
   bool IsVFSMapped = false;
+  // Note: Currently used by a hack in \c FileManager and internally in
+  // \c RedirectingFileSystem. We should change this to "HasVFSMapping" and
+  // *always* return the virtual path, only providing the mapped/external path
+  // when requested.
 
   Status() = default;
   Status(const llvm::sys::fs::file_status );
Index: clang/test/VFS/external-names-multi-overlay.c
===
--- /dev/null
+++