From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 101403 invoked by alias); 15 Nov 2017 22:49:02 -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 101053 invoked by uid 89); 15 Nov 2017 22:48:55 -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 autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 15 Nov 2017 22:48:53 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0038CC0587C3; Wed, 15 Nov 2017 22:48:52 +0000 (UTC) Received: from localhost (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by smtp.corp.redhat.com (Postfix) with ESMTPS id BF8F96046E; Wed, 15 Nov 2017 22:48:51 +0000 (UTC) From: Sergio Durigan Junior To: Simon Marchi Cc: GDB Patches Subject: Re: [PATCH 2/3] Convert SystemTap probe interface to C++ (and perform some cleanups) References: <20171113175901.25367-1-sergiodj@redhat.com> <20171113175901.25367-3-sergiodj@redhat.com> <514b815b-1e3a-ecba-3891-7a231e552dd5@simark.ca> Date: Wed, 15 Nov 2017 22:49:00 -0000 Message-ID: <87zi7n17l8.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2017-11/txt/msg00291.txt.bz2 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 >> >> * 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. 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 *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/