Re: [PATCH] ld: Support customized output section type

[Luca Boccassi <bluca@debian.org>]

[-- Attachment #1: Type: text/plain, Size: 4827 bytes --]

On Thu, 2022-02-03 at 11:46 -0800, Fangrui Song wrote:
> On 2022-02-03, Luca Boccassi wrote:
> > On Wed, 2022-02-02 at 10:32 -0800, Fangrui Song wrote:
> > > On 2022-02-02, Nick Clifton via Binutils wrote:
> > > > Hi Fangrui,
> > > > 
> > > > > The current output section type allows to set the ELF section
> > > > > type to
> > > > > SHT_PROGBITS or SHT_NOLOAD. This patch allows an arbitrary
> > > > > section value
> > > > > to be specified. Some ELF section type names are supported as
> > > > > well,
> > > > 
> > > > Thanks for the patch submission.  I am regression testing it at
> > > > the moment
> > > > but in the meantime a couple of things stood out for me:
> > > > 
> > > > 
> > > > > +@item TYPE = @var{type}
> > > > > +Set the section type to the integer @var{type}. For the ELF
> > > > > output
> > > > > +file, some type names (e.g. @code{SHT_NOTE}) are also
> > > > > allowed for
> > > > > +@var{type}.
> > > > 
> > > > Rather than having users guess, it would probably be best to
> > > > list which
> > > > type names are supported.
> > > > 
> > > > Also it is probably worth documenting that it is the user's
> > > > fault if they
> > > > set the section type to something which has special semantics
> > > > (eg SHT_GROUP)
> > > > but then they do not also arrange for whatever necessary
> > > > support that feature
> > > > needs.  Something like: "it is the user's responsibility to
> > > > ensure that
> > > > any special requirements of the section type are met".
> > > 
> > > Thanks for the quick review! Adopted the wording and the change
> > > below.
> > > The new patch is in the attachment.
> > > 
> > > 
> > > For SHT_GROUP, I think it is useful to support SHT_GROUP as well.
> > > I
> > > actually did an experiment last night but SHT_GROUP led to an
> > > internal
> > > error. There may be some issues that need to be fixed to use the
> > > SHT_GROUP feature.
> > > 
> > > > 
> > > > 
> > > > > +           case type_section:
> > > > > +             if (os->sectype_value->type.node_class ==
> > > > > etree_name
> > > > > +                 && os->sectype_value->type.node_code ==
> > > > > NAME)
> > > > > +               {
> > > > > +                 const char *name = os->sectype_value-
> > > > > >name.name;
> > > > > +                 if (strcmp (name, "SHT_PROGBITS") == 0)
> > > > > +                   type = SHT_PROGBITS;
> > > > > +                 else if (strcmp (name, "SHT_NOTE") == 0)
> > > > > +                   type = SHT_NOTE;
> > > > > +                 else if (strcmp (name, "SHT_NOBITS") == 0)
> > > > > +                   type = SHT_NOBITS;
> > > > > +                 else if (strcmp (name, "SHT_INIT_ARRAY") ==
> > > > > 0)
> > > > > +                   type = SHT_INIT_ARRAY;
> > > > > +                 else if (strcmp (name, "SHT_FINI_ARRAY") ==
> > > > > 0)
> > > > > +                   type = SHT_FINI_ARRAY;
> > > > > +                 else if (strcmp (name, "SHT_PREINIT_ARRAY")
> > > > > == 0)
> > > > > +                   type = SHT_PREINIT_ARRAY;
> > > > > +                 else
> > > > > +                   einfo (_ ("%F%P: invalid type for output
> > > > > section `%s'\n"),
> > > > > +                          os->name);
> > > > 
> > > > It might be worth adding SHT_STRTAB to this list, as I can
> > > > imagine some
> > > > weird sceanario where someone would want it.
> > > 
> > > Added.
> > > 
> > > > 
> > > > Also - given that this is a new feature, there really ought to
> > > > be an entry
> > > > for it in the ld/NEWS file.
> > > 
> > > Added. The NEW entry is for 2.39, but feel free to port it to
> > > 2.38 if
> > > you think appropriate:)
> > 
> > Hi,
> > 
> > I tested this patch, it doesn't seem to allow combining multiple
> > attributes. Tried both in the same brackets and separately, eg:
> > 
> > .note.foo (READONLY) (TYPE=SHT_NOTE)
> > 
> > .note.foo (READONLY TYPE=SHT_NOTE)
> > 
> > Could you please send a new revision that fixes this (no opinion on
> > the
> > syntax), so that we can use it? Thanks!
> > 
> 
> It doesn't, as I am not sure the combination is useful.

I understand you don't like it, but as I've already explained again,
and again, and again, it is needed, and frankly I'm getting exhausted.
If you don't want to update your patch that's fine, it's no problem at
all, I'll send a follow-up myself if it gets merged.

-- 
Kind regards,
Luca Boccassi

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]