From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 76761 invoked by alias); 12 Sep 2017 00:18:09 -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 76741 invoked by uid 89); 12 Sep 2017 00:18:08 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,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; Tue, 12 Sep 2017 00:18:01 +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 C6FB8129862; Tue, 12 Sep 2017 00:17:59 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com C6FB8129862 Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=sergiodj@redhat.com Received: from localhost (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 7FA5269FA0; Tue, 12 Sep 2017 00:17:59 +0000 (UTC) From: Sergio Durigan Junior To: Simon Marchi Cc: , Mark Wielaard Subject: Re: [PATCH 2/2] Make collect_probes return an std::vector References: <1505053377-10061-1-git-send-email-simon.marchi@ericsson.com> <1505053377-10061-2-git-send-email-simon.marchi@ericsson.com> Date: Tue, 12 Sep 2017 00:18:00 -0000 In-Reply-To: <1505053377-10061-2-git-send-email-simon.marchi@ericsson.com> (Simon Marchi's message of "Sun, 10 Sep 2017 16:22:57 +0200") Message-ID: <87tw08ixc9.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-09/txt/msg00320.txt.bz2 On Sunday, September 10 2017, Simon Marchi wrote: > Change collect_probes so it returns an std::vector instead > of a VEC(bound_probe_s). This allows removing some cleanups. It also > seems like enable_probes_command and disable_probes_command were not > freeing that vector. > > The comparison function compare_probes needs to be updated to return a > bool indicating whether the first parameter is "less than" the second > parameter. > > I defined two constructors to bound_probe. The default constructor is > needed, for example, so the instance in struct bp_location can be > constructed without parameters. The constructor with parameters is > useful so we can use emplace_back and pass the values directly. > > The s390 builder on the buildbot shows a weird failure that I can't > explain: > > ../../binutils-gdb/gdb/elfread.c: In function void probe_key_free(bfd*, void*): > ../../binutils-gdb/gdb/elfread.c:1346:8: error: types may not be defined in a for-range-declaration [-Werror] > for (struct probe *probe : *probes) > ^~~~~~ As I've told you on IRC, I've also stumbled upon this problem. The "solution" is to use the typedef instead of the full name of the type: for (probe *probe : *probes) > I guess it's a bug with that specific version< of the compiler, since no > other gcc gives me that error. It is using: > > g++ (GCC) 6.3.1 20161221 (Red Hat 6.3.1-1) > > Any idea about this problem? It seems to me this is indeed a compiler problem. I've found at least one report on GCC's bugzilla about a related issue. Mark, you're the responsible for this specific slave (marist-fedora-s390x); can you check if it's possible to update the GCC there, please? > gdb/ChangeLog: > > * probe.h (struct bound_probe): Define constructors. > * probe.c (bound_probe_s): Remove typedef. > (DEF_VEC_O (bound_probe_s)): Remove VEC. > (collect_probes): Change return type to std::vector, remove > cleanup. > (compare_probes): Return bool, change parameter type. Change > semantic to "less than". > (gen_ui_out_table_header_info): Change parameter to std::vector > and update. > (exists_probe_with_pops): Likewise. > (info_probes_for_ops): Update to std::vector change. > (enable_probes_command): Likewise. > (disable_probes_command): Likewise. > --- > gdb/probe.c | 147 ++++++++++++++++++++++++------------------------------------ > gdb/probe.h | 19 +++++--- > 2 files changed, 72 insertions(+), 94 deletions(-) > > diff --git a/gdb/probe.c b/gdb/probe.c > index 2d68437..384ea9d 100644 > --- a/gdb/probe.c > +++ b/gdb/probe.c > @@ -38,11 +38,6 @@ > #include > #include "common/gdb_optional.h" > > -typedef struct bound_probe bound_probe_s; > -DEF_VEC_O (bound_probe_s); > - > - > - > /* A helper for parse_probes that decodes a probe specification in > SEARCH_PSPACE. It appends matching SALs to RESULT. */ > > @@ -267,17 +262,14 @@ find_probe_by_pc (CORE_ADDR pc) > If POPS is not NULL, only probes of this certain probe_ops will match. > Each argument is a regexp, or NULL, which matches anything. */ > > -static VEC (bound_probe_s) * > +static std::vector > collect_probes (char *objname, char *provider, char *probe_name, > const struct probe_ops *pops) > { > struct objfile *objfile; > - VEC (bound_probe_s) *result = NULL; > - struct cleanup *cleanup; > + std::vector result; > gdb::optional obj_pat, prov_pat, probe_pat; > > - cleanup = make_cleanup (VEC_cleanup (bound_probe_s), &result); > - > if (provider != NULL) > prov_pat.emplace (provider, REG_NOSUB, _("Invalid provider regexp")); > if (probe_name != NULL) > @@ -301,8 +293,6 @@ collect_probes (char *objname, char *provider, char *probe_name, > > for (struct probe *probe : probes) > { > - struct bound_probe bound; > - > if (pops != NULL && probe->pops != pops) > continue; > > @@ -314,46 +304,39 @@ collect_probes (char *objname, char *provider, char *probe_name, > && probe_pat->exec (probe->name, 0, NULL, 0) != 0) > continue; > > - bound.objfile = objfile; > - bound.probe = probe; > - VEC_safe_push (bound_probe_s, result, &bound); > + result.emplace_back (probe, objfile); > } > } > > - discard_cleanups (cleanup); > return result; > } > > /* A qsort comparison function for bound_probe_s objects. */ > > -static int > -compare_probes (const void *a, const void *b) > +static bool > +compare_probes (const bound_probe &a, const bound_probe &b) > { > - const struct bound_probe *pa = (const struct bound_probe *) a; > - const struct bound_probe *pb = (const struct bound_probe *) b; > int v; > > - v = strcmp (pa->probe->provider, pb->probe->provider); > - if (v) > - return v; > + v = strcmp (a.probe->provider, b.probe->provider); > + if (v != 0) > + return v < 0; > > - v = strcmp (pa->probe->name, pb->probe->name); > - if (v) > - return v; > + v = strcmp (a.probe->name, b.probe->name); > + if (v != 0) > + return v < 0; > > - if (pa->probe->address < pb->probe->address) > - return -1; > - if (pa->probe->address > pb->probe->address) > - return 1; > + if (a.probe->address != b.probe->address) > + return a.probe->address < b.probe->address; > > - return strcmp (objfile_name (pa->objfile), objfile_name (pb->objfile)); > + return strcmp (objfile_name (a.objfile), objfile_name (b.objfile)) < 0; > } > > /* Helper function that generate entries in the ui_out table being > crafted by `info_probes_for_ops'. */ > > static void > -gen_ui_out_table_header_info (VEC (bound_probe_s) *probes, > +gen_ui_out_table_header_info (const std::vector &probes, > const struct probe_ops *p) > { > /* `headings' refers to the names of the columns when printing `info > @@ -382,11 +365,9 @@ gen_ui_out_table_header_info (VEC (bound_probe_s) *probes, > VEC_iterate (info_probe_column_s, headings, ix, column); > ++ix) > { > - struct bound_probe *probe; > - int jx; > size_t size_max = strlen (column->print_name); > > - for (jx = 0; VEC_iterate (bound_probe_s, probes, jx, probe); ++jx) > + for (const bound_probe &probe : probes) > { > /* `probe_fields' refers to the values of each new field that this > probe will display. */ > @@ -395,11 +376,11 @@ gen_ui_out_table_header_info (VEC (bound_probe_s) *probes, > const char *val; > int kx; > > - if (probe->probe->pops != p) > + if (probe.probe->pops != p) > continue; > > c2 = make_cleanup (VEC_cleanup (const_char_ptr), &probe_fields); > - p->gen_info_probes_table_values (probe->probe, &probe_fields); > + p->gen_info_probes_table_values (probe.probe, &probe_fields); > > gdb_assert (VEC_length (const_char_ptr, probe_fields) > == headings_size); > @@ -526,14 +507,14 @@ get_number_extra_fields (const struct probe_ops *pops) > featuring the given POPS. It returns 0 otherwise. */ > > static int > -exists_probe_with_pops (VEC (bound_probe_s) *probes, > +exists_probe_with_pops (const std::vector &probes, > const struct probe_ops *pops) > { > struct bound_probe *probe; > int ix; > > - for (ix = 0; VEC_iterate (bound_probe_s, probes, ix, probe); ++ix) > - if (probe->probe->pops == pops) > + for (const bound_probe &probe : probes) > + if (probe.probe->pops == pops) > return 1; > > return 0; > @@ -565,15 +546,13 @@ info_probes_for_ops (const char *arg, int from_tty, > { > char *provider, *probe_name = NULL, *objname = NULL; > struct cleanup *cleanup = make_cleanup (null_cleanup, NULL); > - VEC (bound_probe_s) *probes; > - int i, any_found; > + int any_found; > int ui_out_extra_fields = 0; > size_t size_addr; > size_t size_name = strlen ("Name"); > size_t size_objname = strlen ("Object"); > size_t size_provider = strlen ("Provider"); > size_t size_type = strlen ("Type"); > - struct bound_probe *probe; > struct gdbarch *gdbarch = get_current_arch (); > > parse_probe_linespec (arg, &provider, &probe_name, &objname); > @@ -581,8 +560,8 @@ info_probes_for_ops (const char *arg, int from_tty, > make_cleanup (xfree, probe_name); > make_cleanup (xfree, objname); > > - probes = collect_probes (objname, provider, probe_name, pops); > - make_cleanup (VEC_cleanup (probe_p), &probes); > + std::vector probes > + = collect_probes (objname, provider, probe_name, pops); > > if (pops == NULL) > { > @@ -609,27 +588,23 @@ info_probes_for_ops (const char *arg, int from_tty, > { > ui_out_emit_table table_emitter (current_uiout, > 5 + ui_out_extra_fields, > - VEC_length (bound_probe_s, probes), > - "StaticProbes"); > + probes.size (), "StaticProbes"); > > - if (!VEC_empty (bound_probe_s, probes)) > - qsort (VEC_address (bound_probe_s, probes), > - VEC_length (bound_probe_s, probes), > - sizeof (bound_probe_s), compare_probes); > + std::sort (probes.begin (), probes.end (), compare_probes); > > /* What's the size of an address in our architecture? */ > size_addr = gdbarch_addr_bit (gdbarch) == 64 ? 18 : 10; > > /* Determining the maximum size of each field (`type', `provider', > `name' and `objname'). */ > - for (i = 0; VEC_iterate (bound_probe_s, probes, i, probe); ++i) > + for (const bound_probe &probe : probes) > { > - const char *probe_type = probe->probe->pops->type_name (probe->probe); > + const char *probe_type = probe.probe->pops->type_name (probe.probe); > > size_type = std::max (strlen (probe_type), size_type); > - size_name = std::max (strlen (probe->probe->name), size_name); > - size_provider = std::max (strlen (probe->probe->provider), size_provider); > - size_objname = std::max (strlen (objfile_name (probe->objfile)), > + size_name = std::max (strlen (probe.probe->name), size_name); > + size_provider = std::max (strlen (probe.probe->provider), size_provider); > + size_objname = std::max (strlen (objfile_name (probe.objfile)), > size_objname); > } > > @@ -657,18 +632,18 @@ info_probes_for_ops (const char *arg, int from_tty, > current_uiout->table_header (size_objname, ui_left, "object", _("Object")); > current_uiout->table_body (); > > - for (i = 0; VEC_iterate (bound_probe_s, probes, i, probe); ++i) > + for (const bound_probe &probe : probes) > { > - const char *probe_type = probe->probe->pops->type_name (probe->probe); > + const char *probe_type = probe.probe->pops->type_name (probe.probe); > > ui_out_emit_tuple tuple_emitter (current_uiout, "probe"); > > current_uiout->field_string ("type",probe_type); > - current_uiout->field_string ("provider", probe->probe->provider); > - current_uiout->field_string ("name", probe->probe->name); > - current_uiout->field_core_addr ( > - "addr", probe->probe->arch, > - get_probe_address (probe->probe, probe->objfile)); > + current_uiout->field_string ("provider", probe.probe->provider); > + current_uiout->field_string ("name", probe.probe->name); > + current_uiout->field_core_addr ("addr", probe.probe->arch, > + get_probe_address (probe.probe, > + probe.objfile)); > > if (pops == NULL) > { > @@ -677,20 +652,20 @@ info_probes_for_ops (const char *arg, int from_tty, > > for (ix = 0; VEC_iterate (probe_ops_cp, all_probe_ops, ix, po); > ++ix) > - if (probe->probe->pops == po) > - print_ui_out_info (probe->probe); > + if (probe.probe->pops == po) > + print_ui_out_info (probe.probe); > else if (exists_probe_with_pops (probes, po)) > print_ui_out_not_applicables (po); > } > else > - print_ui_out_info (probe->probe); > + print_ui_out_info (probe.probe); > > current_uiout->field_string ("object", > - objfile_name (probe->objfile)); > + objfile_name (probe.objfile)); > current_uiout->text ("\n"); > } > > - any_found = !VEC_empty (bound_probe_s, probes); > + any_found = !probes.empty (); > } > do_cleanups (cleanup); > > @@ -713,17 +688,15 @@ enable_probes_command (char *arg, int from_tty) > { > char *provider, *probe_name = NULL, *objname = NULL; > struct cleanup *cleanup = make_cleanup (null_cleanup, NULL); > - VEC (bound_probe_s) *probes; > - struct bound_probe *probe; > - int i; > > parse_probe_linespec ((const char *) arg, &provider, &probe_name, &objname); > make_cleanup (xfree, provider); > make_cleanup (xfree, probe_name); > make_cleanup (xfree, objname); > > - probes = collect_probes (objname, provider, probe_name, NULL); > - if (VEC_empty (bound_probe_s, probes)) > + std::vector probes > + = collect_probes (objname, provider, probe_name, NULL); > + if (probes.empty ()) > { > current_uiout->message (_("No probes matched.\n")); > do_cleanups (cleanup); > @@ -732,19 +705,19 @@ enable_probes_command (char *arg, int from_tty) > > /* Enable the selected probes, provided their backends support the > notion of enabling a probe. */ > - for (i = 0; VEC_iterate (bound_probe_s, probes, i, probe); ++i) > + for (const bound_probe &probe: probes) > { > - const struct probe_ops *pops = probe->probe->pops; > + const struct probe_ops *pops = probe.probe->pops; > > if (pops->enable_probe != NULL) > { > - pops->enable_probe (probe->probe); > + pops->enable_probe (probe.probe); > current_uiout->message (_("Probe %s:%s enabled.\n"), > - probe->probe->provider, probe->probe->name); > + probe.probe->provider, probe.probe->name); > } > else > current_uiout->message (_("Probe %s:%s cannot be enabled.\n"), > - probe->probe->provider, probe->probe->name); > + probe.probe->provider, probe.probe->name); > } > > do_cleanups (cleanup); > @@ -757,17 +730,15 @@ disable_probes_command (char *arg, int from_tty) > { > char *provider, *probe_name = NULL, *objname = NULL; > struct cleanup *cleanup = make_cleanup (null_cleanup, NULL); > - VEC (bound_probe_s) *probes; > - struct bound_probe *probe; > - int i; > > parse_probe_linespec ((const char *) arg, &provider, &probe_name, &objname); > make_cleanup (xfree, provider); > make_cleanup (xfree, probe_name); > make_cleanup (xfree, objname); > > - probes = collect_probes (objname, provider, probe_name, NULL /* pops */); > - if (VEC_empty (bound_probe_s, probes)) > + std::vector probes > + = collect_probes (objname, provider, probe_name, NULL /* pops */); > + if (probes.empty ()) > { > current_uiout->message (_("No probes matched.\n")); > do_cleanups (cleanup); > @@ -776,19 +747,19 @@ disable_probes_command (char *arg, int from_tty) > > /* Disable the selected probes, provided their backends support the > notion of enabling a probe. */ > - for (i = 0; VEC_iterate (bound_probe_s, probes, i, probe); ++i) > + for (const bound_probe &probe : probes) > { > - const struct probe_ops *pops = probe->probe->pops; > + const struct probe_ops *pops = probe.probe->pops; > > if (pops->disable_probe != NULL) > { > - pops->disable_probe (probe->probe); > + pops->disable_probe (probe.probe); > current_uiout->message (_("Probe %s:%s disabled.\n"), > - probe->probe->provider, probe->probe->name); > + probe.probe->provider, probe.probe->name); > } > else > current_uiout->message (_("Probe %s:%s cannot be disabled.\n"), > - probe->probe->provider, probe->probe->name); > + probe.probe->provider, probe.probe->name); > } > > do_cleanups (cleanup); > diff --git a/gdb/probe.h b/gdb/probe.h > index 61e3031..75e9a5c 100644 > --- a/gdb/probe.h > +++ b/gdb/probe.h > @@ -214,15 +214,22 @@ struct probe > their point of use. */ > > struct bound_probe > - { > - /* The probe. */ > +{ > + bound_probe () > + {} > > - struct probe *probe; > + bound_probe (struct probe *probe_, struct objfile *objfile_) > + : probe (probe_), objfile (objfile_) > + {} What do you think about putting simple comments on these constructors summarizing what you explained in the commit message? > > - /* The objfile in which the probe originated. */ > + /* The probe. */ > > - struct objfile *objfile; > - }; > + struct probe *probe = NULL; > + > + /* The objfile in which the probe originated. */ > + > + struct objfile *objfile = NULL; > +}; > > /* A helper for linespec that decodes a probe specification. It > returns a std::vector object and updates LOC or > -- > 2.7.4 Otherwise, LGTM. 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/