This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Add dll trampoline code handling for windows 64bit
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Roland Schwingel <roland at onevision dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Wed, 14 Mar 2012 09:13:03 -0700
- Subject: Re: [PATCH] Add dll trampoline code handling for windows 64bit
- References: <4F609EB7.2020801@onevision.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