This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 2/3] Convert SystemTap probe interface to C++ (and perform some cleanups)
- From: Simon Marchi <simark at simark dot ca>
- To: Sergio Durigan Junior <sergiodj at redhat dot com>, GDB Patches <gdb-patches at sourceware dot org>
- Date: Tue, 14 Nov 2017 22:58:35 -0500
- Subject: Re: [PATCH 2/3] Convert SystemTap probe interface to C++ (and perform some cleanups)
- Authentication-results: sourceware.org; auth=none
- References: <20171113175901.25367-1-sergiodj@redhat.com> <20171113175901.25367-3-sergiodj@redhat.com>
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.
> 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.
> +
> + /* 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...
> @@ -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 ?
> +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.
Thanks,
Simon