[Driver] Suppress GCC detection under -B

[MaskRay]

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

[manojgupta]

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]

In D97993#2605157, @manojgupta wrote:

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.

[manojgupta]

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.

[manojgupta]

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]

In D97993#2607523, @manojgupta wrote:

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

[manojgupta]

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]

clang/lib/Driver/ToolChains/Gnu.cpp

1911

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]

clang/lib/Driver/ToolChains/Gnu.cpp

1911

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]

Add release note

[MaskRay]

clang/lib/Driver/ToolChains/Gnu.cpp

1911

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

[MaskRay]

@tstellar :)

[MaskRay]

Ping....

[nickdesaulniers]

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 ↗

(On Diff #328836)

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

76–77 ↗

(On Diff #328836)

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

1911

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.

[MaskRay]

Reword. Drop the special case for Android as well.

clang/docs/ReleaseNotes.rst

76–77 ↗

(On Diff #328836)

Thanks for the feedback. Updated.

clang/lib/Driver/ToolChains/Gnu.cpp

1911

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.

[vvereschaka]

Hi @MaskRay,

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

• https://lab.llvm.org/buildbot/#/builders/119/builds/2807 (Aarch64)
• https://lab.llvm.org/buildbot/#/builders/60/builds/2416 (ARMv7)

[MaskRay]

In D97993#2639844, @vvereschaka wrote:

Hi @MaskRay,

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

• https://lab.llvm.org/buildbot/#/builders/119/builds/2807 (Aarch64)
• https://lab.llvm.org/buildbot/#/builders/60/builds/2416 (ARMv7)

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

[vvereschaka]

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)

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

[MaskRay]

In D97993#2643318, @vvereschaka wrote:

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.

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

[thakis]

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.

[MaskRay]

In D97993#2644664, @thakis wrote:

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.

[malharJ]

Hi,

A couple of things here:

• Has anything been done to address this bug (related to COMPILER_PATH no longer being supported): https://bugs.llvm.org/show_bug.cgi?id=52009
  
  I think the COMPILER_PATH was being added to D.PrefixDirs, but now cannot be used for gcc toolchain detection.

• Also, I noticed that the LIT tests in this patch are passing (tested on AArch64 machine) without/with the source changes (I tried this by checking out this commit and even on top of main yesterday).
  
  Is this expected ?

clang/lib/Driver/ToolChains/Gnu.cpp

void Generic_GCC::GCCInstallationDetector::init(
   // The compatible GCC triples for this particular architecture.
   SmallVector<StringRef, 16> CandidateTripleAliases;
   SmallVector<StringRef, 16> CandidateBiarchTripleAliases;
   CollectLibDirsAndTriples(TargetTriple, BiarchVariantTriple, CandidateLibDirs,
                            CandidateTripleAliases, CandidateBiarchLibDirs,
                            CandidateBiarchTripleAliases);
 
   // Compute the set of prefixes for our search.
-  SmallVector<std::string, 8> Prefixes(D.PrefixDirs.begin(),
-                                       D.PrefixDirs.end());
+  SmallVector<std::string, 8> Prefixes;
+  if (TargetTriple.isAndroid())
+    Prefixes.assign(D.PrefixDirs.begin(), D.PrefixDirs.end());
 
   StringRef GCCToolchainDir = getGCCToolchainDir(Args, D.SysRoot);
   if (GCCToolchainDir != "") {
     if (GCCToolchainDir.back() == '/')
       GCCToolchainDir = GCCToolchainDir.drop_back(); // remove the /
 
     Prefixes.push_back(std::string(GCCToolchainDir));
   } else {

clang/test/Driver/gcc-toolchain.cpp

 //
 // Test for linker toolchain detection. Note that only the '-L' flags will use
 // the same precise formatting of the path as the '-internal-system' flags
 // above, so we just blanket wildcard match the 'crtbegin.o'.
 // CHECK: "{{[^"]*}}ld{{(.exe)?}}"
 // CHECK: "{{[^"]*}}/usr/lib/i386-linux-gnu/gcc/i686-linux-gnu/4.5{{/|\\\\}}crtbegin.o"
 // CHECK: "-L[[TOOLCHAIN]]/usr/lib/i386-linux-gnu/gcc/i686-linux-gnu/4.5"
 // CHECK: "-L[[TOOLCHAIN]]/usr/lib/i386-linux-gnu/gcc/i686-linux-gnu/4.5/../../../.."
+
+/// Test we don't detect GCC installation under -B.
+// RUN: %clangxx -no-canonical-prefixes %s -### -o %t 2>&1 \
+// RUN:   --target=aarch64-suse-linux --gcc-toolchain=%S/Inputs/opensuse_42.2_aarch64_tree/usr | \
+// RUN:   FileCheck %s --check-prefix=AARCH64
+// RUN: %clangxx -no-canonical-prefixes %s -### -o %t 2>&1 \
+// RUN:   --target=aarch64-suse-linux -B%S/Inputs/opensuse_42.2_aarch64_tree/usr | \
+// RUN:   FileCheck %s --check-prefix=NO_AARCH64
+
+// AARCH64:        Inputs{{[^"]+}}aarch64-suse-linux/{{[^"]+}}crt1.o"
+// NO_AARCH64-NOT: Inputs{{[^"]+}}aarch64-suse-linux/{{[^"]+}}crt1.o"