Re: [patch PE-COFF]: Some minor nits

[Kai Tietz <ktietz70 at googlemail dot com>]

2009/3/18 Dave Korn <dave.korn.cygwin@googlemail.com>:
> Kai Tietz wrote:
>> Dave Korn <dave.korn.cygwin@googlemail.com> wrote on 17.03.2009 00:15:12:
>
>>> ? Can you point us at a reference for this? ?And have you run the GDB
>>> testsuite to see if it causes any changes?
>
>> Well, see pe-coff-v8 specification in section "46 Special Sections". Here
>> are the used section flags given.
>
> ?Right, thank you. ?That looks like the correct mapping of bits, but I think
> I spotted something inconsistent:
>
> + ?if ((sec_flags & SEC_COFF_NOREAD) == 0)
> + ? ?styp_flags |= IMAGE_SCN_MEM_READ; ? ? /* Invert NOREAD for read. ?*/
>
> ?Set IMAGE_SCN_MEM_READ if *NOT* COFF_NOREAD.
>
> + ?/* If section disallows read, then set the NOREAD flag. */
> + ?if ((styp_flags & IMAGE_SCN_MEM_READ) == 0)
> + ? ?sec_flags |= SEC_COFF_NOREAD;
>
> ?Set COFF_NOREAD if *NOT* IMAGE_SCN_MEM_READ.
>
> ? ? ? ?case IMAGE_SCN_MEM_READ:
> - ? ? ? ? /* Ignored, assume it always to be true. ?*/
> + ? ? ? ? sec_flags |= SEC_COFF_NOREAD;
>
> ?Set COFF_NOREAD if IMAGE_SCN_MEM_READ. ?This looks like the wrong way round,
> the bit should be set by default and cleared if IMAGE_SCN_MEM_READ is found?
>
> ?I also see that in both these two hunks it would be possible for a section
> that has neither IMAGE_SCN_MEM_READ nor IMAGE_SCN_MEM_WRITE to end up setting
> SEC_READONLY and SEC_COFF_NOREAD simultaneously, because SEC_READONLY gets set
> by default. ?That seems inconsistent to me, could it be a problem? ?I think it
> will lead to the section in memory being writeable but not readable, rather
> than neither.
>
>> I tested it with gdb for w64 and found no regression here. Other platforms
>> I didn't tested with gdb, just by binutil's testsuite.
>
> ?I'm sure just one platform is enough for the GDB testsuite, I was just
> hoping to make sure you'd done a smoke test that it didn't all break.
>
> ? ?cheers,
> ? ? ?DaveK
>
>

Thanks Dave for reviewing it. Here the adjusted patch as discussed.

ChangeLog
2009-03-18  Kai Tietz  <kai.tietz@onevision.com>

       * bfd-in2.h: Regenerated.
       * coffcode.h (sec_to_styp_flags): For pe-coff add SEC_READONLY
       for debugging sections and map memory read/write dependent on
       SEC_COFF_NOREAD.
       (styp_to_sec_flags): Set SEC_COFF_NOREAD for sections
       without memory read flags set.
       * section.c: Add SEC_COFF_NOREAD to section flags.


Tested on mingw-w64 and cygwin without any new regressions.

Ok for apply?

Cheers,
Kai

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

Attachment: noread.diff
Description: Binary data