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

[Richard Smith via cfe-commits]

On 2 June 2017 at 12:26, Aaron Ballman <aaron.ballman at gmail.com> wrote:

> 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


Alternative suggestion: right now, whenever we write a path into an AST
file, we currently check if the path starts with the "base directory" of
the build -- which is the home directory of the module for a module build
and the sysroot for a PCH, and if so we strip off that prefix so that the
AST file can be relocated with the files it describes. Prior to that check,
we could check whether the path starts with the compiler resource directory
path, and if so, strip *that* path off instead (storing a marker with the
path so we know how to reconstruct it). That way, it shouldn't matter what
path we pick to the resource directory (or where the compiler is
installed); we'll get the same AST file regardless.

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