[LLD][ELF] - Support discarding the .dynamic section.

[grimar]

Summary

This is a part of https://bugs.llvm.org/show_bug.cgi?id=39810.

Seems it turns out that supporting /DISCARD/ for the .dynamic section with the
linker script is something we can do easily. The patch does this.

[grimar]

• Restored, updated the removed test case.

[ruiu]

Not needed anymore?

[peter.smith]

Will try and test these patches out on the linux kernel build on Thursday (going to be away Tuesday/Wednesday). One thought I had that could potentially simplify all three patches is to treat .dynamic, .dynstr and and .dynsym as a single discardable unit. For example:

• The .dynsym is not useful without the .dynstr.
• If there is a .dynsym then there must be a symbol that needs looking up with a dynamic loader, hence there is a strong case for the .dynamic section.

[peter.smith]

I've managed to build the linux kernel with D55211, D55215 and D55218 (after working around https://bugs.llvm.org/show_bug.cgi?id=39857) . I've not got the means to run the kernel, but it seems like the rela.dyn is present and correct and can be dumped with readelf.

[nickdesaulniers]

These might still be needed: http://lists.infradead.org/pipermail/linux-arm-kernel/2018-December/617179.html.

[grimar]

In D55211#1316973, @peter.smith wrote:

One thought I had that could potentially simplify all three patches is to treat .dynamic, .dynstr and and .dynsym as a single discardable unit. For example:

• The .dynsym is not useful without the .dynstr.
• If there is a .dynsym then there must be a symbol that needs looking up with a dynamic loader, hence there is a strong case for the .dynamic section.

I think when these sections (or other important ones) are discarded our aim perhaps is not to crash.
My idea from having 3 patches was to allow LLD to produce the output without crashes and without additional thinking about the dependencies.
I.e. just allow the user to do what he or she requests in the script. User assumed to know about some sections relationship and meanings.
Given that removing these sections is a rare scenario, I think it is reasonable to do a minimal code change for simple relaxing the current LLD behavior.

In D55211#1317015, @peter.smith wrote:

I've managed to build the linux kernel with D55211, D55215 and D55218 (after working around https://bugs.llvm.org/show_bug.cgi?id=39857) . I've not got the means to run the kernel, but it seems like the rela.dyn is present and correct and can be dumped with readelf.

Great, thanks!

[grimar]

In D55211#1316881, @ruiu wrote:

Not needed anymore?

In addition to other people arguments, I think particularly this one patch makes the code simpler a bit?
(our code in Writer.cpp that checks the In.DynSymTab and not the In.Dynamic to create _DYNAMIC is correct, but looks a bit strange).

[peter.smith]

I've had a chance to check through this and other 3 patches in series. If we are intending to do the minimum to stop the linker from crashing and assume the user knows what they are doing when they do this, then these look correct to me. I think these changes are important for use of LLD to link the linux kernel.

May I suggest a 4th patch with a test case that shows the scenario from the kernel. This depends on this, D55215 and D55218. It illustrates the intent behind the ability to discard these sections. I've made a proposal here:

# REQUIRES: x86
# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
# RUN: echo "SECTIONS { \
# RUN: .text 0x1000 : { *(.text) } \
# RUN: .data  : { *(.data) } \
# RUN: .rela.dyn : { __relocs_start = . ; *(.rela.dyn) *(.rela.*) __relocs_end = . ; } \
# RUN: /DISCARD/ : { *(.interp) *(.dynsym) *(.dynstr) *(.dynamic) } \
# RUN: } " > %t.script
# RUN: ld.lld -pie -o %t --script %t.script %t.o
# RUN: llvm-readobj --relocs %t | FileCheck -check-prefix=CHECK-RELOCS %s
# RUN: llvm-readobj --section-headers %t | FileCheck -check-prefix=CHECK-HEADERS %s
# RUN: llvm-readobj --symbols %t | FileCheck %s

# This test checks a pattern seen in the linux kernel KASLR configuration. There
# is no dynamic loader to do the relocation or lookup symbols so the
# kernel relocates itself using only relative non-symbol relocations.
# As the relocations are located via symbols. The .dynamic, .dynstr and
# .dynsym can be discarded.

#CHECK-RELOCS: 0x1008 R_X86_64_RELATIVE - 0x1000
#CHECK-RELOCS: 0x1010 R_X86_64_RELATIVE - 0x1020
#CHECK-RELOCS: 0x1018 R_X86_64_RELATIVE - 0x1068

#CHECK-HEADERS: .rela.dyn
#CHECK-HEADERS-NOT: .dynamic
#CHECK-HEADERS-NOT: .dynstr
        #CHECK-HEADERS-NOT: .dynsym

# CHECK:          Name: __relocs_end
# CHECK-NEXT:     Value: 0x1068
# CHECK-NEXT:     Size: 0
# CHECK-NEXT:     Binding: Global
# CHECK-NEXT:     Type: None
# CHECK-NEXT:     Other: 0
# CHECK-NEXT:     Section: .rela.dyn
# CHECK-NEXT:   }
# CHECK-NEXT:   Symbol {
# CHECK-NEXT:     Name: __relocs_start
# CHECK-NEXT:     Value: 0x1020
# CHECK-NEXT:     Size: 0
# CHECK-NEXT:     Binding: Global
# CHECK-NEXT:     Type: None
# CHECK-NEXT:     Other:
# CHECK-NEXT:     Section: .rela.dyn
# CHECK-NEXT:   }


        .text

        .globl  _start
        .p2align        4, 0x90
        .type   _start,@function
_start:
        movq    var@GOTPCREL(%rip), %rax
        retq

        .type   var,@object
        .data
        .globl  var
        .globl  __relocs_start
        .globl  __relocs_end
        .p2align        3
var:
        .quad   _start
        .quad __relocs_start
        .quad __relocs_end

[grimar]

In D55211#1321551, @peter.smith wrote:

I've had a chance to check through this and other 3 patches in series. If we are intending to do the minimum to stop the linker from crashing and assume the user knows what they are doing when they do this, then these look correct to me. I think these changes are important for use of LLD to link the linux kernel.

May I suggest a 4th patch with a test case that shows the scenario from the kernel. This depends on this, D55215 and D55218. It illustrates the intent behind the ability to discard these sections. I've made a proposal here:

Thanks for checking, Peter! I have no objections to land such patch to demonstrate we still producing the expected output when all these sections are discarded at once.
I would have few comments about the test though, so would suggest to push it on review if/when we decide to land these 3.

Rui, what do you think?

[ruiu]

LGTM

This seems like a straightfoward change to allow users to discard .dynamic.

ELF/LinkerScript.cpp

for (const SectionPattern &Pat : Cmd->SectionPatterns) {
     sortInputSections(MutableArrayRef<InputSection *>(Ret).slice(SizeBefore),
                       Pat);
   }
   return Ret;
 }
 
 void LinkerScript::discard(ArrayRef<InputSection *> V) {
   for (InputSection *S : V) {
-    if (S == In.ShStrTab || S == In.Dynamic || S == In.DynSymTab ||
-        S == In.DynStrTab || S == In.RelaPlt || S == In.RelaDyn ||
-        S == In.RelrDyn)
+    if (S == In.ShStrTab || S == In.DynSymTab || S == In.DynStrTab ||
+        S == In.RelaPlt || S == In.RelaDyn || S == In.RelrDyn)
       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.
     if (S == In.GnuHashTab)
       In.GnuHashTab = nullptr;
     if (S == In.HashTab)

ELF/Writer.cpp

for (BaseCommand *Base : Script->SectionCommands)
       if (auto *Sec = dyn_cast<OutputSection>(Base))
         addStartStopSymbols(Sec);
   }
 
   // Add _DYNAMIC symbol. Unlike GNU gold, our _DYNAMIC symbol has no type.
   // It should be okay as no one seems to care about the type.
   // Even the author of gold doesn't remember why gold behaves that way.
   // https://sourceware.org/ml/binutils/2002-03/msg00360.html
-  if (In.DynSymTab)
+  if (In.Dynamic->Parent)
     Symtab->addDefined("_DYNAMIC", STV_HIDDEN, STT_NOTYPE, 0 /*Value*/,
                        /*Size=*/0, STB_WEAK, In.Dynamic,
                        /*File=*/nullptr);
 
   // Define __rel[a]_iplt_{start,end} symbols if needed.
   addRelIpltSymbols();
 
   // RISC-V's gp can address +/- 2 KiB, set it to .sdata + 0x800 if not defined.
template <class ELFT> std::vector<PhdrEntry *> Writer<ELFT>::createPhdrs() {
   PhdrEntry *TlsHdr = make<PhdrEntry>(PT_TLS, PF_R);
   for (OutputSection *Sec : OutputSections)
     if (Sec->Flags & SHF_TLS)
       TlsHdr->add(Sec);
   if (TlsHdr->FirstSec)
     Ret.push_back(TlsHdr);
 
   // Add an entry for .dynamic.
-  if (In.DynSymTab)
-    AddHdr(PT_DYNAMIC, In.Dynamic->getParent()->getPhdrFlags())
-        ->add(In.Dynamic->getParent());
+  if (OutputSection *Sec = In.Dynamic->getParent())
+    AddHdr(PT_DYNAMIC, Sec->getPhdrFlags())->add(Sec);
 
   // PT_GNU_RELRO includes all sections that should be marked as
   // read-only by dynamic linker after proccessing relocations.
   // Current dynamic loaders only support one PT_GNU_RELRO PHDR, give
   // an error message if more than one PT_GNU_RELRO PHDR is required.
   PhdrEntry *RelRo = make<PhdrEntry>(PT_GNU_RELRO, PF_R);
   bool InRelroPhdr = false;
   bool IsRelroFinished = false;

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

 # REQUIRES: x86
 
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
 
 # RUN: echo "SECTIONS { /DISCARD/ : { *(.shstrtab) } }" > %t.script
 # RUN: not ld.lld -o %t --script %t.script %t.o 2>&1 | \
 # RUN:   FileCheck -check-prefix=SHSTRTAB %s
 # SHSTRTAB: discarding .shstrtab section is not allowed
 
-# RUN: echo "SECTIONS { /DISCARD/ : { *(.dynamic) } }" > %t.script
-# RUN: not ld.lld -pie -o %t --script %t.script %t.o 2>&1 | \
-# RUN:   FileCheck -check-prefix=DYNAMIC %s
-# DYNAMIC: discarding .dynamic section is not allowed
-
 # RUN: echo "SECTIONS { /DISCARD/ : { *(.dynsym) } }" > %t.script
 # RUN: not ld.lld -pie -o %t --script %t.script %t.o 2>&1 | \
 # RUN:   FileCheck -check-prefix=DYNSYM %s
 # DYNSYM: discarding .dynsym section is not allowed
 
 # RUN: echo "SECTIONS { /DISCARD/ : { *(.dynstr) } }" > %t.script
 # RUN: not ld.lld -pie -o %t --script %t.script %t.o 2>&1 | \
 # RUN:   FileCheck -check-prefix=DYNSTR %s