[PATCH] D37736: [ELF] - Do not spread specific flags when emiting output sections.

Tue Oct 10 10:20:49 PDT 2017

LGTM.

Rui, what do you think?

Cheers,
Rafael

George Rimar via Phabricator <reviews at reviews.llvm.org> writes:

> grimar updated this revision to Diff 118407.
> grimar added a comment.
>
> - Updated comment.
>
>
> https://reviews.llvm.org/D37736
>
> Files:
>   ELF/LinkerScript.cpp
>   test/ELF/linkerscript/arm-exidx-order.s
>   test/ELF/linkerscript/symbol-only-flags.s
>
>
> Index: test/ELF/linkerscript/symbol-only-flags.s
> ===================================================================
> --- test/ELF/linkerscript/symbol-only-flags.s
> +++ test/ELF/linkerscript/symbol-only-flags.s
> @@ -0,0 +1,20 @@
> +# REQUIRES: x86
> +# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
> +# RUN: echo "SECTIONS { . = SIZEOF_HEADERS; \
> +# RUN:         .tbss : { *(.tbss) }         \
> +# RUN:         .foo : { bar = .; } }" > %t.script
> +# RUN: ld.lld -o %t --script %t.script %t.o
> +# RUN: llvm-readobj -s %t | FileCheck %s
> +
> +## Check .foo does not get SHF_TLS flag.
> +# CHECK:     Section {
> +# CHECK:       Index:
> +# CHECK:       Name: .foo
> +# CHECK-NEXT:  Type: SHT_PROGBITS
> +# CHECK-NEXT:  Flags [
> +# CHECK-NEXT:    SHF_ALLOC
> +# CHECK-NEXT:    SHF_WRITE
> +# CHECK-NEXT:  ]
> +
> +.section .tbss,"awT", at nobits
> +.quad 0
> Index: test/ELF/linkerscript/arm-exidx-order.s
> ===================================================================
> --- test/ELF/linkerscript/arm-exidx-order.s
> +++ test/ELF/linkerscript/arm-exidx-order.s
> @@ -0,0 +1,19 @@
> +# REQUIRES: arm
> +# RUN: llvm-mc -filetype=obj -triple=armv7a-none-linux-gnueabi %s -o %t.o
> +# RUN: echo "SECTIONS { . = SIZEOF_HEADERS;    \
> +# RUN:         .ARM.exidx : { *(.ARM.exidx*) } \
> +# RUN:         .foo : { _foo = 0; } }" > %t.script
> +# RUN: ld.lld -T %t.script %t.o -shared -o %t.so
> +# RUN: llvm-readobj -s %t.so | FileCheck %s
> +
> +# CHECK:      Section {
> +# CHECK:        Index: 
> +# CHECK:        Name: .foo
> +# CHECK-NEXT:   Type: SHT_PROGBITS
> +# CHECK-NEXT:   Flags [
> +# CHECK-NEXT:     SHF_ALLOC
> +# CHECK-NEXT:   ]
> +
> +.fnstart
> +.cantunwind
> +.fnend
> Index: ELF/LinkerScript.cpp
> ===================================================================
> --- ELF/LinkerScript.cpp
> +++ ELF/LinkerScript.cpp
> @@ -656,17 +656,33 @@
>  
>  void LinkerScript::adjustSectionsBeforeSorting() {
>    // If the output section contains only symbol assignments, create a
> -  // corresponding output section. The bfd linker seems to only create them if
> -  // '.' is assigned to, but creating these section should not have any bad
> -  // consequeces and gives us a section to put the symbol in.
> +  // corresponding output section. The issue is what to do with linker script
> +  // like ".foo : { symbol = 42; }". One option would be to convert it to
> +  // "symbol = 42;". That is, move the symbol out of the empty section
> +  // description. That seems to be what bfd does for this simple case. The
> +  // problem is that this is not completely general. bfd will give up and
> +  // create a dummy section too if there is a ". = . + 1" inside the section
> +  // for example.
> +  // Given that we want to create the section, we have to worry what impact
> +  // it will have on the link. For example, if we just create a section with
> +  // 0 for flags, it would change which PT_LOADs are created.
> +  // We could remember that that particular section is dummy and ignore it in
> +  // other parts of the linker, but unfortunately there are quite a few places
> +  // that would need to change:
> +  //   * The program header creation.
> +  //   * The orphan section placement.
> +  //   * The address assignment.
> +  // The other option is to pick flags that minimize the impact the section
> +  // will have on the rest of the linker. That is why we copy the flags from
> +  // the previous sections. Only a few flags are needed to keep the impact low.
>    uint64_t Flags = SHF_ALLOC;
>  
>    for (BaseCommand * Cmd : Opt.Commands) {
>      auto *Sec = dyn_cast<OutputSection>(Cmd);
>      if (!Sec)
>        continue;
>      if (Sec->Live) {
> -      Flags = Sec->Flags;
> +      Flags = Sec->Flags & (SHF_ALLOC | SHF_WRITE | SHF_EXECINSTR);
>        continue;
>      }
>  
>
>
> Index: test/ELF/linkerscript/symbol-only-flags.s
> ===================================================================
> --- test/ELF/linkerscript/symbol-only-flags.s
> +++ test/ELF/linkerscript/symbol-only-flags.s
> @@ -0,0 +1,20 @@
> +# REQUIRES: x86
> +# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
> +# RUN: echo "SECTIONS { . = SIZEOF_HEADERS; \
> +# RUN:         .tbss : { *(.tbss) }         \
> +# RUN:         .foo : { bar = .; } }" > %t.script
> +# RUN: ld.lld -o %t --script %t.script %t.o
> +# RUN: llvm-readobj -s %t | FileCheck %s
> +
> +## Check .foo does not get SHF_TLS flag.
> +# CHECK:     Section {
> +# CHECK:       Index:
> +# CHECK:       Name: .foo
> +# CHECK-NEXT:  Type: SHT_PROGBITS
> +# CHECK-NEXT:  Flags [
> +# CHECK-NEXT:    SHF_ALLOC
> +# CHECK-NEXT:    SHF_WRITE
> +# CHECK-NEXT:  ]
> +
> +.section .tbss,"awT", at nobits
> +.quad 0
> Index: test/ELF/linkerscript/arm-exidx-order.s
> ===================================================================
> --- test/ELF/linkerscript/arm-exidx-order.s
> +++ test/ELF/linkerscript/arm-exidx-order.s
> @@ -0,0 +1,19 @@
> +# REQUIRES: arm
> +# RUN: llvm-mc -filetype=obj -triple=armv7a-none-linux-gnueabi %s -o %t.o
> +# RUN: echo "SECTIONS { . = SIZEOF_HEADERS;    \
> +# RUN:         .ARM.exidx : { *(.ARM.exidx*) } \
> +# RUN:         .foo : { _foo = 0; } }" > %t.script
> +# RUN: ld.lld -T %t.script %t.o -shared -o %t.so
> +# RUN: llvm-readobj -s %t.so | FileCheck %s
> +
> +# CHECK:      Section {
> +# CHECK:        Index: 
> +# CHECK:        Name: .foo
> +# CHECK-NEXT:   Type: SHT_PROGBITS
> +# CHECK-NEXT:   Flags [
> +# CHECK-NEXT:     SHF_ALLOC
> +# CHECK-NEXT:   ]
> +
> +.fnstart
> +.cantunwind
> +.fnend
> Index: ELF/LinkerScript.cpp
> ===================================================================
> --- ELF/LinkerScript.cpp
> +++ ELF/LinkerScript.cpp
> @@ -656,17 +656,33 @@
>  
>  void LinkerScript::adjustSectionsBeforeSorting() {
>    // If the output section contains only symbol assignments, create a
> -  // corresponding output section. The bfd linker seems to only create them if
> -  // '.' is assigned to, but creating these section should not have any bad
> -  // consequeces and gives us a section to put the symbol in.
> +  // corresponding output section. The issue is what to do with linker script
> +  // like ".foo : { symbol = 42; }". One option would be to convert it to
> +  // "symbol = 42;". That is, move the symbol out of the empty section
> +  // description. That seems to be what bfd does for this simple case. The
> +  // problem is that this is not completely general. bfd will give up and
> +  // create a dummy section too if there is a ". = . + 1" inside the section
> +  // for example.
> +  // Given that we want to create the section, we have to worry what impact
> +  // it will have on the link. For example, if we just create a section with
> +  // 0 for flags, it would change which PT_LOADs are created.
> +  // We could remember that that particular section is dummy and ignore it in
> +  // other parts of the linker, but unfortunately there are quite a few places
> +  // that would need to change:
> +  //   * The program header creation.
> +  //   * The orphan section placement.
> +  //   * The address assignment.
> +  // The other option is to pick flags that minimize the impact the section
> +  // will have on the rest of the linker. That is why we copy the flags from
> +  // the previous sections. Only a few flags are needed to keep the impact low.
>    uint64_t Flags = SHF_ALLOC;
>  
>    for (BaseCommand * Cmd : Opt.Commands) {
>      auto *Sec = dyn_cast<OutputSection>(Cmd);
>      if (!Sec)
>        continue;
>      if (Sec->Live) {
> -      Flags = Sec->Flags;
> +      Flags = Sec->Flags & (SHF_ALLOC | SHF_WRITE | SHF_EXECINSTR);
>        continue;
>      }
>  