From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 104857 invoked by alias); 15 Nov 2017 04:40:16 -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 104844 invoked by uid 89); 15 Nov 2017 04:40:15 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.7 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KB_WAM_FROM_NAME_SINGLEWORD,LIKELY_SPAM_BODY,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 04:40:12 +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 1E5A81E030; Tue, 14 Nov 2017 23:40:10 -0500 (EST) Subject: Re: [PATCH 3/3] Convert DTrace probe interface to C++ (and perform some cleanups) To: Sergio Durigan Junior , GDB Patches References: <20171113175901.25367-1-sergiodj@redhat.com> <20171113175901.25367-4-sergiodj@redhat.com> From: Simon Marchi Message-ID: <9cef49ec-5a62-5843-b037-91edd29a6692@simark.ca> Date: Wed, 15 Nov 2017 04:40: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-4-sergiodj@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-11/txt/msg00268.txt.bz2 Hi Sergio, On 2017-11-13 12:59 PM, Sergio Durigan Junior wrote: > This patch converts the DTrace probe > interface (gdb/dtrace-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 dtrace_probe' to 'class > dtrace_probe', and a new 'class dtrace_static_probe_ops' to replace the > use of 'dtrace_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 dtrace-probe interface. > > There are several helper functions used to parse parts of a dtrace > 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. > > I've regtested this patch on BuildBot, no regressions found. > > gdb/ChangeLog: > 2017-11-13 Sergio Durigan Junior > > * dtrace-probe.c (struct probe_ops dtrace_probe_ops): Delete. > (dtrace_probe_arg_s): Delete type and VEC. > (dtrace_probe_enabler_s): Likewise. > (struct dtrace_probe): Replace by... > (class dtrace_static_probe_ops): ...this and... > (class dtrace_probe): ...this. > (dtrace_probe_is_linespec): Rename to... > (dtrace_static_probe_ops::is_linespec): ...this. Adjust code > to reflect change. > (dtrace_process_dof_probe): Use 'std::vector' instead of VEC. > Adjust code. Create new instance of 'dtrace_probe'. > (dtrace_build_arg_exprs): Rename to... > (dtrace_probe::build_arg_exprs): ...this. Adjust code to > reflect change. > (dtrace_get_probes): Rename to... > (dtrace_static_probe_ops::get_probes): ...this. Adjust code > to reflect change. > (dtrace_get_arg): Rename to... > (dtrace_probe::get_arg_by_number): ...this. Adjust code to > reflect change. > (dtrace_probe_is_enabled): Rename to... > (dtrace_probe::is_enabled): ...this. Adjust code to reflect > change. > (dtrace_get_probe_address): Rename to... > (dtrace_probe::get_relocated_address): ...this. Adjust code > to reflect change. > (dtrace_get_probe_argument_count): Rename to... > (dtrace_probe::get_argument_count): ...this. Adjust code to > reflect change. > (dtrace_can_evaluate_probe_arguments): Rename to... > (dtrace_probe::can_evaluate_arguments): ...this. Adjust code > to reflect change. > (dtrace_evaluate_probe_argument): Rename to... > (dtrace_probe::evaluate_argument): ...this. Adjust code to > reflect change. > (dtrace_compile_to_ax): Rename to... > (dtrace_probe::compile_to_ax): ...this. Adjust code to > reflect change. > (dtrace_probe_destroy): Delete. > (dtrace_type_name): Rename to... > (dtrace_static_probe_ops::type_name): ...this. Adjust code to > reflect change. > (dtrace_probe::get_static_ops): New method. > (dtrace_gen_info_probes_table_header): Rename to... > (dtrace_static_probe_ops::gen_info_probes_table_header): > ...this. Adjust code to reflect change. > (dtrace_gen_info_probes_table_values): Rename to... > (dtrace_probe::gen_info_probes_table_values): ...this. Adjust > code to reflect change. > (dtrace_enable_probe): Rename to... > (dtrace_probe::enable_probe): ...this. Adjust code to reflect > change. > (dtrace_disable_probe): Rename to... > (dtrace_probe::disable_probe): ...this. Adjust code to reflect > change. > (struct probe_ops dtrace_probe_ops): Delete. > (dtrace_static_probe_ops::emit_info_probes_extra_fields): New > method. > (info_probes_dtrace_command): Call 'info_probes_for_spops' > instead of 'info_probes_for_ops'. > (_initialize_dtrace_probe): Use 'all_static_probe_ops' instead > of 'all_probe_ops'. > --- > gdb/dtrace-probe.c | 528 +++++++++++++++++++++++++++-------------------------- > 1 file changed, 268 insertions(+), 260 deletions(-) > > diff --git a/gdb/dtrace-probe.c b/gdb/dtrace-probe.c > index 2bbe03e4b3..b53ff7ac6c 100644 > --- a/gdb/dtrace-probe.c > +++ b/gdb/dtrace-probe.c > @@ -41,10 +41,6 @@ > # define SHT_SUNW_dof 0x6ffffff4 > #endif > > -/* Forward declaration. */ > - > -extern const struct probe_ops dtrace_probe_ops; > - > /* The following structure represents a single argument for the > probe. */ > > @@ -60,9 +56,6 @@ struct dtrace_probe_arg > struct expression *expr; > }; > > -typedef struct dtrace_probe_arg dtrace_probe_arg_s; > -DEF_VEC_O (dtrace_probe_arg_s); > - > /* The following structure represents an enabler for a probe. */ > > struct dtrace_probe_enabler > @@ -73,39 +66,121 @@ struct dtrace_probe_enabler > CORE_ADDR address; > }; > > -typedef struct dtrace_probe_enabler dtrace_probe_enabler_s; > -DEF_VEC_O (dtrace_probe_enabler_s); > +/* Class that implements the static probe methods for "stap" probes. */ > + > +class dtrace_static_probe_ops : public static_probe_ops > +{ > +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; > +}; > + > +/* DTrace static_probe_ops. */ > + > +const dtrace_static_probe_ops dtrace_static_probe_ops; > > /* The following structure represents a dtrace probe. */ > > -struct dtrace_probe > +class dtrace_probe : public probe > { > - /* Generic information about the probe. This must 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: > + /* Constructor for dtrace_probe. */ > + dtrace_probe (std::string &&name_, std::string &&provider_, CORE_ADDR address_, > + struct gdbarch *arch_, > + std::vector &args_, > + std::vector &enablers_) I am wondering why you don't use && for the vectors, as you do with the strings. If the constructor is "stealing" from the vectors passed as parameter, I think it would make sense if the parameters were &&, to indicate to the caller that they'll be moved from (and force it to use std::move). > + : probe (std::move (name_), std::move (provider_), address_, arch_), > + m_args (std::move (args_)), > + m_enablers (std::move (enablers_)), > + m_args_expr_built (false) > + {} > + > + /* Destructor for dtrace_probe. */ > + ~dtrace_probe () > + { > + for (struct dtrace_probe_arg arg : m_args) > + { > + xfree (arg.type_str); > + xfree (arg.expr); > + } As for stap_probe, I think defining a destructor in dtrace_probe_arg would be clearer. > + } > + > + /* 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. */ > + const static_probe_ops *get_static_ops () const override; > + > + /* See probe.h. */ > + void gen_info_probes_table_values > + (std::vector *values) const override; > + > + /* See probe.h. */ > + bool can_enable () const override > + { > + return true; > + } > + > + /* See probe.h. */ > + void enable () override; > > + /* See probe.h. */ > + void disable () override; > + > + /* Return the Nth argument of the probe. */ > + struct dtrace_probe_arg *get_arg_by_number (unsigned n, > + struct gdbarch *gdbarch); > + > + /* Build the GDB internal expressiosn that, once evaluated, will > + calculate the values of the arguments of the probe. */ > + void build_arg_exprs (struct gdbarch *gdbarch); > + > + /* Determine whether the probe is "enabled" or "disabled". A > + disabled probe is a probe in which one or more enablers are > + disabled. */ > + bool is_enabled () const; > + > +private: > /* A probe can have zero or more arguments. */ > - int probe_argc; > - VEC (dtrace_probe_arg_s) *args; > + int m_argc; It seems like this field is never set, but used at least once. Should we use args->size() instead? > + std::vector m_args; > > /* A probe can have zero or more "enablers" associated with it. */ > - VEC (dtrace_probe_enabler_s) *enablers; > + std::vector m_enablers; > > /* Whether the expressions for the arguments have been built. */ > - unsigned int args_expr_built : 1; > + bool m_args_expr_built; > }; > > -/* Implementation of the probe_is_linespec method. */ > - > -static int > -dtrace_probe_is_linespec (const char **linespecp) > -{ > - static const char *const keywords[] = { "-pdtrace", "-probe-dtrace", NULL }; > - > - return probe_is_linespec_by_keyword (linespecp, keywords); > -} > - > /* DOF programs can contain an arbitrary number of sections of 26 > different types. In order to support DTrace USDT probes we only > need to handle a subset of these section types, fortunately. These > @@ -322,8 +397,6 @@ dtrace_process_dof_probe (struct objfile *objfile, > char *argtab, uint64_t strtab_size) > { > int i, j, num_probes, num_enablers; > - struct cleanup *cleanup; > - VEC (dtrace_probe_enabler_s) *enablers; > char *p; > > /* Each probe section can define zero or more probes of two > @@ -368,9 +441,7 @@ dtrace_process_dof_probe (struct objfile *objfile, > > /* Build the list of enablers for the probes defined in this Probe > DOF section. */ > - enablers = NULL; > - cleanup > - = make_cleanup (VEC_cleanup (dtrace_probe_enabler_s), &enablers); > + std::vector enablers; > num_enablers = DOF_UINT (dof, probe->dofpr_nenoffs); > for (i = 0; i < num_enablers; i++) > { > @@ -380,38 +451,32 @@ dtrace_process_dof_probe (struct objfile *objfile, > > enabler.address = DOF_UINT (dof, probe->dofpr_addr) > + DOF_UINT (dof, enabler_offset); > - VEC_safe_push (dtrace_probe_enabler_s, enablers, &enabler); > + enablers.push_back (enabler); > } > > for (i = 0; i < num_probes; i++) > { > uint32_t probe_offset > = ((uint32_t *) offtab)[DOF_UINT (dof, probe->dofpr_offidx) + i]; > - struct dtrace_probe *ret = > - XOBNEW (&objfile->per_bfd->storage_obstack, struct dtrace_probe); > - > - ret->p.pops = &dtrace_probe_ops; > - ret->p.arch = gdbarch; > - ret->args_expr_built = 0; > > /* Set the provider and the name of the probe. */ > - ret->p.provider > - = xstrdup (strtab + DOF_UINT (dof, provider->dofpv_name)); > - ret->p.name = xstrdup (strtab + DOF_UINT (dof, probe->dofpr_name)); > + const char *probe_provider > + = strtab + DOF_UINT (dof, provider->dofpv_name); > + const char *name = strtab + DOF_UINT (dof, probe->dofpr_name); > > /* The probe address. */ > - ret->p.address > + CORE_ADDR address > = DOF_UINT (dof, probe->dofpr_addr) + DOF_UINT (dof, probe_offset); > > /* Number of arguments in the probe. */ > - ret->probe_argc = DOF_UINT (dof, probe->dofpr_nargc); > + int probe_argc = DOF_UINT (dof, probe->dofpr_nargc); > > /* Store argument type descriptions. A description of the type > of the argument is in the (J+1)th null-terminated string > starting at 'strtab' + 'probe->dofpr_nargv'. */ > - ret->args = NULL; > + std::vector args; > p = strtab + DOF_UINT (dof, probe->dofpr_nargv); > - for (j = 0; j < ret->probe_argc; j++) > + for (j = 0; j < probe_argc; j++) > { > struct dtrace_probe_arg arg; > expression_up expr; > @@ -442,17 +507,18 @@ dtrace_process_dof_probe (struct objfile *objfile, > if (expr != NULL && expr->elts[0].opcode == OP_TYPE) > arg.type = expr->elts[1].type; > > - VEC_safe_push (dtrace_probe_arg_s, ret->args, &arg); > + args.push_back (arg); > } > > - /* Add the vector of enablers to this probe, if any. */ > - ret->enablers = VEC_copy (dtrace_probe_enabler_s, enablers); > + std::vector enablers_copy = enablers; Why is this copy necessary? What happens with the original vector? > + dtrace_probe *ret = new dtrace_probe (std::string (name), > + std::string (probe_provider), > + address, gdbarch, > + args, enablers_copy); > > /* Successfully created probe. */ > - probesp->push_back ((struct probe *) ret); > + probesp->push_back (ret); > } > - > - do_cleanups (cleanup); > } > > /* Helper function to collect the probes described in the DOF program > @@ -555,28 +621,23 @@ dtrace_process_dof (asection *sect, struct objfile *objfile, > sect->name); > } > > -/* Helper function to build the GDB internal expressiosn that, once > - evaluated, will calculate the values of the arguments of a given > - PROBE. */ > +/* Implementation of 'build_arg_exprs' method. */ > > -static void > -dtrace_build_arg_exprs (struct dtrace_probe *probe, > - struct gdbarch *gdbarch) > +void > +dtrace_probe::build_arg_exprs (struct gdbarch *gdbarch) > { > - struct parser_state pstate; > - struct dtrace_probe_arg *arg; > - int i; > - > - probe->args_expr_built = 1; > + m_args_expr_built = true; > > /* Iterate over the arguments in the probe and build the > corresponding GDB internal expression that will generate the > value of the argument when executed at the PC of the probe. */ > - for (i = 0; i < probe->probe_argc; i++) > + for (int i = 0; i < m_argc; i++) > { > + struct dtrace_probe_arg *arg; > struct cleanup *back_to; > + struct parser_state pstate; > > - arg = VEC_index (dtrace_probe_arg_s, probe->args, i); > + arg = &m_args[i]; > > /* Initialize the expression buffer in the parser state. The > language does not matter, since we are using our own > @@ -606,138 +667,82 @@ dtrace_build_arg_exprs (struct dtrace_probe *probe, > } > } > > -/* Helper function to return the Nth argument of a given PROBE. */ > - > -static struct dtrace_probe_arg * > -dtrace_get_arg (struct dtrace_probe *probe, unsigned n, > - struct gdbarch *gdbarch) > -{ > - if (!probe->args_expr_built) > - dtrace_build_arg_exprs (probe, gdbarch); > - > - return VEC_index (dtrace_probe_arg_s, probe->args, n); > -} > - > -/* Implementation of the get_probes method. */ > +/* Implementation of 'get_arg_by_number' method. */ > > -static void > -dtrace_get_probes (std::vector *probesp, struct objfile *objfile) > +struct dtrace_probe_arg * > +dtrace_probe::get_arg_by_number (unsigned n, struct gdbarch *gdbarch) > { > - bfd *abfd = objfile->obfd; > - asection *sect = NULL; > - > - /* Do nothing in case this is a .debug file, instead of the objfile > - itself. */ > - if (objfile->separate_debug_objfile_backlink != NULL) > - return; > + if (!m_args_expr_built) > + this->build_arg_exprs (gdbarch); > > - /* Iterate over the sections in OBJFILE looking for DTrace > - information. */ > - for (sect = abfd->sections; sect != NULL; sect = sect->next) > - { > - if (elf_section_data (sect)->this_hdr.sh_type == SHT_SUNW_dof) > - { > - bfd_byte *dof; > - > - /* Read the contents of the DOF section and then process it to > - extract the information of any probe defined into it. */ > - if (!bfd_malloc_and_get_section (abfd, sect, &dof)) > - complaint (&symfile_complaints, > - _("could not obtain the contents of" > - "section '%s' in objfile `%s'."), > - sect->name, abfd->filename); > - > - dtrace_process_dof (sect, objfile, probesp, > - (struct dtrace_dof_hdr *) dof); > - xfree (dof); > - } > - } > + return &m_args[n]; It would be nice to add a gdb_assert (n < m_args.size ()) here. Thanks, Simon