From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 96147 invoked by alias); 16 Nov 2017 04:11:23 -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 96135 invoked by uid 89); 16 Nov 2017 04:11:23 -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 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; Thu, 16 Nov 2017 04:11:19 +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 9731D128B; Thu, 16 Nov 2017 04:11:18 +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 42E4660478; Thu, 16 Nov 2017 04:11:18 +0000 (UTC) From: Sergio Durigan Junior To: Simon Marchi Cc: GDB Patches Subject: Re: [PATCH 3/3] Convert DTrace probe interface to C++ (and perform some cleanups) References: <20171113175901.25367-1-sergiodj@redhat.com> <20171113175901.25367-4-sergiodj@redhat.com> <9cef49ec-5a62-5843-b037-91edd29a6692@simark.ca> Date: Thu, 16 Nov 2017 04:11:00 -0000 In-Reply-To: <9cef49ec-5a62-5843-b037-91edd29a6692@simark.ca> (Simon Marchi's message of "Tue, 14 Nov 2017 23:40:09 -0500") Message-ID: <87vaia278a.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/msg00294.txt.bz2 On Tuesday, November 14 2017, Simon Marchi wrote: > Hi Sergio, Hey Simon, Thanks for the review. > 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). That's true. I think I was misunderstanding the relationship between rvalues, lvalues and &&. Fixed. >> + : 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. I used the same approach as in stap-probe.c: expression_up, emplace_back, a constructor that takes care of std::move'ing things, and no destructor. I also converted 'type_str' to std::string. >> + } >> + >> + /* 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? OK, no problem. Done. >> + 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? This copy is necessary because the original vector should be intact on every iteration, but dtrace_probe's constructor performs a 'std::move' on it. I could have passed the original vector as an argument (by value), and not perform the std::move in the constructor, if you prefer. >> + 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. I've added a bound check and an internal_error, just like I did for stap-probe. Thanks, -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/