Re: [PATCH] Add dll trampoline code handling for windows 64bit

[Joel Brobecker <brobecker@adacore.com>]

Hi Roland,

Thanks for the patch.

In addition to Tom's answer, I have a few minor comments. The real
review will have to come from our Windows Maintainer for your patch
to be approved...

> 2012-03-14  Roland Schwingel<roland.schwingel@onevision.com>
> 
>         * amd64-windows-tdep.c: #include "frame.h"
>         (amd64_windows_skip_trampoline_code): New function.
> 	(amd64_windows_init_abi): Add trampoline registration.

(missing period at the end of the first line).

> +/* Stuff for WIN64 PE style DLL's but is pretty generic really.  */

It's great that you thought about adding a description comment to
document the new function, thank you! I just wish the description
was a little more precise. In fact, what this function does should
already be described in gdbarch.h.  For gdbarch callback/"methods",
we usually simply say:

/* Implement the "skip_trampoline_code" gdbarch method.  */

No need to repeat the hook documentation at each instance.

> +    struct gdbarch     *gdbarch = get_frame_arch (frame);
> +    enum bfd_endian     byte_order = gdbarch_byte_order (gdbarch);

Formatting nit: We do not align parameters in GDB code.

Also, the indentation is wrong. We use 2 spaces, not 4, so the whole
function needs to be re-indented.

> +   /* check for jmp *<offset>(%rip) */

Also, the GNU Coding Style which GDB follows requires us to start
sentences with a capital letter, and to end them with a period.
(and 2 spaces after periods).  I would format the comment above as:

        /* Check for "jmp *<offset>(%rip)".  */

> +    if (pc && read_memory_unsigned_integer (pc, 2, byte_order) == 0x25ff)
> +      {
> +	  unsigned long indirect =
> +	    read_memory_unsigned_integer (pc + 2, 4, byte_order);

Can use use ULONGEST here, instead of "unsigned long"?

> +		      CORE_ADDR           destination;
> +		      gdb_byte           *pos, addr[8];

Same as above, please, no extra spacing to align the parameter names.

> +		      read_memory (pc + indirect, addr, 8);
> +		      pos = (gdb_byte *) &destination;
> +		      pos[0] = addr[6];
> +		      pos[1] = addr[7];
> +		      pos[2] = addr[0];
> +		      pos[3] = addr[1];
> +		      pos[4] = addr[2];
> +		      pos[5] = addr[3];
> +		      pos[6] = addr[4];
> +		      pos[7] = addr[5];
> +
> +		      return destination;

Yeah, Tom's suggestion is a better suggestion. I think you are going
to have endianness issues this way.  You could use
read_memory_typed_address as well, but it's a little more involved,
and I don't think it's necessary here.  But otherwise, to me, the
latter is the function to be used for reading addresses from inferior
memory.

I am sorry if it feels like it's a lot of little rules. It is. But
it should be easy to learn them and it allows us to have a consistent
style for our code.

Thanks,
-- 
Joel