r224145 - Allow module deps to be printed in an arbitrary order

[Justin Bogner]

Richard Smith <richard at metafoo.co.uk> writes:
> On 12 Dec 2014 11:24, "Justin Bogner" <mail at justinbogner.com> wrote:
>>
>> Reid Kleckner <reid at kleckner.net> writes:
>> > Author: rnk
>> > Date: Fri Dec 12 13:13:04 2014
>> > New Revision: 224145
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=224145&view=rev
>> > Log:
>> > Allow module deps to be printed in an arbitrary order
>> >
>> > The order is different between Windows and Unix for reasons unknown, but
>> > the compiler output appears to still be determinstic.
>>
>> I don't think this is right. It looks to me like r224055 changed
>> ModuleDependencyListener's semantics unintentionally. I'll look into it.
>
> Thankyou!
>
> I think the change you're referring to is that it removed the pruning of /x/..
> /. That was not unintentional - there were no tests for that behaviour, and
> it's wrong if x is a symlink - but I had intended to investigate further prior
> to checkin and forgot.

Yep, that's what I was looking at. I agree that the behaviour is a bit
iffy, but if such a path were to survive to the VFS writer we'd assert,
as it doesn't support path traversal. I'm looking into making a test
that actually triggers that path so I can figure out what to do with it.

In any case, after looking a bit, the ".." change is not why these tests
started failing. The output order changed here because removeDotPaths
also improved on removePathTraversal by returning early if there are no
changes. If removeDotPaths changes anything, the path separators are
canonicalized and all end up being \\ on windows, but if it doesn't
change anything some of the separators may be /, since that's how they
were passed on the commandline.

I'm not entirely sure how best to handle that. Maybe
ModuleDependencyListener should canonicalize path separators as a
separate step?

> You should be about to revert the changes to ModuleDependecyCollector.cpp to
> get back the old behaviour, but that behaviour seems to be wrong (and will
> probably assert in some cases).