Page MenuHomePhabricator

[Driver] Suppress GCC detection under -B
ClosedPublic

Authored by MaskRay on Mar 4 2021, 5:23 PM.

Details

Summary

In GCC, if -B $prefix is specified, $prefix is used to find executable files and startup files.
$prefix/include is added as an include search directory.

Clang overloads -B with GCC installation detection semantics which make the
behavior less predictable (due to the "largest GCC version wins" rule) and
interact poorly with --gcc-toolchain (--gcc-toolchain can be overridden by -B).

  • clang++ foo.cpp detects GCC installation under /usr.
  • clang++ --gcc-toolchain=Inputs foo.cpp detects GCC installation under Inputs.
  • clang++ -BA --gcc-toolchain=B foo.cpp detects GCC installation under A and B and the larger version wins. With this patch, only B is used for detection.
  • clang++ -BA foo.cpp detects GCC installation under A and /usr, and the larger GCC version wins. With this patch A is not used for detection.

This patch changes -B to drop the GCC detection semantics. Its executable
searching semantics are preserved. --gcc-toolchain is the recommended option to
specify the GCC installation detection directory.

(
Note: Clang detects GCC installation in various target dependent directories.
$sysroot/usr (sysroot defaults to "") is a common directory used by most targets.
Such a directory is expected to contain something like lib{,32,64}/gcc{,-cross}/$triple.
Clang will then construct library/include paths from the directory.
)

Diff Detail

Event Timeline

MaskRay created this revision.Mar 4 2021, 5:23 PM
MaskRay requested review of this revision.Mar 4 2021, 5:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2021, 5:23 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I am not sure of the rationale or upside of this change and why do we want to drop gcc detection? GCC does not need to do the GCC detection because it has the needed information at configure time.

MaskRay added a comment.EditedMar 4 2021, 5:37 PM

I am not sure of the rationale or upside of this change and why do we want to drop gcc detection? GCC does not need to do the GCC detection because it has the needed information at configure time.

The dropped detection is for -B. --gcc-toolchain and --sysroot (default "") still have GCC detection.

-B is not appropriate for GCC installation detection. It can be used to specify an executable search path, but that is it. We should not do GCC installation detection.

The change is to make Linux cross compilation more predictable, not shadowing --gcc-toolchain with -B or system cross compilation packages.

Thanks for explaining that it only affects "-B". While I believe that this change won't affect us in Chrome OS, I think it should be reviewed and approved by a few Linux distro contributors since there is already known reliance e.g. Android on the current behavior.

Another concern is people generally want clang to work out of box without any special arguments. As a user, after installing clang's distro package or building from source, I expect that basic compilation should work out-of-box which includes gcc detection.
i.e. "clang foo.cpp -o foo" should just work in most cases.

If a user now needs to pass "-gcc-toolchain" to find gcc libraries when it just used to work, that is most likely not desirable.

MaskRay added a comment.EditedMar 5 2021, 12:14 PM

Another concern is people generally want clang to work out of box without any special arguments. As a user, after installing clang's distro package or building from source, I expect that basic compilation should work out-of-box which includes gcc detection.
i.e. "clang foo.cpp -o foo" should just work in most cases.

If a user now needs to pass "-gcc-toolchain" to find gcc libraries when it just used to work, that is most likely not desirable.

There may be some confusion. The regular usage all works fine.

  • clang++ foo.cpp detects GCC installation under /usr.
  • clang++ --gcc-toolchain=Inputs foo.cpp detects GCC installation under Inputs.
  • clang++ -BInputs foo.cpp detects GCC installation under Inputs and /usr, and the larger GCC version wins. With this patch Inputs is not used for detection. /usr is still used for detection. Inputs is still used for finding executables (ld, as)
  • clang++ -BA --gcc-toolchain=B foo.cpp detects GCC installation under A and B. With this patch, only B is used for detection.

If the user specifies --sysroot, $sysroot/usr is expected to find GCC installations. The -B logic is unneeded as well.

So this should only affect a minor usage that uses -B without --sysroot/--gcc-toolchain and expects -B to detect GCC installations. (It doesn't in GCC.).

Thanks for the clarification. I do not have any objections but I feel that am not the right person to approve this change.
@tstellar can you please review it?

danalbert added inline comments.Mar 5 2021, 2:55 PM
clang/lib/Driver/ToolChains/Gnu.cpp
1912

I'm not entirely sure what D.PrefixDirs represents so maybe Android doesn't need this either.

The behavior the NDK depends on is being able to find tools co-located with the Clang driver location. Aside from as, these are all LLVM tools (lld and co).

The sysroot is expected to be in $CLANG/../sysroot. All our headers, libraries (aside from libgcc/libatomic), and CRT objects are located there.

The clang driver install location is expected to also be a GCC install directory, so libgcc/libatomic are expected at $CLANG/../lib/gcc/$TRIPLE/$GCC_VERSION.

Typical usage for the NDK does not involve -gcc-toolchain or -B at all.

If I've understood correctly, your change can be applied to Android as well without breaking any of those behaviors. @srhines will need to comment on whether the Android platform build needs this, but aiui anyone depending on this behavior just needs to fix their build to use -gcc-toolchain where they were previously using -B.

Of course, I can't speak to what our users with custom build systems that don't follow our docs might be doing.

srhines added inline comments.Mar 6 2021, 1:36 AM
clang/lib/Driver/ToolChains/Gnu.cpp
1912

From a look at Android's build system, we are using -B but we don't currently use --sysroot or -gcc-toolchain for platform builds. I did try a build with this patch (but not retaining the Android-specific part), and it still worked fine. From looking at the constructed command lines, the platform build generally adds the appropriate include paths, library paths, and invokes separate tools directly, so we aren't relying on -B for this purpose. Cleaning this up is on my list of things to eventually do, but as I see it, we're not going to be negatively impacted even with the more aggressive version of this patch.

Of course, I can't speak to what our users with custom build systems that don't follow our docs might be doing.

I do share this concern a bit, but it's very hard for me to quantify how hard it would be for someone to just adapt their build if we did make a more aggressive change here. If it's really as easy as passing -gcc-toolchain, then perhaps that would be fine for the Release Notes.

MaskRay updated this revision to Diff 328836.Mar 6 2021, 9:31 PM

Add release note

MaskRay edited the summary of this revision. (Show Details)Mar 6 2021, 9:31 PM
MaskRay edited the summary of this revision. (Show Details)
MaskRay edited the summary of this revision. (Show Details)
MaskRay added inline comments.Mar 6 2021, 9:42 PM
clang/lib/Driver/ToolChains/Gnu.cpp
1912

It should be as easy as passing --gcc-toolchain. I edited the summary to state more clearly that the current behavior actually makes interop with --gcc-toolchain difficult.

I'll not touch Android to restrict the influence to regular Linux distributions in this patch.
Android can be fixed separately (which require two test changes.)

nickdesaulniers requested changes to this revision.Thu, Mar 18, 5:22 PM

Let's drop the Android part, too? Update the description (commit message), too?

I checked our oldest supported kernel version, and we don't use -B either: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/Makefile?h=v4.4.262#n606

clang/docs/ReleaseNotes.rst
75

is there an extra > in the second occurrence of prefix on this line?

76–77

The first time I read This behavior is incompatible with GCC, causes interop issues with --gcc-toolchain, and is thus dropped. I thought you were dropping the -B flag entirely. I see now you're referring to This behavior being dropped, but maybe this can be reworded to avoid confusing others?

clang/lib/Driver/ToolChains/Gnu.cpp
1912

I did try a build with this patch (but not retaining the Android-specific part), and it still worked fine.

Then we should drop this Android specific part. If @srhines tested it without, there's no issue.

From a look at Android's build system, we are using -B but we don't currently use --sysroot or -gcc-toolchain for platform builds

We're probably mostly using LLVM utils anyways.

It should be as easy as passing --gcc-toolchain.

Yep.

This revision now requires changes to proceed.Thu, Mar 18, 5:22 PM
MaskRay updated this revision to Diff 331736.Thu, Mar 18, 5:51 PM
MaskRay marked 5 inline comments as done.
MaskRay retitled this revision from [Driver] Suppress GCC detection under -B for non-Android to [Driver] Suppress GCC detection under -B.
MaskRay edited the summary of this revision. (Show Details)

Reword. Drop the special case for Android as well.

clang/docs/ReleaseNotes.rst
76–77

Thanks for the feedback. Updated.

clang/lib/Driver/ToolChains/Gnu.cpp
1912

Thank for the feedback that Android does not need this. I was unsure so I tried to keep Android unchanged. Seems that the special case is not actually needed.

nickdesaulniers accepted this revision.Thu, Mar 18, 6:43 PM
This revision is now accepted and ready to land.Thu, Mar 18, 6:43 PM
This revision was automatically updated to reflect the committed changes.

Hi @MaskRay,

sorry, but these changes break the Clang::gcc-toolchain.cpp test on the ARMv7/AArch64 cross builders:

Hi @MaskRay,

sorry, but these changes break the Clang::gcc-toolchain.cpp test on the ARMv7/AArch64 cross builders:

The Windows bots don't have /usr/local/include/. I think they should have been broken before my change. Anycase, I deleted "-internal-isystem" "/usr/local/include" in 1fe1e996e987426e5d6352dabef358fc4ae619e5

CHECK: "-internal-isystem" "/usr/local/include" didn't break the test on the cross builders before.

Also, the // CHECK: "-L[[TOOLCHAIN]]/usr/lib/i386-linux-gnu/gcc/i686-linux-gnu/4.5/../../../.." check now is failing also (line 31).

I tried to temporary disable a check at line 31 and I got two more problems:

  1. I noticed a wrong place for 2>&1 in RUN statement, it should be placed in front of | (lines 34-35, 37-38 in gcc-toolchain.cpp)
  1. The second problem. Here is an output for Aarch64 cross toolchain win-x-ubuntu linux:
"c:\\buildbot\\temp\\build\\bin\\ld.lld" 
"--sysroot=C:/buildbot/.aarch64-ubuntu" 
"-EL" "--eh-frame-hdr" "-m" "aarch64linux" 
"-dynamic-linker" "/lib/ld-linux-aarch64.so.1" 
"-o" "C:\\buildbot\\temp\\build\\tools\\clang\\test\\Driver\\Output\\gcc-toolchain.cpp.tmp" 
"C:/buildbot/.aarch64-ubuntu/usr/lib/aarch64-linux-gnu\\crt1.o" 
"C:/buildbot/.aarch64-ubuntu/usr/lib/aarch64-linux-gnu\\crti.o" 
"C:\\buildbot\\temp\\llvm-project\\clang\\test\\Driver/Inputs/opensuse_42.2_aarch64_tree/usr/lib64/gcc/aarch64-suse-linux/4.8\\crtbegin.o" 
"-LC:\\buildbot\\temp\\llvm-project\\clang\\test\\Driver/Inputs/opensuse_42.2_aarch64_tree/usr/lib64/gcc/aarch64-suse-linux/4.8" 
"-LC:/buildbot/.aarch64-ubuntu/lib/aarch64-linux-gnu" 
"-LC:/buildbot/.aarch64-ubuntu/usr/lib/aarch64-linux-gnu" 
"-LC:/buildbot/.aarch64-ubuntu/lib" 
"-LC:/buildbot/.aarch64-ubuntu/usr/lib" 
"C:\\Users\\buildbot\\AppData\\Local\\Temp\\2\\gcc-toolchain-addcda.o" 
"-lstdc++" "-lm" "-lgcc_s" "-lgcc" "-lc" "-lgcc_s" "-lgcc" 
"C:\\buildbot\\temp\\llvm-project\\clang\\test\\Driver/Inputs/opensuse_42.2_aarch64_tree/usr/lib64/gcc/aarch64-suse-linux/4.8\\crtend.o" 
"C:/buildbot/.aarch64-ubuntu/usr/lib/aarch64-linux-gnu\\crtn.o"

the Inputs{{[^"]+}}aarch64-suse-linux/{{[^"]+}}crt1.o" check is looking for crt1.o "under" aarch64-suse-linux triple, but it is "under" the default triple for the toolchain - aarch64-linux-gnu.
The only crtbegin.o and crtend.o files are "under" expected triple - aarch64-suse-linux. So, I suppose, there is a bug in the test or somewhere in the implementation.

CHECK: "-internal-isystem" "/usr/local/include" didn't break the test on the cross builders before.

Also, the // CHECK: "-L[[TOOLCHAIN]]/usr/lib/i386-linux-gnu/gcc/i686-linux-gnu/4.5/../../../.." check now is failing also (line 31).

The bot (despite being on Windows) has a default sysroot: "--sysroot=C:/buildbot/.aarch64-ubuntu", which seems to interact badly with --gcc-toolchain.
Perhaps a --sysroot= should be added.

I tried to temporary disable a check at line 31 and I got two more problems:

  1. I noticed a wrong place for 2>&1 in RUN statement, it should be placed in front of | (lines 34-35, 37-38 in gcc-toolchain.cpp)

2>&1 can be anywhere in the command. The convention is at the end, but it is not a must.

  1. The second problem. Here is an output for Aarch64 cross toolchain win-x-ubuntu linux:
"c:\\buildbot\\temp\\build\\bin\\ld.lld" 
"--sysroot=C:/buildbot/.aarch64-ubuntu" 
"-EL" "--eh-frame-hdr" "-m" "aarch64linux" 
"-dynamic-linker" "/lib/ld-linux-aarch64.so.1" 
"-o" "C:\\buildbot\\temp\\build\\tools\\clang\\test\\Driver\\Output\\gcc-toolchain.cpp.tmp" 
"C:/buildbot/.aarch64-ubuntu/usr/lib/aarch64-linux-gnu\\crt1.o" 
"C:/buildbot/.aarch64-ubuntu/usr/lib/aarch64-linux-gnu\\crti.o" 
"C:\\buildbot\\temp\\llvm-project\\clang\\test\\Driver/Inputs/opensuse_42.2_aarch64_tree/usr/lib64/gcc/aarch64-suse-linux/4.8\\crtbegin.o" 
"-LC:\\buildbot\\temp\\llvm-project\\clang\\test\\Driver/Inputs/opensuse_42.2_aarch64_tree/usr/lib64/gcc/aarch64-suse-linux/4.8" 
"-LC:/buildbot/.aarch64-ubuntu/lib/aarch64-linux-gnu" 
"-LC:/buildbot/.aarch64-ubuntu/usr/lib/aarch64-linux-gnu" 
"-LC:/buildbot/.aarch64-ubuntu/lib" 
"-LC:/buildbot/.aarch64-ubuntu/usr/lib" 
"C:\\Users\\buildbot\\AppData\\Local\\Temp\\2\\gcc-toolchain-addcda.o" 
"-lstdc++" "-lm" "-lgcc_s" "-lgcc" "-lc" "-lgcc_s" "-lgcc" 
"C:\\buildbot\\temp\\llvm-project\\clang\\test\\Driver/Inputs/opensuse_42.2_aarch64_tree/usr/lib64/gcc/aarch64-suse-linux/4.8\\crtend.o" 
"C:/buildbot/.aarch64-ubuntu/usr/lib/aarch64-linux-gnu\\crtn.o"

the Inputs{{[^"]+}}aarch64-suse-linux/{{[^"]+}}crt1.o" check is looking for crt1.o "under" aarch64-suse-linux triple, but it is "under" the default triple for the toolchain - aarch64-linux-gnu.
The only crtbegin.o and crtend.o files are "under" expected triple - aarch64-suse-linux. So, I suppose, there is a bug in the test or somewhere in the implementation.

--sysroot= may help crt1.o. I cannot test so hope you can verify it and make the change:)

In the end, testing Linux cross compiling on Windows gives us plethora issues due to backslashes. This actually works on x86-64 Windows machines so perhaps that is fine. And I think we've identified the root issue on aarch64 Windows.
If turns out to be more issues, I'd just suggest UNSUPPORTED: system-windows...

Also broke something in chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=1191244 (since fixed by switching something from -B to --gcc-toolchain. This only landed 4 days ago and already broke a lot of things (see above) -- chances are it's going to break many more things once it makes it into a release. Is this really worth the churn? The benefits seem fairly small from what I understand.

Also broke something in chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=1191244 (since fixed by switching something from -B to --gcc-toolchain. This only landed 4 days ago and already broke a lot of things (see above) -- chances are it's going to break many more things once it makes it into a release. Is this really worth the churn? The benefits seem fairly small from what I understand.

@thakis The benefit is large. With the previous behavior, the selected toolchain could be overridden by the system /usr and --gcc-toolchain or --sysroot could be accidentally overridden by -B.
aeubanks has fixed it via https://chromium-review.googlesource.com/c/chromium/src/+/2781154 That's great.