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

[Duncan P. N. Exon Smith via Phabricator via llvm-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