From: Tristan Gingold <gingold@adacore.com>
To: Catherine Moore <clm@codesourcery.com>
Cc: Ian Lance Taylor <iant@google.com>, binutils@sourceware.org
Subject: Re: [RFA] Linker script extension SECTION_FLAGS
Date: Wed, 25 May 2011 12:28:00 -0000 [thread overview]
Message-ID: <ABCD9D2D-63D3-44E9-95B1-00D726C10070@adacore.com> (raw)
In-Reply-To: <4DDC37D2.3030700@codesourcery.com>
On May 25, 2011, at 12:57 AM, Catherine Moore wrote:
>
> A new patch that addresses these concerns plus a couple that were pointed out by Tristan is attached here. Tristan, I didn't incorporate your comment regarding the backend hook into the new patch. I have now filled in bfd_elf_lookup_section_flags to handle all of the ELF common section header flags.
Ok, that makes sense.
> The backend hook is intended to handle the target-specific flags. Although, I'm not submitting it at this time, the backend hook that I am using for the PowerPC VLE target is attached as an example.
>
> Please let me know if there are additional concerns or comments and whether this is okay to commit.
I am not an ELF maintainer, but still I'd like to make three additional comments:
* I think you'd better to use a for statement for this pattern (in ppc_elf_lookup_section_flags and bfd_elf_lookup_section_flags):
+ struct flag_info_list *tf = finfo->flag_list;
..
+
+ while (tf)
+ {
..
+ tf = tf->next;
+ }
Something like:
for (tf = finfo->flag_list; tf != NULL; tf = tf->next)
clarify the iteration use.
* bfd_generic_lookup_section_flags shouldn't silently accept any flag, but generates an error message.
* I think you should simplify the bfd_lookup_section_flags hook: it should only convert a name to a flag and return an error/success
status. All the logical operations (setting only_with_flag or not_with_flag) and error handling should be made by the caller.
You could add a new function, bfd_lookup_section_flags, to do this work. This approach will also make the hooks smaller.
Sorry, I haven't noticed that from your previous patch.
Fell free to argue.
Tristan.
next prev parent reply other threads:[~2011-05-25 12:28 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-05-18 19:32 Catherine Moore
2011-05-18 19:33 ` Catherine Moore
2011-05-19 0:07 ` Ian Lance Taylor
2011-05-24 22:57 ` Catherine Moore
2011-05-25 12:28 ` Tristan Gingold [this message]
2011-06-06 22:02 ` PING " Catherine Moore
2011-06-07 13:11 ` Nick Clifton
2011-06-22 21:32 ` Catherine Moore
2011-06-28 11:37 ` Nick Clifton
2011-06-28 11:56 ` Tristan Gingold
2011-06-28 12:22 ` Nick Clifton
2011-06-28 13:30 ` Ian Lance Taylor
2011-06-30 21:11 ` Catherine Moore
2011-07-11 13:55 ` Nick Clifton
2012-02-09 5:27 ` Alan Modra
2011-05-19 7:44 ` Tristan Gingold
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ABCD9D2D-63D3-44E9-95B1-00D726C10070@adacore.com \
--to=gingold@adacore.com \
--cc=binutils@sourceware.org \
--cc=clm@codesourcery.com \
--cc=iant@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).