[DNM][VFS] Do not overwrite the path when nesting RedirectingFileSystems

[bnbarham]

Summary

86e2af8043c7728710a711b623f27425801a36c3 aimed to preserve the original
relative path when falling back to the external filesystem. But when
there's nested RedirectingFileSystems this results in the outer-most
VFS overwriting the path of a lower VFS.

For example, take a directory remapping overlay of A -> B and another
of B -> C where both have use-external-names set. If A/foo is
requested this will map A/foo to B/foo to C/foo but when this
result returns to the A -> B VFS, it will then remap the Status and
File to B/foo instead.

This is only a partial fix - it fixes openFileForRead, but status
and getRealPath would need equivalent changes. I'm putting up this
partial PR to gather opinions on whether this seems reasonable first.

[bnbarham]

Also have bug up at https://github.com/llvm/llvm-project/issues/53306.

[dexonsmith]

@keith, any thoughts on this? IIRC, you'd found a few other bugs while you were in there which you'd left for later; not sure if this is related to them or not.

clang/test/VFS/directory.c

42

This makes it hard to read what has changed. Probably better to commit an NFC patch (updating the testcase with no behaviour change) and then rebase this patch on top.

llvm/unittests/Support/VirtualFileSystemTest.cpp

2715–2717

Remapping in two directions seems like an extra layer of complexity; is that the case where this came up, or was it something more straightforward?

[bnbarham]

clang/test/VFS/directory.c

42

Yep, to be clear this is very much a WIP PR - I mostly put it up to get some feedback. I expect there to be a fair bit to clean up.

The only thing actually *changed* here is the addition of checking the path that was written out as well. The rest was to help me understand what was actually going on originally, I'll probably just revert and just add the path if we do end up wanting this (unless people prefer split-file).

llvm/unittests/Support/VirtualFileSystemTest.cpp

2715–2717

It shows up in A -> B -> C (as in the non-unit test) so this one could test that instead. I did this when originally reproducing the issue and only figured out after that it was just due to nesting in general.

The problem is nested RedirectingFileSystems, where the outermost now overwrites the paths written by the nested FS's.

[dexonsmith]

clang/test/VFS/directory.c

42

Just that it's hard to read the WIP PR with the noise of the unrelated change; if the testcase is easier to read with split-file seems like it'd be nice to land the improvement, but up to you.

[bnbarham]

Modified directory.c to make the test difference clearer.

[bnbarham]

clang/test/VFS/directory.c

42

Updated to remove the (large amount of) noise.

[keith]

In D117730#3262622, @dexonsmith wrote:

@keith, any thoughts on this? IIRC, you'd found a few other bugs while you were in there which you'd left for later; not sure if this is related to them or not.

This issue wasn't one of the ones on my radar, but in general this change seems safe to me

[bnbarham]

Rather than trying to fix nested RedirectingFileSystems, I've instead put up a bunch of patches to simplify RedirectingFileSystem. This does mean we can't "chain" remappings any more, but that seems like a good change to me. See:
https://reviews.llvm.org/D121421
https://reviews.llvm.org/D121423
https://reviews.llvm.org/D121424
https://reviews.llvm.org/D121425
https://reviews.llvm.org/D121426