[PATCH] D70020: [lld] Better support for group semantic wrt. notes

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D70020#1742808 <https://reviews.llvm.org/D70020#1742808>, @serge-sans-paille wrote:

> > I think this approach may have a problem when we consider /DISCARD/ in a linker script. I'll propose an alternative fix which I believe will improve the case and improve the compatibility with GNU ld, in the hope that you won't mind :)
>
> The following patch is a frail tentative at supporting /DISCARD/ in linker script:
>
>   diff --git a/lld/ELF/LinkerScript.cpp b/lld/ELF/LinkerScript.cpp
>   index cebbd89..1d5c8d7 100644
>   --- a/lld/ELF/LinkerScript.cpp
>   +++ b/lld/ELF/LinkerScript.cpp
>   @@ -442,6 +442,9 @@ LinkerScript::computeInputSections(const InputSectionDescription *cmd) {
>    }
>   
>    void LinkerScript::discard(InputSectionBase *s) {
>   +  if(!s->isLive())
>   +    return;
>   +
>      if (s == in.shStrTab || s == mainPart->relaDyn || s == mainPart->relrDyn)
>        error("discarding " + s->name + " section is not allowed");
>   
>   diff --git a/lld/test/ELF/linkerscript/discard-section-group.test b/lld/test/ELF/linkerscript/discard-section-group.test
>   new file mode 100644
>   index 0000000..e5b090f
>   --- /dev/null
>   +++ b/lld/test/ELF/linkerscript/discard-section-group.test
>   @@ -0,0 +1,47 @@
>   +# REQUIRES: x86
>   +# RUN: yaml2obj %s -o %t.o
>   +#
>   +# RUN: echo "SECTIONS { /DISCARD/ : { *(.text.foo*) } }" > %t0.script
>   +# RUN: ld.lld -o %t0 --script %t0.script %t.o
>   +# RUN: llvm-readobj -S %t0 | FileCheck %s
>   +#
>   +# RUN: echo "SECTIONS { /DISCARD/ : { *(.text.foo*) } /DISCARD/ : { *(.note*) } }" > %t1.script
>   +# RUN: ld.lld -o %t1 --script %t1.script %t.o
>   +# RUN: llvm-readobj -S %t1 | FileCheck %s
>   +#
>   +# RUN: echo "SECTIONS { /DISCARD/ : { *(.note*) } }" > %t2.script
>   +# RUN: ld.lld -o %t2 --script %t2.script %t.o
>   +# RUN: llvm-readobj -S %t2 | FileCheck %s
>   +
>   +# CHECK-NOT: .text
>   +# CHECK-NOT: .note
>   +
>   +--- !ELF
>   +FileHeader:
>   +  Class:           ELFCLASS64
>   +  Data:            ELFDATA2LSB
>   +  Type:            ET_REL
>   +  Machine:         EM_X86_64
>   +Sections:
>   +  - Name:            .group
>   +    Type:            SHT_GROUP
>   +    Link:            .symtab
>   +    Info:            foo
>   +    Members:
>   +      - SectionOrType:    GRP_COMDAT
>   +      - SectionOrType:    .text.foo
>   +      - SectionOrType:    .note.text.foo
>   +  - Name:            .note.text.foo
>   +    Type:            SHT_NOTE
>   +    Flags:           [ SHF_GROUP ]
>   +    Content:         "DEAD"
>   +  - Name:            .text.foo
>   +    Type:            SHT_PROGBITS
>   +    Flags:           [ SHF_ALLOC, SHF_EXECINSTR, SHF_GROUP ]
>   +    Content:         "00"
>   +Symbols:
>   +  - Name:            foo
>   +    Binding:         STB_GLOBAL
>   +    Type:            STT_FUNC
>   +    Section:         .text.foo
>   +    Size:            0x08
>
>
> So at first glance it looks possible to apply a patch on top of this one to support /DISCARD/, and I'd be happier if we can do it that way rather than discard this review.
>
> That being said, if you're alternative fix improves the case, let's put ego aside and go that way :-)


See D70146 <https://reviews.llvm.org/D70146>:)

The pasted patch changes the test linkerscript/discard-group.s from an infinite recursion to another unexpected behavior (dropping .text for `/DISCARD/ : { *(.note*) }`). I think we need a new field to express the relationship. In GNU ld, `bfd_elf_section_data::next_in_group` tracks such information and is used by its garbage collector.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70020/new/

https://reviews.llvm.org/D70020