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

[Jozef Lawrynowicz <jozef.l@mittosystems.com>]

On Thu, Oct 01, 2020 at 12:22:11PM -0700, Fangrui Song wrote:
> On 2020-09-30, Jozef Lawrynowicz wrote:
> > On Tue, Sep 29, 2020 at 05:10:09PM -0700, 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.
> > > 
> > 
> > Let's assume:
> >  GRP_RETAIN
> >    Sections in a group with this flag set should always be included in
> >    the linked object, even if they appear unused.
> > 
> > One issue I have with the group method is conceptual; groups are supposed
> > to be used to group related sections. They are related because of their
> > meaning from the application perspective, not because of their metadata
> > (i.e. sections to be retained are related by their metadata because they
> > all should be retained).
> > 
> > Just because groups have a side effect that can be leveraged to
> > get a section to be retained in the final link, why does that mean we
> > need to leverage that property when it it's not actually the simplest or
> > most obvious way to implement the new behavior?
> > 
> > SHF_GNU_RETAIN describes in the simplest possible way that the section
> > should be retained in the final link, without relying on other
> > constructs.
> > 
> > Just because groups are discarded or retained as a group, doesn't mean
> > we need to leverage groups to try and implement retention or discarding
> > functionality.
> > 
> > Why wasn't SHF_EXCLUDE implemented as a group flag? After all, groups
> > are included or excluded from the link together.
> >  GRP_EXCLUDE
> >    Sections in a group with GRP_EXCLUDE set should be discarded from
> >    the link.
> > 
> > I mean, you could kind of use groups for anything when you decide
> > grouping sections by metadata is OK.
> > Why define SHT_NOBITS when you can create:
> >  GRP_NOBITS
> >    Sections in this group occupy no space in the file. They must have
> >    type SHT_PROGBITS.
> > 
> > Retention of a section is a property of the section. We are misusing ELF
> > constructs by using groups to indicate an arbitrary section needs to be
> > retained.
> > 
> > Another issue (if more is needed) is about how to name the groups.
> > * If it is mandated that GRP_RETAIN groups have the same name e.g.
> >  "grp_retain", that means you can't put a section you want to
> >  retain in a different logical group that makes sense from the
> >  application perspective. So the other sections you would want to put
> >  in a group with the retained section need to all be put in a bundle
> >  with all the other GRP_RETAIN sections.
> > * If GRP_RETAIN groups can have any name, so you can have multiple
> >  GRP_RETAIN groups, how does the compiler decide how to name the
> >  groups? It seems like it would be a mess. "grp_retain0", "grp_retain2"
> >  ... "grp_retain10" from one object file, "grp_retain0"...
> >  "grp_retain5" from another. Extra processing in the
> >  linker to clean up these group names and keep them unique would be
> >  required when performing a relocatable link.
> > 
> > As a general point, what if I decide that there's enough pressure from
> > the anti-SHF_GNU_RETAIN side that I change the implementation to use
> > groups. But then those who already backed the flag prefer that method
> > over using groups think the implementation should not have been changed.
> > 
> > I feel like we already got enough backing for SHF_GNU_RETAIN between the
> > GNU gABI and Binutils discussions. I understand, and welcome, more
> > feedback, but I haven't been convinced any other method is obviously
> > better than SHF_GNU_RETAIN, so why change it?
> > 
> > I'm not saying it's possible to quantify which mechanism for "retain" is
> > best, but SHF_GNU_RETAIN is the simplest, most obvious, and easiest to
> > understand. Surely that gives it top marks?
> > 
> > I'm also yet to hear one convincing reason why SHF_GNU_RETAIN is bad.
> > As far as I can tell, the main argument against it stems from the fact
> > that you can have two input sections with the same name but different
> > SHF_GNU_RETAIN flag states. Not only is this a non-issue, groups have
> > exactly the same problem!
> > 
> > It would be great if a global maintainer to chime in on whether the
> > attached patch is acceptable, because otherwise we are going to go back
> > and forth forever.
> > 
> > Thanks,
> > Jozef
> 
> Hi Jozef,
> 
> I have checked with a few folks on the LLVM side. James Henderson and James Y
> Knight seem to prefer a section flag to other mechanism. I prefer a
> section flag, too (I was on the fence and wanted the alternatives to be considered).
> 
> About the section flag 'R' in assembly:
> 
> I'd still prefer an error to not add another exception to https://sourceware.org/pipermail/binutils/2020-February/109945.html
> (a consensus GNU as and LLVM's integrated assembler have reached)
> 
>   .section retain,"a",@progbits
>   .section retain,"aR",@progbits  # error for section flags change
> 
> If a separate section is desired, I'd prefer an explicit ,unique,0 or ,unique,1 or ...
> 
>   .section retain,"a",@progbits
>   .section retain,"aR",@progbits,unique,0   # 'unique' is a binutils 2.35 feature (available in LLVM for a while)
> 
> 
> About the use case in GCC (and probably Clang in the future):
> 
>   // a.h
>   __attribute__((section("sec"))) inline void bar() { ... }
> 
>   // a.c
>   #include "a.h"
>   __attribute__((section("sec"), retain))   // retain may be the pre-existing used.
>   void foo() { ... }
> 
> Perhaps there needs a function attribute to lower to ,unique,1 in assembly.

Hi Fangrui,

My revised implementation for the "retain" attribute in GCC is for
declarations with this attribute to always be placed in their own,
uniquely named section. This will leverage the same mechanism that the
-f{function,data}-sections options use. The "unique" option to the
.section directive will not be required.

I don't think there is any downside to doing this, compared to using the
default section name, and it solves the issue of having two sections
with the same name but different flags.

I therefore no longer have any specific need for
>   .section retain,"a",@progbits
>   .section retain,"aR",@progbits  # error for section flags change
to be supported by GAS, and could remove that support from my next
iteration of the Binutils patch.

Thanks,
Jozef