[PATCH 2/3] Convert SystemTap probe interface to C++ (and perform some cleanups)

Sergio Durigan Junior sergiodj@redhat.com
Wed Nov 15 22:49:00 GMT 2017


On Tuesday, November 14 2017, Simon Marchi wrote:

> On 2017-11-13 12:59 PM, Sergio Durigan Junior wrote:
>> This patch converts the SystemTap probe
>> interface (gdb/stap-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 stap_probe' to 'class
>> stap_probe', and a new 'class stap_static_probe_ops' to replace the
>> use of 'stap_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 stap-probe interface.
>> 
>> There are several helper functions used to parse parts of a stap
>> 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.
>
> Hi Sergio,
>
> This look very good to me.  In general, try to use const-references when
> iterating on vector of objects (in all 3 patches).  A few comments below.

Hey Simon,

Thanks for the review.  I also saw your comment on IRC; good catch,
thanks.  I'll update the code.

>> I've regtested this patch on BuildBot, no regressions found.
>> 
>> gdb/ChangeLog:
>> 2017-11-13  Sergio Durigan Junior  <sergiodj@redhat.com>
>> 
>> 	* stap-probe.c (struct probe_ops stap_probe_ops): Delete
>> 	variable.
>> 	(stap_probe_arg_s): Delete type and VEC.
>> 	(struct stap_probe): Delete.  Replace by...
>> 	(class stap_static_probe_ops): ...this and...
>> 	(class stap_probe): ...this.  Rename variables to add 'm_'
>> 	prefix.  Do not use 'union' for arguments anymore.
>> 	(stap_get_expected_argument_type): Receive probe name instead
>> 	of 'struct stap_probe'.  Adjust code.
>> 	(stap_parse_probe_arguments): Rename to...
>> 	(stap_probe::parse_arguments): ...this.  Adjust code to
>> 	reflect change.
>> 	(stap_get_probe_address): Rename to...
>> 	(stap_probe::get_relocated_address): ...this.  Adjust code
>> 	to reflect change.
>> 	(stap_get_probe_argument_count): Rename to...
>> 	(stap_probe::get_argument_count): ...this.  Adjust code
>> 	to reflect change.
>> 	(stap_get_arg): Rename to...
>> 	(stap_probe::get_arg_by_number'): ...this. Adjust code to
>> 	reflect change.
>> 	(can_evaluate_probe_arguments): Rename to...
>> 	(stap_probe::can_evaluate_arguments): ...this.  Adjust code
>> 	to reflect change.
>> 	(stap_evaluate_probe_argument): Rename to...
>> 	(stap_probe::evaluate_argument): ...this.  Adjust code
>> 	to reflect change.
>> 	(stap_compile_to_ax): Rename to...
>> 	(stap_probe::compile_to_ax): ...this.  Adjust code to
>> 	reflect change.
>> 	(stap_probe_destroy): Delete.
>> 	(stap_modify_semaphore): Adjust comment.
>> 	(stap_set_semaphore): Rename to...
>> 	(stap_probe::set_semaphore): ...this.  Adjust code to reflect
>> 	change.
>> 	(stap_clear_semaphore): Rename to...
>> 	(stap_probe::clear_semaphore): ...this.  Adjust code to
>> 	reflect	change.
>> 	(stap_probe::get_static_ops): New method.
>> 	(handle_stap_probe): Adjust code to create instance of
>> 	'stap_probe'.
>> 	(stap_get_probes): Rename to...
>> 	(stap_static_probe_ops::get_probes): ...this.  Adjust code to
>> 	reflect change.
>> 	(stap_probe_is_linespec): Rename to...
>> 	(stap_static_probe_ops::is_linespec): ...this.  Adjust code to
>> 	reflect change.
>> 	(stap_type_name): Rename to...
>> 	(stap_static_probe_ops::type_name): ...this.  Adjust code to
>> 	reflect change.
>> 	(stap_static_probe_ops::emit_info_probes_extra_fields): New
>> 	method.
>> 	(stap_gen_info_probes_table_header): Rename to...
>> 	(stap_static_probe_ops::gen_info_probes_table_header):
>> 	...this.  Adjust code to reflect change.
>> 	(stap_gen_info_probes_table_values): Rename to...
>> 	(stap_probe::gen_info_probes_table_values): ...this.  Adjust
>> 	code to reflect change.
>> 	(struct probe_ops stap_probe_ops): Delete.
>> 	(info_probes_stap_command): Use 'info_probes_for_spops'
>> 	instead of 'info_probes_for_ops'.
>> 	(_initialize_stap_probe): Use 'all_static_probe_ops' instead
>> 	of 'all_probe_ops'.
>> ---
>>  gdb/stap-probe.c | 478 ++++++++++++++++++++++++++++---------------------------
>>  1 file changed, 244 insertions(+), 234 deletions(-)
>> 
>> diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
>> index 6fa0d20280..c169ffc488 100644
>> --- a/gdb/stap-probe.c
>> +++ b/gdb/stap-probe.c
>> @@ -45,10 +45,6 @@
>>  
>>  #define STAP_BASE_SECTION_NAME ".stapsdt.base"
>>  
>> -/* Forward declaration. */
>> -
>> -extern const struct probe_ops stap_probe_ops;
>> -
>>  /* Should we display debug information for the probe's argument expression
>>     parsing?  */
>>  
>> @@ -95,32 +91,135 @@ struct stap_probe_arg
>>    struct expression *aexpr;
>>  };
>>  
>> -typedef struct stap_probe_arg stap_probe_arg_s;
>> -DEF_VEC_O (stap_probe_arg_s);
>> +/* Class that implements the static probe methods for "stap" probes.  */
>>  
>> -struct stap_probe
>> +class stap_static_probe_ops : public static_probe_ops
>>  {
>> -  /* Generic information about the probe.  This shall 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:
>> +  /* 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;
>> +};
>> +
>> +/* SystemTap static_probe_ops.  */
>> +
>> +const stap_static_probe_ops stap_static_probe_ops;
>>  
>> +class stap_probe : public probe
>> +{
>> +public:
>> +  /* Constructor for stap_probe.  */
>> +  stap_probe (std::string &&name_, std::string &&provider_, CORE_ADDR address_,
>> +	      struct gdbarch *arch_, CORE_ADDR sem_addr, const char *args_text)
>> +    : probe (std::move (name_), std::move (provider_), address_, arch_),
>> +      m_sem_addr (sem_addr),
>> +      m_have_parsed_args (false), m_unparsed_args_text (args_text)
>> +  {}
>> +
>> +  /* Destructor for stap_probe.  */
>> +  ~stap_probe ()
>> +  {
>> +    if (m_have_parsed_args)
>> +      for (struct stap_probe_arg arg : m_parsed_args)
>> +	xfree (arg.aexpr);
>> +  }
>
> I would suggest adding a destructor to stap_probe_arg instead, since it's
> the object that "owns" the expression.  You would need to disable copy
> construction and assignment (using DISABLE_COPY_AND_ASSIGN) to avoid double
> free.  You can then add a simple constructor and use emplace_back when
> inserting in the vector.

Good point.  As we discussed on IRC, I implemented things the way you
proposed, with expression_up.  I'll send the updated patch.

>> +
>> +  /* 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.  */
>> +  void set_semaphore (struct objfile *objfile,
>> +		      struct gdbarch *gdbarch) override;
>> +
>> +  /* See probe.h.  */
>> +  void clear_semaphore (struct objfile *objfile,
>> +			struct gdbarch *gdbarch) 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;
>> +
>> +  /* Return argument N of probe.
>> +
>> +     If the probe's arguments have not been parsed yet, parse them.  If
>> +     there are no arguments, throw an exception (error).  Otherwise,
>> +     return the requested argument.  */
>> +  struct stap_probe_arg *get_arg_by_number (unsigned n,
>> +					    struct gdbarch *gdbarch)
>> +  {
>> +    if (!m_have_parsed_args)
>> +      this->parse_arguments (gdbarch);
>> +
>> +    gdb_assert (m_have_parsed_args);
>> +    if (m_parsed_args.empty ())
>> +      internal_error (__FILE__, __LINE__,
>> +		      _("Probe '%s' apparently does not have arguments, but \n"
>> +			"GDB is requesting its argument number %u anyway.  "
>> +			"This should not happen.  Please report this bug."),
>> +		      this->get_name (), n);
>> +
>> +    return &m_parsed_args[n];
>
> There wasn't one before, but it sounds to me like there should be a bound check here...

Will do.

>> @@ -1081,26 +1179,16 @@ stap_parse_argument (const char **arg, struct type *atype,
>>    return p.pstate.expout;
>>  }
>>  
>> -/* Function which parses an argument string from PROBE, correctly splitting
>> -   the arguments and storing their information in properly ways.
>> +/* Implementation of 'parse_probe_arguments' method.  */
>
> of 'parse_arguments' method ?

Fixed.

>> +void
>> +stap_probe::set_semaphore (struct objfile *objfile, struct gdbarch *gdbarch)
>>  {
>> -  struct stap_probe *probe = (struct stap_probe *) probe_generic;
>>    CORE_ADDR addr;
>>  
>> -  gdb_assert (probe_generic->pops == &stap_probe_ops);
>> -
>> -  addr = (probe->sem_addr
>> +  addr = (m_sem_addr
>>  	  + ANOFFSET (objfile->section_offsets, SECT_OFF_DATA (objfile)));
>
> While at it, you could replace this with get_relocated_address() so that this
> computation is not duplicated.  Same with clear_semaphore.

Sure thing, done.

Thanks for the review.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/



More information about the Gdb-patches mailing list