On Fri, Dec 12, 2014 at 12:59 PM, Justin Bogner <[email protected]> wrote:
> Richard Smith <[email protected]> writes: > > On 12 Dec 2014 11:24, "Justin Bogner" <[email protected]> wrote: > >> > >> Reid Kleckner <[email protected]> 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). >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
