Re: PING Re: [RFA] Linker script extension SECTION_FLAGS

[Nick Clifton <nickc at redhat dot com>]

Hi Catherine,

> Hi, Do any of the maintainers have time to review this patch?

Did you see Tristan's follow up comments on your patch ?

  http://sources.redhat.com/ml/binutils/2011-05/msg00350.html


Some of the comments in the code need clarification:

  +  /* Remove sections with incorrect flags.  */
  +  void (*_bfd_lookup_section_flags) (struct bfd_link_info *,
  +                                     struct flag_info *);

I think that you mean "Sets the bitmasks of allowed and disallowed 
section flags" or something like that.  It does not actually remove 
sections at all...

  +  /* This function, if defined, is called to the section flag hex 
value.  */
  +  void (*elf_backend_lookup_section_flags_hook)

The comment here should presumably be: "This function, if defined, is 
called to convert target specific section flags names into hex values."



There appears to be a bug in the change to lang_add_section():

+      if (output->sectflags->only_with_flags != 0
+	  && (output->sectflags->only_with_flags & section->flags) == 0)
+	return;

I think that this will accept any section that contains any combination 
of any of the required flags, rather than only those sections that 
contain all of the required flags.  Ie, I think that the test should be:

+      if (output->sectflags->only_with_flags != 0
+	  && (output->sectflags->only_with_flags & section->flags) != 
output->sectflags->only_with_flags)
+	return;

Either this, or the documentation of how INPUT_SECTION_FLAGS works is wrong.

Following on from this, what would a linker script writer do if they 
wanted to include multiple, different sets of input section flags in 
their output section.  As I read the current the proposed documentation 
this cannot be done.  I suppose that you could add a comma separated 
syntax, ala:

  INPUT_SECTON_FLAGS (SHF_WRITE & SHF_ALLOC, SHF_WRITE & SHF_STRINGS)

But it might be better to go with Ian Lance Taylor's suggestion of 
putting the constraint *inside* the output section description.  Ie 
something like this:

    .text : {
       SECTION_FLAGS(SHF_WRITE & SHF_STRINGS, *(.text))
       SECTION_FLAGS(SHF_WRITE & SHF_ALLOC, *(.text))
     }


In the new bfd_elf_lookup_sections_flags() function there is an awful 
lot of repeated, almost identical code:

  +      if (!strcmp (tf->name, "SHF_WRITE"))
  +	{
  +	  if (tf->with == with_flags)
  +	    with_hex |= SHF_WRITE;
  +	  else if (tf->with == without_flags)
  +	    without_hex |= SHF_WRITE;
  +	}

It would be much cleaner to use an array of section names and flags and 
iterate through it.


Some of the fields in the flag_info structure ought be changed:

  +/* Section flag info.  */
  +struct flag_info
  +{
  +  unsigned only_with_flags;
  +  unsigned not_with_flags;
  +  struct flag_info_list *flag_list;
  +  int done;
  +};

The only_with_flags and not_with_flags fields should be of the type 
"flagword".  The "done" field should be a bfd_boolean and IMHO should be 
renamed to a slightly more descriptive term such as "flags_initialised".

Cheers
  Nick