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

Duncan P. N. Exon Smith via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Nov 12 21:41:02 PST 2021

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

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list