[ELF] Keep local symbols when both --emit-relocs and --discard-all are specified

[MaskRay]

Summary

This fixes a bug as exposed by D77807.

Add tests for {--emit-relocs,-r} x {--discard-locals,--discard-all}. They add coverage for previously undertested cases:

• STT_SECTION associated to GCed sections (gc)
• STT_SECTION associated to retained sections (text)
• STT_SECTION associated to non-SHF_ALLOC sections (.comment)
• STB_LOCAL in GCed sections (unused_gc)

[ikudrin]

lld/ELF/Writer.cpp

646–647

For the purpose of this fix, it would be simpler to extend the condition at the beginning of getDiscard() in Driver.cpp and remove this condition. The other changes are not required.

[MaskRay]

lld/ELF/Writer.cpp

646–647

Do you mean

-  if (args.hasArg(OPT_relocatable))
+ if (args.hasArg(OPT_relocatable) || config->emitRelocs)
    return DiscardPolicy::None;

? That would increase the amount of differences for D77807.

[ikudrin]

lld/ELF/Writer.cpp

646–647

Yes, that is true, but I believe that every patch should solve its issue in the right way and the change in Driver.cpp looks like that. We emit relocations in two cases, -r and --emit-relocs, and both of them should be handled similarly.

[MaskRay]

lld/ELF/Writer.cpp

646–647

I have a different opinion, though. I prefer the current version despite the -r --emit-relocs similarity.

-  if (config->discard != DiscardPolicy::All)
    copyLocalSymbols();

This removes the logic from the caller and makes the intricate logic local to the body of copyLocalSymbols.

[ikudrin]

lld/ELF/Writer.cpp

646–647

My main concern is the unjustified difference in handling the -r and --emit-relocs cases. What about removing the condition in getDiscard() in Driver.cpp and changing config->emitRelocs to config->copyRelocs here?

[ikudrin]

lld/test/ELF/emit-relocs-discard-locals.s

3

overrides?

[grimar]

lld/ELF/Writer.cpp

646–647

I think I am slightly in favor of keeping it here, because the argument about hiding "the intricate logic" probably makes sense to me.

650

Can this be moved closer to the first config->discard check to group them?

if (config->discard == DiscardPolicy::None)
  return true;

(Seems if (config->discard == DiscardPolicy::None) can be moved here).

lld/test/ELF/emit-relocs-discard-locals.s

3

overrides

8

NM_NOGC -> NM-NOGC (we don't use underbars in tests I think)

10

Why it is important to test --gc-sections/--no-gc-sections too?

18

The emit-relocs-discard-locals.s test case name reflects the --emit-relocs, but not -r.
I think this test could be splitted into two perhaps. Or renamed properly.

[MaskRay]

Address comments

lld/test/ELF/emit-relocs-discard-locals.s

8

I've seen underscores in a few places. Thinking more, underscores may not be bad because it does not conflict with -NOT -DAG ... or possible future extensions..

[MaskRay]

Improve tests (check STT_SECTION)
Localize config->discard logic

[grimar]

LGTM with a nit.

lld/test/ELF/emit-relocs-discard-locals.s

10

This comment was forgotten it seems. Please add a comment, I guess the reason to test "-gc-sections" is "just in case"?

[ikudrin]

LGTM with a couple of nits.

lld/ELF/Writer.cpp

646

You may use config->copyRelocs, it is the same.

lld/test/ELF/emit-relocs-discard-locals.s

27–28

Do we really need to check the symbols which are not important to the fix? Unnecessary details make the test fragile.

31–33

The same thoughts about these 0xFFFFFFFFFFFFFFFC values.

[MaskRay]

lld/test/ELF/emit-relocs-discard-locals.s

27–28

The STT_SECTION symbols (also STB_LOCAL) are actually an interesting cast which was neglected. I add them for coverage. I am careful and will not intentionally add useless tests.

31–33

These values don't make tests fragile and have minimum cost (they don't even introduce new lines).

Previously we simply dropped the symbols from relocation records. 0xFFFFFFFFFFFFFFFC is added just in case the addend is replaced by 0.

[ikudrin]

lld/test/ELF/emit-relocs-discard-locals.s

27–28

I meant .text, .comment and _start, not text and unreferenced. And I would have probably removed the section indexes for all symbols.

[MaskRay]

Address comments

[MaskRay]

Drop section indices from llvm-readelf -S output

lld/test/ELF/emit-relocs-discard-locals.s

27–28

unreferenced demonstrates that --discard-locals does not discard the STT_SECTION symbol associated with a GCed section.--gc-sections can discard such symbols.

text represents a subset of STB_LOCAL symbols (STT_SECTION).

[MaskRay]

Rename unreferenced to gc

lld/ELF/Writer.cpp

if (in.symTabShndx)
     add(in.symTabShndx);
   add(in.shStrTab);
   if (in.strTab)
     add(in.strTab);
 }
 
 // The main function of the writer.
 template <class ELFT> void Writer<ELFT>::run() {
-  if (config->discard != DiscardPolicy::All)
-    copyLocalSymbols();
 
   if (config->copyRelocs)
     addSectionSymbols();
 
   // Now that we have a complete set of output sections. This function
   // completes section contents. For example, we need to add strings
   // to the string table, and add entries to .got and .plt.
   // finalizeSections does that.
template <class ELFT> void Writer<ELFT>::run() {
   if (auto e = buffer->commit())
     error("failed to write to the output file: " + toString(std::move(e)));
 }
 
 static bool shouldKeepInSymtab(const Defined &sym) {
   if (sym.isSection())
     return false;
 
-  if (config->discard == DiscardPolicy::None)
-    return true;
-
   // If -emit-reloc is given, all symbols including local ones need to be
   // copied because they may be referenced by relocations.
   if (config->emitRelocs)
     return true;
 
+  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.
   // * The symbol is in a SHF_MERGE section, which is normally the reason for
   //   the assembler keeping the .L symbol.
   StringRef name = sym.getName();
   bool isLocal = name.startswith(".L") || name.empty();
   if (!isLocal)

lld/test/ELF/emit-relocs-discard-locals.s

+# REQUIRES: x86
+## Test that --emit-relocs keeps local symbols and overrides --discard-{locals,all}.
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64 -save-temp-labels %s -o %t.o
+
+# RUN: ld.lld --emit-relocs --discard-locals %t.o -o %tlocal
+# RUN: llvm-nm %tlocal | FileCheck --check-prefixes=NM,NM-NOGC %s
+# RUN: llvm-readobj -r %tlocal | FileCheck --check-prefix=REL %s
+# RUN: ld.lld --emit-relocs --discard-locals --gc-sections %t.o -o %tlocal.gc
+# RUN: llvm-nm %tlocal.gc | FileCheck --check-prefix=NM %s
+# RUN: llvm-readobj -r %tlocal | FileCheck --check-prefix=REL %s
+
+# RUN: ld.lld --emit-relocs --discard-all %t.o -o %tall
+# RUN: llvm-nm %tall | FileCheck --check-prefixes=NM,NM-NOGC %s
+# RUN: llvm-readobj -r %tall | FileCheck --check-prefix=REL %s
+
+# NM:           t .Lunused
+# NM-NOGC-NEXT: t .Lunused_gc
+# NM-NEXT:      t .Lused
+# NM-NEXT:      T _start
+# NM-NEXT:      t unused
+# NM-NOGC-NEXT: t unused_gc
+# NM-NEXT:      t used
+
+# REL:      .rela.text {
+# REL-NEXT:   R_X86_64_PLT32 .Lused 0xFFFFFFFFFFFFFFFC
+# REL-NEXT:   R_X86_64_PLT32 used 0xFFFFFFFFFFFFFFFC
+# REL-NEXT: }
+
+.globl _start
+_start:
+  jmp .Lused@plt
+  call used@plt
+
+.Lunused:
+.Lused:
+unused:
+used:
+
+.section unreferenced,"ax"
+.Lunused_gc:
+unused_gc:
+  ret

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

+# REQUIRES: x86
+## Test that -r keeps local symbols and overrides --discard-{locals,all}.
+## Also see --emit-relocs-discard-locals.s
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64 -save-temp-labels %s -o %t.o
+
+# RUN: ld.lld -r --discard-locals %t.o -o %tlocal.ro
+# RUN: llvm-nm %tlocal.ro | FileCheck --check-prefixes=NM,NM_NOGC %s
+# RUN: llvm-readobj -r %tlocal.ro | FileCheck --check-prefix=REL %s
+
+# RUN: ld.lld -r --discard-all %t.o -o %tall.ro
+# RUN: llvm-nm %tall.ro | FileCheck --check-prefixes=NM,NM_NOGC %s
+# RUN: llvm-readobj -r %tall.ro | FileCheck --check-prefix=REL %s
+
+# NM:           t .Lunused
+# NM_NOGC-NEXT: t .Lunused_gc
+# NM-NEXT:      t .Lused
+# NM-NEXT:      T _start
+# NM-NEXT:      t unused
+# NM_NOGC-NEXT: t unused_gc
+# NM-NEXT:      t used
+
+# REL:      .rela.text {
+# REL-NEXT:   R_X86_64_PLT32 .Lused 0xFFFFFFFFFFFFFFFC
+# REL-NEXT:   R_X86_64_PLT32 used 0xFFFFFFFFFFFFFFFC
+# REL-NEXT: }
+
+.globl _start
+_start:
+  jmp .Lused@plt
+  call used@plt
+
+.Lunused:
+.Lused:
+unused:
+used:
+
+.section unreferenced,"ax"
+.Lunused_gc:
+unused_gc:
+  ret