r269100 - [VFS] Reconstruct the VFS overlay tree for more accurate lookup

[Bruno Cardoso Lopes via cfe-commits]

Hi Sean,

> So sorry! It had gone quiet for a few hours and I wanted to avoid leaving it
> red any longer.

No problem, the priority is to get bot greens!

> Does any platform define a guaranteed directory iteration order? I feel like
> depending on it in the first place is unwise (or maybe I'm misunderstanding?
> e.g. I'm thinking that we should sort the results of directory iteration to
> guarantee a particular order if needed for a comparison in a test)

Yes, I was thinking about that too, however I'm not yet sure this is
actually happening due to iteration order.

>>
>> Or can you help me debug it once I land it again?
>
>
> Sure, but like I mentioned the nature of this error is very troubling to me
> (high-level API use running into low-level system issues). It suggests that
> there is an incorrect layering or something. I hate these annoying windows
> path issues as much as everyone else, so I'd rather we focus energy on
> building an appropriate layering to consolidate all the special handling in
> one place than to continually band-aid issues as they creep into all parts
> of the codebase. VirtualFileSystem.cpp already has quite a bit of technical
> debt that violates our policies in this regard (platform-specific #ifdef's
> outside libSupport; I count 3 in VirtualFileSystem.cpp which is a huge
> number considering how recent that code is).

Totally agree. Part of that is on me - I'm actually relying on
http://reviews.llvm.org/D19842 as a base to do a proper clean up by
using a target independent version of realpath. FileManager is likely
benefiting from that as well (and has itself some layering violations
I took precedence from). However, I'm not even sure what are the root
causes that make stuff fails on windows, given that, it's hard to know
what proper layering should look like, because aside windows-avoid
hacks, the rest looks fine to me; I count on you then to help me with
that, feel free to file PRs about anything specific :)

>>
>> I might
>> check-in the new tests separately from the rest to make it easier to
>> revert.
>
>
> I don't think there's precedent for doing this in LLVM (or at least it would
> be considered as bad practice). We typically check in tests alongside the
> code being tested specifically to avoid the temptation to leave
> untested/broken code in tree.

True. However it's kinda subtle in this case: (1) this patch is not
really a feature, it just re-arranges the tree to allow faster search,
so the current tests passing should be enough. OTOH, the side-effect
is that it fixes directory iteration because different roots pointing
to the same path now can be seen as the same by directory iteration. I
had the intention to test that, to make sure we don't regress in the
future. (2) I have no other way to "debug" on windows10-ps4 platform
besides reducing-by-commit. Anyway, I'm gonna try a more minimal
testcase first.

-- 
Bruno Cardoso Lopes
http://www.brunocardoso.cc