From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10302 invoked by alias); 2 Oct 2014 23:19:16 -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 10288 invoked by uid 89); 2 Oct 2014 23:19:16 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.3 required=5.0 tests=AWL,BAYES_50,LIKELY_SPAM_BODY,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS 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; Thu, 02 Oct 2014 23:19:12 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s92NJAoD022166 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Thu, 2 Oct 2014 19:19:10 -0400 Received: from localhost (dhcp-10-15-16-169.yyz.redhat.com [10.15.16.169]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s92NJ9oo001425 (version=TLSv1/SSLv3 cipher=AES128-GCM-SHA256 bits=128 verify=NO); Thu, 2 Oct 2014 19:19:09 -0400 From: Sergio Durigan Junior To: "Jose E. Marchesi" Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 5/9] New probe type: DTrace USDT probes. References: <1411724905-31234-1-git-send-email-jose.marchesi@oracle.com> <1411724905-31234-6-git-send-email-jose.marchesi@oracle.com> X-URL: http://blog.sergiodj.net Date: Thu, 02 Oct 2014 23:19:00 -0000 In-Reply-To: <1411724905-31234-6-git-send-email-jose.marchesi@oracle.com> (Jose E. Marchesi's message of "Fri, 26 Sep 2014 11:48:21 +0200") Message-ID: <87y4syt5zn.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/msg00046.txt.bz2 On Friday, September 26 2014, Jose E. Marchesi wrote: > This patch adds a new type of probe to GDB: the DTrace USDT probes. The new > type is added by providing functions implementing all the entries of the > `probe_ops' structure defined in `probe.h'. The implementation is > self-contained and does not depend on DTrace source code in any way. Thanks for the patch, Jose. It looks great. I have a few comments (below). > gdb: > > 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. > > * configure.ac (CONFIG_OBS): dtrace-probe.o added if BFD can > handle ELF files. > * Makefile.in (SFILES): dtrace-probe.c added. > * configure: Regenerate. > > * dtrace-probe.c: New file. > (SHT_SUNW_dof): New constant. > (dtrace_probe_type): New enum. > (dtrace_probe_arg): New struct. > (dtrace_probe_arg_s): New typedef. > (struct dtrace_probe_enabler): New struct. > (dtrace_probe_enabler_s): New typedef. > (dtrace_probe): New struct. > (dtrace_probe_is_linespec): New function. > (dtrace_dof_sect_type): New enum. > (DTRACE_DOF_ID_MAG0-3): New constants. > (DTRACE_DOF_ID_ENCODING): Likewise. > (DTRACE_DOF_ENCODE_LSB): Likewise. > (DTRACE_DOF_ENCODE_MSB): Likewise. > (dtrace_dof_hdr): New struct. > (dtrace_dof_sect): Likewise. > (dtrace_dof_provider): Likewise. > (dtrace_dof_probe): Likewise. > (DOF_UINT): New macro. > (DTRACE_DOF_PTR): Likewise. > (DTRACE_DOF_SECT): Likewise. > (dtrace_process_dof_probe): New function. > (dtrace_process_dof): Likewise. > (dtrace_build_arg_exprs): Likewise. > (dtrace_get_arg): Likewise. > (dtrace_get_probes): Likewise. > (dtrace_get_probe_argument_count): Likewise. > (dtrace_can_evaluate_probe_arguments): Likewise. > (dtrace_evaluate_probe_argument): Likewise. > (dtrace_compile_to_ax): Likewise. > (dtrace_set_semaphore): Likewise. > (dtrace_clear_semaphore): Likewise. > (dtrace_probe_destroy): Likewise. > (dtrace_gen_info_probes_table_header): Likewise. > (dtrace_gen_info_probes_table_values): Likewise. > (dtrace_probe_is_enabled): Likewise. > (dtrace_probe_ops): New variable. > (info_probes_dtrace_command): New function. > (_initialize_dtrace_probe): Likewise. > --- > gdb/ChangeLog | 50 ++++ > gdb/Makefile.in | 3 +- > gdb/breakpoint.c | 3 +- > gdb/configure | 2 +- > gdb/configure.ac | 2 +- > gdb/dtrace-probe.c | 816 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 6 files changed, 872 insertions(+), 4 deletions(-) > create mode 100644 gdb/dtrace-probe.c > > diff --git a/gdb/ChangeLog b/gdb/ChangeLog > index 7041cfb..eac03e7 100644 > --- a/gdb/ChangeLog > +++ b/gdb/ChangeLog > @@ -1,5 +1,55 @@ > 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. > + > + * configure.ac (CONFIG_OBS): dtrace-probe.o added if BFD can > + handle ELF files. > + * Makefile.in (SFILES): dtrace-probe.c added. > + * configure: Regenerate. > + > + * dtrace-probe.c: New file. > + (SHT_SUNW_dof): New constant. > + (dtrace_probe_type): New enum. > + (dtrace_probe_arg): New struct. > + (dtrace_probe_arg_s): New typedef. > + (struct dtrace_probe_enabler): New struct. > + (dtrace_probe_enabler_s): New typedef. > + (dtrace_probe): New struct. > + (dtrace_probe_is_linespec): New function. > + (dtrace_dof_sect_type): New enum. > + (DTRACE_DOF_ID_MAG0-3): New constants. > + (DTRACE_DOF_ID_ENCODING): Likewise. > + (DTRACE_DOF_ENCODE_LSB): Likewise. > + (DTRACE_DOF_ENCODE_MSB): Likewise. > + (dtrace_dof_hdr): New struct. > + (dtrace_dof_sect): Likewise. > + (dtrace_dof_provider): Likewise. > + (dtrace_dof_probe): Likewise. > + (DOF_UINT): New macro. > + (DTRACE_DOF_PTR): Likewise. > + (DTRACE_DOF_SECT): Likewise. > + (dtrace_process_dof_probe): New function. > + (dtrace_process_dof): Likewise. > + (dtrace_build_arg_exprs): Likewise. > + (dtrace_get_arg): Likewise. > + (dtrace_get_probes): Likewise. > + (dtrace_get_probe_argument_count): Likewise. > + (dtrace_can_evaluate_probe_arguments): Likewise. > + (dtrace_evaluate_probe_argument): Likewise. > + (dtrace_compile_to_ax): Likewise. > + (dtrace_set_semaphore): Likewise. > + (dtrace_clear_semaphore): Likewise. > + (dtrace_probe_destroy): Likewise. > + (dtrace_gen_info_probes_table_header): Likewise. > + (dtrace_gen_info_probes_table_values): Likewise. > + (dtrace_probe_is_enabled): Likewise. > + (dtrace_probe_ops): New variable. > + (info_probes_dtrace_command): New function. > + (_initialize_dtrace_probe): Likewise. > + > +2014-09-26 Jose E. Marchesi > + > * gdbarch.sh (dtrace_probe_argument): New. > (dtrace_probe_is_enabled): Likewise. > (dtrace_enable_probe): Likewise. > diff --git a/gdb/Makefile.in b/gdb/Makefile.in > index cbec0d2..ad82215 100644 > --- a/gdb/Makefile.in > +++ b/gdb/Makefile.in > @@ -799,7 +799,8 @@ SFILES = ada-exp.y ada-lang.c ada-typeprint.c ada-valprint.c ada-tasks.c \ > cp-abi.c cp-support.c cp-namespace.c cp-valprint.c \ > d-exp.y d-lang.c d-support.c d-valprint.c \ > cp-name-parser.y \ > - dbxread.c demangle.c dictionary.c disasm.c doublest.c dummy-frame.c \ > + dbxread.c demangle.c dictionary.c disasm.c doublest.c \ > + dtrace-probe.c dummy-frame.c \ > dwarf2expr.c dwarf2loc.c dwarf2read.c dwarf2-frame.c \ > dwarf2-frame-tailcall.c \ > elfread.c environ.c eval.c event-loop.c event-top.c \ > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index bec7f68..95323bf 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -16197,7 +16197,8 @@ all_tracepoints (void) > command" [PROBE_MODIFIER] [LOCATION] [thread THREADNUM] [if CONDITION]\n\ > PROBE_MODIFIER shall be present if the command is to be placed in a\n\ > probe point. Accepted values are `-probe' (for a generic, automatically\n\ > -guessed probe type) or `-probe-stap' (for a SystemTap probe).\n\ > +guessed probe type), `-probe-stap' (for a SystemTap probe) or \n\ > +`-probe-dtrace' (for a DTrace probe).\n\ > LOCATION may be a line number, function name, or \"*\" and an address.\n\ > If a line number is specified, break at start of code for that line.\n\ > If a function is specified, break at start of code for that function.\n\ > diff --git a/gdb/configure b/gdb/configure > index 6e7435f..ecd0cff 100755 > --- a/gdb/configure > +++ b/gdb/configure > @@ -13281,7 +13281,7 @@ $as_echo "$gdb_cv_var_elf" >&6; } > LDFLAGS=$OLD_LDFLAGS > LIBS=$OLD_LIBS > if test $gdb_cv_var_elf = yes; then > - CONFIG_OBS="$CONFIG_OBS elfread.o stap-probe.o" > + CONFIG_OBS="$CONFIG_OBS elfread.o stap-probe.o dtrace-probe.o" > > $as_echo "#define HAVE_ELF 1" >>confdefs.h > > diff --git a/gdb/configure.ac b/gdb/configure.ac > index 4f5fb7b..7b1133a 100644 > --- a/gdb/configure.ac > +++ b/gdb/configure.ac > @@ -2103,7 +2103,7 @@ AC_SUBST(WIN32LIBS) > GDB_AC_CHECK_BFD([for ELF support in BFD], gdb_cv_var_elf, > [bfd_get_elf_phdr_upper_bound (NULL)], elf-bfd.h) > if test $gdb_cv_var_elf = yes; then > - CONFIG_OBS="$CONFIG_OBS elfread.o stap-probe.o" > + CONFIG_OBS="$CONFIG_OBS elfread.o stap-probe.o dtrace-probe.o" > AC_DEFINE(HAVE_ELF, 1, > [Define if ELF support should be included.]) > # -ldl is provided by bfd/Makfile.am (LIBDL) . > diff --git a/gdb/dtrace-probe.c b/gdb/dtrace-probe.c > new file mode 100644 > index 0000000..f529ca5 > --- /dev/null > +++ b/gdb/dtrace-probe.c > @@ -0,0 +1,816 @@ > +/* DTrace probe support for GDB. > + > + Copyright (C) 2014 Free Software Foundation, Inc. > + > + Contributed by Oracle, Inc. > + > + This file is part of GDB. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > +#include "defs.h" > +#include "probe.h" > +#include "vec.h" > +#include "elf-bfd.h" > +#include "gdbtypes.h" > +#include "obstack.h" > +#include "objfiles.h" > +#include "complaints.h" > +#include "value.h" > +#include "ax.h" > +#include "ax-gdb.h" > +#include "language.h" > +#include "parser-defs.h" > +#include "inferior.h" > + > +/* The type of the ELF sections where we will find the DOF programs > + with information about probes. */ > + > +#ifndef SHT_SUNW_dof > +# define SHT_SUNW_dof 0x6ffffff4 > +#endif Can this macro exist in another header file that you are including? > + > +/* Forward declaration. */ > + > +static const struct probe_ops dtrace_probe_ops; > + > +/* The following structure represents a single argument for the > + probe. */ > + > +struct dtrace_probe_arg > +{ > + /* The type of the probe argument. */ > + struct type *type; > + > + /* A string describing the type. */ > + char *type_str; > + > + /* The argument converted to an internal GDB expression. */ > + struct expression *expr; > +}; > + > +typedef struct dtrace_probe_arg dtrace_probe_arg_s; > +DEF_VEC_O (dtrace_probe_arg_s); > + > +/* The following structure represents an enabler for a probe. */ > + > +struct dtrace_probe_enabler > +{ > + /* Program counter where the is-enabled probe is installed. The > + contents (nops, whatever...) stored at this address are > + architecture dependent. */ > + CORE_ADDR address; > +}; > + > +typedef struct dtrace_probe_enabler dtrace_probe_enabler_s; > +DEF_VEC_O (dtrace_probe_enabler_s); > + > +/* The following structure represents a dtrace probe. */ > + > +struct dtrace_probe > +{ > + /* Generic information about the probe. This must be the first > + element of this struct, in order to maintain binary compatibility > + with the `struct probe' and be able to fully abstract it. */ > + struct probe p; > + > + /* A probe can have zero or more arguments. */ > + int probe_argc; > + VEC (dtrace_probe_arg_s) *args; > + > + /* A probe can have zero or more "enablers" associated with it. */ > + VEC (dtrace_probe_enabler_s) *enablers; > + > + /* Whether the expressions for the arguments have been built. */ > + int args_expr_built : 1; > +}; > + > +/* Implementation of the probe_is_linespec method. */ > + > +static int > +dtrace_probe_is_linespec (const char **linespecp) > +{ > + static const char *const keywords[] = { "-pdtrace", "-probe-dtrace", NULL }; > + > + return probe_is_linespec_by_keyword (linespecp, keywords); > +} > + > +/* DOF programs can contain an arbitrary number of sections of 26 > + different types. In order to support DTrace USDT probes we only > + need to handle a subset of these section types, fortunately. These > + section types are defined in the following enumeration. > + > + See linux/dtrace/dof_defines.h for a complete list of section types > + along with their values. */ > + > +enum dtrace_dof_sect_type > +{ > + DTRACE_DOF_SECT_TYPE_NONE = 0, > + DTRACE_DOF_SECT_TYPE_ECBDESC = 3, > + DTRACE_DOF_SECT_TYPE_STRTAB = 8, > + DTRACE_DOF_SECT_TYPE_PROVIDER = 15, > + DTRACE_DOF_SECT_TYPE_PROBES = 16, > + DTRACE_DOF_SECT_TYPE_PRARGS = 17, > + DTRACE_DOF_SECT_TYPE_PROFFS = 18, > + DTRACE_DOF_SECT_TYPE_PRENOFFS = 26 > +}; It's better to put a comment in each field, describing them. > + > +/* The following collection of data structures map the structure of > + DOF entities. Again, we only cover the subset of DOF used to > + implement USDT probes. > + > + See linux/dtrace/dof.h header for a complete list of data > + structures. */ > + > +/* Indexes to use in `dofh_ident' below. */ > +#define DTRACE_DOF_ID_MAG0 0 > +#define DTRACE_DOF_ID_MAG1 1 > +#define DTRACE_DOF_ID_MAG2 2 > +#define DTRACE_DOF_ID_MAG3 3 > +#define DTRACE_DOF_ID_ENCODING 5 > + > +/* Possible values for dofh_ident[DOF_ID_ENCODING]. */ > +#define DTRACE_DOF_ENCODE_LSB 1 > +#define DTRACE_DOF_ENCODE_MSB 2 I know it is a matter of taste, but I like more when you use enum's for these kind of definitions (like above) :-). > + > +struct dtrace_dof_hdr > +{ > + uint8_t dofh_ident[16]; > + uint32_t dofh_flags; > + uint32_t dofh_hdrsize; > + uint32_t dofh_secsize; > + uint32_t dofh_secnum; > + uint64_t dofh_secoff; > + uint64_t dofh_loadsz; > + uint64_t dofh_filesz; > + uint64_t dofh_pad; > +}; > + > +struct dtrace_dof_sect > +{ > + uint32_t dofs_type; /* See the enum dtrace_dof_sect above. */ > + uint32_t dofs_align; > + uint32_t dofs_flags; > + uint32_t dofs_entsize; > + uint64_t dofs_offset; /* DOF + offset points to the section data. */ > + uint64_t dofs_size; /* Size of section data in bytes. */ > +}; > + > +struct dtrace_dof_provider > +{ > + uint32_t dofpv_strtab; /* Link to a DTRACE_DOF_SECT_TYPE_STRTAB section */ > + uint32_t dofpv_probes; /* Link to a DTRACE_DOF_SECT_TYPE_PROBES section */ > + uint32_t dofpv_prargs; /* Link to a DTRACE_DOF_SECT_TYPE_PRARGS section */ > + uint32_t dofpv_proffs; /* Link to a DTRACE_DOF_SECT_TYPE_PROFFS section */ > + uint32_t dofpv_name; > + uint32_t dofpv_provattr; > + uint32_t dofpv_modattr; > + uint32_t dofpv_funcattr; > + uint32_t dofpv_nameattr; > + uint32_t dofpv_argsattr; > + uint32_t dofpv_prenoffs; /* Link to a DTRACE_DOF_SECT_PRENOFFS section */ > +}; > + > +struct dtrace_dof_probe > +{ > + uint64_t dofpr_addr; > + uint32_t dofpr_func; > + uint32_t dofpr_name; > + uint32_t dofpr_nargv; > + uint32_t dofpr_xargv; > + uint32_t dofpr_argidx; > + uint32_t dofpr_offidx; > + uint8_t dofpr_nargc; > + uint8_t dofpr_xargc; > + uint16_t dofpr_noffs; > + uint32_t dofpr_enoffidx; > + uint16_t dofpr_nenoffs; > + uint16_t dofpr_pad1; > + uint32_t dofpr_pad2; > +}; Each one of these structs need a comment on top describing it, and I would also appreciate a comment on each field. > + > +/* DOF supports two different encodings: MSB (big-endian) and LSB > + (little-endian). The encoding is itself encoded in the DOF header. > + The following function returns an unsigned value in the host > + endianness. */ > + > +#define DOF_UINT(dof, field) \ > + extract_unsigned_integer ((gdb_byte *) &(field), \ > + sizeof ((field)), \ > + (((dof)->dofh_ident[DTRACE_DOF_ID_ENCODING] == DTRACE_DOF_ENCODE_MSB) \ This line is too long. You can break it before the "==". > + ? BFD_ENDIAN_BIG : BFD_ENDIAN_LITTLE)) > + > +/* The following macro applies a given byte offset to a DOF (a pointer > + to a dtrace_dof_hdr structure) and returns the resulting > + address. */ > + > +#define DTRACE_DOF_PTR(dof, offset) (&((char *) (dof))[(offset)]) > + > +/* The following macro returns a pointer to the beginning of a given > + section in a DOF object. The section is referred to by its index > + in the sections array. */ > + > +#define DTRACE_DOF_SECT(dof, idx) \ > + ((struct dtrace_dof_sect *) \ > + DTRACE_DOF_PTR ((dof), \ > + DOF_UINT ((dof), (dof)->dofh_secoff) \ > + + ((idx) * DOF_UINT ((dof), (dof)->dofh_secsize)))) > + > +/* Helper functions to extract the probe information from the DOF > + object embedded in the .SUNW_dof section. */ Each function should have a comment. I understand this comment applies to a set of functions, but you also need comments for the other functions as well. I would also appreciate if the comment gave a brief explanation of how the function works in terms of the arguments it receives. > + > +static void > +dtrace_process_dof_probe (struct objfile *objfile, > + struct gdbarch *gdbarch, VEC (probe_p) **probesp, > + struct dtrace_dof_hdr *dof, struct dtrace_dof_probe *probe, Line too long. > + struct dtrace_dof_provider *provider, > + char *strtab, char *offtab, char *eofftab, > + char *argtab, uint64_t strtab_size) > +{ > + struct type *ptr_type = builtin_type (gdbarch)->builtin_data_ptr; You're not using this variable anywhere. > + int i, j, num_probes, num_enablers; > + VEC (dtrace_probe_enabler_s) *enablers; > + char *p; > + > + /* Each probe section can define zero or more probes of two > + different types: > + > + - probe->dofpr_noffs regular probes whose program counters are > + stored in 32bit words starting at probe->dofpr_addr + > + offtab[probe->dofpr_offidx]. > + > + - probe->dofpr_nenoffs is-enabled probes whose program counters > + are stored in 32bit words starting at probe->dofpr_addr + > + eofftab[probe->dofpr_enoffidx]. > + > + However is-enabled probes are not probes per-se, but an > + optimization hack that is implemented in the kernel in a very > + similar way than normal probes. This is how we support > + is-enabled probes on gdb: > + > + - Our probes are always DTrace regular probes. > + > + - Our probes can be associated with zero or more "enablers". The > + list of enablers is built from the is-enabled probes defined in > + the Probe section. > + > + - Probes having a non-empty list of enablers can be enabled or > + disabled using the `enable probe' and `disable probe' commands > + respectively. The `Enabled' column in the output of `info > + probes' will read `yes' if the enablers are activated, `no' > + otherwise. > + > + - Probes having an empty list of enablers are always enabled. > + The `Enabled' column in the output of `info probes' will > + read `always'. > + > + It follows that if there are DTrace is-enabled probes defined for > + some provider/name but no DTrace regular probes defined then the > + gdb user wont be able to enable/disable these conditionals. */ Thanks for the explanation! Always good to have descriptive comments in the code :-). While at it, could you please s/gdb/GDB/ in the comment? > + > + num_probes = DOF_UINT (dof, probe->dofpr_noffs); > + if (num_probes == 0) > + return; > + > + /* Build the list of enablers for the probes defined in this Probe > + DOF section. */ > + enablers = NULL; > + num_enablers = DOF_UINT (dof, probe->dofpr_nenoffs); > + for (i = 0; i < num_enablers; i++) > + { > + struct dtrace_probe_enabler enabler; > + uint32_t enabler_offset > + = ((uint32_t *) eofftab)[DOF_UINT (dof, probe->dofpr_enoffidx) + i]; > + > + enabler.address = DOF_UINT (dof, probe->dofpr_addr) + DOF_UINT (dof, enabler_offset); Line too long. > + VEC_safe_push (dtrace_probe_enabler_s, enablers, &enabler); > + } > + > + for (i = 0; i < num_probes; i++) > + { > + uint32_t probe_offset > + = ((uint32_t *) offtab)[DOF_UINT (dof, probe->dofpr_offidx) + i]; > + struct dtrace_probe *ret > + = obstack_alloc (&objfile->per_bfd->storage_obstack, sizeof (*ret)); > + > + ret->p.pops = &dtrace_probe_ops; > + ret->p.arch = gdbarch; > + ret->args_expr_built = 0; > + > + /* Set the provider and the name of the probe. */ > + ret->p.provider = xstrdup (strtab + DOF_UINT (dof, provider->dofpv_name)); Line too long. > + ret->p.name = xstrdup (strtab + DOF_UINT (dof, probe->dofpr_name)); I don't personally like this convention of aligning the function names; it gets in the way when I'm grepping for some attribution. But again, this is a matter of taste, so feel free to ignore the comment. > + > + /* The probe address. */ > + ret->p.address = DOF_UINT (dof, probe->dofpr_addr) + DOF_UINT (dof, probe_offset); Line too long. > + > + /* Number of arguments in the probe. */ > + ret->probe_argc = DOF_UINT (dof, probe->dofpr_nargc); > + > + /* Store argument type descriptions. A description of the type > + of the argument is in the (J+1)th null-terminated string > + starting at `strtab' + `probe->dofpr_nargv'. */ We're not using `' anymore; instead, we're using '' (GNU Coding Style has been updated). > + ret->args = NULL; > + p = strtab + DOF_UINT (dof, probe->dofpr_nargv); > + for (j = 0; j < ret->probe_argc; j++) > + { > + struct dtrace_probe_arg arg; > + struct expression *expr; > + > + arg.type_str = xstrdup (p); > + while (((p - strtab) < strtab_size) /* sentinel. */ > + && *p++); Again a matter of style, but for readability I prefer to write this loop as: /* Use strtab_size as a sentinel. */ while (*p != '\0' && p - strtab < strtab_size) ++p; > + > + /* Try to parse a type expression from the type string. If > + this does not work then we set the type to `long > + int'. */ > + arg.type = builtin_type (gdbarch)->builtin_long; This line has a different indentation than the others. > + expr = parse_expression (arg.type_str); > + if (expr->elts[0].opcode == OP_TYPE) > + arg.type = expr->elts[1].type; > + The line above contains extraneous whitespaces. > + VEC_safe_push (dtrace_probe_arg_s, ret->args, &arg); > + } > + > + /* Add the vector of enablers to this probe, if any. */ > + ret->enablers = VEC_copy (dtrace_probe_enabler_s, enablers); You should free the enablers VEC in the end of the function. You could probably make a cleanup and call it later. > + Extraneous whitespaces > + /* Successfully created probe. */ > + VEC_safe_push (probe_p, *probesp, (struct probe *) ret); > + } > +} > + > +static void > +dtrace_process_dof (asection *sect, struct objfile *objfile, > + VEC (probe_p) **probesp, struct dtrace_dof_hdr *dof) > +{ Just a reminder, this function is missing a comment. > + bfd *abfd = objfile->obfd; > + int size = bfd_get_arch_size (abfd) / 8; > + struct gdbarch *gdbarch = get_objfile_arch (objfile); > + struct dtrace_dof_sect *section; > + int i; > + > + /* The first step is to check for the DOF magic number. If no valid > + DOF data is found in the section then a complaint is issued to > + the user and the section skipped. */ > + if ((dof->dofh_ident[DTRACE_DOF_ID_MAG0] != 0x7F) > + || (dof->dofh_ident[DTRACE_DOF_ID_MAG1] != 'D') > + || (dof->dofh_ident[DTRACE_DOF_ID_MAG2] != 'O') > + || (dof->dofh_ident[DTRACE_DOF_ID_MAG3] != 'F')) Excessive use of parentheses. > + goto invalid_dof_data; > + > + /* Make sure this DOF is not an enabling DOF, i.e. there are no ECB > + Description sections. */ > + section = (struct dtrace_dof_sect *) DTRACE_DOF_PTR (dof, > + DOF_UINT (dof, dof->dofh_secoff)); > + for (i = 0; i < DOF_UINT (dof, dof->dofh_secnum); i++, section++) > + if (section->dofs_type == DTRACE_DOF_SECT_TYPE_ECBDESC) > + return; > + > + /* Iterate over any section of type Provider and extract the probe > + information from them. If there are no "provider" sections on > + the DOF then we just return. */ > + section = (struct dtrace_dof_sect *) DTRACE_DOF_PTR (dof, > + DOF_UINT (dof, dof->dofh_secoff)); > + for (i = 0; i < DOF_UINT (dof, dof->dofh_secnum); i++, section++) > + if (DOF_UINT (dof, section->dofs_type) == DTRACE_DOF_SECT_TYPE_PROVIDER) > + { > + struct dtrace_dof_provider *provider = (struct dtrace_dof_provider *) > + DTRACE_DOF_PTR (dof, DOF_UINT (dof, section->dofs_offset)); > + > + struct dtrace_dof_sect *strtab_s > + = DTRACE_DOF_SECT (dof, DOF_UINT (dof, provider->dofpv_strtab)); > + struct dtrace_dof_sect *probes_s > + = DTRACE_DOF_SECT (dof, DOF_UINT (dof, provider->dofpv_probes)); > + struct dtrace_dof_sect *args_s > + = DTRACE_DOF_SECT (dof, DOF_UINT (dof, provider->dofpv_prargs)); > + struct dtrace_dof_sect *offsets_s > + = DTRACE_DOF_SECT (dof, DOF_UINT (dof, provider->dofpv_proffs)); > + struct dtrace_dof_sect *eoffsets_s > + = DTRACE_DOF_SECT (dof, DOF_UINT (dof, provider->dofpv_prenoffs)); > + > + char *strtab = DTRACE_DOF_PTR (dof, DOF_UINT (dof, strtab_s->dofs_offset)); > + char *offtab = DTRACE_DOF_PTR (dof, DOF_UINT (dof, offsets_s->dofs_offset)); > + char *eofftab = DTRACE_DOF_PTR (dof, DOF_UINT (dof, eoffsets_s->dofs_offset)); > + char *argtab = DTRACE_DOF_PTR (dof, DOF_UINT (dof, args_s->dofs_offset)); > + > + unsigned int entsize = DOF_UINT (dof, probes_s->dofs_entsize); > + int num_probes; No newlines between variables being declared. > + /* Very, unlikely, but could crash gdb if not handled > + properly. */ > + if (entsize == 0) > + goto invalid_dof_data; > + > + num_probes = DOF_UINT (dof, probes_s->dofs_size) / entsize; > + > + for (i = 0; i < num_probes; i++) > + { > + struct dtrace_dof_probe *probe = (struct dtrace_dof_probe *) > + DTRACE_DOF_PTR (dof, DOF_UINT (dof, probes_s->dofs_offset) > + + (i * DOF_UINT (dof, probes_s->dofs_entsize))); > + dtrace_process_dof_probe (objfile, > + gdbarch, probesp, > + dof, probe, > + provider, strtab, offtab, eofftab, argtab, > + DOF_UINT (dof, strtab_s->dofs_size)); And a newline here, between the variable declaration and the function call :-). > + } > + } > + > + return; > + > + invalid_dof_data: > + complaint (&symfile_complaints, > + _("skipping section '%s' which does not contain valid DOF data."), > + sect->name); > + return; You don't need this return. > +} > + > +/* Helper functions to make sure the expressions in the arguments > + array are built as soon as their values are needed. */ > + > +static void > +dtrace_build_arg_exprs (struct dtrace_probe *probe, > + struct gdbarch *gdbarch) > +{ > + struct parser_state pstate; > + struct cleanup *back_to; > + struct dtrace_probe_arg *arg; > + int i; > + > + probe->args_expr_built = 1; > + Extraneous whitespaces. > + /* Iterate over the arguments in the probe and build the > + corresponding GDB internal expression that will generate the > + value of the argument when executed at the PC of the probe. */ > + for (i = 0; i < probe->probe_argc; i++) > + { > + arg = VEC_index (dtrace_probe_arg_s, probe->args, i); > + > + /* Initialize the expression buffer in the parser state. The > + language does not matter, since we are using our own > + parser. */ > + initialize_expout (&pstate, 10, current_language, gdbarch); > + back_to = make_cleanup (free_current_contents, &pstate.expout); Can you declare back_to inside the for block, please? > + > + /* The argument value, which is ABI dependent and casted to > + `long int'. */ > + gdbarch_dtrace_probe_argument (gdbarch, &pstate, i); > + > + discard_cleanups (back_to); > + > + /* Casting to the expected type, but only if the type was > + recognized at probe load time. Otherwise the argument will > + be evaluated as the long integer passed to the probe. */ > + if (arg->type) Please, check explicitly against NULL here. > + { > + write_exp_elt_opcode (&pstate, UNOP_CAST); > + write_exp_elt_type (&pstate, arg->type); > + write_exp_elt_opcode (&pstate, UNOP_CAST); > + } > + > + reallocate_expout (&pstate); > + arg->expr = pstate.expout; > + prefixify_expression (arg->expr); > + } > +} > + > +static struct dtrace_probe_arg * > +dtrace_get_arg (struct dtrace_probe *probe, unsigned n, > + struct gdbarch *gdbarch) > +{ > + if (!probe->args_expr_built) > + dtrace_build_arg_exprs (probe, gdbarch); > + > + return VEC_index (dtrace_probe_arg_s, probe->args, n); > +} > + > +/* Implementation of the get_probes method. */ > + > +static void > +dtrace_get_probes (VEC (probe_p) **probesp, struct objfile *objfile) > +{ > + bfd *abfd = objfile->obfd; > + asection *sect = NULL; > + > + /* Do nothing in case this is a .debug file, instead of the objfile > + itself. */ > + if (objfile->separate_debug_objfile_backlink != NULL) > + return; > + > + /* Iterate over the sections in OBJFILE looking for DTrace > + information. */ > + for (sect = abfd->sections; sect != NULL; sect = sect->next) > + { > + if (elf_section_data (sect)->this_hdr.sh_type == SHT_SUNW_dof) > + { > + struct dtrace_dof_hdr *dof; > + > + /* Read the contents of the DOF section and then process it to > + extract the information of any probe defined into it. */ > + if (!bfd_malloc_and_get_section (abfd, sect, (bfd_byte **) &dof)) > + { > + complaint (&symfile_complaints, > + _("could not obtain the contents of" > + "section '%s' in objfile `%s'."), > + sect->name, abfd->filename); > + return; Why return here? Is there only one section whose type is SHT_SUNW_dof? If no, then I guess the loop should keep rolling. Otherwise, then besides calling return here you should call return after the "xfree" below. Am I getting it right? > + } > + > + dtrace_process_dof (sect, objfile, probesp, dof); > + xfree (dof); > + } > + } What about using bfd_map_over_sections instead of this for loop? I know there is precedence of iterating over BFD sections by hand on GDB code, but bfd_map_over_sections exists for this very purpose. BTW, s/bfd_map_over_sections/bfd_sections_find_if/ if you should terminate the loop when the first section is found. > +} > + > +/* Helper function to determine whether a given probe is "enabled" or > + "disabled". A disabled probe is a probe in which one or more > + enablers are disabled. */ > + > +static int > +dtrace_probe_is_enabled (struct dtrace_probe *probe) > +{ > + int i; > + struct gdbarch *gdbarch = probe->p.arch; > + struct dtrace_probe_enabler *enabler; > + > + for (i = 0; VEC_iterate (dtrace_probe_enabler_s, probe->enablers, i, enabler); i++) Line too long. > + if (!gdbarch_dtrace_probe_is_enabled (gdbarch, enabler->address)) > + return 0; > + > + return 1; > +} > + > +/* Implementation of the get_probe_address method. */ > + > +static CORE_ADDR > +dtrace_get_probe_address (struct probe *probe, struct objfile *objfile) > +{ > + return probe->address + ANOFFSET (objfile->section_offsets, > + SECT_OFF_DATA (objfile)); > +} I know this code is a copy-and-paste of the SDT's code, but it would be nice to do the regular gdb_assert dance here. I will check in a patch for stap-probe.c soon to fix that. > + > +/* Implementation of the get_probe_argument_count method. */ > + > +static unsigned > +dtrace_get_probe_argument_count (struct probe *probe_generic, > + struct frame_info *frame) > +{ > + struct dtrace_probe *dtrace_probe = (struct dtrace_probe *) probe_generic; > + > + gdb_assert (probe_generic->pops == &dtrace_probe_ops); > + > + return dtrace_probe->probe_argc; > +} > + > +/* Implementation of the can_evaluate_probe_arguments method. */ > + > +static int > +dtrace_can_evaluate_probe_arguments (struct probe *probe_generic) > +{ > + struct gdbarch *gdbarch = probe_generic->arch; > + > + return gdbarch_dtrace_probe_argument_p (gdbarch); > +} Likewise (about the gdb_assert call). > + > +/* Implementation of the evaluate_probe_argument method. */ > + > +static struct value * > +dtrace_evaluate_probe_argument (struct probe *probe_generic, unsigned n, > + struct frame_info *frame) > +{ > + struct gdbarch *gdbarch = probe_generic->arch; > + struct dtrace_probe *dtrace_probe = (struct dtrace_probe *) probe_generic; > + struct dtrace_probe_arg *arg; > + int pos = 0; > + > + gdb_assert (probe_generic->pops == &dtrace_probe_ops); > + > + arg = dtrace_get_arg (dtrace_probe, n, gdbarch); > + return evaluate_subexp_standard (arg->type, arg->expr, &pos, EVAL_NORMAL); > +} > + > +/* Implementation of the compile_to_ax method. */ > + > +static void > +dtrace_compile_to_ax (struct probe *probe_generic, struct agent_expr *expr, > + struct axs_value *value, unsigned n) > +{ > + struct dtrace_probe *dtrace_probe = (struct dtrace_probe *) probe_generic; > + struct dtrace_probe_arg *arg; > + union exp_element *pc; > + > + gdb_assert (probe_generic->pops == &dtrace_probe_ops); > + > + arg = dtrace_get_arg (dtrace_probe, n, expr->gdbarch); > + > + pc = arg->expr->elts; > + gen_expr (arg->expr, &pc, expr, value); > + > + require_rvalue (expr, value); > + value->type = arg->type; > +} > + > +/* Implementation of the set_semaphore method. */ > + > +static void > +dtrace_set_semaphore (struct probe *probe_generic, struct objfile *objfile, > + struct gdbarch *gdbarch) > +{ > + gdb_assert (probe_generic->pops == &dtrace_probe_ops); > +} > + > +/* Implementation of the clear_semaphore method. */ > + > +static void > +dtrace_clear_semaphore (struct probe *probe_generic, struct objfile *objfile, > + struct gdbarch *gdbarch) > +{ > + gdb_assert (probe_generic->pops == &dtrace_probe_ops); > +} This shouldn't be needed, because USDT probes don't have the concept of a semaphore, right? I will submit a patch soon to fix the fact that the set/clear_semaphore functions are being called inconditionally. > + > +/* Implementation of the probe_destroy method. */ > + > +static void > +dtrace_probe_destroy (struct probe *probe_generic) > +{ > + struct dtrace_probe *probe = (struct dtrace_probe *) probe_generic; > + struct dtrace_probe_arg *arg; > + int i; > + > + gdb_assert (probe_generic->pops == &dtrace_probe_ops); > + > + for (i = 0; VEC_iterate (dtrace_probe_arg_s, probe->args, i, arg); i++) > + { > + xfree (arg->type_str); > + xfree (arg->expr); > + } > + > + VEC_free (dtrace_probe_enabler_s, probe->enablers); > + VEC_free (dtrace_probe_arg_s, probe->args); > +} > + > +/* Implementation of the gen_info_probes_table_header method. */ > + > +static void > +dtrace_gen_info_probes_table_header (VEC (info_probe_column_s) **heads) > +{ > + info_probe_column_s dtrace_probe_column; > + > + dtrace_probe_column.field_name = "enabled"; > + dtrace_probe_column.print_name = _("Enabled"); > + > + VEC_safe_push (info_probe_column_s, *heads, &dtrace_probe_column); > +} > + > +/* Implementation of the gen_info_probes_table_values method. */ > + > +static void > +dtrace_gen_info_probes_table_values (struct probe *probe_generic, > + VEC (const_char_ptr) **ret) > +{ > + struct dtrace_probe *probe = (struct dtrace_probe *) probe_generic; > + const char *val = NULL; > + > + gdb_assert (probe_generic->pops == &dtrace_probe_ops); > + > + if (VEC_empty (dtrace_probe_enabler_s, probe->enablers)) > + val = "always"; > + else if (!gdbarch_dtrace_probe_is_enabled_p (probe_generic->arch)) > + val = "unknown"; > + else if (dtrace_probe_is_enabled (probe)) > + val = "yes"; > + else > + val = "no"; > + > + VEC_safe_push (const_char_ptr, *ret, val); > +} > + > +/* Implementation of the enable_probe method. */ > + > +static void > +dtrace_enable_probe (struct probe *probe) > +{ > + struct gdbarch *gdbarch = probe->arch; > + struct dtrace_probe *dtrace_probe = (struct dtrace_probe *) probe; > + struct dtrace_probe_enabler *enabler; > + int i; > + > + gdb_assert (probe->pops == &dtrace_probe_ops); > + > + /* Enabling a dtrace probe implies patching the text section of the > + running process, so make sure the inferior is indeed running. */ > + if (ptid_equal (inferior_ptid, null_ptid)) > + error (_("No inferior running")); > + > + /* Fast path. */ > + if (dtrace_probe_is_enabled (dtrace_probe)) > + return; > + > + /* Iterate over all defined enabler in the given probe and enable > + them all using the corresponding gdbarch hook. */ > + > + for (i = 0; > + VEC_iterate (dtrace_probe_enabler_s, dtrace_probe->enablers, i, enabler); > + i++) > + { > + if (gdbarch_dtrace_enable_probe_p (gdbarch)) > + gdbarch_dtrace_enable_probe (gdbarch, enabler->address); > + } No need for the brackets here. > +} > + > + > +/* Implementation of the disable_probe method. */ > + > +static void > +dtrace_disable_probe (struct probe *probe) > +{ > + struct gdbarch *gdbarch = probe->arch; > + struct dtrace_probe *dtrace_probe = (struct dtrace_probe *) probe; > + struct dtrace_probe_enabler *enabler; > + int i; > + > + gdb_assert (probe->pops == &dtrace_probe_ops); > + > + /* Disabling a dtrace probe implies patching the text section of the > + running process, so make sure the inferior is indeed running. */ > + if (ptid_equal (inferior_ptid, null_ptid)) > + error (_("No inferior running")); > + > + /* Fast path. */ > + if (!dtrace_probe_is_enabled (dtrace_probe)) > + return; > + > + /* Are we trying to disable a probe that does not have any enabler > + associated? */ > + if (VEC_empty (dtrace_probe_enabler_s, dtrace_probe->enablers)) > + error (_("Probe %s:%s cannot be disabled."), probe->provider, probe->name); > + > + /* Iterate over all defined enabler in the given probe and disable > + them all using the corresponding gdbarch hook. */ > + > + for (i = 0; > + VEC_iterate (dtrace_probe_enabler_s, dtrace_probe->enablers, i, enabler); > + i++) > + { > + if (gdbarch_dtrace_disable_probe_p (gdbarch)) > + gdbarch_dtrace_disable_probe (gdbarch, enabler->address); > + } > +} > + > +/* DTrace probe_ops. */ > + > +static const struct probe_ops dtrace_probe_ops = > +{ > + dtrace_probe_is_linespec, > + dtrace_get_probes, > + dtrace_get_probe_address, > + dtrace_get_probe_argument_count, > + dtrace_can_evaluate_probe_arguments, > + dtrace_evaluate_probe_argument, > + dtrace_compile_to_ax, > + dtrace_set_semaphore, > + dtrace_clear_semaphore, > + dtrace_probe_destroy, > + dtrace_gen_info_probes_table_header, > + dtrace_gen_info_probes_table_values, > + dtrace_enable_probe, > + dtrace_disable_probe > +}; > + > +/* Implementation of the `info probes dtrace' command. */ > + > +static void > +info_probes_dtrace_command (char *arg, int from_tty) > +{ > + info_probes_for_ops (arg, from_tty, &dtrace_probe_ops); > +} > + > +void _initialize_dtrace_probe (void); > + > +void > +_initialize_dtrace_probe (void) > +{ > + VEC_safe_push (probe_ops_cp, all_probe_ops, &dtrace_probe_ops); > + Extraneous whitespaces. > + add_cmd ("dtrace", class_info, info_probes_dtrace_command, > + _("\ > +Show information about DTrace static probes.\n\ > +Usage: info probes dtrace [PROVIDER [NAME [OBJECT]]]\n\ > +Each argument is a regular expression, used to select probes.\n\ > +PROVIDER matches probe provider names.\n\ > +NAME matches the probe names.\n\ > +OBJECT matches the executable or shared library name."), > + info_probes_cmdlist_get ()); > +} > -- > 1.7.10.4 -- Sergio GPG key ID: 0x65FC5E36 Please send encrypted e-mail if possible http://sergiodj.net/