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

[Kai Tietz]

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