Re: RFC: Dumping of .pdata/.xdata for x64 PE+

[Kai Tietz <ktietz70 at googlemail dot com>]

2009/4/17 Dave Korn <dave.korn.cygwin@googlemail.com>:
> Kai Tietz wrote:
>
>> Ok, here is the patch
>
>> Tested for x86_64-pc-mingw32 target. Is this patch ok to apply?
>
> ?I have a few comments:
>
>> +/* Note we have to make sure not all headers are protected
>> + ? by closurs, so we check define PEI_HEADERS to prevent
>> + ? double including in pei-x86_64.c. ?*/
>
> ?Typo, and I'm not sure closures means that anyway; how about rewording it to
> "wrapped in #ifdef guards"? ?In two places: bfd/coff-x86_64.c and
> bfd/pei-x86_64.c. ?Also, gnu style item: two spaces between the end of the
> comment text and the closing "*/" (only in pei-x86_64.c), not one.
>
>> Index: src/bfd/pei-x86_64.c
>> ===================================================================
>> --- src.orig/bfd/pei-x86_64.c
>> +++ src/bfd/pei-x86_64.c
>> @@ -25,9 +25,9 @@
>>
>> ?#define TARGET_SYM ? ? ? ? ? x86_64pei_vec
>> ?#define TARGET_NAME ? ? ? ? ?"pei-x86-64"
>> -#define COFF_IMAGE_WITH_PE
>> ?#define COFF_WITH_PE
>> ?#define COFF_WITH_pex64
>> +#define COFF_IMAGE_WITH_PE
>> ?#define PCRELOFFSET ? ? ? ? ?TRUE
>> ?#define TARGET_UNDERSCORE ? ?'_'
>> ?/* Long section names not allowed in executable images, only object files. ?*/
>
> ?Might as well undo this needless change while you're at it.
>
>> +static void
>> +pep_xdata_print_uwd_codes(bfd_byte *dta, bfd_vma count, FILE *file,
>> bfd_vma pc_addr)
>
> ?Always a space between function name and opening bracket.
>
>> + ?/* Sort array ascending. Note: it is stored in reveresed order. ?*/
>
> ?Typoe: reversed. ?:) I'll just leave that there.
>
>> + ? ? case 3: case 10:
>> + ? ? case 1:
>> + ? ? case 4: case 6: case 8:
>> + ? ? case 5: case 7: case 9:
>> + ? ? case 0: /* UWOP_PUSH_NONVOL. ?*/
>> + ? ? case 1: /* UWOP_ALLOC_LARGE. ?*/
>
> .... etc., etc.: not right IMO. ?If these constants aren't yet available in
> upstream w32api headers, you can always #define some proper constants
> yourself. ?If there's a whole new header file defining them guess you may need
> to add an autoconf test for it? ?Otherwise if they're defined in an existing
> header file, you could just do "#ifndef UWOP_xxx / #define UWOP_xxx / #endif".
>
>> + ? ? ?switch (dta[1]&0xf) {
>> + ? ? ? if (((dta[1]>>4)&0xf) == 0)
>
> ?Likewise I think accessor macros for the fields would be nice. ?I know it's
> a pain but this code has to live forever and work everywhere, please let's be
> thorough.
>
>> + ? ? case 1: /* UWOP_ALLOC_LARGE. ?*/
>> + ? ? ? if (((dta[1]>>4)&0xf) == 0)
>> + ? ? ? ? tmp = (bfd_vma) (*((unsigned short *)&dta[2]));
>> + ? ? ? else
>> + ? ? ? ? tmp = (bfd_vma) (*((unsigned int *)&dta[2]));
>> + ? ? ? tmp *= 8;
>> + ? ? ? fprintf (file, "save stack region of size 0x");
>
> ?? ?That's not what I understood the algorithm to be;
>
>> UWOP_ALLOC_LARGE (1)2 or 3 nodes
>>
>> Allocate a large-sized area on the stack. There are two forms. If the
>> operation info equals 0, then the size of the allocation divided by 8 is
>> recorded in the next slot, allowing an allocation up to 512K – 8. If the
>> operation info equals 1, then the unscaled size of the allocation is
>> recorded in the next two slots in little-endian format, allowing
>> allocations up to 4GB – 8.
>
> so I think the "tmp *= 8" in both cases is incorrect, isn't it? ?Only case 0
> should be multiplied by 8, case 1 is "unscaled"?

Right, thanks for notice. I'll change it.

>> + ? ? ?/* Array of UNWIND_CODE with count_of_codes elements. This
>> + ? ? ? ? this structure has byte size and the following definition:
>
> ?This this comment has a repeated word!
>
>> + ? ? ?/* if flag has bit 0 set, there is an exception hander. ?*/
>> + ? ? ?bfd_vma exception_handler = 0;
>> + ? ? ?/* if flag has bit 2 set, there is a function entry present. ?*/
>> + ? ? ?bfd_vma function_entry = 0;
>> + ? ? ?/* if flag has bit 0 set, there is an additionally exception data present. ?*/
>
> ?These comments are unclear, omit leading capital letters, typo "hander",
> incorrect grammar "an additionally", and the two that both relate to bit 0
> should be merged. ?How about:
>
>>> + ? ? ?/* ?If flag has bit 0 set, there is an exception handler
>>> + ? ? ? ? ?and additional (language-specific) exception data
>>> + ? ? ? ? ?present. ?*/
>>> + ? ? ?bfd_vma exception_handler = 0;
>>> + ? ? ?/* ?If flag has bit 2 set, there is a function entry present. ?*/
>>> + ? ? ?bfd_vma function_entry = 0;
>
> ... or similar?
>
>> + ? ? ?if (!data) return;
>
> ?Should be a separate line + indent for the return statement.
>
>> + ? ? ?switch ((flags & 0x1f))
>> + ? ? ? ?{
>> + ? ? ? ? ?case 0:
>> + ? ? ? ? fprintf (file, "none");
>> + ? ? ? ? break;
>> + ? ? ? case 1:
>
> ?The "case 0" is misaligned, needs a TAB instead of leading eight spaces.
>
>> + ? ? ?if ((flags & 1) != 0)
>> + ? ? ? ?{
>> + ? ? ? exception_handler = bfd_get_32 (abfd, data + addr);
>> + ? ? ? ? ?addr += 4;
>> + ? ? ? ?}
>> + ? ? ?else if ((flags & 2) != 0)
>> + ? ? {
>> + ? ? ? exception_handler = bfd_get_32 (abfd, data + addr);
>> + ? ? ? ? ?addr += 4;
>> + ? ? }
>> + ? ? ?else if ((flags & 4) != 0)
>> + ? ? ? ?{
>> + ? ? ? function_entry = bfd_get_32 (abfd, data + addr);
>> + ? ? ? addr += 4; /* RUNTIME_FUNCTION * */
>> + ? ? ? ?}
>
> ?Also some wonky indentation here. ?Needs reTABifying. ?I already mentioned
> the hard-coded constants issue.
>
>> +# define PDATA_ROW_SIZE ? ? ?(3 * 4)
>> +#undef PDATA_ROW_SIZE
>
> ?I get that you're making it a local definition to the function, but I don't
> think that's necessary. ?Why not just make it a file-scope #define at the top
> of the file alongside PEAOUTHDR? ?It's not just unlikely to clash with
> anything that would mean anything to the coff-x86_64.c include, if anything it
> would be likely relevant to any other coff target that chose to implement
> *PDATA*-anything!
>
>> +static bfd_boolean
>> +_xx_bfd_print_pdata (bfd *abfd, void *vfile)
>
>> +#define bfd_pe_print_pdata ? _xx_bfd_print_pdata
>
> ?Needs renaming now it's not gonna get the auto-xx-substitution any more!
>
> ?Apart from those issues, the basic shape and functionality of the patch
> looked right, but there could easily be typos I didn't spot among all the
> bit-twiddling and hard-coded constants; once you've done macros for them, any
> such errors will be either immediately obvious or obviously not there, so I
> really reckon it's worth the effort of respinning.
>
> ? ?cheers,
> ? ? ?DaveK
>
>
>

I'll rework this patch and add to the x64_64.h in coff the necessary
defines for it. I'll add the binary and explanded structures for it
there, too. So it can be a base for the implementation later, too.

The constants aren't present in standard header-sets. AFAIK some of
them are in ntddk. So I don't expect here any redefinition issues.

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