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

[abrachet]

Summary

This matches bfd.ld's behavior

[jrtc27]

lld/ELF/Driver.cpp

717 ↗

(On Diff #503587)

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 ↗

(On Diff #503587)

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

Comment is now wrong...

[mcgrathr]

lld/ELF/Writer.cpp

655

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

• 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/Writer.cpp

static bool shouldKeepInSymtab(const Defined &sym) {
 
   if (config->discard == DiscardPolicy::None)
     return true;
   if (config->discard == DiscardPolicy::All)
     return false;
 
   // In ELF assembly .L symbols are normally discarded by the assembler.
   // If the assembler fails to do so, the linker discards them if
-  // * --discard-locals is used.
+  // * --discard-locals is used, or if the architecture is riscv.
   // * The symbol is in a SHF_MERGE section, which is normally the reason for
   //   the assembler keeping the .L symbol.
   if (sym.getName().startswith(".L") &&
       (config->discard == DiscardPolicy::Locals ||
+       config->emachine == EM_RISCV ||
        (sym.section && (sym.section->flags & SHF_MERGE))))
     return false;
   return true;
 }
 
 static bool includeInSymtab(const Symbol &b) {
   if (auto *d = dyn_cast<Defined>(&b)) {
     // Always include absolute symbols.

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: