PING Re: [RFA] Linker script extension SECTION_FLAGS

[Catherine Moore]

Hi Nick,

Thanks for the patch review.  I've developed a new patch that addresses 
the comments that both you and Tristan made regarding the original 
patch.  I've now associated the INPUT_SECTION_FLAGS with the input 
section specifications instead of the output sectionas you and others 
suggested.  I've tested arm-coff, mips-elf and ppc-elf.  What do you 
think?  Is this okay to commit?

Thanks,
Catherine

Does
On 06/07/2011 09:11 AM, Nick Clifton wrote:
> 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

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: include.cl
URL: <https://sourceware.org/pipermail/binutils/attachments/20110622/104daa21/attachment.ksh>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: include.patch
Type: text/x-patch
Size: 805 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/binutils/attachments/20110622/104daa21/attachment.bin>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: ld.cl
URL: <https://sourceware.org/pipermail/binutils/attachments/20110622/104daa21/attachment-0001.ksh>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ld.patch
Type: text/x-patch
Size: 12966 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/binutils/attachments/20110622/104daa21/attachment-0001.bin>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: ld-test.cl
URL: <https://sourceware.org/pipermail/binutils/attachments/20110622/104daa21/attachment-0002.ksh>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ld-test.patch
Type: text/x-patch
Size: 3567 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/binutils/attachments/20110622/104daa21/attachment-0002.bin>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: bfd.cl
URL: <https://sourceware.org/pipermail/binutils/attachments/20110622/104daa21/attachment-0003.ksh>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bfd.patch
Type: text/x-patch
Size: 25902 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/binutils/attachments/20110622/104daa21/attachment-0003.bin>