From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29916 invoked by alias); 8 Oct 2014 19:32:57 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 29905 invoked by uid 89); 8 Oct 2014 19:32:56 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL,BAYES_00,LIKELY_SPAM_BODY,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 08 Oct 2014 19:32:55 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s98JWq32015150 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Wed, 8 Oct 2014 15:32:52 -0400 Received: from localhost (dhcp-10-15-16-169.yyz.redhat.com [10.15.16.169]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s98JWqIg021913 (version=TLSv1/SSLv3 cipher=AES128-GCM-SHA256 bits=128 verify=NO); Wed, 8 Oct 2014 15:32:52 -0400 From: Sergio Durigan Junior To: "Jose E. Marchesi" Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 6/9] Support for DTrace USDT probes in x86_64 targets. References: <1411724905-31234-1-git-send-email-jose.marchesi@oracle.com> <1411724905-31234-7-git-send-email-jose.marchesi@oracle.com> X-URL: http://blog.sergiodj.net Date: Wed, 08 Oct 2014 19:32:00 -0000 In-Reply-To: <1411724905-31234-7-git-send-email-jose.marchesi@oracle.com> (Jose E. Marchesi's message of "Fri, 26 Sep 2014 11:48:22 +0200") Message-ID: <874mvebbmk.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2014-10/txt/msg00171.txt.bz2 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 > > * 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 > > + * 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 > + > * 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/