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

[Aaron Ballman via cfe-commits]

On Fri, Jun 2, 2017 at 3:22 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> 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?

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