This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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
http://sergiodj.net/


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]