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!

-- 
Kind regards,
Luca Boccassi