Re: [PATCH 2/2] Make collect_probes return an std::vector

Sergio Durigan Junior
2017-09-12T00:18:09Z

On Sunday, September 10 2017, Simon Marchi wrote:

> Change collect_probes so it returns an std::vector<bound_probe> 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 <algorithm>
> #include "common/gdb_optional.h"
>
> -typedef struct bound_probe bound_probe_s;
> -DEF_VEC_O (bound_probe_s);
> -
> -\f
> -
> /* 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<bound_probe>
> 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<bound_probe> result;
> gdb::optional<compiled_regex> 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<bound_probe> &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<bound_probe> &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<bound_probe> 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<bound_probe> 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<bound_probe> 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<symtab_and_line> 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