On Fri, Jun 2, 2017 at 3:22 PM, Richard Smith <rich...@metafoo.co.uk> wrote: > On 2 June 2017 at 12:04, Richard Smith <rich...@metafoo.co.uk> wrote: >> >> On 2 June 2017 at 11:39, Aaron Ballman via Phabricator via cfe-commits >> <cfe-commits@lists.llvm.org> wrote: >>> >>> aaron.ballman added a comment. >>> >>> In https://reviews.llvm.org/D33788#771671, @bruno wrote: >>> >>> > > I'm unaware of any other API that canonicalizes the path, which is >>> > > what users of this API are going to expect. >>> > >>> > You can also call `sys::path::remove_dots(Path, >>> > /*remove_dot_dot=*/true);` >>> >>> >>> Yes, but that does not canonicalize the path. For instance, it won't >>> handle doubled-up slashes or symlinks. >> >> >> That's at least partly a feature, not a bug. Resolving symlinks and >> removing x/.. are both potentially breaking changes when applied to the path >> -- if you canonicalize, you break installations where the file is actually a >> symlink to a file that does *not* also have a resource directory relative to >> it. >> >> The path with the bin/../lib in it is presumably the path from the clang >> binary, not the path from libclang, > > > I'm not sure about this any more. Looks like clang::Driver::Driver applies > parent_path to the .../bin directory instead of adding a .. component. So > maybe the bin/../lib path comes from how the dynamic loader finds the > libclang.so file relative to c-index-test?
What we're finding is that the path we get back from dladdr() in getClangResourcesPath() is what contains the "..", so I think you're correct. > In any case, adding canonicalization in just one of the places where we > compute the resource path will break this test for anyone who has symlinks > elsewhere in the path (then the paths will differ because the symlinks are > resolved in the libclang path but not in the clang binary path). And > canonicalization everywhere will break existing clang installations that > rely on the symlinks *not* being resolved. I think we need to find another > option. I can probably switch to just removing the dots to get the test case to stop failing, but as you point out below, this is a bit of a mess. :-D ~Aaron > >> so it's not clear to me that this patch would even help -- the clang >> driver also does not canonicalize the path (see SetInstallDir in >> tools/driver/driver.cpp). > > > ... but if you run -cc1 directly rather than from the clang driver, we > instead make a call that does use getMainExecutable(). What a mess. > >>> Ultimately, the value returned from this API gets passed to POSIX >>> functions which expect a canonical path. I don't think `remove_dots()` is >>> sufficient. It should do the same thing as `getMainExecutable()`, I believe. >> >> >> Note that we don't actually use getMainExecutable for the main clang >> driver, just for tooling purposes, and it *also* does not canonicalize. > > _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
