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

[Richard Smith]

On Fri, Dec 12, 2014 at 12:59 PM, Justin Bogner <mail at justinbogner.com>
wrote:

> 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.
>

Thanks, that sounds like a good start. This seems like a very tricky
problem, but it also seems important to get it right; directory symlinks in
include directories are not uncommon.

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.
>

Aha, yes, that was an entirely unintentional change. (Is that actually a
problem, or was it just something the tests weren't prepared for?)

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


Sounds like a good idea. Adding a call to llvm::sys::path::native should do
the trick. Thanks for figuring this out!


> > 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).
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141212/63471b3c/attachment.html>