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

Aaron Ballman via cfe-commits cfe-commits at lists.llvm.org
Fri Jun 2 12:23:12 PDT 2017


On Fri, Jun 2, 2017 at 3:04 PM, 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.

Breaking changes would be bad, but I'm not particularly sympathetic to
that use case compared to expecting callers to canonicalize the
results before passing off to POSIX APIs (as we do). Effectively, by
pushing this off to callers, we create a more fragile system that's
harder to reason about. You could argue that while this does break
such an installation, the installation is rather defective for
expecting the symlink to not resolve.

> The path with the bin/../lib in it is presumably the path from the clang
> binary, not the path from libclang, 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).

To be clear: this change fixed the test case breakage we were seeing.
So I do believe it helps. ;-)

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

Maybe I'm confused, but in main() (ToolChain.cpp), I see a call to
GetExecutablePath(), and it canonicalizes prefixes unless passing
-no-canonical-prefixes. When canonicalizing, it calls
getMainExecutable(), which calls real_path() in at least two cases
(one of which is for __APPLE__ targets).

~Aaron


More information about the cfe-commits mailing list