[Driver] Pass -lunwind along with compiler-rt when necessary on Linux

[mgorny]

Summary

Pass -lunwind when compiler-rt is used as the runtime library on regular
Linux (non-Android) and an unwinder is needed. This fixes building
C++ programs with '-rtlib=compiler-rt -stdlib=libc++' out-of-the-box,
and linking to compiler-rt when using '-static'. In both cases clang
did not pass any unwinder library so far.

This aims to match the current '-rtlib=libgcc' behavior which includes
the gcc unwinder library (libgcc_s or libgcc_eh) unconditionally.
However, -lunwind is added for C++ only if libc++ is used as
the standard C++ library since libstdc++ implicitly includes
the unwinder. Furthermore, -lunwind is only used for C++ and static
compiler-rt executables to avoid adding unnecessary dependencies for
regular C executables that could make bootstrap harder.

This was originally requested in D25008 by @EricWF in order to remove
the necessity of including -lunwind in the linker script. Most notably,
this allows us to reduce the risk of including two unwinder libraries in
a single linking action (libgcc_s via -rtlib, and libunwind via
the linker script).

The change is limited to Linux platform to avoid accidentally breaking
other platforms. Furthermore, some of them include -lunwind
unconditionally already.

[joerg]

So, what's the test change that matches the difference in output this creates?

[mgorny]

I knew this looked too easy ;-).

[mgorny]

I've updated the NetBSD tests (@joerg, please confirm if it's desired), and added a Linux test for the case. AFAICS most of the other targets override the method, some of them adding -lunwind already. I'd rather not modify the other targets that I don't really know.

[joerg]

No, they are not desirable, in fact, they pretty much break clang. That's why I wanted to see the test diff...

[joerg]

I don't agree with the motivation for this change, BTW. Whether -lunwind is needed or not is an implementation detail of the way the ABI implementation is linked. That's why libc++ is now a linker script, to hide this kinds of implementation details.

[mgorny]

In D25402#565557, @joerg wrote:

No, they are not desirable, in fact, they pretty much break clang. That's why I wanted to see the test diff...

Thank you for your feedback. However, I would really appreciate if you kindly said what you meant up front next time instead of requesting me to perform pointless work. You should note that your behavior as well as tone is strongly discouraging to contributors like me, and I doubt this is really of any benefit to anyone.

[mgorny]

I've updated the patch to apply to Linux toolchain only.

[joerg]

It's difficult to say whether if one change to certain driver functions affect some target or not. That's one reason why the test change is so important.

Looking at the latest version, I still don't understand why the change for Linux is needed. libc++ should provide a linker script now and that should include -lunwind as appropriate.

[mgorny]

In D25402#566070, @joerg wrote:

It's difficult to say whether if one change to certain driver functions affect some target or not. That's one reason why the test change is so important.

Looking at the latest version, I still don't understand why the change for Linux is needed. libc++ should provide a linker script now and that should include -lunwind as appropriate.

Most importantly, the linker script can't predict which -rtlib will be used and therefore whether -lgcc_s or -lunwind should be used.

[Hahnfeld]

lib/Driver/ToolChains.cpp

4704 ↗

(On Diff #74115)

As just written in D25008: This will probably result in problems if a system default libunwind.so.8 is installed...

[mgorny]

lib/Driver/ToolChains.cpp

4704 ↗

(On Diff #74115)

I think this is the correct behavior in this case. Clang should work with whichever unwinder implementation is available, and if you want to change that, change the libunwind.so symlink.

However, I don't really think it's a good idea to have both unwinder libraries installed alongside, especially that they are sharing the same name.

[Hahnfeld]

lib/Driver/ToolChains.cpp

4704 ↗

(On Diff #74115)

No, we can't: The nongnu libunwind is used by other system libraries that need and are tested against this version.

It's certainly not a good idea, but it's reality in many distros. Most of them don't ship LLVM's libunwind.

[mgorny]

lib/Driver/ToolChains.cpp

4704 ↗

(On Diff #74115)

Well, IMO only sane solution to the problem is to have LLVM's libunwind renamed. Otherwise, using the two libraries is going always to be an unreliable, awful hackery with high risk that someone will accidentally end up with wrong one or -- even worse -- both of them in the same executable.

[Hahnfeld]

lib/Driver/ToolChains.cpp

4704 ↗

(On Diff #74115)

Well, then the worse thing will happen with this patch if both libraries are installed...

[mgorny]

Here's a v2. It turns out that you also need -lunwind when using to link C programs with -static -rtlib=compiler-rt. I've also disabled the changes for Android targets.

[compnerd]

This really needs a new driver flag (-unwinder?) similar to -rtlib, as there are multiple unwinders, and it is unclear which unwinder is the proper one on a given target. This has been something on my TODO list for a while. Mixing and matching undwinders is not really possible, and it is perfectly valid to use compiler-rt with the gcc unwinder on Linux.

[mgorny]

@compnerd, I presume that the option would do nothing when using -rtlib=libgcc, correct? Or do we support combining libgcc with another unwinder?

lib/Driver/Tools.cpp

if (Args.hasArg(options::OPT_Z_Xlinker__no_demangle))
     CmdArgs.push_back("--no-demangle");
 
   bool NeedsSanitizerDeps = addSanitizerRuntimes(ToolChain, Args, CmdArgs);
   bool NeedsXRayDeps = addXRayRuntime(ToolChain, Args, CmdArgs);
   AddLinkerInputs(ToolChain, Inputs, Args, CmdArgs, JA);
   // The profile runtime also needs access to system libraries.
   getToolChain().addProfileRTLibs(Args, CmdArgs);
 
+  bool NeedsUnwinder = false;
   if (D.CCCIsCXX() &&
       !Args.hasArg(options::OPT_nostdlib, options::OPT_nodefaultlibs)) {
     bool OnlyLibstdcxxStatic = Args.hasArg(options::OPT_static_libstdcxx) &&
                                !Args.hasArg(options::OPT_static);
     if (OnlyLibstdcxxStatic)
       CmdArgs.push_back("-Bstatic");
     ToolChain.AddCXXStdlibLibArgs(Args, CmdArgs);
     if (OnlyLibstdcxxStatic)
       CmdArgs.push_back("-Bdynamic");
     CmdArgs.push_back("-lm");
+    // C++ always needs some unwinder
+    NeedsUnwinder = true;
   }
   // Silence warnings when linking C code with a C++ '-stdlib' argument.
   Args.ClaimAllArgs(options::OPT_stdlib_EQ);
 
   if (!Args.hasArg(options::OPT_nostdlib)) {
     if (!Args.hasArg(options::OPT_nodefaultlibs)) {
-      if (Args.hasArg(options::OPT_static))
+      if (Args.hasArg(options::OPT_static)) {
         CmdArgs.push_back("--start-group");
+        // when linking compiler-rt statically, we also need unwinder
+        // (note: this is implicit no-op for libgcc builds)
+        NeedsUnwinder = true;
+      }
 
       if (NeedsSanitizerDeps)
         linkSanitizerRuntimeDeps(ToolChain, CmdArgs);
 
       if (NeedsXRayDeps)
         linkXRayRuntimeDeps(ToolChain, Args, CmdArgs);
 
       bool WantPthread = Args.hasArg(options::OPT_pthread) ||
if (!Args.hasArg(options::OPT_nodefaultlibs)) {
           // Already diagnosed.
           break;
         }
         if (JA.isHostOffloading(Action::OFK_OpenMP))
           CmdArgs.push_back("-lomptarget");
       }
 
       AddRunTimeLibs(ToolChain, D, CmdArgs, Args);
+      // if unwinder is needed and we're using compiler-rt we need
+      // to explicitly link -lunwind in (libgcc has its own unwinder)
+      if (NeedsUnwinder && !isAndroid &&
+          ToolChain.GetRuntimeLibType(Args) == ToolChain::RLT_CompilerRT)
+        CmdArgs.push_back("-lunwind");
 
       if (WantPthread && !isAndroid)
         CmdArgs.push_back("-lpthread");
 
       if (Args.hasArg(options::OPT_fsplit_stack))
         CmdArgs.push_back("--wrap=pthread_create");
 
       CmdArgs.push_back("-lc");

test/Driver/linux-ld.c

 // CHECK-BASIC-LIBCXX-C-LINK-NOT: warning:
 // CHECK-BASIC-LIBCXX-C-LINK: "{{[^"]*}}clang{{[^"]*}}" "-cc1"
 // CHECK-BASIC-LIBCXX-C-LINK: "-isysroot" "[[SYSROOT:[^"]+]]"
 // CHECK-BASIC-LIBCXX-C-LINK-NOT: "-internal-isystem" "[[SYSROOT]]/usr/bin/../include/c++/v1"
 // CHECK-BASIC-LIBCXX-C-LINK: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
 // CHECK-BASIC-LIBCXX-C-LINK: "--sysroot=[[SYSROOT]]"
 // CHECK-BASIC-LIBCXX-C-LINK: "-L[[SYSROOT]]/usr/bin/../lib"
 //
+// Test that libc++ combined with compiler-rt includes -lunwind
+// (and no gcc libraries).
+// RUN: %clangxx -no-canonical-prefixes -x c++ %s -### -o %t.o 2>&1 \
+// RUN:     -target x86_64-unknown-linux-gnu \
+// RUN:     -stdlib=libc++ -rtlib=compiler-rt \
+// RUN:     -ccc-install-dir %S/Inputs/basic_linux_tree/usr/bin \
+// RUN:     --gcc-toolchain="" \
+// RUN:     --sysroot=%S/Inputs/basic_linux_libcxx_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-BASIC-LIBCXX-CLANGRT %s
+// CHECK-BASIC-LIBCXX-CLANGRT: "-lc++"
+// CHECK-BASIC-LIBCXX-CLANGRT: "-lunwind"
+// CHECK-BASIC-LIBCXX-CLANGRT-NOT: "-lgcc_s"
+// CHECK-BASIC-LIBCXX-CLANGRT-NOT: "-lgcc"
+// CHECK-BASIC-LIBCXX-CLANGRT-NOT: "-lgcc_eh"
+//
+// Test that compiler-rt with -static includes -lunwind.
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN:     --target=x86_64-unknown-linux-gnu \
+// RUN:     --gcc-toolchain="" \
+// RUN:     --sysroot=%S/Inputs/basic_linux_tree \
+// RUN:     --rtlib=compiler-rt \
+// RUN:     -static \
+// RUN:   | FileCheck --check-prefix=CHECK-BASIC-STATIC-CLANGRT %s
+// CHECK-BASIC-STATIC-CLANGRT: "-lunwind"
+// CHECK-BASIC-STATIC-CLANGRT-NOT: "-lgcc_s"
+// CHECK-BASIC-STATIC-CLANGRT-NOT: "-lgcc"
+// CHECK-BASIC-STATIC-CLANGRT-NOT: "-lgcc_eh"
+//
 // Test a very broken version of multiarch that shipped in Ubuntu 11.04.
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN:     --target=i386-unknown-linux \
 // RUN:     --gcc-toolchain="" \
 // RUN:     --sysroot=%S/Inputs/ubuntu_11.04_multiarch_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-UBUNTU-11-04 %s
 // CHECK-UBUNTU-11-04: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
 // CHECK-UBUNTU-11-04: "{{.*}}/usr/lib/i386-linux-gnu/gcc/i686-linux-gnu/4.5{{/|\\\\}}crtbegin.o"