Re: [PATCH 3/3] Convert DTrace probe interface to C++ (and perform some cleanups)

[Simon Marchi <simark at simark dot ca>]

Hi Sergio,

On 2017-11-13 12:59 PM, Sergio Durigan Junior wrote:
> This patch converts the DTrace probe
> interface (gdb/dtrace-probe.[ch]) to C++, and also performs some
> cleanups that were on my TODO list for a while.
> 
> The main changes were the conversion of 'struct dtrace_probe' to 'class
> dtrace_probe', and a new 'class dtrace_static_probe_ops' to replace the
> use of 'dtrace_probe_ops'.  Both classes implement the virtual methods
> exported by their parents, 'class probe' and 'class static_probe_ops',
> respectively.  I believe it's now a bit simpler to understand the
> logic behind the dtrace-probe interface.
> 
> There are several helper functions used to parse parts of a dtrace
> probe, and since they are generic and don't need to know about the
> probe they're working on, I decided to leave them as simple static
> functions (instead of e.g. converting them to class methods).
> 
> I've also converted a few uses of "VEC" to "std::vector", which makes
> the code simpler and easier to maintain.  And, as usual, some cleanups
> here and there.
> 
> Even though I'm sending a series of patches, they need to be tested
> and committed as a single unit, because of inter-dependencies.  But it
> should be easier to review in separate logical units.
> 
> I've regtested this patch on BuildBot, no regressions found.
> 
> gdb/ChangeLog:
> 2017-11-13  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	* dtrace-probe.c (struct probe_ops dtrace_probe_ops): Delete.
> 	(dtrace_probe_arg_s): Delete type and VEC.
> 	(dtrace_probe_enabler_s): Likewise.
> 	(struct dtrace_probe): Replace by...
> 	(class dtrace_static_probe_ops): ...this and...
> 	(class dtrace_probe): ...this.
> 	(dtrace_probe_is_linespec): Rename to...
> 	(dtrace_static_probe_ops::is_linespec): ...this.  Adjust code
> 	to reflect change.
> 	(dtrace_process_dof_probe): Use 'std::vector' instead of VEC.
> 	Adjust code.  Create new instance of 'dtrace_probe'.
> 	(dtrace_build_arg_exprs): Rename to...
> 	(dtrace_probe::build_arg_exprs): ...this.  Adjust code to
> 	reflect change.
> 	(dtrace_get_probes): Rename to...
> 	(dtrace_static_probe_ops::get_probes): ...this.  Adjust code
> 	to reflect change.
> 	(dtrace_get_arg): Rename to...
> 	(dtrace_probe::get_arg_by_number): ...this.  Adjust code to
> 	reflect change.
> 	(dtrace_probe_is_enabled): Rename to...
> 	(dtrace_probe::is_enabled): ...this.  Adjust code to reflect
> 	change.
> 	(dtrace_get_probe_address): Rename to...
> 	(dtrace_probe::get_relocated_address): ...this.  Adjust code
> 	to reflect change.
> 	(dtrace_get_probe_argument_count): Rename to...
> 	(dtrace_probe::get_argument_count): ...this.  Adjust code to
> 	reflect change.
> 	(dtrace_can_evaluate_probe_arguments): Rename to...
> 	(dtrace_probe::can_evaluate_arguments): ...this.  Adjust code
> 	to reflect change.
> 	(dtrace_evaluate_probe_argument): Rename to...
> 	(dtrace_probe::evaluate_argument): ...this.  Adjust code to
> 	reflect change.
> 	(dtrace_compile_to_ax): Rename to...
> 	(dtrace_probe::compile_to_ax): ...this.  Adjust code to
> 	reflect change.
> 	(dtrace_probe_destroy): Delete.
> 	(dtrace_type_name): Rename to...
> 	(dtrace_static_probe_ops::type_name): ...this.  Adjust code to
> 	reflect change.
> 	(dtrace_probe::get_static_ops): New method.
> 	(dtrace_gen_info_probes_table_header): Rename to...
> 	(dtrace_static_probe_ops::gen_info_probes_table_header):
> 	...this.  Adjust code to reflect change.
> 	(dtrace_gen_info_probes_table_values): Rename to...
> 	(dtrace_probe::gen_info_probes_table_values): ...this.  Adjust
> 	code to reflect change.
> 	(dtrace_enable_probe): Rename to...
> 	(dtrace_probe::enable_probe): ...this.  Adjust code to reflect
> 	change.
> 	(dtrace_disable_probe): Rename to...
> 	(dtrace_probe::disable_probe): ...this.  Adjust code to reflect
> 	change.
> 	(struct probe_ops dtrace_probe_ops): Delete.
> 	(dtrace_static_probe_ops::emit_info_probes_extra_fields): New
> 	method.
> 	(info_probes_dtrace_command): Call 'info_probes_for_spops'
> 	instead of 'info_probes_for_ops'.
> 	(_initialize_dtrace_probe): Use 'all_static_probe_ops' instead
> 	of 'all_probe_ops'.
> ---
>  gdb/dtrace-probe.c | 528 +++++++++++++++++++++++++++--------------------------
>  1 file changed, 268 insertions(+), 260 deletions(-)
> 
> diff --git a/gdb/dtrace-probe.c b/gdb/dtrace-probe.c
> index 2bbe03e4b3..b53ff7ac6c 100644
> --- a/gdb/dtrace-probe.c
> +++ b/gdb/dtrace-probe.c
> @@ -41,10 +41,6 @@
>  # define SHT_SUNW_dof	0x6ffffff4
>  #endif
>  
> -/* Forward declaration.  */
> -
> -extern const struct probe_ops dtrace_probe_ops;
> -
>  /* The following structure represents a single argument for the
>     probe.  */
>  
> @@ -60,9 +56,6 @@ struct dtrace_probe_arg
>    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
> @@ -73,39 +66,121 @@ struct dtrace_probe_enabler
>    CORE_ADDR address;
>  };
>  
> -typedef struct dtrace_probe_enabler dtrace_probe_enabler_s;
> -DEF_VEC_O (dtrace_probe_enabler_s);
> +/* Class that implements the static probe methods for "stap" probes.  */
> +
> +class dtrace_static_probe_ops : public static_probe_ops
> +{
> +public:
> +  /* See probe.h.  */
> +  bool is_linespec (const char **linespecp) const override;
> +
> +  /* See probe.h.  */
> +  void get_probes (std::vector<probe *> *probesp,
> +		   struct objfile *objfile) const override;
> +
> +  /* See probe.h.  */
> +  const char *type_name () const override;
> +
> +  /* See probe.h.  */
> +  bool emit_info_probes_extra_fields () const override;
> +
> +  /* See probe.h.  */
> +  void gen_info_probes_table_header
> +    (std::vector<struct info_probe_column> *heads) const override;
> +};
> +
> +/* DTrace static_probe_ops.  */
> +
> +const dtrace_static_probe_ops dtrace_static_probe_ops;
>  
>  /* The following structure represents a dtrace probe.  */
>  
> -struct dtrace_probe
> +class dtrace_probe : public 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;
> +public:
> +  /* Constructor for dtrace_probe.  */
> +  dtrace_probe (std::string &&name_, std::string &&provider_, CORE_ADDR address_,
> +		struct gdbarch *arch_,
> +		std::vector<struct dtrace_probe_arg> &args_,
> +		std::vector<struct dtrace_probe_enabler> &enablers_)

I am wondering why you don't use && for the vectors, as you do with the strings.
If the constructor is "stealing" from the vectors passed as parameter, I think it
would make sense if the parameters were &&, to indicate to the caller that they'll
be moved from (and force it to use std::move).

> +    : probe (std::move (name_), std::move (provider_), address_, arch_),
> +      m_args (std::move (args_)),
> +      m_enablers (std::move (enablers_)),
> +      m_args_expr_built (false)
> +  {}
> +
> +  /* Destructor for dtrace_probe.  */
> +  ~dtrace_probe ()
> +  {
> +    for (struct dtrace_probe_arg arg : m_args)
> +      {
> +	xfree (arg.type_str);
> +	xfree (arg.expr);
> +      }

As for stap_probe, I think defining a destructor in dtrace_probe_arg would
be clearer.

> +  }
> +
> +  /* See probe.h.  */
> +  CORE_ADDR get_relocated_address (struct objfile *objfile) override;
> +
> +  /* See probe.h.  */
> +  unsigned get_argument_count (struct frame_info *frame) override;
> +
> +  /* See probe.h.  */
> +  int can_evaluate_arguments () const override;
> +
> +  /* See probe.h.  */
> +  struct value *evaluate_argument (unsigned n,
> +				   struct frame_info *frame) override;
> +
> +  /* See probe.h.  */
> +  void compile_to_ax (struct agent_expr *aexpr,
> +		      struct axs_value *axs_value,
> +		      unsigned n) override;
> +
> +  /* See probe.h.  */
> +  const static_probe_ops *get_static_ops () const override;
> +
> +  /* See probe.h.  */
> +  void gen_info_probes_table_values
> +    (std::vector<const char *> *values) const override;
> +
> +  /* See probe.h.  */
> +  bool can_enable () const override
> +  {
> +    return true;
> +  }
> +
> +  /* See probe.h.  */
> +  void enable () override;
>  
> +  /* See probe.h.  */
> +  void disable () override;
> +
> +  /* Return the Nth argument of the probe.  */
> +  struct dtrace_probe_arg *get_arg_by_number (unsigned n,
> +					      struct gdbarch *gdbarch);
> +
> +  /* Build the GDB internal expressiosn that, once evaluated, will
> +     calculate the values of the arguments of the probe.  */
> +  void build_arg_exprs (struct gdbarch *gdbarch);
> +
> +  /* Determine whether the probe is "enabled" or "disabled".  A
> +     disabled probe is a probe in which one or more enablers are
> +     disabled.  */
> +  bool is_enabled () const;
> +
> +private:
>    /* A probe can have zero or more arguments.  */
> -  int probe_argc;
> -  VEC (dtrace_probe_arg_s) *args;
> +  int m_argc;

It seems like this field is never set, but used at least once.  Should we use args->size() instead?

> +  std::vector<struct dtrace_probe_arg> m_args;
>  
>    /* A probe can have zero or more "enablers" associated with it.  */
> -  VEC (dtrace_probe_enabler_s) *enablers;
> +  std::vector<struct dtrace_probe_enabler> m_enablers;
>  
>    /* Whether the expressions for the arguments have been built.  */
> -  unsigned int args_expr_built : 1;
> +  bool m_args_expr_built;
>  };
>  
> -/* 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
> @@ -322,8 +397,6 @@ dtrace_process_dof_probe (struct objfile *objfile,
>  			  char *argtab, uint64_t strtab_size)
>  {
>    int i, j, num_probes, num_enablers;
> -  struct cleanup *cleanup;
> -  VEC (dtrace_probe_enabler_s) *enablers;
>    char *p;
>  
>    /* Each probe section can define zero or more probes of two
> @@ -368,9 +441,7 @@ dtrace_process_dof_probe (struct objfile *objfile,
>  
>    /* Build the list of enablers for the probes defined in this Probe
>       DOF section.  */
> -  enablers = NULL;
> -  cleanup
> -    = make_cleanup (VEC_cleanup (dtrace_probe_enabler_s), &enablers);
> +  std::vector<struct dtrace_probe_enabler> enablers;
>    num_enablers = DOF_UINT (dof, probe->dofpr_nenoffs);
>    for (i = 0; i < num_enablers; i++)
>      {
> @@ -380,38 +451,32 @@ dtrace_process_dof_probe (struct objfile *objfile,
>  
>        enabler.address = DOF_UINT (dof, probe->dofpr_addr)
>  	+ DOF_UINT (dof, enabler_offset);
> -      VEC_safe_push (dtrace_probe_enabler_s, enablers, &enabler);
> +      enablers.push_back (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 =
> -	XOBNEW (&objfile->per_bfd->storage_obstack, struct dtrace_probe);
> -
> -      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));
> -      ret->p.name = xstrdup (strtab + DOF_UINT (dof, probe->dofpr_name));
> +      const char *probe_provider
> +	= strtab + DOF_UINT (dof, provider->dofpv_name);
> +      const char *name = strtab + DOF_UINT (dof, probe->dofpr_name);
>  
>        /* The probe address.  */
> -      ret->p.address
> +      CORE_ADDR address
>  	= DOF_UINT (dof, probe->dofpr_addr) + DOF_UINT (dof, probe_offset);
>  
>        /* Number of arguments in the probe.  */
> -      ret->probe_argc = DOF_UINT (dof, probe->dofpr_nargc);
> +      int 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'.  */
> -      ret->args = NULL;
> +      std::vector<struct dtrace_probe_arg> args;
>        p = strtab + DOF_UINT (dof, probe->dofpr_nargv);
> -      for (j = 0; j < ret->probe_argc; j++)
> +      for (j = 0; j < probe_argc; j++)
>  	{
>  	  struct dtrace_probe_arg arg;
>  	  expression_up expr;
> @@ -442,17 +507,18 @@ dtrace_process_dof_probe (struct objfile *objfile,
>  	  if (expr != NULL && expr->elts[0].opcode == OP_TYPE)
>  	    arg.type = expr->elts[1].type;
>  
> -	  VEC_safe_push (dtrace_probe_arg_s, ret->args, &arg);
> +	  args.push_back (arg);
>  	}
>  
> -      /* Add the vector of enablers to this probe, if any.  */
> -      ret->enablers = VEC_copy (dtrace_probe_enabler_s, enablers);
> +      std::vector<struct dtrace_probe_enabler> enablers_copy = enablers;

Why is this copy necessary?  What happens with the original vector?

> +      dtrace_probe *ret = new dtrace_probe (std::string (name),
> +					    std::string (probe_provider),
> +					    address, gdbarch,
> +					    args, enablers_copy);
>  
>        /* Successfully created probe.  */
> -      probesp->push_back ((struct probe *) ret);
> +      probesp->push_back (ret);
>      }
> -
> -  do_cleanups (cleanup);
>  }
>  
>  /* Helper function to collect the probes described in the DOF program
> @@ -555,28 +621,23 @@ dtrace_process_dof (asection *sect, struct objfile *objfile,
>  	     sect->name);
>  }
>  
> -/* Helper function to build the GDB internal expressiosn that, once
> -   evaluated, will calculate the values of the arguments of a given
> -   PROBE.  */
> +/* Implementation of 'build_arg_exprs' method.  */
>  
> -static void
> -dtrace_build_arg_exprs (struct dtrace_probe *probe,
> -			struct gdbarch *gdbarch)
> +void
> +dtrace_probe::build_arg_exprs (struct gdbarch *gdbarch)
>  {
> -  struct parser_state pstate;
> -  struct dtrace_probe_arg *arg;
> -  int i;
> -
> -  probe->args_expr_built = 1;
> +  m_args_expr_built = true;
>  
>    /* 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++)
> +  for (int i = 0; i < m_argc; i++)
>      {
> +      struct dtrace_probe_arg *arg;
>        struct cleanup *back_to;
> +      struct parser_state pstate;
>  
> -      arg = VEC_index (dtrace_probe_arg_s, probe->args, i);
> +      arg = &m_args[i];
>  
>        /* Initialize the expression buffer in the parser state.  The
>  	 language does not matter, since we are using our own
> @@ -606,138 +667,82 @@ dtrace_build_arg_exprs (struct dtrace_probe *probe,
>      }
>  }
>  
> -/* Helper function to return the Nth argument of a given PROBE.  */
> -
> -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.  */
> +/* Implementation of 'get_arg_by_number' method.  */
>  
> -static void
> -dtrace_get_probes (std::vector<probe *> *probesp, struct objfile *objfile)
> +struct dtrace_probe_arg *
> +dtrace_probe::get_arg_by_number (unsigned n, struct gdbarch *gdbarch)
>  {
> -  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;
> +  if (!m_args_expr_built)
> +    this->build_arg_exprs (gdbarch);
>  
> -  /* 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)
> -	{
> -	  bfd_byte *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, &dof))
> -	    complaint (&symfile_complaints,
> -		       _("could not obtain the contents of"
> -			 "section '%s' in objfile `%s'."),
> -		       sect->name, abfd->filename);
> -      
> -	  dtrace_process_dof (sect, objfile, probesp,
> -			      (struct dtrace_dof_hdr *) dof);
> -	  xfree (dof);
> -	}
> -    }
> +  return &m_args[n];

It would be nice to add a gdb_assert (n < m_args.size ()) here.

Thanks,

Simon