[Driver] Pass -X to ld for riscv*-{elf,freebsd,linux}

[MaskRay]

Summary

GNU ld has a hack that defaults to -X (--discard-locals) in the emulation file
riscvelf.em. The recommended way, as gcc/config/arm does, is to let the
compiler driver pass -X to ld.
(The motivation is likely to discard a plethora of .L symbols due to
linker relaxation.)

lld default to --discard-none. To make clang+lld use match GNU ld's behavior,
pass -X to ld.

Note: GNU ld has a special rule to treat ld -r -s as ld -r -S -x. With -X, driver -r -Wl,-s
will behave as ld -r -S -X. This removes fewer symbols than -r -S -x but is safe.

[kito-cheng]

ld's help message just confused me, that say -X is default, but actually default action is Discard local temporary symbols in SEC_MERGE sections. which is no option can enable that but default.

$ ld.bfd --help
...
  -x, --discard-all           Discard all local symbols
  -X, --discard-locals        Discard temporary local symbols (default)
  --discard-none              Don't discard any local symbols
...

Anyway, back to the RISC-V part, that set -X (--discard-locals) IF discard policy is default, which match the behavior of emultempl/riscvelf.em, however I've little concern about that might change the behavior for ld.bfd when we try to link with clang -target riscv64 -Wl,-r -Wl,-s: ld will treat ld -r -s as ld -r -S -x, but this change changed this behavior because we always append a -X here, so it the behavior is become treat ld -r -s as ld -r -S, but I must say I really not sure how important of this behavior - the behavior here is over 20 years[2, 3] and I didn't found any document to describe why.

But generally I believe discard fewer symbol isn't harmful - just made the object file fatter little bit, so I give a LGTM here.

[1] https://github.com/bminor/binutils-gdb/blob/master/ld/emultempl/riscvelf.em#L34

[2] https://github.com/bminor/binutils-gdb/blob/252b5132c753830d5fd56823373aed85f2a0db63/ld/ldmain.c#L272
[3] https://github.com/bminor/binutils-gdb/commit/f5fa8ca231d47662321addbfbde105e2bed0be07#diff-3b7cfc539d765310991a11f7f328f095fc6f9e17fea1a8510c949c210804deb5R281

/* Treat ld -r -s as ld -r -S -x (i.e., strip all local symbols).  I
   don't see how else this can be handled, since in this case we
   must preserve all externally visible symbols.  */
if (bfd_link_relocatable (&link_info) && link_info.strip == strip_all)
  {    
    link_info.strip = strip_debugger;
    if (link_info.discard == discard_sec_merge)
      link_info.discard = discard_all;
  }

[MaskRay]

Thanks for the archaeology!

ld will treat ld -r -s as ld -r -S -x,

This seems really weird... I agree that using driver-passed -X instead of the implied -x is safe.

[MaskRay]

clang/lib/Driver/ToolChains/FreeBSD.cpp

226

@dim FYI

[brad]

This should probably be applied to Fuchsia and NetBSD as well.

clang/lib/Driver/ToolChains/FreeBSD.cpp

case llvm::Triple::mips64el:
     if (tools::mips::hasMipsAbiArg(Args, "n32"))
       CmdArgs.push_back("elf32ltsmipn32_fbsd");
     else
       CmdArgs.push_back("elf64ltsmip_fbsd");
     break;
   case llvm::Triple::riscv32:
     CmdArgs.push_back("-m");
     CmdArgs.push_back("elf32lriscv");
+    CmdArgs.push_back("-X");
     break;
   case llvm::Triple::riscv64:
     CmdArgs.push_back("-m");
     CmdArgs.push_back("elf64lriscv");
+    CmdArgs.push_back("-X");
     break;
   default:
     break;
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_G)) {
     if (ToolChain.getTriple().isMIPS()) {
       StringRef v = A->getValue();

clang/lib/Driver/ToolChains/Gnu.cpp

void tools::gnutools::Linker::ConstructJob(Compilation &C, const JobAction &JA,
 
   if (const char *LDMOption = getLDMOption(ToolChain.getTriple(), Args)) {
     CmdArgs.push_back("-m");
     CmdArgs.push_back(LDMOption);
   } else {
     D.Diag(diag::err_target_unknown_triple) << Triple.str();
     return;
   }
+  if (Triple.isRISCV())
+    CmdArgs.push_back("-X");
 
   if (Args.hasArg(options::OPT_shared))
     CmdArgs.push_back("-shared");
 
   if (IsStatic) {
     CmdArgs.push_back("-static");
   } else {
     if (Args.hasArg(options::OPT_rdynamic))

clang/lib/Driver/ToolChains/RISCVToolchain.cpp

void RISCV::Linker::ConstructJob(Compilation &C, const JobAction &JA,
 
   bool IsRV64 = ToolChain.getArch() == llvm::Triple::riscv64;
   CmdArgs.push_back("-m");
   if (IsRV64) {
     CmdArgs.push_back("elf64lriscv");
   } else {
     CmdArgs.push_back("elf32lriscv");
   }
+  CmdArgs.push_back("-X");
 
   std::string Linker = getToolChain().GetLinkerPath();
 
   bool WantCRTs =
       !Args.hasArg(options::OPT_nostdlib, options::OPT_nostartfiles);
 
   const char *crtbegin, *crtend;
   auto RuntimeLib = ToolChain.GetRuntimeLibType(Args);

clang/test/Driver/riscv32-toolchain.c

 // RUN: %clang -### %s -fuse-ld= \
 // RUN:   --target=riscv32-unknown-elf --rtlib=platform \
 // RUN:   --gcc-toolchain=%S/Inputs/basic_riscv32_tree \
 // RUN:   --sysroot=%S/Inputs/basic_riscv32_tree/riscv32-unknown-elf 2>&1 \
 // RUN:   | FileCheck -check-prefix=C-RV32-BAREMETAL-ILP32 %s
 
 // C-RV32-BAREMETAL-ILP32: "{{.*}}Inputs/basic_riscv32_tree/lib/gcc/riscv32-unknown-elf/8.0.1/../../..{{/|\\\\}}..{{/|\\\\}}bin{{/|\\\\}}riscv32-unknown-elf-ld"
 // C-RV32-BAREMETAL-ILP32: "--sysroot={{.*}}/Inputs/basic_riscv32_tree/riscv32-unknown-elf"
+// C-RV64-BAREMETAL-LP64-SAME: "-X"
 // C-RV32-BAREMETAL-ILP32: "{{.*}}/Inputs/basic_riscv32_tree/riscv32-unknown-elf/lib{{/|\\\\}}crt0.o"
 // C-RV32-BAREMETAL-ILP32: "{{.*}}/Inputs/basic_riscv32_tree/lib/gcc/riscv32-unknown-elf/8.0.1{{/|\\\\}}crtbegin.o"
 // C-RV32-BAREMETAL-ILP32: "-L{{.*}}/Inputs/basic_riscv32_tree/lib/gcc/riscv32-unknown-elf/8.0.1"
 // C-RV32-BAREMETAL-ILP32: "-L{{.*}}/Inputs/basic_riscv32_tree/riscv32-unknown-elf/lib"
 // C-RV32-BAREMETAL-ILP32: "--start-group" "-lc" "-lgloss" "--end-group" "-lgcc"
 // C-RV32-BAREMETAL-ILP32: "{{.*}}/Inputs/basic_riscv32_tree/lib/gcc/riscv32-unknown-elf/8.0.1{{/|\\\\}}crtend.o"
 
 // RUN: %clang -### %s -fuse-ld= \

 // RUN: env "PATH=" %clang -### %s -fuse-ld=ld -no-pie \
 // RUN:   --target=riscv32-unknown-linux-gnu --rtlib=platform -mabi=ilp32 \
 // RUN:   --gcc-toolchain=%S/Inputs/multilib_riscv_linux_sdk \
 // RUN:   --sysroot=%S/Inputs/multilib_riscv_linux_sdk/sysroot 2>&1 \
 // RUN:   | FileCheck -check-prefix=C-RV32-LINUX-MULTI-ILP32 %s
 
 // C-RV32-LINUX-MULTI-ILP32: "{{.*}}/Inputs/multilib_riscv_linux_sdk/lib/gcc/riscv64-unknown-linux-gnu/7.2.0/../../..{{/|\\\\}}..{{/|\\\\}}riscv64-unknown-linux-gnu/bin{{/|\\\\}}ld"
 // C-RV32-LINUX-MULTI-ILP32: "--sysroot={{.*}}/Inputs/multilib_riscv_linux_sdk/sysroot"
-// C-RV32-LINUX-MULTI-ILP32: "-m" "elf32lriscv"
+// C-RV32-LINUX-MULTI-ILP32: "-m" "elf32lriscv" "-X"
 // C-RV32-LINUX-MULTI-ILP32: "-dynamic-linker" "/lib/ld-linux-riscv32-ilp32.so.1"
 // C-RV32-LINUX-MULTI-ILP32: "{{.*}}/Inputs/multilib_riscv_linux_sdk/lib/gcc/riscv64-unknown-linux-gnu/7.2.0/lib32/ilp32{{/|\\\\}}crtbegin.o"
 // C-RV32-LINUX-MULTI-ILP32: "-L{{.*}}/Inputs/multilib_riscv_linux_sdk/lib/gcc/riscv64-unknown-linux-gnu/7.2.0/lib32/ilp32"
 // C-RV32-LINUX-MULTI-ILP32: "-L{{.*}}/Inputs/multilib_riscv_linux_sdk/sysroot/lib32/ilp32"
 // C-RV32-LINUX-MULTI-ILP32: "-L{{.*}}/Inputs/multilib_riscv_linux_sdk/sysroot/usr/lib32/ilp32"
 
 // RUN: env "PATH=" %clang -### %s -fuse-ld=ld -no-pie \
 // RUN:   --target=riscv32-unknown-linux-gnu --rtlib=platform -march=rv32imafd \

clang/test/Driver/riscv64-toolchain.c

 // RUN: env "PATH=" %clang -### %s -fuse-ld= \
 // RUN:   --target=riscv64-unknown-elf --rtlib=platform \
 // RUN:   --gcc-toolchain=%S/Inputs/basic_riscv64_tree \
 // RUN:   --sysroot=%S/Inputs/basic_riscv64_tree/riscv64-unknown-elf 2>&1 -no-pie \
 // RUN:   | FileCheck -check-prefix=C-RV64-BAREMETAL-LP64 %s
 
 // C-RV64-BAREMETAL-LP64: "{{.*}}Inputs/basic_riscv64_tree/lib/gcc/riscv64-unknown-elf/8.0.1/../../..{{/|\\\\}}..{{/|\\\\}}bin{{/|\\\\}}riscv64-unknown-elf-ld"
 // C-RV64-BAREMETAL-LP64: "--sysroot={{.*}}/Inputs/basic_riscv64_tree/riscv64-unknown-elf"
+// C-RV64-BAREMETAL-LP64-SAME: "-X"
 // C-RV64-BAREMETAL-LP64: "{{.*}}/Inputs/basic_riscv64_tree/riscv64-unknown-elf/lib{{/|\\\\}}crt0.o"
 // C-RV64-BAREMETAL-LP64: "{{.*}}/Inputs/basic_riscv64_tree/lib/gcc/riscv64-unknown-elf/8.0.1{{/|\\\\}}crtbegin.o"
 // C-RV64-BAREMETAL-LP64: "-L{{.*}}/Inputs/basic_riscv64_tree/lib/gcc/riscv64-unknown-elf/8.0.1"
 // C-RV64-BAREMETAL-LP64: "-L{{.*}}/Inputs/basic_riscv64_tree/riscv64-unknown-elf/lib"
 // C-RV64-BAREMETAL-LP64: "--start-group" "-lc" "-lgloss" "--end-group" "-lgcc"
 // C-RV64-BAREMETAL-LP64: "{{.*}}/Inputs/basic_riscv64_tree/lib/gcc/riscv64-unknown-elf/8.0.1{{/|\\\\}}crtend.o"
 
 // RUN: env "PATH=" %clang -### %s -fuse-ld= \

 // RUN: env "PATH=" %clang -### %s -fuse-ld= -no-pie \
 // RUN:   --target=riscv64-unknown-linux-gnu --rtlib=platform -mabi=lp64 \
 // RUN:   --gcc-toolchain=%S/Inputs/multilib_riscv_linux_sdk \
 // RUN:   --sysroot=%S/Inputs/multilib_riscv_linux_sdk/sysroot 2>&1 \
 // RUN:   | FileCheck -check-prefix=C-RV64-LINUX-MULTI-LP64 %s
 
 // C-RV64-LINUX-MULTI-LP64: "{{.*}}/Inputs/multilib_riscv_linux_sdk/lib/gcc/riscv64-unknown-linux-gnu/7.2.0/../../..{{/|\\\\}}..{{/|\\\\}}riscv64-unknown-linux-gnu/bin{{/|\\\\}}ld"
 // C-RV64-LINUX-MULTI-LP64: "--sysroot={{.*}}/Inputs/multilib_riscv_linux_sdk/sysroot"
-// C-RV64-LINUX-MULTI-LP64: "-m" "elf64lriscv"
+// C-RV64-LINUX-MULTI-LP64: "-m" "elf64lriscv" "-X"
 // C-RV64-LINUX-MULTI-LP64: "-dynamic-linker" "/lib/ld-linux-riscv64-lp64.so.1"
 // C-RV64-LINUX-MULTI-LP64: "{{.*}}/Inputs/multilib_riscv_linux_sdk/lib/gcc/riscv64-unknown-linux-gnu/7.2.0/lib64/lp64{{/|\\\\}}crtbegin.o"
 // C-RV64-LINUX-MULTI-LP64: "-L{{.*}}/Inputs/multilib_riscv_linux_sdk/lib/gcc/riscv64-unknown-linux-gnu/7.2.0/lib64/lp64"
 // C-RV64-LINUX-MULTI-LP64: "-L{{.*}}/Inputs/multilib_riscv_linux_sdk/sysroot/lib64/lp64"
 // C-RV64-LINUX-MULTI-LP64: "-L{{.*}}/Inputs/multilib_riscv_linux_sdk/sysroot/usr/lib64/lp64"
 
 // RUN: env "PATH=" %clang -### %s -fuse-ld=ld -no-pie \
 // RUN:   --target=riscv64-unknown-linux-gnu --rtlib=platform -march=rv64imafd \