[ELF] Make --discard-locals default on riscv

[abrachet]

Summary

This matches bfd.ld's behavior

[jrtc27]

lld/ELF/Driver.cpp

717

Seems like the semantics of Default should change, otherwise it's not the default?..

[jrtc27]

Anyway this seems to contradict the approach chosen by D127826

[mcgrathr]

lld/ELF/Driver.cpp

717

Agreed. The only meaning of Default really is what the final case in shouldKeepInSymtab does, and I think it makes more sense to just change that one if to add an || config->emachine == EM_RISCV there.

[jrtc27]

lld/ELF/Writer.cpp

655 ↗

(On Diff #503590)

Comment is now wrong...

[mcgrathr]

lld/ELF/Writer.cpp

655 ↗

(On Diff #503590)

Indeed. This merits a nontrivial comment anyway, something like " * RISC-V, where the assembler doesn't discard them by default as other targets do, so as to facilitate relaxation that can move things around and would break if labels were reduced to section offsets".

[jrtc27]

lld/ELF/Writer.cpp

653 ↗

(On Diff #503591)

• A or B
• C

when it's already implicitly or'ed is weird, be consistent and do

• A
• B
• C

[jrtc27]

Anyway, this definitely still merits a review from @MaskRay given his commit to tackle the problem differently

[MaskRay]

Yeah; I wish that we don't do this. The target difference doesn't seem useful.

IIRC even a RISC-V binutils maintainers said the behavior is strange but perhaps people don't have enough motivation to fix that.
We don't really need to copy the behavior as Clang Driver has passed -X.

lld/ELF/Driver.cpp

static bool isOutputFormatBinary(opt::InputArgList &args) {
   StringRef s = args.getLastArgValue(OPT_oformat, "elf");
   if (s == "binary")
     return true;
   if (!s.startswith("elf"))
     error("unknown --oformat value: " + s);
   return false;
 }
 
-static DiscardPolicy getDiscard(opt::InputArgList &args) {
+static DiscardPolicy getDiscard(opt::InputArgList &args, uint16_t emachine) {
   auto *arg =
       args.getLastArg(OPT_discard_all, OPT_discard_locals, OPT_discard_none);
   if (!arg)
-    return DiscardPolicy::Default;
+    return emachine == EM_RISCV ? DiscardPolicy::Locals : DiscardPolicy::Default;
   if (arg->getOption().getID() == OPT_discard_all)
     return DiscardPolicy::All;
   if (arg->getOption().getID() == OPT_discard_locals)
     return DiscardPolicy::Locals;
   return DiscardPolicy::None;
 }
 
 static StringRef getDynamicLinker(opt::InputArgList &args) {
static void readConfigs(opt::InputArgList &args) {
   config->compressDebugSections = getCompressDebugSections(args);
   config->cref = args.hasArg(OPT_cref);
   config->optimizeBBJumps =
       args.hasFlag(OPT_optimize_bb_jumps, OPT_no_optimize_bb_jumps, false);
   config->demangle = args.hasFlag(OPT_demangle, OPT_no_demangle, true);
   config->dependencyFile = args.getLastArgValue(OPT_dependency_file);
   config->dependentLibraries = args.hasFlag(OPT_dependent_libraries, OPT_no_dependent_libraries, true);
   config->disableVerify = args.hasArg(OPT_disable_verify);
-  config->discard = getDiscard(args);
+  config->discard = getDiscard(args, config->emachine);
   config->dwoDir = args.getLastArgValue(OPT_plugin_opt_dwo_dir_eq);
   config->dynamicLinker = getDynamicLinker(args);
   config->ehFrameHdr =
       args.hasFlag(OPT_eh_frame_hdr, OPT_no_eh_frame_hdr, false);
   config->emitLLVM = args.hasArg(OPT_plugin_opt_emit_llvm, false);
   config->emitRelocs = args.hasArg(OPT_emit_relocs);
   config->callGraphProfileSort = args.hasFlag(
       OPT_call_graph_profile_sort, OPT_no_call_graph_profile_sort, true);

lld/test/ELF/discard-locals-riscv.s

+# REQUIRES: riscv
+
+# RUN: llvm-mc -filetype=obj -triple=riscv64 %s -o %t.o
+# RUN: ld.lld %t.o -o - | llvm-readelf -s - | FileCheck %s
+
+# RUN: llvm-mc -filetype=obj -triple=riscv64 %s -o %t.o -save-temp-labels
+# RUN: ld.lld %t.o -o - -discard-none | llvm-readelf -s - | FileCheck %s --check-prefix=NONE
+
+# CHECK-NOT: .Llabel
+# NONE: .Llabel
+
+.Llabel: