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

[Keith Smiley 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 rGc972175649f4: [VFS] Use original path when falling back to 
external FS (authored by keith).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

Files:
  clang/test/VFS/relative-path-errors.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
@@ -1627,6 +1627,114 @@
   EXPECT_EQ(0, NumDiagnostics);
 }
 
+TEST_F(VFSFromYAMLTest, ReturnsRequestedPathVFSMiss) {
+  IntrusiveRefCntPtr 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 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 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 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 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 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 Lower(new DummyFileSystem());
   

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

[Keith Smiley via Phabricator via cfe-commits]

keith updated this revision to Diff 387030.
keith added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

Files:
  clang/test/VFS/relative-path-errors.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
@@ -1627,6 +1627,114 @@
   EXPECT_EQ(0, NumDiagnostics);
 }
 
+TEST_F(VFSFromYAMLTest, ReturnsRequestedPathVFSMiss) {
+  IntrusiveRefCntPtr 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 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 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 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 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 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 Lower(new DummyFileSystem());
   Lower->addRegularFile("//root/foo/bar/a");
Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- 

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

[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.

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.)




Comment at: llvm/unittests/Support/VirtualFileSystemTest.cpp:1738
+
+TEST_F(VFSFromYAMLTest, RelativePathHitWithoutCWD) {
+  IntrusiveRefCntPtr BaseFS(

keith wrote:
> 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?
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).



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

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

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

[Keith Smiley via Phabricator via cfe-commits]

keith added a comment.

@JDevlieghere can you take another pass?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

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

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

[Keith Smiley via Phabricator via cfe-commits]

keith added a comment.

@dexonsmith can you take another look?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

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

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

[Keith Smiley via Phabricator via cfe-commits]

keith added a comment.

@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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

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

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

[Keith Smiley via Phabricator via cfe-commits]

keith updated this revision to Diff 383406.
keith added a comment.

Remove broken test for now

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

Files:
  clang/test/VFS/relative-path-errors.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
@@ -1627,6 +1627,114 @@
   EXPECT_EQ(0, NumDiagnostics);
 }
 
+TEST_F(VFSFromYAMLTest, ReturnsRequestedPathVFSMiss) {
+  IntrusiveRefCntPtr 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 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 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 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 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 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 Lower(new DummyFileSystem());
   Lower->addRegularFile("//root/foo/bar/a");
Index: 

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

[Keith Smiley via Phabricator via cfe-commits]

keith added inline comments.



Comment at: llvm/unittests/Support/VirtualFileSystemTest.cpp:1738
+
+TEST_F(VFSFromYAMLTest, RelativePathHitWithoutCWD) {
+  IntrusiveRefCntPtr BaseFS(

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

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

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

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

dexonsmith added inline comments.



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1179-1180
 
+  if (ExternalFS)
+ExternalFS->setCurrentWorkingDirectory(Path);
+

keith wrote:
> dexonsmith wrote:
> > keith wrote:
> > > JDevlieghere wrote:
> > > > I'm pretty sure there was a reason we stopped doing this. There should 
> > > > be some discussion about that in my original patch. 
> > > 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?
> > 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.
> > 
> 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.
> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

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

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

[Keith Smiley via Phabricator via cfe-commits]

keith updated this revision to Diff 380821.
keith added a comment.

Fix test paths on windows

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

Files:
  clang/test/VFS/relative-path-errors.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
@@ -1627,6 +1627,153 @@
   EXPECT_EQ(0, NumDiagnostics);
 }
 
+TEST_F(VFSFromYAMLTest, ReturnsRequestedPathVFSMiss) {
+  IntrusiveRefCntPtr 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 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 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 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 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 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, RelativePathHitWithoutCWD) {
+  IntrusiveRefCntPtr BaseFS(
+  new vfs::InMemoryFileSystem);
+  BaseFS->addFile("//root/foo/realname", 0,
+  

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

[Keith Smiley via Phabricator via cfe-commits]

keith added inline comments.



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1179-1180
 
+  if (ExternalFS)
+ExternalFS->setCurrentWorkingDirectory(Path);
+

dexonsmith wrote:
> keith wrote:
> > JDevlieghere wrote:
> > > I'm pretty sure there was a reason we stopped doing this. There should be 
> > > some discussion about that in my original patch. 
> > 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?
> 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.
> 
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

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

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

[Keith Smiley via Phabricator via cfe-commits]

keith updated this revision to Diff 380802.
keith marked 2 inline comments as done.
keith added a comment.

Remove `cd` of ExternalFS


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

Files:
  clang/test/VFS/relative-path-errors.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
@@ -1627,6 +1627,153 @@
   EXPECT_EQ(0, NumDiagnostics);
 }
 
+TEST_F(VFSFromYAMLTest, ReturnsRequestedPathVFSMiss) {
+  IntrusiveRefCntPtr 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 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 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 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 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 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, RelativePathHitWithoutCWD) {
+  IntrusiveRefCntPtr BaseFS(
+  new vfs::InMemoryFileSystem);
+  BaseFS->addFile("//root/foo/realname", 0,
+  MemoryBuffer::getMemBuffer("contents 

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

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

dexonsmith added inline comments.



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1179-1180
 
+  if (ExternalFS)
+ExternalFS->setCurrentWorkingDirectory(Path);
+

keith wrote:
> JDevlieghere wrote:
> > I'm pretty sure there was a reason we stopped doing this. There should be 
> > some discussion about that in my original patch. 
> 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?
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

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

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

[Keith Smiley via Phabricator via cfe-commits]

keith updated this revision to Diff 380125.
keith added a comment.

Improve lit test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

Files:
  clang/test/VFS/relative-path-errors.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
@@ -1627,6 +1627,114 @@
   EXPECT_EQ(0, NumDiagnostics);
 }
 
+TEST_F(VFSFromYAMLTest, ReturnsRequestedPathVFSMiss) {
+  IntrusiveRefCntPtr 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 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 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 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 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 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 Lower(new DummyFileSystem());
   Lower->addRegularFile("//root/foo/bar/a");
Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- 

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

[Keith Smiley via Phabricator via cfe-commits]

keith updated this revision to Diff 379882.
keith added a comment.

Fix format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

Files:
  clang/test/VFS/relative-path-errors.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
@@ -1627,6 +1627,114 @@
   EXPECT_EQ(0, NumDiagnostics);
 }
 
+TEST_F(VFSFromYAMLTest, ReturnsRequestedPathVFSMiss) {
+  IntrusiveRefCntPtr 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 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 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 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 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 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 Lower(new DummyFileSystem());
   Lower->addRegularFile("//root/foo/bar/a");
Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- 

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

[Keith Smiley via Phabricator via cfe-commits]

keith updated this revision to Diff 379881.
keith marked an inline comment as done.
keith added a comment.

Extract fallback status logic to another function


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

Files:
  clang/test/VFS/relative-path-errors.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
@@ -1627,6 +1627,114 @@
   EXPECT_EQ(0, NumDiagnostics);
 }
 
+TEST_F(VFSFromYAMLTest, ReturnsRequestedPathVFSMiss) {
+  IntrusiveRefCntPtr 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 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 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 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 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 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 Lower(new DummyFileSystem());
   Lower->addRegularFile("//root/foo/bar/a");
Index: llvm/lib/Support/VirtualFileSystem.cpp

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

[Keith Smiley via Phabricator via cfe-commits]

keith added inline comments.



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1179-1180
 
+  if (ExternalFS)
+ExternalFS->setCurrentWorkingDirectory(Path);
+

JDevlieghere wrote:
> I'm pretty sure there was a reason we stopped doing this. There should be 
> some discussion about that in my original patch. 
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

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

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

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1179-1180
 
+  if (ExternalFS)
+ExternalFS->setCurrentWorkingDirectory(Path);
+

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



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:2012
+  if (auto Result = ExternalFS->status(CanonicalPath)) {
+return Result.get().copyWithNewName(Result.get(), OriginalPath);
+  } else {

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? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

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

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

[Keith Smiley via Phabricator via cfe-commits]

keith added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

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

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

[Keith Smiley via Phabricator via cfe-commits]

keith updated this revision to Diff 379559.
keith marked 4 inline comments as done.
keith added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

Files:
  clang/test/VFS/relative-path-errors.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
@@ -1627,6 +1627,114 @@
   EXPECT_EQ(0, NumDiagnostics);
 }
 
+TEST_F(VFSFromYAMLTest, ReturnsRequestedPathVFSMiss) {
+  IntrusiveRefCntPtr 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 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 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 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 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 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 Lower(new DummyFileSystem());
   Lower->addRegularFile("//root/foo/bar/a");
Index: 

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

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

dexonsmith added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

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

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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

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

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

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

dexonsmith added a comment.

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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

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

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

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

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

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

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

dexonsmith added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

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

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

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

dexonsmith added a comment.



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 getWithPath(std::unique_ptr F,
 const Twine );
  
  protected:
// API for mutating the path. Override to avoid needing a wrapper
// object for File::getWithPath.
virtual bool setPath(const Twine ) { return false; }
  };

then getWithPath can be:

  namespace {
  class WrapperFile : public File { /* Change the observed path */ };
  }
  
  std::unique_ptr File::getWithPath(std::unique_ptr F, const Twine 
) {
if (F->setPath(P))
  return F;
return std::make_unique(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?




Comment at: llvm/lib/Support/VirtualFileSystem.cpp:2025-2028
 ErrorOr>
-RedirectingFileSystem::openFileForRead(const Twine _) {
-  SmallString<256> Path;
-  Path_.toVector(Path);
+RedirectingFileSystem::openFileForRead(const Twine ) {
+  SmallString<256> CanonicalPath;
+  Path.toVector(CanonicalPath);

With the new comment, I think it'd be useful to have SmallString versions of 
path the original and canonical paths. In which case:
```
lang=c++
SmallString<256> Path;
Path_.toVector(Path);
SmallString<256> CanonicalPath = Path;
```
(with the original `Path_` for the parameter) seems cleanest?



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:2036-2040
+  auto Result = ExternalFS->openFileForRead(Path);
+  if (!Result.getError())
+return Result;
+
+  return ExternalFS->openFileForRead(CanonicalPath);

I think the correct logic here is something like:
```
lang=c++
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.



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:2053-2059
+if (shouldFallBackToExternalFS(ExternalFile.getError(), Result->E)) {
+  auto Result = ExternalFS->openFileForRead(Path);
+  if (!Result.getError())
+return Result;
+
+  return ExternalFS->openFileForRead(CanonicalPath);
+}

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



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:2069
   Status S = getRedirectedFileStatus(
-  Path, RE->useExternalName(UseExternalNames), *ExternalStatus);
+  CanonicalPath, RE->useExternalName(UseExternalNames), 

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

[Keith Smiley via Phabricator via cfe-commits]

keith added a comment.

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());


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

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

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

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

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

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

[Keith Smiley via Phabricator via cfe-commits]

keith updated this revision to Diff 372041.
keith added a comment.

Handle all cases by passing relative path to ExternalFS first


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

Files:
  clang/test/VFS/relative-path-errors.c
  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
@@ -1607,6 +1607,26 @@
   EXPECT_EQ(0, NumDiagnostics);
 }
 
+TEST_F(VFSFromYAMLTest, RelativePathFallthrough) {
+  IntrusiveRefCntPtr 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 Name = (*OpenedF)->getName();
+  ASSERT_FALSE(Name.getError());
+  auto OpenedS = (*OpenedF)->status();
+  ASSERT_FALSE(OpenedS.getError());
+  EXPECT_EQ("a", Name.get());
+  EXPECT_EQ("a", OpenedS->getName());
+  EXPECT_FALSE(OpenedS->IsVFSMapped);
+  EXPECT_EQ(0, NumDiagnostics);
+}
+
 TEST_F(VFSFromYAMLTest, CaseInsensitive) {
   IntrusiveRefCntPtr Lower(new DummyFileSystem());
   Lower->addRegularFile("//root/foo/bar/a");
Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1141,6 +1141,9 @@
   if (!exists(Path))
 return errc::no_such_file_or_directory;
 
+  if (ExternalFS)
+ExternalFS->setCurrentWorkingDirectory(Path);
+
   SmallString<128> AbsolutePath;
   Path.toVector(AbsolutePath);
   if (std::error_code EC = makeAbsolute(AbsolutePath))
@@ -1149,15 +1152,17 @@
   return {};
 }
 
-std::error_code RedirectingFileSystem::isLocal(const Twine _,
+std::error_code RedirectingFileSystem::isLocal(const Twine ,
bool ) {
-  SmallString<256> Path;
-  Path_.toVector(Path);
+  if (!ExternalFS->isLocal(Path, Result))
+return {};
 
-  if (std::error_code EC = makeCanonical(Path))
+  SmallString<256> CanonicalPath;
+  Path.toVector(CanonicalPath);
+  if (std::error_code EC = makeCanonical(CanonicalPath))
 return {};
 
-  return ExternalFS->isLocal(Path, Result);
+  return ExternalFS->isLocal(CanonicalPath, Result);
 }
 
 std::error_code RedirectingFileSystem::makeAbsolute(SmallVectorImpl ) const {
@@ -1191,23 +1196,28 @@
 
 directory_iterator RedirectingFileSystem::dir_begin(const Twine ,
 std::error_code ) {
-  SmallString<256> Path;
-  Dir.toVector(Path);
+  SmallString<256> CanonicalPath;
+  Dir.toVector(CanonicalPath);
 
-  EC = makeCanonical(Path);
+  EC = makeCanonical(CanonicalPath);
   if (EC)
 return {};
 
-  ErrorOr Result = lookupPath(Path);
+  ErrorOr Result = lookupPath(CanonicalPath);
   if (!Result) {
 EC = Result.getError();
-if (shouldFallBackToExternalFS(EC))
-  return ExternalFS->dir_begin(Path, EC);
+if (shouldFallBackToExternalFS(EC)) {
+  directory_iterator Result = ExternalFS->dir_begin(Dir, EC);
+  if (!EC)
+return Result;
+
+  return ExternalFS->dir_begin(CanonicalPath, EC);
+}
 return {};
   }
 
   // Use status to make sure the path exists and refers to a directory.
-  ErrorOr S = status(Path, *Result);
+  ErrorOr S = status(CanonicalPath, *Result);
   if (!S) {
 if (shouldFallBackToExternalFS(S.getError(), Result->E))
   return ExternalFS->dir_begin(Dir, EC);
@@ -1231,18 +1241,18 @@
   // Update the paths in the results to use the virtual directory's path.
   DirIter =
   directory_iterator(std::make_shared(
-  std::string(Path), DirIter));
+  std::string(CanonicalPath), DirIter));
 }
   } else {
 auto DE = cast(Result->E);
 DirIter = directory_iterator(std::make_shared(
-Path, DE->contents_begin(), DE->contents_end(), EC));
+CanonicalPath, DE->contents_begin(), DE->contents_end(), EC));
   }
 
   if (!shouldUseExternalFS())
 return DirIter;
   return directory_iterator(std::make_shared(
-  DirIter, ExternalFS, std::string(Path), EC));
+  DirIter, ExternalFS, std::string(CanonicalPath), EC));
 }
 
 void RedirectingFileSystem::setExternalContentsPrefixDir(StringRef PrefixDir) {
@@ -1957,23 +1967,33 @@
   return Status::copyWithNewName(DE->getStatus(), Path);
 }
 
-ErrorOr RedirectingFileSystem::status(const Twine _) {
-  SmallString<256> Path;
-  Path_.toVector(Path);
+ErrorOr RedirectingFileSystem::status(const Twine ) {
+  

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

[Keith Smiley via Phabricator via cfe-commits]

keith added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

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

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

[Keith Smiley via Phabricator via cfe-commits]

keith added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

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

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

[Keith Smiley via Phabricator via cfe-commits]

keith added a comment.

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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

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

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

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

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`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

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

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

[Keith Smiley via Phabricator via cfe-commits]

keith updated this revision to Diff 370480.
keith marked 2 inline comments as done.
keith added a comment.

Update variable name and add unit test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

Files:
  clang/test/VFS/relative-path-errors.c
  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
@@ -1607,6 +1607,26 @@
   EXPECT_EQ(0, NumDiagnostics);
 }
 
+TEST_F(VFSFromYAMLTest, RelativePathFallthrough) {
+  IntrusiveRefCntPtr 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 Name = (*OpenedF)->getName();
+  ASSERT_FALSE(Name.getError());
+  auto OpenedS = (*OpenedF)->status();
+  ASSERT_FALSE(OpenedS.getError());
+  EXPECT_EQ("a", Name.get());
+  EXPECT_EQ("a", OpenedS->getName());
+  EXPECT_FALSE(OpenedS->IsVFSMapped);
+  EXPECT_EQ(0, NumDiagnostics);
+}
+
 TEST_F(VFSFromYAMLTest, CaseInsensitive) {
   IntrusiveRefCntPtr Lower(new DummyFileSystem());
   Lower->addRegularFile("//root/foo/bar/a");
Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -2003,14 +2003,14 @@
 } // namespace
 
 ErrorOr>
-RedirectingFileSystem::openFileForRead(const Twine _) {
-  SmallString<256> Path;
-  Path_.toVector(Path);
+RedirectingFileSystem::openFileForRead(const Twine ) {
+  SmallString<256> CanonicalPath;
+  Path.toVector(CanonicalPath);
 
-  if (std::error_code EC = makeCanonical(Path))
+  if (std::error_code EC = makeCanonical(CanonicalPath))
 return EC;
 
-  ErrorOr Result = lookupPath(Path);
+  ErrorOr Result = 
lookupPath(CanonicalPath);
   if (!Result) {
 if (shouldFallBackToExternalFS(Result.getError()))
   return ExternalFS->openFileForRead(Path);
@@ -2036,7 +2036,7 @@
 
   // FIXME: Update the status with the name and VFSMapped.
   Status S = getRedirectedFileStatus(
-  Path, RE->useExternalName(UseExternalNames), *ExternalStatus);
+  CanonicalPath, RE->useExternalName(UseExternalNames), *ExternalStatus);
   return std::unique_ptr(
   std::make_unique(std::move(*ExternalFile), S));
 }
Index: clang/test/VFS/relative-path-errors.c
===
--- /dev/null
+++ clang/test/VFS/relative-path-errors.c
@@ -0,0 +1,8 @@
+// RUN: mkdir -p %t
+// RUN: cd %t
+// RUN: echo '{"roots": [],"version": 0}' > %t.yaml
+// RUN: cp %s main.c
+// RUN: not %clang_cc1 -ivfsoverlay %t.yaml main.c 2>&1 | FileCheck %s
+
+// CHECK: {{^}}main.c:8:1: error:
+foobarbaz


Index: llvm/unittests/Support/VirtualFileSystemTest.cpp
===
--- llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -1607,6 +1607,26 @@
   EXPECT_EQ(0, NumDiagnostics);
 }
 
+TEST_F(VFSFromYAMLTest, RelativePathFallthrough) {
+  IntrusiveRefCntPtr 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 Name = (*OpenedF)->getName();
+  ASSERT_FALSE(Name.getError());
+  auto OpenedS = (*OpenedF)->status();
+  ASSERT_FALSE(OpenedS.getError());
+  EXPECT_EQ("a", Name.get());
+  EXPECT_EQ("a", OpenedS->getName());
+  EXPECT_FALSE(OpenedS->IsVFSMapped);
+  EXPECT_EQ(0, NumDiagnostics);
+}
+
 TEST_F(VFSFromYAMLTest, CaseInsensitive) {
   IntrusiveRefCntPtr Lower(new DummyFileSystem());
   Lower->addRegularFile("//root/foo/bar/a");
Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -2003,14 +2003,14 @@
 } // namespace
 
 ErrorOr>
-RedirectingFileSystem::openFileForRead(const Twine _) {
-  SmallString<256> Path;
-  Path_.toVector(Path);
+RedirectingFileSystem::openFileForRead(const Twine ) {
+  SmallString<256> CanonicalPath;
+  Path.toVector(CanonicalPath);
 
-  if (std::error_code EC = makeCanonical(Path))
+  if (std::error_code EC = 

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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

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.




Comment at: llvm/lib/Support/VirtualFileSystem.cpp:2006
 ErrorOr>
 RedirectingFileSystem::openFileForRead(const Twine _) {
   SmallString<256> Path;

dexonsmith wrote:
> I suggest giving this a real name if it's being used for anything. Maybe 
> `PathAsWritten`?
Another idea is to keep this variable `Path` and rename old Path to 
`CanonicalPath`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

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

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

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

dexonsmith added reviewers: vsapsai, dexonsmith.
dexonsmith requested changes to this revision.
dexonsmith added a subscriber: vsapsai.
dexonsmith added a comment.
This revision now requires changes to proceed.

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.




Comment at: llvm/lib/Support/VirtualFileSystem.cpp:2006
 ErrorOr>
 RedirectingFileSystem::openFileForRead(const Twine _) {
   SmallString<256> Path;

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

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

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

[Varun Gandhi via Phabricator via cfe-commits]

varungandhi-apple added a comment.

cc @JDevlieghere


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

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

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

[Keith Smiley via Phabricator via cfe-commits]

keith updated this revision to Diff 370155.
keith added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

Files:
  clang/test/VFS/relative-path-errors.c
  llvm/lib/Support/VirtualFileSystem.cpp


Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -2013,7 +2013,7 @@
   ErrorOr Result = lookupPath(Path);
   if (!Result) {
 if (shouldFallBackToExternalFS(Result.getError()))
-  return ExternalFS->openFileForRead(Path);
+  return ExternalFS->openFileForRead(Path_);
 return Result.getError();
   }
 
@@ -2026,7 +2026,7 @@
   auto ExternalFile = ExternalFS->openFileForRead(ExtRedirect);
   if (!ExternalFile) {
 if (shouldFallBackToExternalFS(ExternalFile.getError(), Result->E))
-  return ExternalFS->openFileForRead(Path);
+  return ExternalFS->openFileForRead(Path_);
 return ExternalFile;
   }
 
Index: clang/test/VFS/relative-path-errors.c
===
--- /dev/null
+++ clang/test/VFS/relative-path-errors.c
@@ -0,0 +1,8 @@
+// RUN: mkdir -p %t
+// RUN: cd %t
+// RUN: echo '{"roots": [],"version": 0}' > %t.yaml
+// RUN: cp %s main.c
+// RUN: not %clang_cc1 -ivfsoverlay %t.yaml main.c 2>&1 | FileCheck %s
+
+// CHECK: {{^}}main.c:8:1: error:
+foobarbaz


Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -2013,7 +2013,7 @@
   ErrorOr Result = lookupPath(Path);
   if (!Result) {
 if (shouldFallBackToExternalFS(Result.getError()))
-  return ExternalFS->openFileForRead(Path);
+  return ExternalFS->openFileForRead(Path_);
 return Result.getError();
   }
 
@@ -2026,7 +2026,7 @@
   auto ExternalFile = ExternalFS->openFileForRead(ExtRedirect);
   if (!ExternalFile) {
 if (shouldFallBackToExternalFS(ExternalFile.getError(), Result->E))
-  return ExternalFS->openFileForRead(Path);
+  return ExternalFS->openFileForRead(Path_);
 return ExternalFile;
   }
 
Index: clang/test/VFS/relative-path-errors.c
===
--- /dev/null
+++ clang/test/VFS/relative-path-errors.c
@@ -0,0 +1,8 @@
+// RUN: mkdir -p %t
+// RUN: cd %t
+// RUN: echo '{"roots": [],"version": 0}' > %t.yaml
+// RUN: cp %s main.c
+// RUN: not %clang_cc1 -ivfsoverlay %t.yaml main.c 2>&1 | FileCheck %s
+
+// CHECK: {{^}}main.c:8:1: error:
+foobarbaz
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Keith Smiley via Phabricator via cfe-commits]

keith created this revision.
keith added a reviewer: JDevlieghere.
Herald added subscribers: dexonsmith, hiraditya.
keith requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D109128

Files:
  clang/test/VFS/relative-path-errors.c
  llvm/lib/Support/VirtualFileSystem.cpp


Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1202,7 +1202,7 @@
   if (!Result) {
 EC = Result.getError();
 if (shouldFallBackToExternalFS(EC))
-  return ExternalFS->dir_begin(Path, EC);
+  return ExternalFS->dir_begin(Dir, EC);
 return {};
   }
 
@@ -1967,13 +1967,13 @@
   ErrorOr Result = lookupPath(Path);
   if (!Result) {
 if (shouldFallBackToExternalFS(Result.getError()))
-  return ExternalFS->status(Path);
+  return ExternalFS->status(Path_);
 return Result.getError();
   }
 
   ErrorOr S = status(Path, *Result);
   if (!S && shouldFallBackToExternalFS(S.getError(), Result->E))
-S = ExternalFS->status(Path);
+S = ExternalFS->status(Path_);
   return S;
 }
 
@@ -2013,7 +2013,7 @@
   ErrorOr Result = lookupPath(Path);
   if (!Result) {
 if (shouldFallBackToExternalFS(Result.getError()))
-  return ExternalFS->openFileForRead(Path);
+  return ExternalFS->openFileForRead(Path_);
 return Result.getError();
   }
 
@@ -2026,7 +2026,7 @@
   auto ExternalFile = ExternalFS->openFileForRead(ExtRedirect);
   if (!ExternalFile) {
 if (shouldFallBackToExternalFS(ExternalFile.getError(), Result->E))
-  return ExternalFS->openFileForRead(Path);
+  return ExternalFS->openFileForRead(Path_);
 return ExternalFile;
   }
 
@@ -2053,7 +2053,7 @@
   ErrorOr Result = lookupPath(Path);
   if (!Result) {
 if (shouldFallBackToExternalFS(Result.getError()))
-  return ExternalFS->getRealPath(Path, Output);
+  return ExternalFS->getRealPath(Path_, Output);
 return Result.getError();
   }
 
@@ -2062,14 +2062,14 @@
   if (auto ExtRedirect = Result->getExternalRedirect()) {
 auto P = ExternalFS->getRealPath(*ExtRedirect, Output);
 if (!P && shouldFallBackToExternalFS(P, Result->E)) {
-  return ExternalFS->getRealPath(Path, Output);
+  return ExternalFS->getRealPath(Path_, Output);
 }
 return P;
   }
 
   // If we found a DirectoryEntry, still fall back to ExternalFS if allowed,
   // because directories don't have a single external contents path.
-  return shouldUseExternalFS() ? ExternalFS->getRealPath(Path, Output)
+  return shouldUseExternalFS() ? ExternalFS->getRealPath(Path_, Output)
: llvm::errc::invalid_argument;
 }
 
Index: clang/test/VFS/relative-path-errors.c
===
--- /dev/null
+++ clang/test/VFS/relative-path-errors.c
@@ -0,0 +1,8 @@
+// RUN: mkdir -p %t
+// RUN: cd %t
+// RUN: echo '{"roots": [],"version": 0}' > %t.yaml
+// RUN: cp %s main.c
+// RUN: not %clang_cc1 -ivfsoverlay %t.yaml main.c 2>&1 | FileCheck %s
+
+// CHECK: {{^}}main.c:8:1: error:
+foobarbaz


Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1202,7 +1202,7 @@
   if (!Result) {
 EC = Result.getError();
 if (shouldFallBackToExternalFS(EC))
-  return ExternalFS->dir_begin(Path, EC);
+  return ExternalFS->dir_begin(Dir, EC);
 return {};
   }
 
@@ -1967,13 +1967,13 @@
   ErrorOr Result = lookupPath(Path);
   if (!Result) {
 if (shouldFallBackToExternalFS(Result.getError()))
-  return ExternalFS->status(Path);
+  return ExternalFS->status(Path_);
 return Result.getError();
   }
 
   ErrorOr S = status(Path, *Result);
   if (!S && shouldFallBackToExternalFS(S.getError(), Result->E))
-S = ExternalFS->status(Path);
+S = ExternalFS->status(Path_);
   return S;
 }
 
@@ -2013,7 +2013,7 @@
   ErrorOr Result = lookupPath(Path);
   if (!Result) {
 if (shouldFallBackToExternalFS(Result.getError()))
-  return ExternalFS->openFileForRead(Path);
+  return ExternalFS->openFileForRead(Path_);
 return Result.getError();
   }
 
@@ -2026,7 +2026,7 @@
   auto ExternalFile = ExternalFS->openFileForRead(ExtRedirect);
   if (!ExternalFile) {
 if (shouldFallBackToExternalFS(ExternalFile.getError(), Result->E))
-  return ExternalFS->openFileForRead(Path);
+  return