This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: RFC: Dumping of .pdata/.xdata for x64 PE+
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"?
> + /* 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