Page MenuHomePhabricator

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

Authored by bnbarham on Jan 19 2022, 4:30 PM.

Details

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.

Diff Detail

Event Timeline

bnbarham created this revision.Jan 19 2022, 4:30 PM
bnbarham requested review of this revision.Jan 19 2022, 4:30 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 19 2022, 4:30 PM

@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 added inline comments.Jan 21 2022, 2:48 PM
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 added inline comments.Jan 21 2022, 3:02 PM
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 updated this revision to Diff 402130.Jan 21 2022, 3:59 PM

Modified directory.c to make the test difference clearer.

bnbarham added inline comments.Jan 21 2022, 4:01 PM
clang/test/VFS/directory.c
42

Updated to remove the (large amount of) noise.

@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 abandoned this revision.Mar 10 2022, 4:05 PM

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

Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2022, 4:05 PM