Page MenuHomePhabricator

Try to shorten system header paths when using -MD depfiles
ClosedPublic

Authored by Lekensteyn on Sep 16 2017, 8:32 PM.

Details

Summary

GCC tries to shorten system headers in depfiles using its real path
(resolving components like ".." and following symlinks). Mimic this
feature to ensure that the Ninja build tool detects the correct
dependencies when a symlink changes directory levels, see
https://github.com/ninja-build/ninja/issues/1330

An option to disable this feature is added in case "these changed header
paths may conflict with some compilation environments", see
https://gcc.gnu.org/ml/gcc-patches/2012-09/msg00287.html

Note that the original feature request for GCC
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52974) also included paths
preprocessed output (-E) and diagnostics. That is not implemented now
since I am not sure if it breaks something else.

Diff Detail

Repository
rL LLVM

Event Timeline

Lekensteyn created this revision.Sep 16 2017, 8:32 PM

I tried to contact Simon (the author of the GCC) patch with a question about the -fno-canonical-system-headers, but his Google address is bouncing.

GCC has long survived with doing this by default, I wonder if this option could just me omitted, enabling the feature by default.

joerg added a subscriber: joerg.Sep 17 2017, 2:46 PM

The comments at the very least are misleading. Resolving the path doesn't necessary shorten the string at all, in many cases it will be longer. I'm really not sure if clang should resolve symlinks like this as it can be quite surprising.

Resolving the path doesn't necessary shorten the string at all, in many cases it will be longer.

The description was based on the GCC manual page (" When preprocessing, do not shorten system header paths with canonicalization.")
Maybe something like:
"Do not canonicalize absolute system header paths in depfiles"

I'm really not sure if clang should resolve symlinks like this as it can be quite surprising.

This provides compatibility with GCC and fixes a bug with Ninja. At minimum, this canonicalization should be done when the path contains ".." components.
Can you think of a practical reason why this would cause issues?

joerg added a comment.Sep 17 2017, 4:17 PM

ninja is not the only consumer of this. For human inspection, it can be quite surprising to see symlinks resolved, i.e. /usr/include/machine on NetBSD. Build systems have different ideas on whether they want absolute resolved paths or not, so it seems like ninja should be able to cope with either format. This is a lossy transformation, so I'm somewhat reluctant to agree with it.

ninja is not the only consumer of this. For human inspection, it can be quite surprising to see symlinks resolved, i.e. /usr/include/machine on NetBSD.

No problem, NetBSD disables this option by default:
https://github.com/IIJ-NetBSD/netbsd-src/blob/dd946191000153f9c8a927e5257e726879f48140/share/mk/bsd.sys.mk#L26
This option was added in https://github.com/IIJ-NetBSD/netbsd-src/commit/c7e9228e67fab47a6bbfb548117da93ebb20ff5c
(I am unable to find a (mailing list) discussion about the exact problem though.)

Build systems have different ideas on whether they want absolute resolved paths or not, so it seems like ninja should be able to cope with either format. This is a lossy transformation, so I'm somewhat reluctant to agree with it.

Can you be specific about the build systems you have in mind? Note:

  1. GCC has added and enabled this option by default in 2012 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52974). and
  2. Ninja is currently slightly broken with Clang whereas it works fine with GCC.
  3. To the best of my knowledge, there are no systems that break after this fix.

Looking at the GCC change, it also checks whether the canonicalized path is actually shorter. In that case the documented "shorten header paths" seems to make sense.

I thought it would be a good idea to make Clang compatible with GCC in this case, not sure what else could break.
If it turns out to be bad, it can be removed and/or worked around with the -fno-canonical-system-headers option.

I think for depfile generation, we generally try to be gcc-compatible (I can link to prior changes in this spirit), so I think this seems like a good thing to do to me. gcc only does this for system headers, yes?

Does gcc support the non-negated spelling of this flag (-f-canonical-system-headers) too? If so, we should probably support that too for completeness, even if specifying it explicitly won't change behavior.

Lekensteyn retitled this revision from Expand absolute system header paths when using -MD depfiles to Try to shorten system header paths when using -MD depfiles.
Lekensteyn edited the summary of this revision. (Show Details)

Thanks for the feedback, I have the patch accordingly. Changes in v2:

  • Add (no-op) -fcanonical-system-headers option as suggested by @thakis (yes, GCC supports this option, but GCC also has a configure option that can change the default which Clang does not have)
  • Check whether path is shorter instead of checking for absolute path (matches the original GCC bug, should address @joerg's concern)
  • Update tests to use "%t" instead of "%T" (noticed D35396 after looking for whether "mkdir -p" is allowed) + other minor text/redundancy tweaks
joerg added a comment.Sep 20 2017, 2:01 AM

Well, the background for the use of the option in NetBSD is related to inducted differences in reproducable builds. From that perspective, it is even worth to sometimes shorten the dependency and sometimes not.

Any objection for merging these patches? I rebased today (no changes needed) and it still passes clang-tests.

ping? I'll push it next week if there is no new feedback.

This revision was automatically updated to reflect the committed changes.
rsmith edited edge metadata.Oct 19 2017, 5:26 PM

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

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.

cfe/trunk/lib/Driver/ToolChains/Clang.cpp
967–968

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.

cfe/trunk/lib/Frontend/DependencyFile.cpp
296

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.

joerg added a comment.Oct 19 2017, 5:39 PM

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

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!

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?

cfe/trunk/lib/Driver/ToolChains/Clang.cpp
967–968

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

cfe/trunk/lib/Frontend/DependencyFile.cpp
296

OK.

joerg added a comment.Oct 20 2017, 4:09 AM

This is not about any operating system, but basic consistent behavior. either do the canonicalisation or not. Doing it sometimes is just bogus. You've effectively implemented -fcanonical-system-headers=sometimes.

I think the diagnosis on the original issue was incorrect.

It seems to me that it was caused by the prefix being set as "/bin" instead of "/usr/bin", because clang _doesn't_ actually canonicalize its prefix, even when -no-canonical-prefixes isn't specified! All it does, now, is to make the prefix absolute -- without fully canonicalizing. I think that's simply a bug.

If the prefix had been, properly, /usr/bin, then the include path would've been:

/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/7.2.0/../../../../include/c++/7.2.0/iostream

And in that case, it would've worked properly with ninja, without adding the feature in this patch.

Reference clang/tools/driver/driver.cpp:297:

// FIXME: We don't actually canonicalize this, we just make it absolute.
if (CanonicalPrefixes)
  llvm::sys::fs::make_absolute(InstalledPath);

(FWIW, the suggestion to use FileManager:getCanonicalName did not work because I have no DirectoryEntry. And -no-canonical-prefixes it used very early and stripped from normal options, it seems used for a different purpose. Given the opposition, I think that this patch is a no-go.)

It seems to me that it was caused by the prefix being set as "/bin" instead of "/usr/bin", because clang _doesn't_ actually canonicalize its prefix, even when -no-canonical-prefixes isn't specified! All it does, now, is to make the prefix absolute -- without fully canonicalizing. I think that's simply a bug.

If the prefix had been, properly, /usr/bin, then the include path would've been:

/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/7.2.0/../../../../include/c++/7.2.0/iostream

And in that case, it would've worked properly with ninja, without adding the feature in this patch.

Reference clang/tools/driver/driver.cpp:297:

// FIXME: We don't actually canonicalize this, we just make it absolute.
if (CanonicalPrefixes)
  llvm::sys::fs::make_absolute(InstalledPath);

If this fixes the issue, then it would definitely be a much better approach. If this has changed between Clang 4 -> 5, then it is possible the root cause.

On my system (Arch Linux) /bin is a symlink to usr/bin. Output of clang -v:

clang version 5.0.0 (tags/RELEASE_500/final)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /bin
Found candidate GCC installation: /bin/../lib/gcc/x86_64-pc-linux-gnu/7.2.0
Found candidate GCC installation: /bin/../lib64/gcc/x86_64-pc-linux-gnu/7.2.0
Found candidate GCC installation: /usr/lib/gcc/x86_64-pc-linux-gnu/7.2.0
Found candidate GCC installation: /usr/lib64/gcc/x86_64-pc-linux-gnu/7.2.0
Selected GCC installation: /bin/../lib64/gcc/x86_64-pc-linux-gnu/7.2.0
Candidate multilib: .;@m64
Candidate multilib: 32;@m32
Selected multilib: .;@m64