Re: [PATCH 6/9] Support for DTrace USDT probes in x86_64 targets.

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Friday, September 26 2014, Jose E. Marchesi wrote:

> This patch adds the target-specific code in order to support the
> calculation of DTrace probes arguments in x86_64 targets, and also the
> enabling and disabling of probes.  This is done by implementing the
> `dtrace_*' gdbarch handlers.

Thanks.  Comments below.

> gdb:
>
> 2014-09-26  Jose E. Marchesi  <jose.marchesi@oracle.com>
>
> 	* amd64-linux-tdep.h: Prototypes for
> 	`amd64_dtrace_probe_argument', `amd64_dtrace_enable_probe',
> 	`amd64_dtrace_disable_probe' and `gdbarch_dtrace_probe_is_enabled'.
>
> 	* amd64-linux-tdep.c (amd64_dtrace_probe_argument): New function.
> 	(amd64_dtrace_probe_is_enabled): Likewise.
> 	(amd64_dtrace_enable_probe): Likewise.
> 	(amd64_dtrace_disable_probe): Likewise.
> 	(amd64_linux_init_abi): Register the
> 	`gdbarch_dtrace_probe_argument', `gdbarch_dtrace_enable_probe',
> 	`gdbarch_dtrace_disable_probe' and `gdbarch_dtrace_probe_is_enabled'.
> ---
>  gdb/ChangeLog          |   14 +++++
>  gdb/amd64-linux-tdep.c |  150 ++++++++++++++++++++++++++++++++++++++++++++++++
>  gdb/amd64-linux-tdep.h |   11 ++++
>  3 files changed, 175 insertions(+)
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index eac03e7..a32d303 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,5 +1,19 @@
>  2014-09-26  Jose E. Marchesi  <jose.marchesi@oracle.com>
>  
> +	* amd64-linux-tdep.h: Prototypes for
> +	`amd64_dtrace_probe_argument', `amd64_dtrace_enable_probe',
> +	`amd64_dtrace_disable_probe' and `gdbarch_dtrace_probe_is_enabled'.
> +
> +	* amd64-linux-tdep.c (amd64_dtrace_probe_argument): New function.
> +	(amd64_dtrace_probe_is_enabled): Likewise.
> +	(amd64_dtrace_enable_probe): Likewise.
> +	(amd64_dtrace_disable_probe): Likewise.
> +	(amd64_linux_init_abi): Register the
> +	`gdbarch_dtrace_probe_argument', `gdbarch_dtrace_enable_probe',
> +	`gdbarch_dtrace_disable_probe' and `gdbarch_dtrace_probe_is_enabled'.
> +
> +2014-09-26  Jose E. Marchesi  <jose.marchesi@oracle.com>
> +
>  	* breakpoint.c (BREAK_ARGS_HELP): help string updated to mention
>  	the -probe-dtrace new vpossible value for PROBE_MODIFIER.
>  
> diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
> index 850ca20..273f5c4 100644
> --- a/gdb/amd64-linux-tdep.c
> +++ b/gdb/amd64-linux-tdep.c
> @@ -28,6 +28,8 @@
>  #include "gdbtypes.h"
>  #include "reggroups.h"
>  #include "regset.h"
> +#include "parser-defs.h"
> +#include "user-regs.h"
>  #include "amd64-linux-tdep.h"
>  #include "i386-linux-tdep.h"
>  #include "linux-tdep.h"
> @@ -1609,6 +1611,148 @@ amd64_linux_core_read_description (struct gdbarch *gdbarch,
>      }
>  }
>  
> +/* Implementation of `gdbarch_dtrace_probe_is_enabled', as defined in
> +   gdbarch.h.  */
> +
> +int
> +amd64_dtrace_probe_is_enabled (struct gdbarch *gdbarch, CORE_ADDR addr)
> +{
> +  /* The instruction sequence used in x86_64 machines for a disabled
> +     is-enabled probe is:
> +
> +              xor %rax, %rax  =>  48 33 C0
> +     ADDR:    nop             =>  90
> +              nop             =>  90
> +
> +     or
> +
> +              xor %rax, %rax  =>  48 33 C0
> +     ADDR:    ret             =>  c3
> +              nop             =>  90
> +
> +     This function returns 1 if the instructions at ADDR do _not_
> +     follow any of these patterns.
> +
> +     Note that ADDR is offset 3 bytes from the beginning of these
> +     sequences.  */

This comment could be placed on the top of the function (after the
"Implementation of ..."), or after the declaration of the variables.
Sorry for the nitpick, but I find it strange to make the comment before
declaring the variables in the body of the function (and I don't
remember seeing this on GDB very frequently).

> +  gdb_byte buf[5];
> +  read_memory (addr - 3, buf, 5);
> +
> +  return !((buf[0] == 0x48) && (buf[1] == 0x33) && (buf[2] == 0xc0) /* xor */
> +	   && ((buf[3] == 0x90) || (buf[3] == 0xc3))                /* nop | ret */
> +	   && (buf[4] == 0x90));                                    /* nop */
> +}
> +
> +/* Implementation of `gdbarch_dtrace_enable_probe', as defined in
> +   gdbarch.h.  */
> +
> +void
> +amd64_dtrace_enable_probe (struct gdbarch *gdbarch, CORE_ADDR addr)
> +{
> +  /* We use the following instruction sequence for enabling an
> +     is-enabled probe:
> +
> +        mov $0x1, %eax => b8 01 00 00 00
> +
> +     Note also that ADDR is offset 3 bytes from the beginning of the
> +     sequence.  */
> +
> +  gdb_byte buf[5];
> +
> +  buf[0] = 0xb8; buf[1] = 0x01; buf[2] = 0x00; buf[3] = 0x00; buf[4] = 0x00;

Each assignment should go on its own line.

> +  write_memory (addr - 3, buf, 5);
> +}
> +
> +/* Implementation of `gdbarch_dtrace_disable_probe', as defined in
> +   gdbarch.h.  */
> +
> +void
> +amd64_dtrace_disable_probe (struct gdbarch *gdbarch, CORE_ADDR addr)
> +{
> +  /* We use the following instruction sequence for disabling an
> +     is-enabled probe:
> +
> +     xor %rax, %rax; nop; nop  =>  48 33 C0 90 90
> +
> +     Note that ADDR is offset 3 bytes from the beginning of the
> +     sequence.  */
> +
> +  gdb_byte buf[5];
> +
> +  buf[0] = 0x48; buf[1] = 0x33; buf[2] = 0xc0; buf[3] = 0x90; buf[4] = 0x90;

Likewise.

> +  write_memory (addr - 3, buf, 5);
> +}
> +
> +/* Implementation of `gdbarch_dtrace_parse_special_token', as defined in
> +   gdbarch.h.  */

This comment needs an update, there is no
"gdbarch_dtrace_parse_special_token" :-).

> +
> +void
> +amd64_dtrace_probe_argument (struct gdbarch *gdbarch,
> +			     struct parser_state *pstate,
> +			     int narg)
> +{
> +  static int arg_reg_map[6] =
> +    {
> +      AMD64_RDI_REGNUM,  /* Arg 1.  */
> +      AMD64_RSI_REGNUM,  /* Arg 2.  */
> +      AMD64_RDX_REGNUM,  /* Arg 3.  */
> +      AMD64_RCX_REGNUM,  /* Arg 4.  */
> +      AMD64_R8_REGNUM,   /* Arg 5.  */
> +      AMD64_R9_REGNUM    /* Arg 6.  */
> +    };
> +
> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +  struct frame_info *this_frame = get_selected_frame (NULL);
> +  struct stoken str;

No newline between variables being declared.

> +
> +  /* DTrace probe arguments can be found on the ABI-defined places for
> +     regular arguments at the current PC.  The probe abstraction
> +     currently supports up to 12 arguments for probes.  */
> +
> +  if (narg < 6)
> +    {
> +      int regno = arg_reg_map [narg];

No space between variable name and index operator "[".

> +      const char *regname = user_reg_map_regnum_to_name (gdbarch, regno);
> +
> +      write_exp_elt_opcode (pstate, OP_REGISTER);
> +      str.ptr = regname;
> +      str.length = strlen (regname);
> +      write_exp_string (pstate, str);
> +      write_exp_elt_opcode (pstate, OP_REGISTER);
> +    }
> +  else
> +    {
> +      /* Additional arguments are passed on the stack.  */
> +
> +      CORE_ADDR sp;

Spurious newline.

> +      const char *regname = user_reg_map_regnum_to_name (gdbarch, AMD64_RSP_REGNUM);
> +
> +      /* Displacement.  */
> +      write_exp_elt_opcode  (pstate, OP_LONG);
> +      write_exp_elt_type    (pstate, builtin_type (gdbarch)->builtin_long);
> +      write_exp_elt_longcst (pstate, narg - 6);
> +      write_exp_elt_opcode  (pstate, OP_LONG);

As I said in another message, I'm not a fan of aligning the open paren
for function calls; it messes with a simple grep, for example.

> +
> +      /* Register: SP.  */
> +      write_exp_elt_opcode (pstate, OP_REGISTER);
> +      str.ptr = regname;
> +      str.length = strlen (regname);
> +      write_exp_string (pstate, str);
> +      write_exp_elt_opcode (pstate, OP_REGISTER);
> +
> +      write_exp_elt_opcode (pstate, BINOP_ADD);
> +
> +      /* Cast to long. */
> +      write_exp_elt_opcode (pstate, UNOP_CAST);
> +      write_exp_elt_type   (pstate,
> +			    lookup_pointer_type (builtin_type (gdbarch)->builtin_long));
> +      write_exp_elt_opcode (pstate, UNOP_CAST);
> +
> +      write_exp_elt_opcode (pstate, UNOP_IND);
> +    }
> +}
> +
>  static void
>  amd64_linux_init_abi_common(struct gdbarch_info info, struct gdbarch *gdbarch)
>  {
> @@ -1872,6 +2016,12 @@ amd64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
>    /* GNU/Linux uses SVR4-style shared libraries.  */
>    set_solib_svr4_fetch_link_map_offsets
>      (gdbarch, svr4_lp64_fetch_link_map_offsets);
> +
> +  /* Register DTrace handlers.  */
> +  set_gdbarch_dtrace_probe_argument (gdbarch, amd64_dtrace_probe_argument);
> +  set_gdbarch_dtrace_probe_is_enabled (gdbarch, amd64_dtrace_probe_is_enabled);
> +  set_gdbarch_dtrace_enable_probe (gdbarch, amd64_dtrace_enable_probe);
> +  set_gdbarch_dtrace_disable_probe (gdbarch, amd64_dtrace_disable_probe);
>  }
>  
>  static void
> diff --git a/gdb/amd64-linux-tdep.h b/gdb/amd64-linux-tdep.h
> index 25563b8..b28dc50 100644
> --- a/gdb/amd64-linux-tdep.h
> +++ b/gdb/amd64-linux-tdep.h
> @@ -594,4 +594,15 @@ enum amd64_x32_syscall {
>    amd64_x32_sys_getsockopt = (amd64_x32_syscall_bit + 542),
>  };
>  
> +/* DTrace related functions.  */
> +
> +extern void amd64_dtrace_probe_argument (struct gdbarch *gdbarch,
> +					 struct parser_state *pstate,
> +					 int narg);
> +
> +extern int amd64_dtrace_probe_is_enabled (struct gdbarch *gdbarch, CORE_ADDR addr);
> +
> +extern void amd64_dtrace_enable_probe (struct gdbarch *gdbarch, CORE_ADDR addr);
> +extern void amd64_dtrace_disable_probe (struct gdbarch *gdbarch, CORE_ADDR addr);
> +
>  #endif /* amd64-linux-tdep.h */
> -- 
> 1.7.10.4

Sorry for not being able to do an extensive review, but dtrace is
proprietary and I know very little about its architecture.

-- 
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/