This is an archive of the discontinued LLVM Phabricator instance.

[clang] Fix the canonicalization of paths in -fdiagnostics-absolute-paths
ClosedPublic

Authored by Ka-Ka on Nov 21 2019, 1:29 AM.

Details

Summary

In the current implementation of clang the canonicalization of paths in
diagnostic messages (when using -fdiagnostics-absolute-paths) only works
if the symbolic link is in the directory part of the filename, not if
the file itself is a symbolic link to another file.

This patch adds support to canonicalize the complete path including the
file.

Diff Detail

Event Timeline

Ka-Ka created this revision.Nov 21 2019, 1:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 21 2019, 1:29 AM

Personally, I would prefer to see the file name and path to be changed as little as possible because that would help to recognize the files better. We cannot use remove_dots() on POSIX OSes to simplify paths, because it may return an invalid path; thus we have to use getRealPath(). If I understand it right, there is no similar problem with the file name itself.

So, which issues this patch is going to solve?

rnk added a comment.Dec 16 2019, 2:21 PM

From auditing the call sites, it seems that almost all of them could be simplified by using the new API:
http://llvm-cs.pcc.me.uk/tools/clang/lib/Basic/FileManager.cpp/rgetCanonicalName

clang/include/clang/Basic/FileManager.h
225–226

You could make the key void* and save a DenseMap here. Nobody ever iterates CanonicalDirNames to look at the keys.

rnk added a comment.Dec 16 2019, 2:25 PM

Personally, I would prefer to see the file name and path to be changed as little as possible because that would help to recognize the files better. We cannot use remove_dots() on POSIX OSes to simplify paths, because it may return an invalid path; thus we have to use getRealPath(). If I understand it right, there is no similar problem with the file name itself.

So, which issues this patch is going to solve?

It seems clear to me, the filename could be an absolute symlink to a real file somewhere far removed from the realpath of the parent directory. It seems reasonable that -fdiagnostics-absolute-paths would look through symlinks in this case.

Ka-Ka added a comment.Dec 17 2019, 3:56 AM
In D70527#1786718, @rnk wrote:

Personally, I would prefer to see the file name and path to be changed as little as possible because that would help to recognize the files better. We cannot use remove_dots() on POSIX OSes to simplify paths, because it may return an invalid path; thus we have to use getRealPath(). If I understand it right, there is no similar problem with the file name itself.

So, which issues this patch is going to solve?

It seems clear to me, the filename could be an absolute symlink to a real file somewhere far removed from the realpath of the parent directory. It seems reasonable that -fdiagnostics-absolute-paths would look through symlinks in this case.

This fix is important when building with Bazel (google build system) as Bazel set up a temporary sandbox (with a series of symbolic links) to be be able to control the dependencies and keep a valid build cache. Currently when using the clang option -fdiagnostics-absolute-paths it will only resolve the sybolic names in the dir-path of the filename which end up with warning- and error-messages to files inside the sandbox instead of the correct source inside the repo you are working in.

In D70527#1786698, @rnk wrote:

From auditing the call sites, it seems that almost all of them could be simplified by using the new API:
http://llvm-cs.pcc.me.uk/tools/clang/lib/Basic/FileManager.cpp/rgetCanonicalName

I agree, that they can be simplified, but I guess it is easier to do those changes in separate patches.

clang/include/clang/Basic/FileManager.h
225–226

Sure, I will update the patch ...

Ka-Ka updated this revision to Diff 234268.Dec 17 2019, 4:09 AM
Ka-Ka marked 2 inline comments as done.

Updated patch according to review comments.

rnk accepted this revision.Dec 18 2019, 2:32 PM

lgtm

clang/lib/Basic/FileManager.cpp
550–551

These FIXMEs look stale. We are already using VFS getRealPath which ultimately probably boils down to whatever the original author thought sys::fs::canonical would be.

567

I don't think we need to carry this FIXME here.

569

uber nit: use the .insert get-or-create pattern

clang/test/Frontend/absolute-paths-symlinks.c
16

I wish we had a more precise feature to indicate symlink support, but this is what we do elsewhere, so there's nothing to do here.

This revision is now accepted and ready to land.Dec 18 2019, 2:32 PM
This revision was automatically updated to reflect the committed changes.
Ka-Ka marked 2 inline comments as done.
MaskRay added inline comments.
clang/test/Frontend/absolute-paths-symlinks.c
16

I wrote:

## Don't make symlinks on Windows.
# UNSUPPORTED: system-windows

in D71302. Yeah, please tell me which precise feature I should use :)

MaskRay added inline comments.Dec 23 2019, 1:04 PM
clang/lib/Basic/FileManager.cpp
562

There is no need to change now, but I think try_emplace may be slightly more idiomatic as it saves a copy-list-initialization.

572

There is no need to change now. StringRef ... = may be more common.

clang/test/Frontend/absolute-paths-symlinks.c
4

No need to change now. Quotes are not needed.

6

No need to change now. Spaces around |

Ka-Ka marked 3 inline comments as done.Dec 26 2019, 1:17 AM

I have now updated the testcase according to comments by @MaskRay in commit 073cdb239044
Thanks for post-commit review comments.