[ToolChains] Link to compiler-rt with -L + -l when possible

[mstorsjo]

Summary

This avoids a libtool issue (https://debbugs.gnu.org/cgi/bugreport.cgi?bug=27866), where libtool fails to pick up the default linked libraries when they are referred to via a direct path to the static library, instead of as a -L + -l pair, as for libgcc.

Or is there some reason not to add the clang_rt dir to the linker path with -L?

[beanz]

Looping in @phosek.

[manojgupta]

Just a minor comment regarding test cases: Since you are adding both -L/path/ and -l<libname>, the test cases should be updated to check for the -L/path/ argument as well.

[lebedev.ri]

Since the libraries will no longer be specified with their full path, how will you know that the right library will be picked, the one compiler intended?

[mstorsjo]

In D51440#1217839, @manojgupta wrote:

Just a minor comment regarding test cases: Since you are adding both -L/path/ and -l<libname>, the test cases should be updated to check for the -L/path/ argument as well.

I guess I could do that, although we don't know the path in the test, so we can only check for -L.*.

In D51440#1217868, @lebedev.ri wrote:

Since the libraries will no longer be specified with their full path, how will you know that the right library will be picked, the one compiler intended?

We don't; the same goes for libgcc. I guess the risk for unintentional clashes is rather low; if the linker found another lib with the same name, I would expect it to be intentional, and using it wouldn't be wrong.

[manojgupta]

In D51440#1217910, @mstorsjo wrote:

In D51440#1217839, @manojgupta wrote:

Just a minor comment regarding test cases: Since you are adding both -L/path/ and -l<libname>, the test cases should be updated to check for the -L/path/ argument as well.

I guess I could do that, although we don't know the path in the test, so we can only check for -L.*.

You can add a testcase that passes a known path via -resource-dir=path argument similar to --sysroot argument.

[phosek]

What about other compiler-rt runtimes? Shouldn't we use the same approach for those?

In case of multiarch runtime layout, we already add the runtime directory to the new LibraryPaths list, which is added to the list of library paths. We could consider doing the same for the default layout, in which case all getCompilerRT would need to return is the name of the library rather than a path, but we would need to use -l for all compiler-rt runtimes, not just builtins. That's going to require more changes, but I think it's going to result in a cleaner design.

@beanz would that work for Darwin as well?

lib/Driver/ToolChains/CommonArgs.cpp

1164

Rather than parsing the return string below, wouldn't it be cleaner to modify ToolChain::getCompilerRT to return a tuple (ToolChain::getCompilerRTArgString could still return a string) and then use if here?

1168

When the multiarch runtime layout is being used, this will result in this path being added twice, once in the ToolChain constructor and once here.

[mstorsjo]

In D51440#1218817, @phosek wrote:

What about other compiler-rt runtimes? Shouldn't we use the same approach for those?

Yes, I guess so.

In case of multiarch runtime layout, we already add the runtime directory to the new LibraryPaths list, which is added to the list of library paths. We could consider doing the same for the default layout, in which case all getCompilerRT would need to return is the name of the library rather than a path, but we would need to use -l for all compiler-rt runtimes, not just builtins. That's going to require more changes, but I think it's going to result in a cleaner design.

Yes, so it sounds.

I'll see if I can get to looking at that sometime soon. I had this patch lying around as an attempt to work around the libtool issue in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=27866 which doesn't seem to be going anywhere (and even if it did, it probably takes quite a bit of time before a new libtool release is made and it gets propagated to most places where I'd like to use this), and was curious if there was any specific reason for having this the way it was, or just the usual; historical reasons that it has started out like this and haven't had a need to change until now. If you otherwise would happen to be touching the same areas, feel free to pick it up ;-) otherwise I'll try to look at addressing your points in a few days.

[phosek]

In D51440#1218859, @mstorsjo wrote:

I'll see if I can get to looking at that sometime soon. I had this patch lying around as an attempt to work around the libtool issue in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=27866 which doesn't seem to be going anywhere (and even if it did, it probably takes quite a bit of time before a new libtool release is made and it gets propagated to most places where I'd like to use this), and was curious if there was any specific reason for having this the way it was, or just the usual; historical reasons that it has started out like this and haven't had a need to change until now. If you otherwise would happen to be touching the same areas, feel free to pick it up ;-) otherwise I'll try to look at addressing your points in a few days.

I'd be happy to pick this up. I already planned to do more refactoring/cleanup of this part of the driver. @beanz is improving the Darwin toolchain driver to make more like other Clang drivers, so I want to first sync up with him to make sure this is also going to work for them.

[mstorsjo]

In D51440#1220925, @phosek wrote:

In D51440#1218859, @mstorsjo wrote:

I'll see if I can get to looking at that sometime soon. I had this patch lying around as an attempt to work around the libtool issue in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=27866 which doesn't seem to be going anywhere (and even if it did, it probably takes quite a bit of time before a new libtool release is made and it gets propagated to most places where I'd like to use this), and was curious if there was any specific reason for having this the way it was, or just the usual; historical reasons that it has started out like this and haven't had a need to change until now. If you otherwise would happen to be touching the same areas, feel free to pick it up ;-) otherwise I'll try to look at addressing your points in a few days.

I'd be happy to pick this up. I already planned to do more refactoring/cleanup of this part of the driver. @beanz is improving the Darwin toolchain driver to make more like other Clang drivers, so I want to first sync up with him to make sure this is also going to work for them.

Oh, great - thanks a lot!

[beanz]

Unfortunately I can't make this kind of policy decision about whether or not this would be acceptable for Darwin. That would probably need to be @dexonsmith.

[steven_wu]

I do prefer the current approach especially on Darwin. Some concerns of switching to use "-L + -l" are:

1. clang and compiler-rt are rev-locked. Inferring the compiler-rt from resource-dir and passing to linker with the full path can prevent mistakes of mixing them up.
2. This change does change linker semantics on Darwin. ld64 treats the inputs from command line with highest priority. Currently ld64 will use compiler-rt symbols before any other libraries passed in with -l flag. If clang decide to pass compiler-rt with -l flag, any other libraries (static or dynamic) that passed with -l flag will takes the priority over compiler-rt. This can lead to unexpected behavior.

[dexonsmith]

In D51440#1225318, @steven_wu wrote:

I do prefer the current approach especially on Darwin. Some concerns of switching to use "-L + -l" are:

1. clang and compiler-rt are rev-locked. Inferring the compiler-rt from resource-dir and passing to linker with the full path can prevent mistakes of mixing them up.
2. This change does change linker semantics on Darwin. ld64 treats the inputs from command line with highest priority. Currently ld64 will use compiler-rt symbols before any other libraries passed in with -l flag. If clang decide to pass compiler-rt with -l flag, any other libraries (static or dynamic) that passed with -l flag will takes the priority over compiler-rt. This can lead to unexpected behavior.

I tend to agree with Steven. I'd rather avoid a semantic change here.

lib/Driver/ToolChains/CommonArgs.cpp

 }
 
 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"));
+  case ToolChain::RLT_CompilerRT: {
+    StringRef Path = TC.getCompilerRTArgString(Args, "builtins");
+    StringRef Dir = llvm::sys::path::parent_path(Path);
+    StringRef Filename = llvm::sys::path::filename(Path);
+    if (Filename.consume_front("lib") && Filename.consume_back(".a")) {
+      CmdArgs.push_back(Args.MakeArgString("-L" + Dir));
+      CmdArgs.push_back(Args.MakeArgString("-l" + Filename));
+    } else
+      CmdArgs.push_back(Path);
     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";

test/Driver/arm-compiler-rt.c

 // RUN: %clang -target arm-linux-gnueabi -rtlib=compiler-rt -### %s 2>&1 | FileCheck %s -check-prefix ARM-GNUEABI
-// ARM-GNUEABI: "{{.*[/\\]}}libclang_rt.builtins-arm.a"
+// ARM-GNUEABI: "-lclang_rt.builtins-arm"
 
 // RUN: %clang -target arm-linux-gnueabi -rtlib=compiler-rt -mfloat-abi=hard -### %s 2>&1 | FileCheck %s -check-prefix ARM-GNUEABI-ABI
-// ARM-GNUEABI-ABI: "{{.*[/\\]}}libclang_rt.builtins-armhf.a"
+// ARM-GNUEABI-ABI: "-lclang_rt.builtins-armhf"
 
 // RUN: %clang -target arm-linux-gnueabihf -rtlib=compiler-rt -### %s 2>&1 | FileCheck %s -check-prefix ARM-GNUEABIHF
-// ARM-GNUEABIHF: "{{.*[/\\]}}libclang_rt.builtins-armhf.a"
+// ARM-GNUEABIHF: "-lclang_rt.builtins-armhf"
 
 // RUN: %clang -target arm-linux-gnueabihf -rtlib=compiler-rt -mfloat-abi=soft -### %s 2>&1 | FileCheck %s -check-prefix ARM-GNUEABIHF-ABI
-// ARM-GNUEABIHF-ABI: "{{.*[/\\]}}libclang_rt.builtins-arm.a"
+// ARM-GNUEABIHF-ABI: "-lclang_rt.builtins-arm"
 
 // RUN: %clang -target arm-windows-itanium -rtlib=compiler-rt -### %s 2>&1 | FileCheck %s -check-prefix ARM-WINDOWS
 // ARM-WINDOWS: "{{.*[/\\]}}clang_rt.builtins-arm.lib"
 
 // RUN: %clang -target arm-linux-androideabi -rtlib=compiler-rt -### %s 2>&1 | FileCheck %s -check-prefix ARM-ANDROID
-// ARM-ANDROID: "{{.*[/\\]}}libclang_rt.builtins-arm-android.a"
+// ARM-ANDROID: "-lclang_rt.builtins-arm-android"
 
 // RUN: %clang -target arm-linux-androideabi -rtlib=compiler-rt -mfloat-abi=hard -### %s 2>&1 | FileCheck %s -check-prefix ARM-ANDROIDHF
-// ARM-ANDROIDHF: "{{.*[/\\]}}libclang_rt.builtins-armhf-android.a"
+// ARM-ANDROIDHF: "-lclang_rt.builtins-armhf-android"
 

test/Driver/fuchsia.c

 // CHECK: "-pie"
 // CHECK: "--build-id"
 // CHECK: "--hash-style=gnu"
 // CHECK: "-dynamic-linker" "ld.so.1"
 // CHECK: Scrt1.o
 // CHECK-NOT: crti.o
 // CHECK-NOT: crtbegin.o
 // CHECK: "-L[[SYSROOT]]{{/|\\\\}}lib"
-// CHECK-X86_64: "{{.*[/\\]}}libclang_rt.builtins.a"
-// CHECK-AARCH64: "{{.*[/\\]}}libclang_rt.builtins.a"
+// CHECK-X86_64: -lclang_rt.builtins
+// CHECK-AARCH64: -lclang_rt.builtins
 // CHECK: "-lc"
 // CHECK-NOT: crtend.o
 // CHECK-NOT: crtn.o
 
 // RUN: %clang %s -### --target=x86_64-fuchsia -rtlib=libgcc -fuse-ld=lld 2>&1 \
 // RUN:     | FileCheck %s -check-prefix=CHECK-RTLIB
 // CHECK-RTLIB: error: invalid runtime library name in argument '-rtlib=libgcc'
 

test/Driver/fuchsia.cpp

 // CHECK: "-pie"
 // CHECK: "--build-id"
 // CHECK: "-dynamic-linker" "ld.so.1"
 // CHECK: Scrt1.o
 // CHECK-NOT: crti.o
 // CHECK-NOT: crtbegin.o
 // CHECK: "-L[[SYSROOT]]{{/|\\\\}}lib"
 // CHECK: "-lc++" "-lm"
-// CHECK: "{{.*[/\\]}}libclang_rt.builtins-x86_64.a"
+// CHECK: "-lclang_rt.builtins-x86_64"
 // CHECK: "-lc"
 // CHECK-NOT: crtend.o
 // CHECK-NOT: crtn.o
 
 // RUN: %clangxx %s -### --target=x86_64-unknown-fuchsia -stdlib=libstdc++ \
 // RUN:     -fuse-ld=lld 2>&1 \
 // RUN:     | FileCheck %s -check-prefix=CHECK-STDLIB
 // CHECK-STDLIB: error: invalid library name in argument '-stdlib=libstdc++'

test/Driver/linux-ld.c

 // CHECK-LD-RT: "-m" "elf_x86_64"
 // CHECK-LD-RT: "-dynamic-linker"
 // CHECK-LD-RT: "{{.*}}/usr/lib/gcc/x86_64-unknown-linux/4.6.0{{/|\\\\}}crtbegin.o"
 // CHECK-LD-RT: "-L[[SYSROOT]]/usr/lib/gcc/x86_64-unknown-linux/4.6.0"
 // CHECK-LD-RT: "-L[[SYSROOT]]/usr/lib/gcc/x86_64-unknown-linux/4.6.0/../../../../x86_64-unknown-linux/lib"
 // CHECK-LD-RT: "-L[[SYSROOT]]/usr/lib/gcc/x86_64-unknown-linux/4.6.0/../../.."
 // CHECK-LD-RT: "-L[[SYSROOT]]/lib"
 // CHECK-LD-RT: "-L[[SYSROOT]]/usr/lib"
-// CHECK-LD-RT: libclang_rt.builtins-x86_64.a"
+// CHECK-LD-RT: "-lclang_rt.builtins-x86_64"
 // CHECK-LD-RT: "-lc"
-// CHECK-LD-RT: libclang_rt.builtins-x86_64.a"
+// CHECK-LD-RT: "-lclang_rt.builtins-x86_64"
 //
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN:     --target=i686-unknown-linux \
 // RUN:     --gcc-toolchain="" \
 // RUN:     --sysroot=%S/Inputs/basic_linux_tree \
 // RUN:     --rtlib=compiler-rt \
 // RUN:   | FileCheck --check-prefix=CHECK-LD-RT-I686 %s
 // CHECK-LD-RT-I686-NOT: warning:
 // CHECK-LD-RT-I686: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
 // CHECK-LD-RT-I686: "--eh-frame-hdr"
 // CHECK-LD-RT-I686: "-m" "elf_i386"
 // CHECK-LD-RT-I686: "-dynamic-linker"
 // CHECK-LD-RT-I686: "{{.*}}/usr/lib/gcc/i686-unknown-linux/4.6.0{{/|\\\\}}crtbegin.o"
 // CHECK-LD-RT-I686: "-L[[SYSROOT]]/usr/lib/gcc/i686-unknown-linux/4.6.0"
 // CHECK-LD-RT-I686: "-L[[SYSROOT]]/usr/lib/gcc/i686-unknown-linux/4.6.0/../../../../i686-unknown-linux/lib"
 // CHECK-LD-RT-I686: "-L[[SYSROOT]]/usr/lib/gcc/i686-unknown-linux/4.6.0/../../.."
 // CHECK-LD-RT-I686: "-L[[SYSROOT]]/lib"
 // CHECK-LD-RT-I686: "-L[[SYSROOT]]/usr/lib"
-// CHECK-LD-RT-I686: libclang_rt.builtins-i386.a"
+// CHECK-LD-RT-I686: "-lclang_rt.builtins-i386"
 // CHECK-LD-RT-I686: "-lc"
-// CHECK-LD-RT-I686: libclang_rt.builtins-i386.a"
+// CHECK-LD-RT-I686: "-lclang_rt.builtins-i386"
 //
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN:     --target=arm-linux-androideabi \
 // RUN:     --gcc-toolchain="" \
 // RUN:     --sysroot=%S/Inputs/basic_android_tree/sysroot \
 // RUN:     --rtlib=compiler-rt \
 // RUN:   | FileCheck --check-prefix=CHECK-LD-RT-ANDROID %s
 // CHECK-LD-RT-ANDROID-NOT: warning:
 // CHECK-LD-RT-ANDROID: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
 // CHECK-LD-RT-ANDROID: "--eh-frame-hdr"
 // CHECK-LD-RT-ANDROID: "-m" "armelf_linux_eabi"
 // CHECK-LD-RT-ANDROID: "-dynamic-linker"
-// CHECK-LD-RT-ANDROID: libclang_rt.builtins-arm-android.a"
+// CHECK-LD-RT-ANDROID: "-lclang_rt.builtins-arm-android"
 // CHECK-LD-RT-ANDROID: "-lc"
-// CHECK-LD-RT-ANDROID: libclang_rt.builtins-arm-android.a"
+// CHECK-LD-RT-ANDROID: "-lclang_rt.builtins-arm-android"
 //
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN:     --target=x86_64-unknown-linux -rtlib=platform \
 // RUN:     --gcc-toolchain="" \
 // RUN:     --sysroot=%S/Inputs/basic_linux_tree \
 // RUN:     --rtlib=libgcc \
 // RUN:   | FileCheck --check-prefix=CHECK-LD-GCC %s
 // CHECK-LD-GCC-NOT: warning:

test/Driver/wasm-toolchain.c

 // A basic clang -cc1 command-line. WebAssembly is somewhat special in
 // enabling -fvisibility=hidden by default.
 
 // RUN: %clang %s -### -no-canonical-prefixes -target wasm32-unknown-unknown 2>&1 | FileCheck -check-prefix=CC1 %s
 // CC1: clang{{.*}} "-cc1" "-triple" "wasm32-unknown-unknown" {{.*}} "-fvisibility" "hidden" {{.*}}
 
 // Ditto, but ensure that a user -fvisibility=default disables the default
 // -fvisibility=hidden.
 
 // RUN: %clang %s -### -target wasm32-unknown-unknown -fvisibility=default 2>&1 | FileCheck -check-prefix=FVISIBILITY_DEFAULT %s
 // FVISIBILITY_DEFAULT-NOT: hidden
 
 // A basic C link command-line.
 
 // RUN: %clang -### -no-canonical-prefixes -target wasm32-unknown-unknown --sysroot=/foo -fuse-ld=wasm-ld %s 2>&1 | FileCheck -check-prefix=LINK %s
 // LINK: clang{{.*}}" "-cc1" {{.*}} "-o" "[[temp:[^"]*]]"
-// LINK: wasm-ld{{.*}}" "-L/foo/lib" "crt1.o" "[[temp]]" "-lc" "{{.*[/\\]}}libclang_rt.builtins-wasm32.a" "-o" "a.out"
+// LINK: wasm-ld{{.*}}" "-L/foo/lib" "crt1.o" "[[temp]]" "-lc" "-lclang_rt.builtins-wasm32" "-o" "a.out"
 
 // A basic C link command-line with optimization.
 
 // RUN: %clang -### -O2 -no-canonical-prefixes -target wasm32-unknown-unknown --sysroot=/foo -fuse-ld=wasm-ld %s 2>&1 | FileCheck -check-prefix=LINK_OPT %s
 // LINK_OPT: clang{{.*}}" "-cc1" {{.*}} "-o" "[[temp:[^"]*]]"
-// LINK_OPT: wasm-ld{{.*}}" "-L/foo/lib" "crt1.o" "[[temp]]" "-lc" "{{.*[/\\]}}libclang_rt.builtins-wasm32.a" "-o" "a.out"
+// LINK_OPT: wasm-ld{{.*}}" "-L/foo/lib" "crt1.o" "[[temp]]" "-lc" "-lclang_rt.builtins-wasm32" "-o" "a.out"

test/Driver/wasm-toolchain.cpp

 // A basic clang -cc1 command-line. WebAssembly is somewhat special in
 // enabling -fvisibility=hidden by default.
 
 // RUN: %clangxx %s -### -no-canonical-prefixes -target wasm32-unknown-unknown 2>&1 | FileCheck -check-prefix=CC1 %s
 // CC1: clang{{.*}} "-cc1" "-triple" "wasm32-unknown-unknown" {{.*}} "-fvisibility" "hidden" {{.*}}
 
 // Ditto, but ensure that a user -fvisibility=default disables the default
 // -fvisibility=hidden.
 
 // RUN: %clangxx %s -### -target wasm32-unknown-unknown -fvisibility=default 2>&1 | FileCheck -check-prefix=FVISIBILITY_DEFAULT %s
 // FVISIBILITY_DEFAULT-NOT: hidden
 
 // A basic C++ link command-line.
 
 // RUN: %clangxx -### -no-canonical-prefixes -target wasm32-unknown-unknown --sysroot=/foo --stdlib=c++ -fuse-ld=wasm-ld %s 2>&1 | FileCheck -check-prefix=LINK %s
 // LINK: clang{{.*}}" "-cc1" {{.*}} "-o" "[[temp:[^"]*]]"
-// LINK: wasm-ld{{.*}}" "-L/foo/lib" "crt1.o" "[[temp]]" "-lc++" "-lc++abi" "-lc" "{{.*[/\\]}}libclang_rt.builtins-wasm32.a" "-o" "a.out"
+// LINK: wasm-ld{{.*}}" "-L/foo/lib" "crt1.o" "[[temp]]" "-lc++" "-lc++abi" "-lc" "-lclang_rt.builtins-wasm32" "-o" "a.out"
 
 // A basic C++ link command-line with optimization.
 
 // RUN: %clangxx -### -O2 -no-canonical-prefixes -target wasm32-unknown-unknown --sysroot=/foo %s --stdlib=c++ -fuse-ld=wasm-ld 2>&1 | FileCheck -check-prefix=LINK_OPT %s
 // LINK_OPT: clang{{.*}}" "-cc1" {{.*}} "-o" "[[temp:[^"]*]]"
-// LINK_OPT: wasm-ld{{.*}}" "-L/foo/lib" "crt1.o" "[[temp]]" "-lc++" "-lc++abi" "-lc" "{{.*[/\\]}}libclang_rt.builtins-wasm32.a" "-o" "a.out"
+// LINK_OPT: wasm-ld{{.*}}" "-L/foo/lib" "crt1.o" "[[temp]]" "-lc++" "-lc++abi" "-lc" "-lclang_rt.builtins-wasm32" "-o" "a.out"