<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Dec 12, 2014 at 12:59 PM, Justin Bogner <span dir="ltr"><<a href="mailto:mail@justinbogner.com" target="_blank">mail@justinbogner.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> writes:<br>
> On 12 Dec 2014 11:24, "Justin Bogner" <<a href="mailto:mail@justinbogner.com">mail@justinbogner.com</a>> wrote:<br>
>><br>
>> Reid Kleckner <<a href="mailto:reid@kleckner.net">reid@kleckner.net</a>> writes:<br>
>> > Author: rnk<br>
>> > Date: Fri Dec 12 13:13:04 2014<br>
>> > New Revision: 224145<br>
>> ><br>
>> > URL: <a href="http://llvm.org/viewvc/llvm-project?rev=224145&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=224145&view=rev</a><br>
>> > Log:<br>
>> > Allow module deps to be printed in an arbitrary order<br>
>> ><br>
>> > The order is different between Windows and Unix for reasons unknown, but<br>
>> > the compiler output appears to still be determinstic.<br>
>><br>
>> I don't think this is right. It looks to me like r224055 changed<br>
>> ModuleDependencyListener's semantics unintentionally. I'll look into it.<br>
><br>
> Thankyou!<br>
><br>
> I think the change you're referring to is that it removed the pruning of /x/..<br>
> /. That was not unintentional - there were no tests for that behaviour, and<br>
> it's wrong if x is a symlink - but I had intended to investigate further prior<br>
> to checkin and forgot.<br>
<br>
</span>Yep, that's what I was looking at. I agree that the behaviour is a bit<br>
iffy, but if such a path were to survive to the VFS writer we'd assert,<br>
as it doesn't support path traversal. I'm looking into making a test<br>
that actually triggers that path so I can figure out what to do with it.<br></blockquote><div><br></div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
In any case, after looking a bit, the ".." change is not why these tests<br>
started failing. The output order changed here because removeDotPaths<br>
also improved on removePathTraversal by returning early if there are no<br>
changes. If removeDotPaths changes anything, the path separators are<br>
canonicalized and all end up being \\ on windows, but if it doesn't<br>
change anything some of the separators may be /, since that's how they<br>
were passed on the commandline.<br></blockquote><div><br></div><div>Aha, yes, that was an entirely unintentional change. (Is that actually a problem, or was it just something the tests weren't prepared for?)</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I'm not entirely sure how best to handle that. Maybe<br>
ModuleDependencyListener should canonicalize path separators as a<br>
separate step?</blockquote><div><br></div><div>Sounds like a good idea. Adding a call to llvm::sys::path::native should do the trick. Thanks for figuring this out!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">
> You should be about to revert the changes to ModuleDependecyCollector.cpp to<br>
> get back the old behaviour, but that behaviour seems to be wrong (and will<br>
> probably assert in some cases).<br>
</div></div></blockquote></div><br></div></div>