This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch]: Support coff noread flag for segments


2009/5/16 Dave Korn <dave.korn.cygwin@googlemail.com>:
> Kai Tietz wrote:
>> Hi,
>
> ? ?Hi Kai,
>
>> As I added some time ago the NOREAD flag support to bfd, this is the
>> final part for allowing the declaration of sections with
>> SEC_COFF_NOREAD flag set. I used for this flag character 'y' (possibly
>> another one is prefered?).
>>
>> ChangeLog
>>
>> 2009-05-15 ?Kai Tietz ?<kai.tietz@onevision.com>
>>
>> ? ? ? * config/obj-coff.c (obj_coff_section): Add 'y' as
>> ? ? ? specifier for SEC_COFF_NOREAD section flag.
>>
>> I tested x86_64-pc-mingw32 and i686-pc-cygwin.
>> Is this patch ok for apply?
>
> ?Just a couple of very minor points:
>
> @@ -1589,6 +1590,10 @@
> ? ? ? ? ? ? ? ?case 'o': /* STYP_OVER */
> ? ? ? ? ? ? ? ? ?as_warn (_("unsupported section attribute '%c'"), attr);
> ? ? ? ? ? ? ? ? ?break;
> + ? ? ? ? ? ? ? case 'y':
> + ? ? ? ? ? ? ? ? flags |= SEC_COFF_NOREAD;
> + ? ? ? ? ? ? ? ? flags &= ~SEC_READONLY;
> + ? ? ? ? ? ? ? ? break;
>
> ? ? ? ? ? ? ? ?default:
> ? ? ? ? ? ? ? ? ?as_warn (_("unknown section attribute '%c'"), attr);
>
> 1. ?I think you ought to be setting readonly_removed in this clause?
> 2. ?There should be a blank line before the 'case' so it doesn't look like a
> fall-through.
> 3. ?While you're doing that anyway, why not move it above cases 'i', 'l' and
> 'o', so that all the functional case clauses are in one uninterrupted stretch
> at the start of the switch and both the error cases are right at end?
>
> ?Ok once we're clear about the readonly_removed issue (and with a re-test if
> I'm correct and we should be setting it).
>
> ? ?cheers,
> ? ? ?DaveK
>
>

To point 2 and 3: I'll move it up to the 'i','l', and 'o' case with
blank line. This makes sense.
Hmm, well we should set the read-only flag here, not zero'ing it out.
It makes not much differences here as we set READONLY in bfd IIRC, but
it makes things more clear to read.

Cheers,
Kai


-- 
|  (\_/) This is Bunny. Copy and paste
| (='.'=) Bunny into your signature to help
| (")_(") him gain world domination


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]