[PATCH] D37954: Try to shorten system header paths when using -MD depfiles

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

In https://reviews.llvm.org/D37954#901878, @rsmith wrote:

> In https://reviews.llvm.org/D37954#884029, @Lekensteyn wrote:
>
> > Any objection for merging these patches? I rebased today (no changes needed) and it still passes clang-tests.
>
>
> This is not the way that we do code review in LLVM and Clang. If you don't get a response to a patch, you're expected to keep pinging it (typically once a week) until you do get an answer.


There was no negative feedback and looking in the git logs also showed some examples without explicit review approval, so I (mistakenly) thought it was acceptable.

>   This past week in particular has been especially bad as we had the mailing deadline for the C++ committee meeting on Monday and the LLVM developer meeting yesterday and today.

Oh, where could I have learned about this? I am sporadically contributing and not following every day.

> I reverted this in r316195, because it breaks users using `-fno-canonical-prefixes` to request that we do not do this kind of path canonicalization.

I can propose a new patch and also add a test for this case. Thanks for your review and pointers!

In https://reviews.llvm.org/D37954#901884, @joerg wrote:

> The behavior of sometimes transforming the path and sometimes not is IMO completely unacceptable. I don't care if GCC created that bug first.


Would it help if a compile-time option is added so you can disable that for OpenBSD? All I am trying to do is to fix a bug in Ninja that is triggered because Clang deviates from GCC here. And Ninja will apparently not be fixed, so that leaves only this option. Do you have an alternative proposal?



================
Comment at: cfe/trunk/lib/Driver/ToolChains/Clang.cpp:967-968
 
+  if (Arg *A = Args.getLastArg(options::OPT_fno_canonical_system_headers,
+                               options::OPT_fcanonical_system_headers)) {
+    if (A->getOption().matches(options::OPT_fno_canonical_system_headers)) {
----------------
rsmith wrote:
> Use `hasFlag` instead.
> 
> Also, defaulting this on will break existing users who use `-fno-canonical-prefixes` to turn off the (wrong in general) assumption that using `realpath` is meaningful. Please default to the value of that flag instead. I would guess (but haven't checked) that flag controls this behavior in GCC.
Ok, I will apply the suggestions.

This patch does not seem to have any effect on clang, the path still starts with `/bin/../lib64/...` rather than `/usr/lib/...`

With GCC: just `-no-canonical-prefixes` does not have any effect.

`-fno-canonical-system-headers -no-canonical-prefixes` produces:
`/bin/../lib/gcc/x86_64-pc-linux-gnu/7.2.0/../../../../include/c++/7.2.0/cstdio`

`-fno-canonical-system-headers` produces:
`/usr/lib/gcc/x86_64-pc-linux-gnu/7.2.0/../../../../include/c++/7.2.0/cstdio`


================
Comment at: cfe/trunk/lib/Frontend/DependencyFile.cpp:296
+  if (CanonicalSystemHeaders && isSystem(FileType)) {
+    StringRef RealPath = FE->tryGetRealPathName();
+    if (!RealPath.empty() && RealPath.size() < Filename.size()) {
----------------
rsmith wrote:
> This is not an appropriate use of `tryGetRealPathName`. Its purpose is to get the way the file name was stored in disk (particularly, preserving the original case on a case-sensitive file system), not to resolve symlinks.
> 
> `FileManager::getCanonicalName` would be the right thing to call.
OK.


Repository:
  rL LLVM

https://reviews.llvm.org/D37954