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