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

  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.

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
  

> From: "Jose E. Marchesi" <jose.marchesi@oracle.com>
> Date: Fri, 26 Sep 2014 11:48:21 +0200
> 
> +  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 ());

See my other mail with questions about the meaning of nesting of [..].
  

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
  

Hi Sergio.

    > +/* 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?

That macro is defined in elf.h in Solaris, Minix, and probably other
systems too.  I would not be surprised if it is eventually added to the
elf headers in GNU/Linux, and also in binutils.  I strongly recommend to
keep that sentinel in place to avoid potential problems with indirect
includes in the future.

    > +
    > +      /* 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).

A hard-to-die habit after so many years... :)
    
    > +      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;

What you are suggesting is not exactly equivalent: it leaves `p' at the
blank character, while the idea is to leave `p' at the character next ot
the blank character.  I changed the loop to:

/* Use strtab_size as a sentinel.  */
while (*p++ != '\0' && p - strtab < strtab_size);

Which makes the comparison explicit and thus may be more palatable for
you :)
    
    > +	  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.

Hmm, I don't see the need of doing a deep copy of the vector, nor I
remember why I felt it was necessary to do it when I wrote the original
code.

I changed that to:

/* Add the vector of enablers to this probe, if any.  */
ret->enablers = enablers;

But maybe(probably) I am missing something? :?

    > +/* 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?

Yeah, in principle there can be more than one sections of type
SHT_SUNW_dof.  I changed the code as suggested.
    
    > +	    }
    > +	  
    > +	  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.

I considered that, but the need to define a new structure type for
passing `objfile' and `probesp' to the handler (not to mention the
handler itself) makes it a bit overkill to use bfd_map_over_sections in
this specific case IMO...  especially considering that
dtrace_process_dof is only called by this function.

    > +/* 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.

Correct, that should not be needed and can go away as soon as you do
that change.
  

On Friday, October 10 2014, Jose E. Marchesi wrote:

>     > +/* 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?
>
> That macro is defined in elf.h in Solaris, Minix, and probably other
> systems too.  I would not be surprised if it is eventually added to the
> elf headers in GNU/Linux, and also in binutils.  I strongly recommend to
> keep that sentinel in place to avoid potential problems with indirect
> includes in the future.

Sure thing :-).  I was mostly curious.

>     > +
>     > +      /* 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).
>
> A hard-to-die habit after so many years... :)

Haha, yeah, I can imagine :-).

>     > +      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;
>
> What you are suggesting is not exactly equivalent: it leaves `p' at the
> blank character, while the idea is to leave `p' at the character next ot
> the blank character.  I changed the loop to:
>
> /* Use strtab_size as a sentinel.  */
> while (*p++ != '\0' && p - strtab < strtab_size);
>
> Which makes the comparison explicit and thus may be more palatable for
> you :)

Ops, indeed, thanks for catching this :-).  No wonder I proposed to make
the loop clearer :-P.

>     > +	  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.
>
> Hmm, I don't see the need of doing a deep copy of the vector, nor I
> remember why I felt it was necessary to do it when I wrote the original
> code.
>
> I changed that to:
>
> /* Add the vector of enablers to this probe, if any.  */
> ret->enablers = enablers;
>
> But maybe(probably) I am missing something? :?

Hm, right.  But if you do that, you will have to adjust
dtrace_probe_destroy, because it will be freeing the same 'enablers'
over and over...

>     > +	    }
>     > +	  
>     > +	  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.
>
> I considered that, but the need to define a new structure type for
> passing `objfile' and `probesp' to the handler (not to mention the
> handler itself) makes it a bit overkill to use bfd_map_over_sections in
> this specific case IMO...  especially considering that
> dtrace_process_dof is only called by this function.

OK, fair enough.

>     > +/* 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.
>
> Correct, that should not be needed and can go away as soon as you do
> that change.

I should be able to post something today.  Will put you on the loop.

Cheers,
  

>     You should free the enablers VEC in the end of the function.  You could
    >     probably make a cleanup and call it later.
    >
    > Hmm, I don't see the need of doing a deep copy of the vector, nor I
    > remember why I felt it was necessary to do it when I wrote the original
    > code.
    >
    > I changed that to:
    >
    > /* Add the vector of enablers to this probe, if any.  */
    > ret->enablers = enablers;
    >
    > But maybe(probably) I am missing something? :?
    
    Hm, right.  But if you do that, you will have to adjust
    dtrace_probe_destroy, because it will be freeing the same 'enablers'
    over and over...

Aaah, that was indeed the reason!  A "DOF probe" translates into 0 or
more gdb probes, and they all share the same vector of enablers.

Maintaining a copy of the enablers per gdb probe makes it trivial to
manage its memory.  Otherwise we would need to keep track of which
enabler vectors are shared by which gdb probes... argh!

I will roll back to the deep copy approach :)

    >     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.
    >
    > Correct, that should not be needed and can go away as soon as you do
    > that change.
    
    I should be able to post something today.  Will put you on the loop.
    
Ah nice, I will update the patches accordingly.
  

On Friday, October 10 2014, Jose E. Marchesi wrote:

>     >     You should free the enablers VEC in the end of the function.  You could
>     >     probably make a cleanup and call it later.
>     >
>     > Hmm, I don't see the need of doing a deep copy of the vector, nor I
>     > remember why I felt it was necessary to do it when I wrote the original
>     > code.
>     >
>     > I changed that to:
>     >
>     > /* Add the vector of enablers to this probe, if any.  */
>     > ret->enablers = enablers;
>     >
>     > But maybe(probably) I am missing something? :?
>     
>     Hm, right.  But if you do that, you will have to adjust
>     dtrace_probe_destroy, because it will be freeing the same 'enablers'
>     over and over...
>
> Aaah, that was indeed the reason!  A "DOF probe" translates into 0 or
> more gdb probes, and they all share the same vector of enablers.
>
> Maintaining a copy of the enablers per gdb probe makes it trivial to
> manage its memory.  Otherwise we would need to keep track of which
> enabler vectors are shared by which gdb probes... argh!
>
> I will roll back to the deep copy approach :)

:-)

And then you need that cleanup I mentioned before :-P.