[PATCH] D33788: Return a canonical path from getClangResourcePath()

[Richard Smith via cfe-commits]

On 2 June 2017 at 12:04, Richard Smith <richard at metafoo.co.uk> wrote:

> On 2 June 2017 at 11:39, Aaron Ballman via Phabricator via cfe-commits <
> cfe-commits at 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?

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.

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