Re: [PATCH 5/9] New probe type: DTrace USDT probes.

Sergio Durigan Junior
2014-10-02T23:19:16Z

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 <jose.marchesi@oracle.com>
>
> * 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 <jose.marchesi@oracle.com>
>
> + * 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 <jose.marchesi@oracle.com>
> +
> * 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) <PLUGINS>.
> 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 <http://www.gnu.org/licenses/>. */
> +
> +#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