[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 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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<vfs::InMemoryFileSystem> 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