[Compiler-RT] If unwind/c++abi is set, don't include libgcc

[rengolin]

Summary

Currently, for --rtlib=compiler-rt, when -lunwind or -lc++abi
are in the arguments list, both libgcc_s and libgcc_eh are still
added, which is wrong and duplicates symbols and may break stuff.

This patch makes sure that, when adding compiler-rt companion
libraries (unwind/c++), they haven't been added already.

[rengolin]

More context.

[rengolin]

test/Driver/linux-ld.c

1560

These tests pass on x86_64, ARM and AArch64, but they may not pass on others. Should I specify the target like the others above?

[rengolin]

Specifically test x86_64/ARM/AArch64, to make sure we don't assume anything about any other arch.

[logan]

I have some concern on the design of this change. IMO, it is unintuitive that one -l option will affect the other -l options. Can't we simply rely on --rtlib=compiler-rt? Or, alternatively, can we simply emit a warning instead of changing the behavior quietly? Thanks.

[rengolin]

In D11153#204688, @logan wrote:

I have some concern on the design of this change. IMO, it is unintuitive that one -l option will affect the other -l options. Can't we simply rely on --rtlib=compiler-rt? Or, alternatively, can we simply emit a warning instead of changing the behavior quietly? Thanks.

So, I did the original change, and that was a bad decision, which I only realised when trying to bundle RT with libc++. :)

The idea here is that we should *only* add an unwinder and a C++ library IFF there isn't one yet. This may be an overly simplistic approach, but it's better than no approach.

Since there's no way to remove libraries from the list (because the compiler adds them), there is no way a warning would be effective. Ie. the user would get a warning and would be able to do nothing.

[rengolin]

In D11153#204688, @logan wrote:

Can't we simply rely on --rtlib=compiler-rt?

Specifically about --rtlib=compiler-rt, there are a few choices:

1. Assume RT == unwind + c++abi. This is good for architectures where those libraries work reliably and are always available, either built in with Clang or as a package. Since it's not easy to compile libc++abi, and it requires Clang in the first place, I'd rather not have that until our build system is smart enough to do a two-stage build, like GCC.

2. Assume RT is in a GNU system. This is what we have now, and to make debug, profile and exception code work out-of-the-box, we need the libraries. I have taken the bad choice of including both libgcc libraries.

3. Assume RT is just part of the compiler library. This would force users to include -lgcc_s/-lunwind and/or -lgcc_eh/-lc++abi on every use of --rtlib. Clang includes gcc_s/eh already when rtlib=libgcc (obviously), and so does GCC. If we don't include some unwind/exception libraries with RT, the behaviour would be very different and make build systems a lot more complex.

This change is a preparation for a bigger one. The plan is:

Allow tools to have default libraries on different systems. Something like:

void GetCompilerLibraries() {
    // pick the defaults
    switch(OS/Env) {
        default:
            llvm_unreachable("must have a default");
        case GNU: // default GNU, --rtlib=libgcc
            RT=libgcc; UNW=libgcc_s; EH=libgcc_eh;
        case GNURT: // GNU with --rtlib=compiler-rt
            RT=compiler-rt; UNW=libgcc_s; EH=libgcc_eh;
        case FreeBSD: // BSD style
            RT=compiler-rt; UNW=libunwind; EH=libc++abi;
        case Darwin: // and so on...
            RT=...
    }
    // override with arguments
    for (lib : libArgs) {
        if (lib == libunwind) UNW=lib;
        else if (lib == libc++abi) EH=lib;
        else if (lib == libgcc_s) UNW=lib;
        else if (lib == libgcc_eh) UNW=lib;
    }
}

This way you have complete flexibility, and still produce the expected default results on all architectures.

GCC doesn't need that complexity because they're tied up with libgcc and friends.

[logan]

Since there's no way to remove libraries from the list (because the compiler adds them), there is no way a warning would be effective. Ie. the user would get a warning and would be able to do nothing.

Although it is difficult to use, we can remove the default libraries with -nostdlib. This is what I am using when I am building different configurations of libc++abi and libunwind on Linux.

For your long-term plan on the default standard library, I agree that we should select the default standard library according to the environment. However, I am still concerning the overloaded meaning for -l options. For example, what will happen if the users specified -lunwind when they are mean to link with libunwind[1] from Savannah? The libunwind from Savannah does not include C++ level 1 unwinding library by default.

[1] http://www.nongnu.org/libunwind/

[rengolin]

In D11153#209397, @logan wrote:

Although it is difficult to use, we can remove the default libraries with -nostdlib. This is what I am using when I am building different configurations of libc++abi and libunwind on Linux.

Right. In that case, I'd rather --rtlib=compiler-rt didn't include anything, and forced you to include your own libraries, instead of assuming a GNU environment.

For your long-term plan on the default standard library, I agree that we should select the default standard library according to the environment. However, I am still concerning the overloaded meaning for -l options. For example, what will happen if the users specified -lunwind when they are mean to link with libunwind[1] from Savannah? The libunwind from Savannah does not include C++ level 1 unwinding library by default.

That's a good point. That's why I didn't want to call our unwind libraries "libunwind". But that's also not an excuse to rely on string-match.

I think that removing the automatic include of libgcc is the least problematic solution.

Though it would be good to have an easier way to include unwind/eh libraries if detected, I'd rather do that on detection of presence, than on absence. This would be for another patch, of course.

Do you agree I should remove the libgcc inclusions altogether?

[rengolin]

Would it work to always add "--as-needed -lgcc_s" and let -lunwind override the behaviour? Is that the semantics here?

[compnerd]

Why not introduce a flag to switch between the various implementations (similar to -rt-lib=)?

[compnerd]

In D11153#210023, @rengolin wrote:

Would it work to always add "--as-needed -lgcc_s" and let -lunwind override the behaviour? Is that the semantics here?

I don't think that will work. The ordering of the arguments is important. The --as-needed will apply to gcc_s.so, but because there are symbols being referenced, it will be pulled even though -lunwind comes later without --as-needed. You need to either break compilation, assume the unwinder, or provide a switch.

[rengolin]

In D11153#210453, @compnerd wrote:

Why not introduce a flag to switch between the various implementations (similar to -rt-lib=)?

I think this is the better option, and dissociated from Compiler-RT. I'll revert my original change (to add libgcc_s/eh) and work towards the unwinder/c++ in a separate thread.

cheers,
--renato

lib/Driver/Tools.cpp

                        ArgStringList &CmdArgs) {
   CmdArgs.push_back(Args.MakeArgString(getCompilerRT(TC, "builtins")));
 
-  if (!TC.getTriple().isOSWindows()) {
-    // FIXME: why do we link against gcc when we are using compiler-rt?
-    CmdArgs.push_back("-lgcc_s");
-    if (TC.getDriver().CCCIsCXX())
-      CmdArgs.push_back("-lgcc_eh");
+  if (TC.getTriple().isOSWindows())
+    return;
+
+  // FIXME: Get default library from each target/os.
+  // From here on, we assume this is a GNU target. Even if it's not GNU/Linux,
+  // the system toolchain is assumed GNU based, which could be wrong based on
+  // the OS and/or the Arch.
+
+  // Get other libraries, to only include what's necessary and avoid
+  // duplication of symbols.
+  bool HasLibUnwind = false, HasLibCxxAbi = false;
+  for (const Arg *A : Args.filtered(options::OPT_l)) {
+    StringRef Value = A->getValue();
+    if (Value == "unwind")
+      HasLibUnwind = true;
+    if (Value == "c++abi")
+      HasLibCxxAbi = true;
   }
+  // If we're including -lunwind, don't include -lgcc_s
+  if (!HasLibUnwind)
+    CmdArgs.push_back("-lgcc_s");
+  // If we're including -lc++abi, don't include -lgcc_eh
+  if (!HasLibCxxAbi && TC.getDriver().CCCIsCXX())
+    CmdArgs.push_back("-lgcc_eh");
 }
 
 static void addProfileRT(const ToolChain &TC, const ArgList &Args,

test/Driver/linux-ld.c

 // CHECK-ARMV7EB: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
 // CHECK-ARMV7EB: "--be8"
 // CHECK-ARMV7EB: "-m" "armebelf_linux_eabi"
+
+// Test libunwind / libc++abi with compiler-rt
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o \
+// RUN:     --driver-mode=g++ --rtlib=compiler-rt 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-RTGNU %s
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o \
+// RUN:     --driver-mode=g++ --rtlib=compiler-rt -lunwind 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-RTUNW %s
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o \
+// RUN:     --driver-mode=g++ --rtlib=compiler-rt -lc++abi 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-RTCXX %s
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o \
+// RUN:     --driver-mode=g++ --rtlib=compiler-rt -lunwind -lc++abi 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-RTALL %s
+
+// CHECK-RTGNU: "{{.*}}ld{{(.exe)?}}"
+// CHECK-RTGNU: "-lgcc_s"
+// CHECK-RTGNU: libclang_rt.builtins-{{.*}}.a
+// CHECK-RTGNU: "-lgcc_eh"
+
+// CHECK-RTUNW: "{{.*}}ld{{(.exe)?}}"
+// CHECK-RTUNW: "-lunwind"
+// CHECK-RTUNW: libclang_rt.builtins-{{.*}}.a
+// CHECK-RTUNW: "-lgcc_eh"
+
+// CHECK-RTCXX: "{{.*}}ld{{(.exe)?}}"
+// CHECK-RTCXX: "-lc++abi"
+// CHECK-RTCXX: libclang_rt.builtins-{{.*}}.a
+// CHECK-RTCXX: "-lgcc_s"
+
+// CHECK-RTALL: "{{.*}}ld{{(.exe)?}}"
+// CHECK-RTALL: "-lunwind"
+// CHECK-RTALL: "-lc++abi"
+// CHECK-RTALL: libclang_rt.builtins-{{.*}}.a