[ELF] -Map --why-extract=: print despite errors

[MaskRay]

Summary

Fix https://github.com/llvm/llvm-project/issues/53073

In case of a relocation error, GNU ld's link map includes
the archive member extraction information but not output sections.

Our -Map and --why-extract= are currently no-op in case of an error.
This change makes the two options work.

[MaskRay]

This comment was removed by MaskRay.

[ikudrin]

The output of --why-extract looks useful indeed, but does the map file without output sections make much sense?

[peter.smith]

Presumably the relocation error is detected early, and not one detected late during writeTo()? I suspect that a late error would lead to a more complete map file being produced. In Arm's proprietary linker the ability to write a map file when a relocation out of range error has been detected has been really useful to debug problems in the thunk creation logic. I think it should be possible to detect if the OutputSections are incomplete, if that is the case it may be worth suppressing the OutputSections in the map file, or maybe putting a warning in the map file to state that output is incomplete.

Definitely support the idea of outputting as much diagnostics as possible, even when there is an error though.

[MaskRay]

delete another if (errorCount()) return; to make addresses available in case of an error

[peter.smith]

Thanks for the update LGTM on my side. It could be worth adding a comment after finalizeAddressDependentContent() just before the error check, to say something like All information needed for OutputSection part of Map file available. That might help someone that adds an extra error check and sees a test fail.

[MaskRay]

Add All information needed for OutputSection part of Map file is available.

Thanks for the suggestion!

[ikudrin]

lld/ELF/LinkerScript.cpp

566–567

There is no test that covers this change. And maybe it'll be better to fix the issue directly in DynamicSection<ELFT>::computeContents()?

[MaskRay]

lld/ELF/LinkerScript.cpp

566–567

discard-section-err.s covers this. Without the change `discard-section-err.s segfaults.

[ikudrin]

lld/ELF/LinkerScript.cpp

566–567

Well, for some reason, it passed on Windows in the Release build.

When you look at the code without the knowledge of all other parts of the linker, it is a bit puzzling that mainPart->relrDyn is reset while in.shStrTab is not. It either deserves an explanatory comment or in.shStrTab should be cleared, too, so that the code looks more consistent. Or maybe both.

[MaskRay]

update comment

[MaskRay]

allow discarding .relr.dyn

[ikudrin]

LGTM.

Changes in LinkerScript.cpp and discard-section-err.s should be committed separately.

[MaskRay]

In D116838#3237351, @ikudrin wrote:

LGTM.

Changes in LinkerScript.cpp and discard-section-err.s should be committed separately.

Agree, the unified diff is for an easier overview. I will push it separately.

[MaskRay]

rebase

lld/ELF/LinkerScript.cpp

LinkerScript::computeInputSections(const InputSectionDescription *cmd,
   }
   // Matched sections after the last SORT* are sorted by (--sort-alignment,
   // input order).
   sortByPositionThenCommandLine(sizeAfterPrevSort, ret.size());
   return ret;
 }
 
 void LinkerScript::discard(InputSectionBase &s) {
-  if (&s == in.shStrTab.get() || &s == mainPart->relrDyn.get())
+  if (&s == in.shStrTab.get())
     error("discarding " + s.name + " section is not allowed");
 
-  // You can discard .hash and .gnu.hash sections by linker scripts. Since
-  // they are synthesized sections, we need to handle them differently than
-  // other regular sections.
+  // You can discard .hash, .gnu.hash, and .relr.dyn sections by linker scripts.
+  // Since they are synthesized sections, we need to handle them differently
+  // than other regular sections.
   if (&s == mainPart->gnuHashTab)
     mainPart->gnuHashTab = nullptr;
-  if (&s == mainPart->hashTab)
+  else if (&s == mainPart->hashTab)
     mainPart->hashTab = nullptr;
+  else if (&s == mainPart->relrDyn.get())
+    mainPart->relrDyn.reset();
 
   s.markDead();
   s.parent = nullptr;
   for (InputSection *sec : s.dependentSections)
     discard(*sec);
 }
 
 void LinkerScript::discardSynthetic(OutputSection &outCmd) {

lld/ELF/Writer.cpp

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.
   finalizeSections();
   checkExecuteOnly();
-  if (errorCount())
-    return;
 
   // If --compressed-debug-sections is specified, compress .debug_* sections.
   // Do it right now because it changes the size of output sections.
   for (OutputSection *sec : outputSections)
     sec->maybeCompress<ELFT>();
 
   if (script->hasSectionsCommand)
     script->allocateHeaders(mainPart->phdrs);
for (Partition &part : MutableArrayRef<Partition>(partitions).slice(1)) {
         syms.insert(e.sym);
       for (DynamicReloc &reloc : part.relaDyn->relocs)
         if (reloc.sym && reloc.needsDynSymIndex() &&
             syms.insert(reloc.sym).second)
           part.dynSymTab->addSymbol(reloc.sym);
     }
   }
 
-  // Do not proceed if there was an undefined symbol.
-  if (errorCount())
-    return;
-
   if (in.mipsGot)
     in.mipsGot->build();
 
   removeUnusedSyntheticSections();
   script->diagnoseOrphanHandling();
 
   sortSections();
 
template <class ELFT> void Writer<ELFT>::finalizeSections() {
   // 2) Update the sections. We need to generate content that depends on the
   //    address of InputSections. For example, MIPS GOT section content or
   //    android packed relocations sections content.
   //
   // 3) Assign the final values for the linker script symbols. Linker scripts
   //    sometimes using forward symbol declarations. We want to set the correct
   //    values. They also might change after adding the thunks.
   finalizeAddressDependentContent();
+
+  // All information needed for OutputSection part of Map file is available.
   if (errorCount())
     return;
 
   {
     llvm::TimeTraceScope timeScope("Finalize synthetic sections");
     // finalizeAddressDependentContent may have added local symbols to the
     // static symbol table.
     finalizeSynthetic(in.symTab.get());

lld/test/ELF/linkerscript/discard-section-err.s

 # RUN: echo "SECTIONS { /DISCARD/ : { *(.rela.dyn) } }" > %t.script
 # RUN: ld.lld -pie -o %t %t.o
 # RUN: llvm-readobj -S %t | FileCheck --check-prefix=RELADYN %s
 # RELADYN: Name: .rela.dyn
 # RUN: ld.lld -pie -o %t --script %t.script %t.o
 # RUN: llvm-readobj -S %t | FileCheck /dev/null --implicit-check-not='Name: .rela.dyn'
 
 # RUN: echo "SECTIONS { /DISCARD/ : { *(.relr.dyn) } }" > %t.script
-# RUN: not ld.lld -pie --pack-dyn-relocs=relr -o /dev/null --script %t.script %t.o 2>&1 | \
-# RUN:   FileCheck -check-prefix=RELRDYN %s
-# RELRDYN: discarding .relr.dyn section is not allowed
+# RUN: ld.lld -pie --pack-dyn-relocs=relr -T %t.script %t.o -o %t
+# RUN: llvm-readobj -S %t | FileCheck /dev/null --implicit-check-not='Name: .relr.dyn'
 
 .data
 .align 8
 foo:
 ## Emits an R_X86_64_RELATIVE in -pie mode.
 .quad foo

lld/test/ELF/map-file.s

 # REQUIRES: x86
 
 # RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t1.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64 %p/Inputs/map-file2.s -o %t2.o
 # RUN: echo '.global bah; bah:' | llvm-mc -filetype=obj -triple=x86_64 - -o %t3.o
 # RUN: echo '.global baz; baz: ret' | llvm-mc -filetype=obj -triple=x86_64 - -o %t4.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64 %p/Inputs/map-file5.s -o %t5.o
 # RUN: echo '.global hey; hey: ret' | llvm-mc -filetype=obj -triple=x86_64 - -o %t6.o
+# RUN: echo '.reloc ., R_X86_64_RELATIVE, 0' | llvm-mc -filetype=obj -triple=x86_64 - -o %t7.o
 # RUN: ld.lld -shared %t5.o -o %t5.so -soname dso
 # RUN: rm -f %t4.a
 # RUN: llvm-ar rc %t4.a %t4.o
 # RUN: rm -f %t6.a
 # RUN: llvm-ar rcT %t6.a %t6.o
 # RUN: ld.lld %t1.o %t2.o %t3.o %t4.a %t5.so %t6.a -o %t -M | FileCheck --match-full-lines --strict-whitespace %s
 # RUN: ld.lld %t1.o %t2.o %t3.o %t4.a %t5.so %t6.a -o %t --print-map | FileCheck --match-full-lines -strict-whitespace %s
 # RUN: ld.lld %t1.o %t2.o %t3.o %t4.a %t5.so %t6.a -o %t -Map=%t.map
 # RUN: FileCheck -match-full-lines -strict-whitespace %s < %t.map
 
+## A relocation error does not suppress the output.
+# RUN: not ld.lld %t1.o %t2.o %t3.o %t4.a %t5.so %t6.a %t7.o -o /dev/null -M | FileCheck --strict-whitespace --check-prefix=CHECK2 %s
+
 .global _start
 _start:
 .cfi_startproc
 .cfi_endproc
  .quad sharedFoo
  .quad sharedBar
  .byte 0xe8
  .long sharedFunc1 - .

 # CHECK-NEXT:               0                0        8     1         <internal>:(.comment)
 # CHECK-NEXT:               0                0      1b0     8 .symtab
 # CHECK-NEXT:               0                0      1b0     8         <internal>:(.symtab)
 # CHECK-NEXT:               0                0       84     1 .shstrtab
 # CHECK-NEXT:               0                0       84     1         <internal>:(.shstrtab)
 # CHECK-NEXT:               0                0       71     1 .strtab
 # CHECK-NEXT:               0                0       71     1         <internal>:(.strtab)
 
+# CHECK2:                 VMA              LMA     Size Align Out     In      Symbol
+# CHECK2-NEXT:         200200           200200       78     8 .dynsym
+# CHECK2-NEXT:         200200           200200       78     8         <internal>:(.dynsym)
+# CHECK2-NEXT:         200278           200278       2c     8 .gnu.hash
+# CHECK2-NEXT:         200278           200278       2c     8         <internal>:(.gnu.hash)
+
 # RUN: not ld.lld %t1.o %t2.o %t3.o %t4.a -o /dev/null -Map=/ 2>&1 \
 # RUN:  | FileCheck --check-prefix=FAIL %s
 # FAIL: cannot open map file /

lld/test/ELF/why-extract.s

 # REQUIRES: x86
 
 # RUN: rm -rf %t && split-file %s %t
 # RUN: llvm-mc -filetype=obj -triple=x86_64 %t/main.s -o %t/main.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64 %t/a.s -o %t/a.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64 %t/a_b.s -o %t/a_b.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64 %t/b.s -o %t/b.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %t/err.s -o %t/err.o
 # RUN: llvm-ar rc %t/a.a %t/a.o
 # RUN: llvm-ar rc %t/a_b.a %t/a_b.o
 # RUN: llvm-ar rc %t/b.a %t/b.o
 # RUN: cd %t
 
 ## Nothing is extracted from an archive. The file is created with just a header.
 # RUN: ld.lld main.o a.o b.a -o /dev/null --why-extract=why1.txt
 # RUN: FileCheck %s --input-file=why1.txt --check-prefix=CHECK1 --match-full-lines --strict-whitespace
 
 #      CHECK1:reference extracted symbol
 #  CHECK1-NOT:{{.}}
 
 ## Some archive members are extracted.
 # RUN: ld.lld main.o a_b.a b.a -o /dev/null --why-extract=why2.txt
 # RUN: FileCheck %s --input-file=why2.txt --check-prefix=CHECK2 --match-full-lines --strict-whitespace
 
+## A relocation error does not suppress the output.
+# RUN: rm -f why2.txt && not ld.lld main.o a_b.a b.a err.o -o /dev/null --why-extract=why2.txt
+# RUN: FileCheck %s --input-file=why2.txt --check-prefix=CHECK2 --match-full-lines --strict-whitespace
+
 #      CHECK2:reference extracted symbol
 # CHECK2-NEXT:main.o  a_b.a(a_b.o)  a
 # CHECK2-NEXT:a_b.a(a_b.o)  b.a(b.o)  b()
 
 ## Check that backward references are supported.
 ## - means stdout.
 # RUN: ld.lld b.a a_b.a main.o -o /dev/null --why-extract=- | FileCheck %s --check-prefix=CHECK3
 
a:
   call _Z1bv
 
 #--- b.s
 .globl _Z1bv
 _Z1bv:
 
 #--- a.lds
 a = _Z1bv;
+
+#--- err.s
+.reloc ., R_X86_64_RELATIVE, 0