From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 35398 invoked by alias); 15 Nov 2017 03:58:40 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 35388 invoked by uid 89); 15 Nov 2017 03:58:39 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.7 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KB_WAM_FROM_NAME_SINGLEWORD,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 15 Nov 2017 03:58:37 +0000 Received: from [10.0.0.11] (192-222-251-162.qc.cable.ebox.net [192.222.251.162]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id DFFD91E0A6; Tue, 14 Nov 2017 22:58:35 -0500 (EST) Subject: Re: [PATCH 2/3] Convert SystemTap probe interface to C++ (and perform some cleanups) To: Sergio Durigan Junior , GDB Patches References: <20171113175901.25367-1-sergiodj@redhat.com> <20171113175901.25367-3-sergiodj@redhat.com> From: Simon Marchi Message-ID: <514b815b-1e3a-ecba-3891-7a231e552dd5@simark.ca> Date: Wed, 15 Nov 2017 03:58:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171113175901.25367-3-sergiodj@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-11/txt/msg00267.txt.bz2 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 > > * 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 *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 *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 *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