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

[Nick Clifton <nickc@redhat.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