Re: [PATCH 01/15] gdbarch: add instruction predicate methods

[Jan Kratochvil <jan.kratochvil@redhat.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