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

[Fangrui Song <i@maskray.me>]

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.

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. 

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.


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

   I have personally used a .reloc to retain
   __attribute__((used, section(".data.PERSIST_SIG"))) static char SigAfl[] = "##SIG_AFL_PERSISTENT##";

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

> 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

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

>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