Page MenuHomePhabricator

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

Authored by grimar on Mon, Dec 3, 4:33 AM.

Details

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.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

grimar created this revision.Mon, Dec 3, 4:33 AM
grimar updated this revision to Diff 176380.Mon, Dec 3, 5:48 AM
  • Restored, updated the removed test case.
ruiu added a comment.Mon, Dec 3, 8:10 AM

Not needed anymore?

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.

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.

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.

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 added a comment.EditedTue, Dec 4, 12:56 AM

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).

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

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 accepted this revision.Fri, Dec 7, 2:48 PM

LGTM

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

This revision is now accepted and ready to land.Fri, Dec 7, 2:48 PM
This revision was automatically updated to reflect the committed changes.