<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 2 June 2017 at 12:04, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="">On 2 June 2017 at 11:39, Aaron Ballman via Phabricator via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">aaron.ballman added a comment.<br>
<span class="m_-7842864213824150436gmail-"><br>
In <a href="https://reviews.llvm.org/D33788#771671" rel="noreferrer" target="_blank">https://reviews.llvm.org/D3378<wbr>8#771671</a>, @bruno wrote:<br>
<br>
> > I'm unaware of any other API that canonicalizes the path, which is what users of this API are going to expect.<br>
><br>
> You can also call `sys::path::remove_dots(Path, /*remove_dot_dot=*/true);`<br>
<br>
<br>
</span>Yes, but that does not canonicalize the path. For instance, it won't handle doubled-up slashes or symlinks.<br></blockquote><div><br></div></span><div>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.</div><div><br></div><div>The path with the bin/../lib in it is presumably the path from the clang binary, not the path from libclang,</div></div></div></div></blockquote><div><br></div><div>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?</div><div><br></div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>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).</div></div></div></div></blockquote><div><br></div><div>... but if you run -cc1 directly rather than from the clang driver, we instead make a call that does use getMainExecutable(). What a mess.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
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.</blockquote><div><br></div></span><div>Note that we don't actually use getMainExecutable for the main clang driver, just for tooling purposes, and it *also* does not canonicalize. </div></div></div></div>
</blockquote></div><br></div></div>