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
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.
@@ -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);
+ }
+
+ /* 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];
+ }
+
+ /* Function which parses an argument string from the probe,
+ correctly splitting the arguments and storing their information
+ in properly ways.
+
+ Consider the following argument string (x86 syntax):
+
+ `4@%eax 4@$10'
+
+ We have two arguments, `%eax' and `$10', both with 32-bit
+ unsigned bitness. This function basically handles them, properly
+ filling some structures with this information. */
+ void parse_arguments (struct gdbarch *gdbarch);
+
+private:
/* If the probe has a semaphore associated, then this is the value of
it, relative to SECT_OFF_DATA. */
- CORE_ADDR sem_addr;
+ CORE_ADDR m_sem_addr;
- /* One if the arguments have been parsed. */
- unsigned int args_parsed : 1;
+ /* True if the arguments have been parsed. */
+ bool m_have_parsed_args;
- union
- {
- const char *text;
+ /* The text version of the probe's arguments, unparsed. */
+ const char *m_unparsed_args_text;
- /* Information about each argument. This is an array of `stap_probe_arg',
- with each entry representing one argument. */
- VEC (stap_probe_arg_s) *vec;
- }
- args_u;
+ /* Information about each argument. This is an array of `stap_probe_arg',
+ with each entry representing one argument. This is only valid if
+ M_ARGS_PARSED is true. */
+ std::vector<struct stap_probe_arg> m_parsed_args;
};
/* When parsing the arguments, we have to establish different precedences
@@ -326,7 +425,7 @@ stap_get_opcode (const char **s)
static struct type *
stap_get_expected_argument_type (struct gdbarch *gdbarch,
enum stap_arg_bitness b,
- const struct stap_probe *probe)
+ const char *probe_name)
{
switch (b)
{
@@ -361,8 +460,7 @@ stap_get_expected_argument_type (struct gdbarch *gdbarch,
return builtin_type (gdbarch)->builtin_uint64;
default:
- error (_("Undefined bitness for probe '%s'."),
- probe->p.name);
+ error (_("Undefined bitness for probe '%s'."), probe_name);
break;
}
}
@@ -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. */
- Consider the following argument string (x86 syntax):
-
- `4@%eax 4@$10'
-
- We have two arguments, `%eax' and `$10', both with 32-bit unsigned bitness.
- This function basically handles them, properly filling some structures with
- this information. */
-
-static void
-stap_parse_probe_arguments (struct stap_probe *probe, struct gdbarch *gdbarch)
+void
+stap_probe::parse_arguments (struct gdbarch *gdbarch)
{
const char *cur;
- gdb_assert (!probe->args_parsed);
- cur = probe->args_u.text;
- probe->args_parsed = 1;
- probe->args_u.vec = NULL;
+ gdb_assert (!m_have_parsed_args);
+ cur = m_unparsed_args_text;
+ m_have_parsed_args = true;
if (cur == NULL || *cur == '\0' || *cur == ':')
return;
@@ -1159,7 +1247,7 @@ stap_parse_probe_arguments (struct stap_probe *probe, struct gdbarch *gdbarch)
/* We have an error, because we don't expect anything
except 1, 2, 4 and 8. */
warning (_("unrecognized bitness %s%c' for probe `%s'"),
- got_minus ? "`-" : "`", *cur, probe->p.name);
+ got_minus ? "`-" : "`", *cur, this->get_name ());
return;
}
}
@@ -1173,7 +1261,7 @@ stap_parse_probe_arguments (struct stap_probe *probe, struct gdbarch *gdbarch)
arg.bitness = STAP_ARG_BITNESS_UNDEFINED;
arg.atype = stap_get_expected_argument_type (gdbarch, arg.bitness,
- probe);
+ this->get_name ());
expr = stap_parse_argument (&cur, arg.atype, gdbarch);
@@ -1191,35 +1279,31 @@ stap_parse_probe_arguments (struct stap_probe *probe, struct gdbarch *gdbarch)
/* Start it over again. */
cur = skip_spaces (cur);
- VEC_safe_push (stap_probe_arg_s, probe->args_u.vec, &arg);
+ m_parsed_args.push_back (arg);
}
}
-/* Implementation of the get_probe_address method. */
+/* Implementation of the get_relocated_address method. */
-static CORE_ADDR
-stap_get_probe_address (struct probe *probe, struct objfile *objfile)
+CORE_ADDR
+stap_probe::get_relocated_address (struct objfile *objfile)
{
- return probe->address + ANOFFSET (objfile->section_offsets,
- SECT_OFF_DATA (objfile));
+ return this->get_address () + ANOFFSET (objfile->section_offsets,
+ SECT_OFF_DATA (objfile));
}
/* Given PROBE, returns the number of arguments present in that probe's
argument string. */
-static unsigned
-stap_get_probe_argument_count (struct probe *probe_generic,
- struct frame_info *frame)
+unsigned
+stap_probe::get_argument_count (struct frame_info *frame)
{
- struct stap_probe *probe = (struct stap_probe *) probe_generic;
struct gdbarch *gdbarch = get_frame_arch (frame);
- gdb_assert (probe_generic->pops == &stap_probe_ops);
-
- if (!probe->args_parsed)
+ if (!m_have_parsed_args)
{
- if (can_evaluate_probe_arguments (probe_generic))
- stap_parse_probe_arguments (probe, gdbarch);
+ if (this->can_evaluate_arguments ())
+ this->parse_arguments (gdbarch);
else
{
static int have_warned_stap_incomplete = 0;
@@ -1234,13 +1318,12 @@ stap_get_probe_argument_count (struct probe *probe_generic,
}
/* Marking the arguments as "already parsed". */
- probe->args_u.vec = NULL;
- probe->args_parsed = 1;
+ m_have_parsed_args = true;
}
}
- gdb_assert (probe->args_parsed);
- return VEC_length (stap_probe_arg_s, probe->args_u.vec);
+ gdb_assert (m_have_parsed_args);
+ return m_parsed_args.size ();
}
/* Return 1 if OP is a valid operator inside a probe argument, or zero
@@ -1279,36 +1362,12 @@ stap_is_operator (const char *op)
return ret;
}
-/* Return argument N of probe 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. */
-
-static struct stap_probe_arg *
-stap_get_arg (struct stap_probe *probe, unsigned n, struct gdbarch *gdbarch)
-{
- if (!probe->args_parsed)
- stap_parse_probe_arguments (probe, gdbarch);
-
- gdb_assert (probe->args_parsed);
- if (probe->args_u.vec == NULL)
- 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."),
- probe->p.name, n);
-
- return VEC_index (stap_probe_arg_s, probe->args_u.vec, n);
-}
-
-/* Implement the `can_evaluate_probe_arguments' method of probe_ops. */
+/* Implement the `can_evaluate_arguments' method. */
-static int
-stap_can_evaluate_probe_arguments (struct probe *probe_generic)
+int
+stap_probe::can_evaluate_arguments () const
{
- struct stap_probe *stap_probe = (struct stap_probe *) probe_generic;
- struct gdbarch *gdbarch = stap_probe->p.arch;
+ struct gdbarch *gdbarch = this->get_gdbarch ();
/* For SystemTap probes, we have to guarantee that the method
stap_is_single_operand is defined on gdbarch. If it is not, then it
@@ -1319,35 +1378,28 @@ stap_can_evaluate_probe_arguments (struct probe *probe_generic)
/* Evaluate the probe's argument N (indexed from 0), returning a value
corresponding to it. Assertion is thrown if N does not exist. */
-static struct value *
-stap_evaluate_probe_argument (struct probe *probe_generic, unsigned n,
- struct frame_info *frame)
+struct value *
+stap_probe::evaluate_argument (unsigned n, struct frame_info *frame)
{
- struct stap_probe *stap_probe = (struct stap_probe *) probe_generic;
- struct gdbarch *gdbarch = get_frame_arch (frame);
struct stap_probe_arg *arg;
int pos = 0;
+ struct gdbarch *gdbarch = get_frame_arch (frame);
- gdb_assert (probe_generic->pops == &stap_probe_ops);
-
- arg = stap_get_arg (stap_probe, n, gdbarch);
+ arg = this->get_arg_by_number (n, gdbarch);
return evaluate_subexp_standard (arg->atype, arg->aexpr, &pos, EVAL_NORMAL);
}
/* Compile the probe's argument N (indexed from 0) to agent expression.
Assertion is thrown if N does not exist. */
-static void
-stap_compile_to_ax (struct probe *probe_generic, struct agent_expr *expr,
- struct axs_value *value, unsigned n)
+void
+stap_probe::compile_to_ax (struct agent_expr *expr, struct axs_value *value,
+ unsigned n)
{
- struct stap_probe *stap_probe = (struct stap_probe *) probe_generic;
struct stap_probe_arg *arg;
union exp_element *pc;
- gdb_assert (probe_generic->pops == &stap_probe_ops);
-
- arg = stap_get_arg (stap_probe, n, expr->gdbarch);
+ arg = this->get_arg_by_number (n, expr->gdbarch);
pc = arg->aexpr->elts;
gen_expr (arg->aexpr, &pc, expr, value);
@@ -1355,35 +1407,12 @@ stap_compile_to_ax (struct probe *probe_generic, struct agent_expr *expr,
require_rvalue (expr, value);
value->type = arg->atype;
}
-
-/* Destroy (free) the data related to PROBE. PROBE memory itself is not feed
- as it is allocated on an obstack. */
-
-static void
-stap_probe_destroy (struct probe *probe_generic)
-{
- struct stap_probe *probe = (struct stap_probe *) probe_generic;
-
- gdb_assert (probe_generic->pops == &stap_probe_ops);
-
- if (probe->args_parsed)
- {
- struct stap_probe_arg *arg;
- int ix;
-
- for (ix = 0; VEC_iterate (stap_probe_arg_s, probe->args_u.vec, ix, arg);
- ++ix)
- xfree (arg->aexpr);
- VEC_free (stap_probe_arg_s, probe->args_u.vec);
- }
-}
-
/* Set or clear a SystemTap semaphore. ADDRESS is the semaphore's
- address. SET is zero if the semaphore should be cleared, or one
- if it should be set. This is a helper function for `stap_semaphore_down'
- and `stap_semaphore_up'. */
+ address. SET is zero if the semaphore should be cleared, or one if
+ it should be set. This is a helper function for
+ 'stap_probe::set_semaphore' and 'stap_probe::clear_semaphore'. */
static void
stap_modify_semaphore (CORE_ADDR address, int set, struct gdbarch *gdbarch)
@@ -1419,43 +1448,58 @@ stap_modify_semaphore (CORE_ADDR address, int set, struct gdbarch *gdbarch)
warning (_("Could not write the value of a SystemTap semaphore."));
}
-/* Set a SystemTap semaphore. SEM is the semaphore's address. Semaphores
- act as reference counters, so calls to this function must be paired with
- calls to `stap_semaphore_down'.
+/* Implementation of the 'set_semaphore' method.
- This function and `stap_semaphore_down' race with another tool changing
- the probes, but that is too rare to care. */
+ SystemTap semaphores act as reference counters, so calls to this
+ function must be paired with calls to 'clear_semaphore'.
-static void
-stap_set_semaphore (struct probe *probe_generic, struct objfile *objfile,
- struct gdbarch *gdbarch)
+ This function and 'clear_semaphore' race with another tool
+ changing the probes, but that is too rare to care. */
+
+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)));
stap_modify_semaphore (addr, 1, gdbarch);
}
-/* Clear a SystemTap semaphore. SEM is the semaphore's address. */
+/* Implementation of the 'clear_semaphore' method. */
-static void
-stap_clear_semaphore (struct probe *probe_generic, struct objfile *objfile,
- struct gdbarch *gdbarch)
+void
+stap_probe::clear_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)));
stap_modify_semaphore (addr, 0, gdbarch);
}
+/* Implementation of the 'get_static_ops' method. */
+
+const static_probe_ops *
+stap_probe::get_static_ops () const
+{
+ return &stap_static_probe_ops;
+}
+
+/* Implementation of the 'gen_info_probes_table_values' method. */
+
+void
+stap_probe::gen_info_probes_table_values
+ (std::vector<const char *> *values) const
+{
+ const char *val = NULL;
+
+ if (m_sem_addr != 0)
+ val = print_core_address (this->get_gdbarch (), m_sem_addr);
+
+ values->push_back (val);
+}
+
/* Helper function that parses the information contained in a
SystemTap's probe. Basically, the information consists in:
@@ -1478,21 +1522,14 @@ handle_stap_probe (struct objfile *objfile, struct sdt_note *el,
int size = bfd_get_arch_size (abfd) / 8;
struct gdbarch *gdbarch = get_objfile_arch (objfile);
struct type *ptr_type = builtin_type (gdbarch)->builtin_data_ptr;
- CORE_ADDR base_ref;
- const char *probe_args = NULL;
- struct stap_probe *ret;
-
- ret = XOBNEW (&objfile->per_bfd->storage_obstack, struct stap_probe);
- ret->p.pops = &stap_probe_ops;
- ret->p.arch = gdbarch;
/* Provider and the name of the probe. */
- ret->p.provider = (char *) &el->data[3 * size];
- ret->p.name = ((const char *)
- memchr (ret->p.provider, '\0',
- (char *) el->data + el->size - ret->p.provider));
+ const char *provider = (const char *) &el->data[3 * size];
+ const char *name = ((const char *)
+ memchr (provider, '\0',
+ (char *) el->data + el->size - provider));
/* Making sure there is a name. */
- if (ret->p.name == NULL)
+ if (name == NULL)
{
complaint (&symfile_complaints, _("corrupt probe name when "
"reading `%s'"),
@@ -1503,32 +1540,32 @@ handle_stap_probe (struct objfile *objfile, struct sdt_note *el,
return;
}
else
- ++ret->p.name;
+ ++name;
/* Retrieving the probe's address. */
- ret->p.address = extract_typed_address (&el->data[0], ptr_type);
+ CORE_ADDR address = extract_typed_address (&el->data[0], ptr_type);
/* Link-time sh_addr of `.stapsdt.base' section. */
- base_ref = extract_typed_address (&el->data[size], ptr_type);
+ CORE_ADDR base_ref = extract_typed_address (&el->data[size], ptr_type);
/* Semaphore address. */
- ret->sem_addr = extract_typed_address (&el->data[2 * size], ptr_type);
+ CORE_ADDR sem_addr = extract_typed_address (&el->data[2 * size], ptr_type);
- ret->p.address += base - base_ref;
- if (ret->sem_addr != 0)
- ret->sem_addr += base - base_ref;
+ address += base - base_ref;
+ if (sem_addr != 0)
+ sem_addr += base - base_ref;
/* Arguments. We can only extract the argument format if there is a valid
name for this probe. */
- probe_args = ((const char*)
- memchr (ret->p.name, '\0',
- (char *) el->data + el->size - ret->p.name));
+ const char *probe_args = ((const char*)
+ memchr (name, '\0',
+ (char *) el->data + el->size - name));
if (probe_args != NULL)
++probe_args;
if (probe_args == NULL
- || (memchr (probe_args, '\0', (char *) el->data + el->size - ret->p.name)
+ || (memchr (probe_args, '\0', (char *) el->data + el->size - name)
!= el->data + el->size - 1))
{
complaint (&symfile_complaints, _("corrupt probe argument when "
@@ -1539,11 +1576,11 @@ handle_stap_probe (struct objfile *objfile, struct sdt_note *el,
return;
}
- ret->args_parsed = 0;
- ret->args_u.text = probe_args;
+ stap_probe *ret = new stap_probe (std::string (name), std::string (provider),
+ address, gdbarch, sem_addr, probe_args);
/* Successfully created probe. */
- probesp->push_back ((struct probe *) ret);
+ probesp->push_back (ret);
}
/* Helper function which tries to find the base address of the SystemTap
@@ -1584,11 +1621,21 @@ get_stap_base_address (bfd *obfd, bfd_vma *base)
return 1;
}
-/* Helper function for `elf_get_probes', which gathers information about all
- SystemTap probes from OBJFILE. */
+/* Implementation of the 'is_linespec' method. */
-static void
-stap_get_probes (std::vector<probe *> *probesp, struct objfile *objfile)
+bool
+stap_static_probe_ops::is_linespec (const char **linespecp) const
+{
+ static const char *const keywords[] = { "-pstap", "-probe-stap", NULL };
+
+ return probe_is_linespec_by_keyword (linespecp, keywords);
+}
+
+/* Implementation of the 'get_probes' method. */
+
+void
+stap_static_probe_ops::get_probes (std::vector<probe *> *probesp,
+ struct objfile *objfile) const
{
/* If we are here, then this is the first time we are parsing the
SystemTap probe's information. We basically have to count how many
@@ -1640,83 +1687,46 @@ stap_get_probes (std::vector<probe *> *probesp, struct objfile *objfile)
/* Implementation of the type_name method. */
-static const char *
-stap_type_name (struct probe *probe)
+const char *
+stap_static_probe_ops::type_name () const
{
- gdb_assert (probe->pops == &stap_probe_ops);
return "stap";
}
-static int
-stap_probe_is_linespec (const char **linespecp)
-{
- static const char *const keywords[] = { "-pstap", "-probe-stap", NULL };
+/* Implementation of the 'emit_info_probes_extra_fields' method. */
- return probe_is_linespec_by_keyword (linespecp, keywords);
+bool
+stap_static_probe_ops::emit_info_probes_extra_fields () const
+{
+ return true;
}
-static void
-stap_gen_info_probes_table_header (VEC (info_probe_column_s) **heads)
+/* Implementation of the 'gen_info_probes_table_header' method. */
+
+void
+stap_static_probe_ops::gen_info_probes_table_header
+ (std::vector<struct info_probe_column> *heads) const
{
- info_probe_column_s stap_probe_column;
+ struct info_probe_column stap_probe_column;
stap_probe_column.field_name = "semaphore";
stap_probe_column.print_name = _("Semaphore");
- VEC_safe_push (info_probe_column_s, *heads, &stap_probe_column);
-}
-
-static void
-stap_gen_info_probes_table_values (struct probe *probe_generic,
- VEC (const_char_ptr) **ret)
-{
- struct stap_probe *probe = (struct stap_probe *) probe_generic;
- struct gdbarch *gdbarch;
- const char *val = NULL;
-
- gdb_assert (probe_generic->pops == &stap_probe_ops);
-
- gdbarch = probe->p.arch;
-
- if (probe->sem_addr != 0)
- val = print_core_address (gdbarch, probe->sem_addr);
-
- VEC_safe_push (const_char_ptr, *ret, val);
+ heads->push_back (stap_probe_column);
}
-/* SystemTap probe_ops. */
-
-const struct probe_ops stap_probe_ops =
-{
- stap_probe_is_linespec,
- stap_get_probes,
- stap_get_probe_address,
- stap_get_probe_argument_count,
- stap_can_evaluate_probe_arguments,
- stap_evaluate_probe_argument,
- stap_compile_to_ax,
- stap_set_semaphore,
- stap_clear_semaphore,
- stap_probe_destroy,
- stap_type_name,
- stap_gen_info_probes_table_header,
- stap_gen_info_probes_table_values,
- NULL, /* enable_probe */
- NULL /* disable_probe */
-};
-
/* Implementation of the `info probes stap' command. */
static void
info_probes_stap_command (const char *arg, int from_tty)
{
- info_probes_for_ops (arg, from_tty, &stap_probe_ops);
+ info_probes_for_spops (arg, from_tty, &stap_static_probe_ops);
}
void
_initialize_stap_probe (void)
{
- all_probe_ops.push_back (&stap_probe_ops);
+ all_static_probe_ops.push_back (&stap_static_probe_ops);
add_setshow_zuinteger_cmd ("stap-expression", class_maintenance,
&stap_expression_debug,