[Driver] Correctly handle static C++ standard library

[phosek]

Summary

When statically linking C++ standard library, we shouldn't add -Bdynamic
after including the library on the link line because that might override
user settings like -static and -static-pie. Rather, we should surround
the library with --push-state/--pop-state to make sure that -Bstatic
only applies to C++ standard library and nothing else. This has been
supported since GNU ld 2.25 (2014) so backwards compatibility should
no longer be a concern.

[MaskRay]

LGTM.

I looked at this in 2019 but did not change because GNU ld 2.25 (2014) introduced --push-state. (gold added it in 2016 but I think we can have higher version requirement for gold.)
In 2021, binutils 2.24 (2013) compatibility should be irrelevant...

clang/lib/Driver/ToolChains/Fuchsia.cpp

143

--as-needed only applies to shared objects so you can move it into the else code path.

clang/lib/Driver/ToolChains/Gnu.cpp

581

The --as-needed change should be dropped from this patch.

[MaskRay]

clang/test/Driver/linux-ld.c

499

-no-canonical-prefixes is only useful when the CHECK lines inspect the spelling of clang.
Otherwise -no-canonical-prefixes can be omitted.

Omit -o %t.o since it is not used.

504

To improve the robustness of the test, use the -SAME: {{^}} style in linux-cross.cpp, or just place these consecutive options on the same line.

509

Prefer --target= to -target

[MaskRay]

Thanks!

[MaskRay]

Please mention GNU ld 2.25 (2014) in the description.

[phosek]

Looks like the linker used on clang-ppc64le-rhel bot doesn't support --push-state/--pop-state.

[phosek]

@MaskRay Do you think we should gate the use of this feature on -fbinutils-version= or -fuse-ld=lld? It'd be nice if the owner of clang-ppc64le-rhel builder could update the binutils version but I'm not sure how feasible is it.

[MaskRay]

clang/lib/Driver/ToolChains/Gnu.cpp

581

--push-state should only be added when -Bstatic is added.

Then the old clang-ppc64le-rhel issue may be moot because they probably don't use -static-libstdc++ ...

[MaskRay]

clang/lib/Driver/ToolChains/Fuchsia.cpp

145

as-needed needs push-state as well.

[nemanjai]

In D110128#3018992, @phosek wrote:

@MaskRay Do you think we should gate the use of this feature on -fbinutils-version= or -fuse-ld=lld? It'd be nice if the owner of clang-ppc64le-rhel builder could update the binutils version but I'm not sure how feasible is it.

We certainly don't mind updating binutils to a supported version. However, if a specific version of binutils (or really any other software package) is required by Clang/LLVM, that should be documented at https://llvm.org/docs/GettingStarted.html#software

Of course, if our bot has a version of binutils that is older than the one listed there currently, we will be sure to update it ASAP.

But of course, as @MaskRay pointed out, this may not be an issue.

[phosek]

In D110128#3020468, @nemanjai wrote:

In D110128#3018992, @phosek wrote:

@MaskRay Do you think we should gate the use of this feature on -fbinutils-version= or -fuse-ld=lld? It'd be nice if the owner of clang-ppc64le-rhel builder could update the binutils version but I'm not sure how feasible is it.

We certainly don't mind updating binutils to a supported version. However, if a specific version of binutils (or really any other software package) is required by Clang/LLVM, that should be documented at https://llvm.org/docs/GettingStarted.html#software

Of course, if our bot has a version of binutils that is older than the one listed there currently, we will be sure to update it ASAP.

But of course, as @MaskRay pointed out, this may not be an issue.

Thanks for reaching out @nemanjai, I assume you're responsible for this builder? Do you know what version of Binutils is currently installed on that machine? Is newer version available for RHEL 7?

We don't specify Binutils version on https://llvm.org/docs/GettingStarted.html but I think we should. We need to determine that baseline first though. It looks like 2.25 roughly corresponds to GCC 5.1 which is the minimum requirement for the compiler.

[thakis]

This breaks our packaging builders too: https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket/8835201798501639249/+/u/package_clang/stdout?format=raw

[thakis]

Note that stage 2 fails, where we're supposed to use lld. So I think this is probably some compiler-rt test config problem you need to sort out, and not a fundamental problem with the patch.

[thakis]

Re-reverted for now in 6ece82e9006d16b7fba7660ce09b2c62ab8460fa.

[phosek]

In D110128#3021876, @thakis wrote:

Note that stage 2 fails, where we're supposed to use lld. So I think this is probably some compiler-rt test config problem you need to sort out, and not a fundamental problem with the patch.

It looks like the LLVM_ENABLE_LLD option doesn't affect compiler-rt tests.

clang/lib/Driver/ToolChains/Fuchsia.cpp

if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nodefaultlibs)) {
 
     if (D.CCCIsCXX()) {
       if (ToolChain.ShouldLinkCXXStdlib(Args)) {
         bool OnlyLibstdcxxStatic = Args.hasArg(options::OPT_static_libstdcxx) &&
                                    !Args.hasArg(options::OPT_static);
         CmdArgs.push_back("--push-state");
         CmdArgs.push_back("--as-needed");
         if (OnlyLibstdcxxStatic)
           CmdArgs.push_back("-Bstatic");
         ToolChain.AddCXXStdlibLibArgs(Args, CmdArgs);
-        if (OnlyLibstdcxxStatic)
-          CmdArgs.push_back("-Bdynamic");
-        CmdArgs.push_back("-lm");
         CmdArgs.push_back("--pop-state");
+        CmdArgs.push_back("-lm");
       }
     }
 
     if (NeedsSanitizerDeps)
       linkSanitizerRuntimeDeps(ToolChain, CmdArgs);
 
     if (NeedsXRayDeps)
       linkXRayRuntimeDeps(ToolChain, CmdArgs);

clang/lib/Driver/ToolChains/Gnu.cpp

void tools::gnutools::Linker::ConstructJob(Compilation &C, const JobAction &JA,
   // The profile runtime also needs access to system libraries.
   getToolChain().addProfileRTLibs(Args, CmdArgs);
 
   if (D.CCCIsCXX() &&
       !Args.hasArg(options::OPT_nostdlib, options::OPT_nodefaultlibs)) {
     if (ToolChain.ShouldLinkCXXStdlib(Args)) {
       bool OnlyLibstdcxxStatic = Args.hasArg(options::OPT_static_libstdcxx) &&
                                  !Args.hasArg(options::OPT_static);
+      CmdArgs.push_back("--push-state");
+      CmdArgs.push_back("--as-needed");
       if (OnlyLibstdcxxStatic)
         CmdArgs.push_back("-Bstatic");
       ToolChain.AddCXXStdlibLibArgs(Args, CmdArgs);
-      if (OnlyLibstdcxxStatic)
-        CmdArgs.push_back("-Bdynamic");
+      CmdArgs.push_back("--pop-state");
     }
     CmdArgs.push_back("-lm");
   }
   // 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)) {

clang/test/Driver/fuchsia.cpp

 // CHECK: "-dynamic-linker" "ld.so.1"
 // CHECK: Scrt1.o
 // CHECK-NOT: crti.o
 // CHECK-NOT: crtbegin.o
 // CHECK: "-L[[SYSROOT]]{{/|\\\\}}lib"
 // CHECK: "--push-state"
 // CHECK: "--as-needed"
 // CHECK: "-lc++"
-// CHECK: "-lm"
 // CHECK: "--pop-state"
+// CHECK: "-lm"
 // CHECK-X86_64: "[[RESOURCE_DIR]]{{/|\\\\}}lib{{/|\\\\}}x86_64-unknown-fuchsia{{/|\\\\}}libclang_rt.builtins.a"
 // CHECK-AARCH64: "[[RESOURCE_DIR]]{{/|\\\\}}lib{{/|\\\\}}aarch64-unknown-fuchsia{{/|\\\\}}libclang_rt.builtins.a"
 // CHECK-RISCV64: "[[RESOURCE_DIR]]{{/|\\\\}}lib{{/|\\\\}}riscv64-unknown-fuchsia{{/|\\\\}}libclang_rt.builtins.a"
 // 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++'
 
 // RUN: %clangxx %s -### --target=x86_64-unknown-fuchsia -static-libstdc++ \
 // RUN:     -fuse-ld=lld 2>&1 \
 // RUN:     | FileCheck %s -check-prefix=CHECK-STATIC
 // CHECK-STATIC: "--push-state"
 // CHECK-STATIC: "--as-needed"
 // CHECK-STATIC: "-Bstatic"
 // CHECK-STATIC: "-lc++"
-// CHECK-STATIC: "-Bdynamic"
-// CHECK-STATIC: "-lm"
 // CHECK-STATIC: "--pop-state"
+// CHECK-STATIC: "-lm"
 // CHECK-STATIC: "-lc"
 
 // RUN: %clangxx %s -### --target=x86_64-unknown-fuchsia -nostdlib++ -fuse-ld=lld 2>&1 \
 // RUN:     | FileCheck %s -check-prefix=CHECK-NOSTDLIBXX
 // CHECK-NOSTDLIBXX-NOT: "-lc++"
 // CHECK-NOSTDLIBXX-NOT: "-lm"
 // CHECK-NOSTDLIBXX: "-lc"
 

clang/test/Driver/linux-ld.c

 // RUN:     --target=i386-unknown-linux -rtlib=platform -m32 \
 // RUN:     -ccc-install-dir %S/Inputs/gcc_version_parsing1/bin \
 // RUN:     --gcc-toolchain="" \
 // RUN:     --sysroot=%S/Inputs/basic_linux_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-GCC-VERSION1 %s
 // CHECK-GCC-VERSION1: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
 // CHECK-GCC-VERSION1: "{{.*}}/Inputs/basic_linux_tree/usr/lib/gcc/i386-unknown-linux/10.2.0{{/|\\\\}}crtbegin.o"
 
+// RUN: %clangxx -no-canonical-prefixes -x c++ %s -### -o %t.o 2>&1 \
+// RUN:     -target x86_64-unknown-linux-gnu \
+// RUN:     -stdlib=libc++ \
+// RUN:   | FileCheck --check-prefix=CHECK-BASIC-LIBCXX-SHARED %s
+// CHECK-BASIC-LIBCXX-SHARED: "--push-state"
+// CHECK-BASIC-LIBCXX-SHARED: "--as-needed"
+// CHECK-BASIC-LIBCXX-SHARED: "-lc++"
+// CHECK-BASIC-LIBCXX-SHARED: "--pop-state"
+// CHECK-BASIC-LIBCXX-SHARED: "-lm"
+// RUN: %clangxx -no-canonical-prefixes -x c++ %s -### -o %t.o 2>&1 \
+// RUN:     -target x86_64-unknown-linux-gnu \
+// RUN:     -stdlib=libc++ -static-libstdc++ \
+// RUN:   | FileCheck --check-prefix=CHECK-BASIC-LIBCXX-STATIC %s
+// CHECK-BASIC-LIBCXX-STATIC: "--push-state"
+// CHECK-BASIC-LIBCXX-STATIC: "--as-needed"
+// CHECK-BASIC-LIBCXX-STATIC: "-Bstatic"
+// CHECK-BASIC-LIBCXX-STATIC: "-lc++"
+// CHECK-BASIC-LIBCXX-STATIC: "--pop-state"
+// CHECK-BASIC-LIBCXX-STATIC: "-lm"
+
 // Test a simulated installation of libc++ on Linux, both through sysroot and
 // the installation path of Clang.
 // RUN: %clangxx -no-canonical-prefixes -x c++ %s -### -o %t.o 2>&1 \
 // RUN:     -target x86_64-unknown-linux-gnu \
 // RUN:     -stdlib=libc++ \
 // RUN:     -ccc-install-dir %S/Inputs/basic_linux_tree/usr/bin \
 // RUN:     --gcc-toolchain="" \
 // RUN:     -resource-dir=%S/Inputs/resource_dir \

 // RUN:   | FileCheck --check-prefix=CHECK-BASIC-LIBCXX-C-LINK %s
 // 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"
-//
+
 // Check multi arch support on Ubuntu 12.04 LTS.
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN:     --target=arm-unknown-linux-gnueabihf -rtlib=platform \
 // RUN:     --gcc-toolchain="" \
 // RUN:     --sysroot=%S/Inputs/ubuntu_12.04_LTS_multiarch_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-UBUNTU-12-04-ARM-HF %s
 // CHECK-UBUNTU-12-04-ARM-HF: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
 // CHECK-UBUNTU-12-04-ARM-HF: "{{.*}}/usr/lib/arm-linux-gnueabihf{{/|\\\\}}crt1.o"