Re: [PATCH v2] Support for SHF_GNU_RETAIN ELF Section Flag

[Michael Matz <matz@suse.de>]

Hello,

On Tue, 29 Sep 2020, Roland McGrath wrote:

> Since group semantics mean that if any section in a group is retained the
> whole group must be retained, it seems to me that making the feature a
> group flag rather than a section flag makes sense. I don't think it really
> has any downside. It's a little bit more structure to generate a group but
> everything already knows how to do it so that should be no problem.  While
> it is true that multiple sections with the same name and different
> attributes should already be fine, the place where multiple different
> sections of the same name are usually seen today is when each different
> section reusing a name is in a different group from the other sections with
> the same name.

pseudo-code for retaining a section in producer.  SHF_GNU_RETAIN:

  retain_section(S):
    S.flag |= SHF_GNU_RETAIN

GRP_RETAIN:

  retain_section(S):
    if !retain_group:
      retain_group = new_group(GRP_RETAIN)
    retain_group.add (S)

pseudo-code for interpreting this in the consumer (which presumably is 
section based as basic building block, as groups are optional).  
SHF_GNU_RETAIN:

  in the 'output_section_p' predicate, add " || S.flag & SHF_GNU_RETAIN".

GRP_RETAIN:

  in preparatory per-object parsing, in the existing group parser:
  
    if G.type == GRP_RETAIN:
      foreach S in G:
        S.needed = true

  in the 'output_section_p' predicate, add " || S.needed"

That's the additional structure you mention.  While it's not much 
down-side, what is the up-side?  There should be one, not just only small 
down-sides.


Ciao,
Michael.

 > 
> On Tue, Sep 29, 2020 at 2:37 PM Jozef Lawrynowicz <jozef.l@mittosystems.com>
> wrote:
> 
> > On Tue, Sep 29, 2020 at 12:38:06PM -0700, Fangrui Song wrote:
> > > On 2020-09-29, Jozef Lawrynowicz wrote:
> > > > There are more logic holes in your other email that I didn't
> > > > respond to, but I've already spent a tonne of time trying to address
> > > > your points (most of which you ignored, so why did I bother?), so I
> > > > think I'm done debating this.
> > > > If other people back up your suggestion, and address my concerns, then
> > > > we can re-open this.
> > >
> > > Hi Jozef, apologies if I did not respond to your previous replies in
> > > time or if I acted as if I "blocked" your change. I maintain LLVM's
> > > linker LLD and am also a contributor to its binary utilities
> > > (llvm-readelf, llvm-objdump, ...). If GCC makes use of this flag for
> > __attribute__((used)),
> > > LLD and LLVM's binary utilities do need to support it.
> > > So I want to participate in the design before we regret :)
> > >
> > > I have studied linker GC quite a lot (contributed patches in LLD
> > > and coordinated on the feature parity among the 3 linkers
> > > (e.g. https://sourceware.org/pipermail/binutils/2020-August/112732.html)
> > >
> > > LLD and GNU ld, LLVM's integrated assembler and GNU as have achieved
> > consensus on many aspects
> > > on SHF_LINK_ORDER and 'unique' linkage. Credits to H.J. for all the
> > binutils side work.
> > > )
> > >
> > > I have asked a couple of toolchain folks. Many do share the same feeling
> > > with me: an additional GC feature is useful but whether it should be
> > > this exact form of a new flag we don't know.
> >
> > Hi Fangrui,
> >
> > Thank you for providing some more information to backup your points, I
> > will address those below.
> >
> > >
> > > Your patch also deprives an OS-specific flag from ELFOSABI_FREEBSD:)
> > > I am not a committer of FreeBSD and I am surely not authoritative on
> > > whether this is acceptable. But, FreeBSD does use the aforementioned
> > > LLVM tools and I don't want to dirty the code base by adding
> > > if (osabi == ELFOSABI_FREEBSD) do something.
> >
> > I don't have any insight into the ELFOSABI_FREEBSD change, H.J. added that
> > part, I assume it is very closely aligned with the "pure" GNU OSABI, so
> > any changes to one should be kept in parity with the other. However,
> > that is only my assumption.
> >
> > >
> > > We have 8 bits in SHF_MASKOS: not too few, but also not too many.  We
> > > should be prudent with new flags, especially if we already have a
> > > facility for section retaining. There is a difference ".reloc requires a
> > > known live section" which might cause friction but it has the advantage
> > that it is
> > > time-tested and has good toolchain support.
> >
> > Yes, this is something I considered, but there are 6 bits remaining to
> > be used, with SHF_GNU_RETAIN there will be 5 bits remaining.
> > Given how little movement there has been within the SHF_MAKOS bits for
> > the past 20 years, I don't think we need to be too protective of the
> > bits. They are there to be used after all, and IMO SHF_GNU_RETAIN is a
> > very well-defined use case to use one of those bits.
> >
> > For the .reloc mechanism, in every object file with a retained section
> > you will need to have an empty input section which is guaranteed to be
> > kept by the linker. How is the compiler to know what that section is?
> >
> > >
> > >
> > > Opinions from others:
> > >
> > > * Peter Smith mentioned that Arm's proprietary toolchain does something
> > >   similar to SHF_GNU_RETAIN
> > >   (
> > https://www.keil.com/support/man/docs/armcc/armcc_chr1359124983230.htm)
> > >   "I put __attribute__((used)) on my function and yet the tools removed
> > >   it anyway."
> > >
> > > * James Henderson mentioned whether we can
> > >   "create a new section type listing section indexes that should be
> > >   retained"
> > >   As a variant, we could use a SHT_GROUP with a new GRP_* bit.
> > >   Anchor: [group] (see below)
> >
> > Using groups was discussed on the GNU gABI thread, but I don't really
> > see the benefit of the added complication. Doesn't handling new section
> > group types still require significant coordination between all the
> > different compilers, assemblers and linkers?
> >
> > Any attempt to reference a section which does not exist in the object
> > file, and is not guaranteed to be present in the linked output file, is
> > a non-starter in my opinion. We give users the freedom to do what they
> > want with linker scripts as ELF doesn't mandate any section must be
> > linked into the final executable. I think it's better to consolidate
> > the requirement to retain within the section that requires retaining,
> > instead of spreading the requirement to retain between ELF constructs
> > which have unrelated purposes, and need to be aligned between different
> > stages of the build process.
> >
> > >
> > > > Do we want to make life easier for ourselves, or easier for our users?
> > > >
> > > > I get that ABI changes can be a bit disruptive, but this new flag in
> > particular really isn't complicated anyway.
> > >
> > > On 2020-09-29, Jozef Lawrynowicz wrote:
> > > > On Mon, Sep 28, 2020 at 09:43:53PM -0700, Fangrui Song wrote:
> > > > > On 2020-09-28, Jozef Lawrynowicz wrote:
> > > > > > The attached patch is version 2 of the SHF_GNU_RETAIN patch that
> > was
> > > > > > previously discussed here:
> > > > > >
> > https://sourceware.org/pipermail/binutils/2020-September/113406.html
> > > > > >
> > > > > > The following changes have been made:
> > > > > > - Removed the .retain directive
> > > > > > - The assembler will create different input sections for sections
> > with
> > > > > >  the same name but SHF_GNU_RETAIN set/unset (thanks to H.J. for
> > that).
> > > > > >  This means the linker will be able to do a better job garbage
> > > > > >  collecting input sections, as the "retain" attribute applied to a
> > > > > >  symbol declaration in the source code will not cause other parts
> > of
> > > > > >  the program that are not required, but are in the same section,
> > to be
> > > > > >  unnecessarily retained.
> > > > > > - Added GNU OSABI handling (also thanks to H.J.).
> > > > >
> > > > > My point from
> > https://sourceware.org/pipermail/binutils/2020-September/113466.html
> > stands.
> > > > > Section flags are a bit cumbersome. If the following
> > > > >
> > > > >   // a.h
> > > > >   __attribute__((section("sec")))
> > > > >   inline void bar() { ... }
> > > > >   // a.c
> > > > >   #include "a.h"
> > > > >   __attribute__((section("sec"), retain))
> > > > >   void foo() {
> > > > >   }
> > > > >
> > > > > compiles to
> > > > >
> > > > >   .section sec,"a",@progbits
> > > > >   ...
> > > > >   .section sec,"aR",@progbits
> > > > >   ...
> > > > >
> > > > > You will get a gas error for changing section flags
> > > > > (https://sourceware.org/pipermail/binutils/2020-February/109945.html
> > )
> > > > >
> > > >
> > > > There is no error in this case.
> > > > The original patch handled it, and it has been further improved in v2.
> > > > There's even a testcase testing exactly the functionality you say is
> > > > broken.
> > > >
> > > >  gas/testsuite/gas/elf/section22.s:
> > > >      .section       .bss,"aw"
> > > >    ...
> > > >      .section       .bss,"awR",%nobits
> > > >
> > > > I already gave a detailed response to your reloc proposal:
> > > > https://sourceware.org/pipermail/binutils/2020-September/113450.html
> > >
> > > Letting the following assembly creates two .bss sections adds an
> > exception to
> > > "gas: error for section type, attr, or entsize change"
> > > (https://sourceware.org/pipermail/binutils/2020-February/109945.html)
> > > for which I have achieved consensus with Alan Modra and some LLVM
> > > toolchain folks. That is why I hope we could re-consider this design
> > > choice.
> > >
> > > .section      .bss,"aw"
> > > .section      .bss,"awR",%nobits
> >
> > I agree that this could be considered slightly inelegant, but much less
> > so than having .reloc directives in arbitrary empty sections.
> > Once someone understands what the "R" flag does, it should be clear why
> > we want to create a separate input section for it.
> >
> > If we really want, we can force the compiler to create unique section
> > names for declarations which have the "retain" attribute applied. I
> > assume most of the time, users will want this fine section
> > granularity. I would prefer not to go that route and leave it to the
> > users to specify unique sections if they want.
> >
> > >
> > > See above for [group]: it is actually nice to use a group here, because
> > > the quadruple
> > > (section name, group name if "G", linked-to section if "o", unique id of
> > > "unique") is the section key. The following creates a section different
> > > from the default .bss:
> > >
> > >   .section    .bss,"awG",%nobits,gc_root
> >
> > I don't think it is really very important whether input sections with
> > the same name are unique or not within an object file. ELF doesn't care
> > about this. You can have sections with the same name and same flags in
> > different object files and the linker can just deal with it. It's the
> > same situation if you have two sections from the same object file which
> > have the same name.
> >
> > Within an object file, you'll end up with all the non-SHF_GNU_RETAIN
> > sections with the same name being merged together by the assembler, and
> > all those with SHF_GNU_RETAIN being merged. I don't think there is any
> > benefit to keep SHF_GNU_RETAIN sections with the same name as separate
> > input sections in the object file.
> >
> > Thanks,
> > Jozef
> >
> > >
> > > > Why don't you first answer my questions about why using relocs is a
> > > > really bad idea, is a hack, doesn't conceptually make any sense, and
> > > > will confuse users?
> > > >
> > > > You never actually gave a reason that using relocs is better than
> > > > SHF_GNU_RETAIN, you seem to be pushing this alternative implementation
> > just
> > > > because it is an alternative implementation. I don't count your
> > > > description of section flags as "cumbersome" as a reason we should
> > > > shelve this proposal.
> > >
> > > I hope my previous paragraphs have answered this point.
> > >
> > > > > .reloc is really convenience in this case. You can add as many .reloc
> > > > > directives as you like, each contributing one R_*_NONE to the object
> > > > > file but zero cost to the linked image.
> > > >
> > > > Are we really counting a section flag as adding "cost" to a linked
> > > > image?  Your method requires a "benign zero-sized section" (suggested
> > in
> > > > your previous email), doesn't this also add cost? What about the cost
> > of
> > > > having nonsensical relocs in object files?
> > > >
> > > > If we are using the term "cost" loosely, and to further our own point,
> > > > then SHF_GNU_RETAIN actually has negative cost to the linked image. You
> > > > can have different input sections with the same name, but with
> > different
> > > > states for SHF_GNU_RETAIN (set and unset), and so garbage collection
> > > > will operate with finer granularity, removing more unneeded parts of
> > the
> > > > program compared to your reloc mechanism, which would keep the entire
> > > > section containing the symbol referenced by the reloc.
> > > >
> > > > Thanks,
> > > > Jozef
> >
> >
>