Add --unwindlib=[libgcc|compiler-rt] to parallel --rtlib=

[saugustine]

Summary

"clang++ hello.cc --rtlib=compiler-rt"

now works without specifying additional unwind or exception
handling libraries.

[rsmith]

Looks good to me, thanks! Let's try this and see if it shakes out any problems.

We really need to give LLVM's unwinder a better name.

clang/include/clang/Driver/Options.td

2572 ↗

(On Diff #183211)

I think libgcc_eh here should be just libgcc, and compiler-rt-unwind should be just compiler-rt, right?

[saugustine]

• Correct help text for --unwindlib options.

clang/include/clang/Driver/Options.td

2572 ↗

(On Diff #183211)

Good catch. My original design had their own names, but in my own testing I found it awkward so switched out and forgot to update these.

[phosek]

This has broken all our toolchain builders (https://luci-milo.appspot.com/p/fuchsia/g/clang/console). These are using the runtimes build https://github.com/llvm/llvm-project/blob/master/llvm/runtimes/CMakeLists.txt which can be used to build compiler runtimes using the just-built Clang. The way that works is runtimes build first builds builtins for all specified targets (by invoking builtins build directly in https://github.com/llvm/llvm-project/blob/master/compiler-rt/lib/builtins/CMakeLists.txt), and then as second step builds remaining runtimes (libunwind, libc++abi, libc++, and the rest of compiler-rt). Now, with this change this breaks the second phase because where before Clang has been only looking for compiler-rt builtins (which have been built in the first phase) now Clang is also looking for -luwind which hasn't been built yet. There's actually a cycle because CMake checks ran by libunwind build are looking for libunwind (I see a lot of ld.lld: error: unable to find library -lunwind errors in the CMake log).

I think we'll probably need to update the CMake build to pass -nostdlib when running the checks in llvm/cmake/modules/HandleLLVMOptions.cmake (that's invoked from https://github.com/llvm/llvm-project/blob/master/llvm/runtimes/CMakeLists.txt#L117). However, it'll take a me while to prepare that change and test it, is there any chance we could revert this in the meantime?

[phosek]

One more thing, shouldn't libunwind be only included for C++ (since it's the internal dependency of libc++abi)? There shouldn't be any need to include it for C.

[kristina]

In D57128#1374928, @phosek wrote:

One more thing, shouldn't libunwind be only included for C++ (since it's the internal dependency of libc++abi)? There shouldn't be any need to include it for C.

There's some dark magic done by glibc and thread cancellation, in absence of libgcc_s I think it may weakly link against symbols that would be provided by any unwinder library. I think it not being exposed may break glibc if using thread cancellation (not sure if that's used by a lot) without libgcc_s and without libc++abi loaded (to pull in the unwinder).

In D57128#1370712, @rsmith wrote:

Looks good to me, thanks! Let's try this and see if it shakes out any problems.

We really need to give LLVM's unwinder a better name.

I think most just call it libunwind_llvm?

[mstorsjo]

FWIW, this also broke my bootstraps of mingw-w64 environments/toolchains. After building compiler-rt builtins, before having any libunwind/libcxx built, I previously regarded my toolchain as complete for building and testing C apps and libraries, but that fails now.

Would it be possible to add a third alternative, --unwindlib=none, to signal that while I'm using --rtlib=compiler-rt, I don't want to link to any unwinder? (In my case, I'm injecting libunwind in libc++.a so it only gets added when linking C++ code.) Or at least make it possible to only add this linker flag when linking C++? Alternatively I'll need to provide a dummy libunwind.a until the real one has been built.

[kristina]

In D57128#1375040, @mstorsjo wrote:

FWIW, this also broke my bootstraps of mingw-w64 environments/toolchains. After building compiler-rt builtins, before having any libunwind/libcxx built, I previously regarded my toolchain as complete for building and testing C apps and libraries, but that fails now.

Would it be possible to add a third alternative, --unwindlib=none, to signal that while I'm using --rtlib=compiler-rt, I don't want to link to any unwinder? (In my case, I'm injecting libunwind in libc++.a so it only gets added when linking C++ code.) Or at least make it possible to only add this linker flag when linking C++? Alternatively I'll need to provide a dummy libunwind.a until the real one has been built.

+1 on this, although consolidated libc++/libc++abi/libunwind_llvm have been supported "experimentally" for at least two years now, if this somehow breaks it, I think a lot of users would be frustrated.

(Also is it time to make merging libc++.a and libc++abi.a not-an-experimental (in CMake) feature anymore?).

[phosek]

In D57128#1375049, @kristina wrote:

In D57128#1375040, @mstorsjo wrote:

FWIW, this also broke my bootstraps of mingw-w64 environments/toolchains. After building compiler-rt builtins, before having any libunwind/libcxx built, I previously regarded my toolchain as complete for building and testing C apps and libraries, but that fails now.

Would it be possible to add a third alternative, --unwindlib=none, to signal that while I'm using --rtlib=compiler-rt, I don't want to link to any unwinder? (In my case, I'm injecting libunwind in libc++.a so it only gets added when linking C++ code.) Or at least make it possible to only add this linker flag when linking C++? Alternatively I'll need to provide a dummy libunwind.a until the real one has been built.

+1 on this, although consolidated libc++/libc++abi/libunwind_llvm have been supported "experimentally" for at least two years now, if this somehow breaks it, I think a lot of users would be frustrated.

(Also is it time to make merging libc++.a and libc++abi.a not-an-experimental (in CMake) feature anymore?).

+2 this is how we're shipping static libunwind in our (Fuchsia) toolchain for all platforms, we merge it (together with libc++abi.a) into libc++.a, so beyond the bootstrap issues, this change is going to break our build unless we provide dummy libunwind.a. Ideally there should be a vendor flag (e.g. a CMake option) to set this as default when building Clang (akin to [CLANG_DEFAULT_CXX_STDLIB](https://github.com/llvm/llvm-project/blob/master/clang/CMakeLists.txt#L244) and [CLANG_DEFAULT_RTLIB](https://github.com/llvm/llvm-project/blob/master/clang/CMakeLists.txt#L254)), having to manually always set --unwindlib=none for every compiler invocation is going to be incredibly annoying.

[uabelho]

We're having issues with this as well, we don't have any libunwind so this change causes
/usr/bin/ld: cannot find -lunwind

[saugustine]

Sorry for the breakage everyone, I'm preparing a rollback now.

I think this shows that everyone using rtlib had their own method of getting the unwind symbols. We ought to figure out a standardized method.

glibc's pthread cancel does assume that the unwind symbols would be available. The --as-needed flags should prevent it from being a hard dependency in plain C, but that doesn't solve the "no file present" problem.

The biggest issue with setting it separately is that would require a second command-line argument every time. Need to think about how to handle that case.

[saugustine]

This was rolled back with r352524. I'll work out something better.

cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td

 def err_drv_invalid_linker_name : Error<
   "invalid linker name in argument '%0'">;
 def err_drv_invalid_pgo_instrumentor : Error<
   "invalid PGO instrumentor in argument '%0'">;
 def err_drv_invalid_rtlib_name : Error<
   "invalid runtime library name in argument '%0'">;
 def err_drv_unsupported_rtlib_for_platform : Error<
   "unsupported runtime library '%0' for platform '%1'">;
+def err_drv_invalid_unwindlib_name : Error<
+  "invalid unwind library name in argument '%0'">;
+def err_drv_incompatible_unwindlib : Error<
+  "--rtlib=libgcc requires --unwindlib=libgcc">;
 def err_drv_invalid_stdlib_name : Error<
   "invalid library name in argument '%0'">;
 def err_drv_invalid_output_with_multiple_archs : Error<
   "cannot use '%0' output with multiple -arch options">;
 def err_drv_no_input_files : Error<"no input files">;
 def err_drv_use_of_Z_option : Error<
   "unsupported use of internal gcc -Z option '%0'">;
 def err_drv_output_argument_with_multiple_files : Error<

cfe/trunk/include/clang/Driver/Options.td

ValuesCode<[{
     const char *Values =
     #define LANGSTANDARD(id, name, lang, desc, features) name ","
     #define LANGSTANDARD_ALIAS(id, alias) alias ","
     #include "clang/Frontend/LangStandards.def"
     ;
   }]>;
 def stdlib_EQ : Joined<["-", "--"], "stdlib=">, Flags<[CC1Option]>,
   HelpText<"C++ standard library to use">, Values<"libc++,libstdc++,platform">;
+def unwindlib_EQ : Joined<["-", "--"], "unwindlib=">, Flags<[CC1Option]>,
+  HelpText<"Unwind library to use">, Values<"libgcc_eh,compiler-rt-unwind,platform">;
 def sub__library : JoinedOrSeparate<["-"], "sub_library">;
 def sub__umbrella : JoinedOrSeparate<["-"], "sub_umbrella">;
 def system_header_prefix : Joined<["--"], "system-header-prefix=">,
   Group<clang_i_Group>, Flags<[CC1Option]>, MetaVarName<"<prefix>">,
   HelpText<"Treat all #include paths starting with <prefix> as including a "
            "system header.">;
 def : Separate<["--"], "system-header-prefix">, Alias<system_header_prefix>;
 def no_system_header_prefix : Joined<["--"], "no-system-header-prefix=">,

cfe/trunk/include/clang/Driver/ToolChain.h

public:
   virtual RuntimeLibType GetDefaultRuntimeLibType() const {
     return ToolChain::RLT_Libgcc;
   }
 
   virtual CXXStdlibType GetDefaultCXXStdlibType() const {
     return ToolChain::CST_Libstdcxx;
   }
 
+  virtual RuntimeLibType GetDefaultUnwindLibType() const {
+    return ToolChain::RLT_Libgcc;
+  }
+
   virtual std::string getCompilerRTPath() const;
 
   virtual std::string getCompilerRT(const llvm::opt::ArgList &Args,
                                     StringRef Component,
                                     bool Shared = false) const;
 
   const char *getCompilerRTArgString(const llvm::opt::ArgList &Args,
                                      StringRef Component,
public:
   // given compilation arguments.
   virtual RuntimeLibType
   GetRuntimeLibType(const llvm::opt::ArgList &Args) const;
 
   // GetCXXStdlibType - Determine the C++ standard library type to use with the
   // given compilation arguments.
   virtual CXXStdlibType GetCXXStdlibType(const llvm::opt::ArgList &Args) const;
 
+  // GetUnwindLibType - Determine the unwind library type to use with the
+  // given compilation arguments.
+  virtual RuntimeLibType GetUnwindLibType(const llvm::opt::ArgList &Args) const;
+
   /// AddClangCXXStdlibIncludeArgs - Add the clang -cc1 level arguments to set
   /// the include paths to use for the given C++ standard library type.
   virtual void
   AddClangCXXStdlibIncludeArgs(const llvm::opt::ArgList &DriverArgs,
                                llvm::opt::ArgStringList &CC1Args) const;
 
   /// Returns if the C++ standard library should be linked in.
   /// Note that e.g. -lm should still be linked even if this returns false.

cfe/trunk/lib/Driver/ToolChain.cpp

else if (LibName == "platform")
     return GetDefaultRuntimeLibType();
 
   if (A)
     getDriver().Diag(diag::err_drv_invalid_rtlib_name) << A->getAsString(Args);
 
   return GetDefaultRuntimeLibType();
 }
 
+ToolChain::RuntimeLibType ToolChain
+::GetUnwindLibType(
+    const ArgList &Args) const {
+  const Arg *A = Args.getLastArg(options::OPT_unwindlib_EQ);
+  // If nothing has been specified, follow the runtime lib type.
+  if (!A)
+    return GetRuntimeLibType(Args);
+
+  StringRef LibName = A->getValue();
+  if (LibName == "compiler-rt") {
+    if (GetRuntimeLibType(Args) == RLT_Libgcc)
+      getDriver().Diag(diag::err_drv_incompatible_unwindlib);
+    return ToolChain::RLT_CompilerRT;
+  }
+  else if (LibName == "libgcc")
+    return ToolChain::RLT_Libgcc;
+  else if (LibName == "platform") {
+    return GetRuntimeLibType(Args);
+  }
+
+  if (A)
+    getDriver().Diag(diag::err_drv_invalid_unwindlib_name)
+        << A->getAsString(Args);
+
+  return GetDefaultUnwindLibType();
+}
+
 ToolChain::CXXStdlibType ToolChain::GetCXXStdlibType(const ArgList &Args) const{
   const Arg *A = Args.getLastArg(options::OPT_stdlib_EQ);
   StringRef LibName = A ? A->getValue() : CLANG_DEFAULT_CXX_STDLIB;
 
   // Only use "platform" in tests to override CLANG_DEFAULT_CXX_STDLIB!
   if (LibName == "libc++")
     return ToolChain::CST_Libcxx;
   else if (LibName == "libstdc++")

cfe/trunk/lib/Driver/ToolChains/CommonArgs.cpp

 }
 
 /// Determine whether Objective-C automated reference counting is
 /// enabled.
 bool tools::isObjCAutoRefCount(const ArgList &Args) {
   return Args.hasFlag(options::OPT_fobjc_arc, options::OPT_fno_objc_arc, false);
 }
 
-static void AddLibgcc(const llvm::Triple &Triple, const Driver &D,
-                      ArgStringList &CmdArgs, const ArgList &Args) {
-  bool isAndroid = Triple.isAndroid();
-  bool isCygMing = Triple.isOSCygMing();
-  bool IsIAMCU = Triple.isOSIAMCU();
-  bool StaticLibgcc = Args.hasArg(options::OPT_static_libgcc) ||
-                      Args.hasArg(options::OPT_static);
+enum class LibGccType { UnspecifiedLibGcc, StaticLibGcc, SharedLibGcc };
+
+static LibGccType getLibGccType(const ArgList &Args) {
+  bool Static = Args.hasArg(options::OPT_static_libgcc) ||
 
-  bool SharedLibgcc = Args.hasArg(options::OPT_shared_libgcc);
-  bool UnspecifiedLibgcc = !StaticLibgcc && !SharedLibgcc;
+  bool Shared = Args.hasArg(options::OPT_shared_libgcc);
+  if (Shared)
+    return LibGccType::SharedLibGcc;
+  if (Static)
+    return LibGccType::StaticLibGcc;
+  return LibGccType::UnspecifiedLibGcc;
+}
+
+static void AddUnwindLibrary(const ToolChain &TC, const Driver &D,
+                             ArgStringList &CmdArgs, const ArgList &Args) {
+  // Targets that don't use unwind libraries.
+  if (TC.getTriple().isAndroid() || TC.getTriple().isOSIAMCU() ||
+      TC.getTriple().isOSBinFormatWasm())
+    return;
+
+  ToolChain::RuntimeLibType UNW = TC.GetUnwindLibType(Args);
+  switch (UNW) {
+  case ToolChain::RLT_Libgcc: {
+    LibGccType LGT = getLibGccType(Args);
+    if (LGT == LibGccType::UnspecifiedLibGcc || LGT == LibGccType::SharedLibGcc)
+      CmdArgs.push_back("-lgcc_s");
+    else if (LGT == LibGccType::StaticLibGcc)
+      CmdArgs.push_back("-lgcc_eh");
+    break;
+  }
+  case ToolChain::RLT_CompilerRT:
+    CmdArgs.push_back("-lunwind");
+    break;
+  }
+}
 
-  // Gcc adds libgcc arguments in various ways:
-  //
-  // gcc <none>: -lgcc --as-needed -lgcc_s --no-as-needed
-  // g++ <none>:                   -lgcc_s               -lgcc
-  // gcc shared:                   -lgcc_s               -lgcc
-  // g++ shared:                   -lgcc_s               -lgcc
-  // gcc static: -lgcc             -lgcc_eh
-  // g++ static: -lgcc             -lgcc_eh
-  //
-  // Also, certain targets need additional adjustments.
 
-  bool LibGccFirst = (D.CCCIsCC() && UnspecifiedLibgcc) || StaticLibgcc;
+static void AddLibgcc(const ToolChain &TC, const Driver &D,
+                      ArgStringList &CmdArgs, const ArgList &Args) {
+  bool isAndroid = TC.getTriple().isAndroid();
+  LibGccType LGT = getLibGccType(Args);
+
+  bool LibGccFirst = (D.CCCIsCC() && LGT == LibGccType::UnspecifiedLibGcc) ||
+                     LGT == LibGccType::StaticLibGcc;
   if (LibGccFirst)
     CmdArgs.push_back("-lgcc");
 
-  bool AsNeeded = D.CCCIsCC() && UnspecifiedLibgcc && !isAndroid && !isCygMing;
+  bool AsNeeded = D.CCCIsCC() && LGT == LibGccType::UnspecifiedLibGcc &&
+                  !isAndroid && !TC.getTriple().isOSCygMing();
   if (AsNeeded)
     CmdArgs.push_back("--as-needed");
 
-  if ((UnspecifiedLibgcc || SharedLibgcc) && !isAndroid)
-    CmdArgs.push_back("-lgcc_s");
-
-  else if (StaticLibgcc && !isAndroid && !IsIAMCU)
-    CmdArgs.push_back("-lgcc_eh");
+  AddUnwindLibrary(TC, D, CmdArgs, Args);
 
   if (AsNeeded)
     CmdArgs.push_back("--no-as-needed");
 
   if (!LibGccFirst)
     CmdArgs.push_back("-lgcc");
 
   // According to Android ABI, we have to link with libdl if we are
   // linking with non-static libgcc.
   //
   // NOTE: This fixes a link error on Android MIPS as well.  The non-static
   // libgcc for MIPS relies on _Unwind_Find_FDE and dl_iterate_phdr from libdl.
-  if (isAndroid && !StaticLibgcc)
+  if (isAndroid && LGT != LibGccType::StaticLibGcc)
     CmdArgs.push_back("-ldl");
 }
 
 void tools::AddRunTimeLibs(const ToolChain &TC, const Driver &D,
                            ArgStringList &CmdArgs, const ArgList &Args) {
   // Make use of compiler-rt if --rtlib option is used
   ToolChain::RuntimeLibType RLT = TC.GetRuntimeLibType(Args);
 
   switch (RLT) {
   case ToolChain::RLT_CompilerRT:
     CmdArgs.push_back(TC.getCompilerRTArgString(Args, "builtins"));
+    AddUnwindLibrary(TC, D, CmdArgs, Args);
     break;
   case ToolChain::RLT_Libgcc:
     // Make sure libgcc is not used under MSVC environment by default
     if (TC.getTriple().isKnownWindowsMSVCEnvironment()) {
       // Issue error diagnostic if libgcc is explicitly specified
       // through command line as --rtlib option argument.
       if (Args.hasArg(options::OPT_rtlib_EQ)) {
         TC.getDriver().Diag(diag::err_drv_unsupported_rtlib_for_platform)
             << Args.getLastArg(options::OPT_rtlib_EQ)->getValue() << "MSVC";
       }
     } else
-      AddLibgcc(TC.getTriple(), D, CmdArgs, Args);
+      AddLibgcc(TC, D, CmdArgs, Args);
     break;
   }
 }
 
 /// Add OpenMP linker script arguments at the end of the argument list so that
 /// the fat binary is built by embedding each of the device images into the
 /// host. The linker script also defines a few symbols required by the code
 /// generation so that the images can be easily retrieved at runtime by the

cfe/trunk/test/Driver/compiler-rt-unwind.c

+// General tests that the driver handles combinations of --rtlib=XXX and
+// --unwindlib=XXX properly.
+//
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN:     --target=x86_64-unknown-linux \
+// RUN:     --gcc-toolchain="" \
+// RUN:   | FileCheck --check-prefix=RTLIB-EMPTY %s
+// RTLIB-EMPTY: "{{.*}}lgcc"
+// RTLIB-EMPTY: "{{.*}}-lgcc_s"
+//
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN:     --target=x86_64-unknown-linux -rtlib=gcc \
+// RUN:     --gcc-toolchain="" \
+// RUN:   | FileCheck --check-prefix=RTLIB-GCC %s
+// RTLIB-GCC: "{{.*}}lgcc"
+// RTLIB-GCC: "{{.*}}lgcc_s"
+//
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN:     --target=x86_64-unknown-linux -rtlib=gcc --unwindlib=compiler-rt \
+// RUN:     --gcc-toolchain="" \
+// RUN:   | FileCheck --check-prefix=RTLIB-GCC-UNWINDLIB-COMPILER-RT %s
+// RTLIB-GCC-UNWINDLIB-COMPILER-RT: "{{.*}}lgcc"
+// RTLIB-GCC-UNWINDLIB-COMPILER-RT: "{{.*}}lunwind"
+//
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1   \
+// RUN:     --target=x86_64-unknown-linux -rtlib=compiler-rt \
+// RUN:     --gcc-toolchain="" \
+// RUN:   | FileCheck --check-prefix=RTLIB-COMPILER-RT %s
+// RTLIB-COMPILER-RT: "{{.*}}libclang_rt.builtins-x86_64.a"
+// RTLIB-COMPILER-RT: "{{.*}}-lunwind"
+//
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1   \
+// RUN:     --target=x86_64-unknown-linux -rtlib=compiler-rt --unwindlib=gcc \
+// RUN:     --gcc-toolchain="" \
+// RUN:   | FileCheck --check-prefix=RTLIB-COMPILER-RT-UNWINDLIB-GCC %s
+// RTLIB-COMPILER-RT-UNWINDLIB-GCC: "{{.*}}libclang_rt.builtins-x86_64.a"
+// RTLIB-COMPILER-RT-UNWINDLIB-GCC: "{{.*}}lgcc_s"
+//
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1              \
+// RUN:     --target=x86_64-unknown-linux -rtlib=compiler-rt --unwindlib=gcc \
+// RUN:     -static --gcc-toolchain="" \
+// RUN:   | FileCheck --check-prefix=RTLIB-COMPILER-RT-UNWINDLIB-GCC-STATIC %s
+// RTLIB-COMPILER-RT-UNWINDLIB-GCC-STATIC: "{{.*}}libclang_rt.builtins-x86_64.a"
+// RTLIB-COMPILER-RT-UNWINDLIB-GCC-STATIC: "{{.*}}lgcc_eh"
+//
+// RUN: not %clang -no-canonical-prefixes %s -o %t.o 2> %t.err              \
+// RUN:     --target=x86_64-unknown-linux -rtlib=libgcc --unwindlib=compiler-rt \
+// RUN:     --gcc-toolchain="" \
+// RUN: FileCheck --input-file=%t.err --check-prefix=RTLIB-GCC-UNWINDLIB-COMPILER_RT %s
+// RTLIB-GCC-UNWINDLIB-COMPILER_RT: "{{[.|\\\n]*}}--rtlib=libgcc requires --unwindlib=libgcc"