This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 01/15] gdbarch: add instruction predicate methods
- From: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- To: Markus Metzger <markus dot t dot metzger at intel dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Mon, 13 May 2013 17:23:21 +0200
- Subject: Re: [PATCH 01/15] gdbarch: add instruction predicate methods
- References: <1367496216-21217-1-git-send-email-markus dot t dot metzger at intel dot com> <1367496216-21217-2-git-send-email-markus dot t dot metzger at intel dot com>
On Thu, 02 May 2013 14:03:22 +0200, Markus Metzger wrote:
[...]
> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> index 3ab74f0..30013b3 100644
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -1365,6 +1365,22 @@ amd64_absolute_jmp_p (const struct amd64_insn *details)
> }
>
Function comment/description.
> static int
> +amd64_jmp_p (const struct amd64_insn *details)
> +{
> + const gdb_byte *insn = &details->raw_insn[details->opcode_offset];
> +
> + /* jump short, relative. */
> + if (insn[0] == 0xeb)
> + return 1;
> +
> + /* jump near, relative. */
> + if (insn[0] == 0xe9)
> + return 1;
> +
> + return amd64_absolute_jmp_p (details);
> +}
> +
> +static int
> amd64_absolute_call_p (const struct amd64_insn *details)
> {
> const gdb_byte *insn = &details->raw_insn[details->opcode_offset];
> @@ -1435,6 +1451,53 @@ amd64_syscall_p (const struct amd64_insn *details, int *lengthp)
> return 0;
> }
>
> +/* Classify the instruction at ADDR using PRED. */
+ /* Throw error if it cannot be read. */
> +
> +static int
> +amd64_classify_insn_at (struct gdbarch *gdbarch, CORE_ADDR addr,
> + int (*pred) (const struct amd64_insn *))
> +{
> + struct amd64_insn details;
> + gdb_byte *buf;
> + int len, classification;
> +
> + len = gdbarch_max_insn_length (gdbarch);
> + buf = xzalloc (len);
xmalloc is sufficient. But as read_memory can throw we need make_cleanup with
xfree. But that seems as too complicated, use just alloca here.
(I see there is already the same bug in amd64_relocate_instruction.)
> +
> + read_memory (addr, buf, len);
gdbarch_max_insn_length may be longer than the real instruction and readable
memory but the same bug is in amd64_relocate_instruction so it can be fixed in
the future together, OK with it as is.
> + amd64_get_insn_details (buf, &details);
> +
> + classification = pred (&details);
> +
> + xfree (buf);
> +
> + return classification;
> +}
> +
[...]
> --- a/gdb/gdbarch.sh
> +++ b/gdb/gdbarch.sh
> @@ -972,6 +972,15 @@ m:void:iterate_over_objfiles_in_search_order:iterate_over_objfiles_in_search_ord
>
> # Ravenscar arch-dependent ops.
> v:struct ravenscar_arch_ops *:ravenscar_ops:::NULL:NULL::0:host_address_to_string (gdbarch->ravenscar_ops)
> +
> +# Return non-zero if the instruction at ADDR is a call; zero otherwise.
> +M:int:insn_call_p:CORE_ADDR addr:addr
> +
> +# Return non-zero if the instruction at ADDR is a return; zero otherwise.
> +M:int:insn_ret_p:CORE_ADDR addr:addr
> +
> +# Return non-zero if the instruction at ADDR is a jump; zero otherwise.
> +M:int:insn_jump_p:CORE_ADDR addr:addr
These *_p functions with 'M' lead to calls like:
if (!gdbarch_insn_call_p_p (gdbarch))
return NULL;
There are already existing functions like amd64_ret_p but it would be better
not to follow this naming for gdbarch.
gdbarch can have methods like insn_is_call, insn_is_ret etc.
Additionally you can also provide default function here always returning zero
by
M:int:insn_is_call:CORE_ADDR addr:addr::default_insn_is_call
to simplify
if (gdbarch_insn_ret_p_p (gdbarch) && gdbarch_insn_ret_p (gdbarch, lpc))
->
if (gdbarch_insn_ret_p (gdbarch, lpc))
but maybe you are already aware of it and chose it as is intentionally.
> EOF
> }
>
> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
> index 930d6fc..1d011de 100644
> --- a/gdb/i386-tdep.c
> +++ b/gdb/i386-tdep.c
> @@ -473,6 +473,20 @@ i386_absolute_jmp_p (const gdb_byte *insn)
> }
>
Function comment/description.
> static int
> +i386_jmp_p (const gdb_byte *insn)
> +{
> + /* jump short, relative. */
> + if (insn[0] == 0xeb)
> + return 1;
> +
> + /* jump near, relative. */
> + if (insn[0] == 0xe9)
> + return 1;
> +
> + return i386_absolute_jmp_p (insn);
> +}
> +
> +static int
> i386_absolute_call_p (const gdb_byte *insn)
> {
> /* call far, absolute. */
[...]
Thanks,
Jan