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
