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

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

[Simon Marchi]

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)
        ^~~~~~

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?

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_)
+  {}
 
-    /* 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

[PATCH 1/2] Make probe_ops::get_probes fill an std::vector

[Simon Marchi]

This patch changes one usage of VEC to std::vector.  It is a relatively
straightforward 1:1 change.  The implementations of
sym_probe_fns::sym_get_probes return a borrowed reference to their probe
vectors, meaning that the caller should not free it.  In the new code, I
made them return a const reference to the vector.

This patch and the following one were tested by the buildbot.  I didn't
see any failures that looked related to this one.

gdb/ChangeLog:

	* probe.h (struct probe_ops) <get_probes>: Change parameter from
	vec to std::vector.
	* probe.c (parse_probes_in_pspace): Update.
	(find_probes_in_objfile): Update.
	(find_probe_by_pc): Update.
	(collect_probes): Update.
	(probe_any_get_probes): Update.
	* symfile.h (struct sym_probe_fns) <sym_get_probes> Change
	return type to reference to std::vector.
	* dtrace-probe.c (dtrace_process_dof_probe): Change parameter to
	std::vector and update.
	(dtrace_process_dof): Likewise.
	(dtrace_get_probes): Likewise.
	* elfread.c (elf_get_probes): Change return type to std::vector,
	store an std::vector in bfd_data.
	(probe_key_free): Update to std::vector.
	* stap-probe.c (handle_stap_probe): Change parameter to
	std::vector and update.
	(stap_get_probes): Likewise.
	* symfile-debug.c (debug_sym_get_probes): Change return type to
	std::vector and update.
---
 gdb/dtrace-probe.c  |  9 +++++----
 gdb/elfread.c       | 27 ++++++++++-----------------
 gdb/probe.c         | 38 ++++++++++++++------------------------
 gdb/probe.h         |  2 +-
 gdb/stap-probe.c    | 10 +++++-----
 gdb/symfile-debug.c |  8 ++++----
 gdb/symfile.h       |  7 ++-----
 7 files changed, 41 insertions(+), 60 deletions(-)

diff --git a/gdb/dtrace-probe.c b/gdb/dtrace-probe.c
index c1a8cb0..f9209ec 100644
--- a/gdb/dtrace-probe.c
+++ b/gdb/dtrace-probe.c
@@ -313,7 +313,8 @@ struct dtrace_dof_probe
 
 static void
 dtrace_process_dof_probe (struct objfile *objfile,
-			  struct gdbarch *gdbarch, VEC (probe_p) **probesp,
+			  struct gdbarch *gdbarch,
+			  std::vector<probe *> *probesp,
 			  struct dtrace_dof_hdr *dof,
 			  struct dtrace_dof_probe *probe,
 			  struct dtrace_dof_provider *provider,
@@ -448,7 +449,7 @@ dtrace_process_dof_probe (struct objfile *objfile,
       ret->enablers = VEC_copy (dtrace_probe_enabler_s, enablers);
 
       /* Successfully created probe.  */
-      VEC_safe_push (probe_p, *probesp, (struct probe *) ret);
+      probesp->push_back ((struct probe *) ret);
     }
 
   do_cleanups (cleanup);
@@ -461,7 +462,7 @@ dtrace_process_dof_probe (struct objfile *objfile,
 
 static void
 dtrace_process_dof (asection *sect, struct objfile *objfile,
-		    VEC (probe_p) **probesp, struct dtrace_dof_hdr *dof)
+		    std::vector<probe *> *probesp, struct dtrace_dof_hdr *dof)
 {
   struct gdbarch *gdbarch = get_objfile_arch (objfile);
   struct dtrace_dof_sect *section;
@@ -620,7 +621,7 @@ dtrace_get_arg (struct dtrace_probe *probe, unsigned n,
 /* Implementation of the get_probes method.  */
 
 static void
-dtrace_get_probes (VEC (probe_p) **probesp, struct objfile *objfile)
+dtrace_get_probes (std::vector<probe *> *probesp, struct objfile *objfile)
 {
   bfd *abfd = objfile->obfd;
   asection *sect = NULL;
diff --git a/gdb/elfread.c b/gdb/elfread.c
index f3d4641..8a64865 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -1309,35 +1309,30 @@ elf_symfile_init (struct objfile *objfile)
 
 /* Implementation of `sym_get_probes', as documented in symfile.h.  */
 
-static VEC (probe_p) *
+static const std::vector<probe *> &
 elf_get_probes (struct objfile *objfile)
 {
-  VEC (probe_p) *probes_per_bfd;
+  std::vector<probe *> *probes_per_bfd;
 
   /* Have we parsed this objfile's probes already?  */
-  probes_per_bfd = (VEC (probe_p) *) bfd_data (objfile->obfd, probe_key);
+  probes_per_bfd = (std::vector<probe *> *) bfd_data (objfile->obfd, probe_key);
 
-  if (!probes_per_bfd)
+  if (probes_per_bfd == NULL)
     {
       int ix;
       const struct probe_ops *probe_ops;
+      probes_per_bfd = new std::vector<probe *>;
 
       /* Here we try to gather information about all types of probes from the
 	 objfile.  */
       for (ix = 0; VEC_iterate (probe_ops_cp, all_probe_ops, ix, probe_ops);
 	   ix++)
-	probe_ops->get_probes (&probes_per_bfd, objfile);
-
-      if (probes_per_bfd == NULL)
-	{
-	  VEC_reserve (probe_p, probes_per_bfd, 1);
-	  gdb_assert (probes_per_bfd != NULL);
-	}
+	probe_ops->get_probes (probes_per_bfd, objfile);
 
       set_bfd_data (objfile->obfd, probe_key, probes_per_bfd);
     }
 
-  return probes_per_bfd;
+  return *probes_per_bfd;
 }
 
 /* Helper function used to free the space allocated for storing SystemTap
@@ -1346,14 +1341,12 @@ elf_get_probes (struct objfile *objfile)
 static void
 probe_key_free (bfd *abfd, void *d)
 {
-  int ix;
-  VEC (probe_p) *probes = (VEC (probe_p) *) d;
-  struct probe *probe;
+  std::vector<probe *> *probes = (std::vector<probe *> *) d;
 
-  for (ix = 0; VEC_iterate (probe_p, probes, ix, probe); ix++)
+  for (struct probe *probe : *probes)
     probe->pops->destroy (probe);
 
-  VEC_free (probe_p, probes);
+  delete probes;
 }
 
 \f
diff --git a/gdb/probe.c b/gdb/probe.c
index 686e90e..2d68437 100644
--- a/gdb/probe.c
+++ b/gdb/probe.c
@@ -58,10 +58,6 @@ parse_probes_in_pspace (const struct probe_ops *probe_ops,
 
   ALL_PSPACE_OBJFILES (search_pspace, objfile)
     {
-      VEC (probe_p) *probes;
-      struct probe *probe;
-      int ix;
-
       if (!objfile->sf || !objfile->sf->sym_probe_fns)
 	continue;
 
@@ -71,9 +67,10 @@ parse_probes_in_pspace (const struct probe_ops *probe_ops,
 			   objfile_namestr) != 0)
 	continue;
 
-      probes = objfile->sf->sym_probe_fns->sym_get_probes (objfile);
+      const std::vector<probe *> &probes
+	= objfile->sf->sym_probe_fns->sym_get_probes (objfile);
 
-      for (ix = 0; VEC_iterate (probe_p, probes, ix, probe); ix++)
+      for (struct probe *probe : probes)
 	{
 	  if (probe_ops != &probe_ops_any && probe->pops != probe_ops)
 	    continue;
@@ -211,15 +208,14 @@ VEC (probe_p) *
 find_probes_in_objfile (struct objfile *objfile, const char *provider,
 			const char *name)
 {
-  VEC (probe_p) *probes, *result = NULL;
-  int ix;
-  struct probe *probe;
+  VEC (probe_p) *result = NULL;
 
   if (!objfile->sf || !objfile->sf->sym_probe_fns)
     return NULL;
 
-  probes = objfile->sf->sym_probe_fns->sym_get_probes (objfile);
-  for (ix = 0; VEC_iterate (probe_p, probes, ix, probe); ix++)
+  const std::vector<probe *> &probes
+    = objfile->sf->sym_probe_fns->sym_get_probes (objfile);
+  for (struct probe *probe : probes)
     {
       if (strcmp (probe->provider, provider) != 0)
 	continue;
@@ -246,17 +242,14 @@ find_probe_by_pc (CORE_ADDR pc)
 
   ALL_OBJFILES (objfile)
   {
-    VEC (probe_p) *probes;
-    int ix;
-    struct probe *probe;
-
     if (!objfile->sf || !objfile->sf->sym_probe_fns
 	|| objfile->sect_index_text == -1)
       continue;
 
     /* If this proves too inefficient, we can replace with a hash.  */
-    probes = objfile->sf->sym_probe_fns->sym_get_probes (objfile);
-    for (ix = 0; VEC_iterate (probe_p, probes, ix, probe); ix++)
+    const std::vector<probe *> &probes
+      = objfile->sf->sym_probe_fns->sym_get_probes (objfile);
+    for (struct probe *probe : probes)
       if (get_probe_address (probe, objfile) == pc)
 	{
 	  result.objfile = objfile;
@@ -294,10 +287,6 @@ collect_probes (char *objname, char *provider, char *probe_name,
 
   ALL_OBJFILES (objfile)
     {
-      VEC (probe_p) *probes;
-      struct probe *probe;
-      int ix;
-
       if (! objfile->sf || ! objfile->sf->sym_probe_fns)
 	continue;
 
@@ -307,9 +296,10 @@ collect_probes (char *objname, char *provider, char *probe_name,
 	    continue;
 	}
 
-      probes = objfile->sf->sym_probe_fns->sym_get_probes (objfile);
+      const std::vector<probe *> &probes
+	= objfile->sf->sym_probe_fns->sym_get_probes (objfile);
 
-      for (ix = 0; VEC_iterate (probe_p, probes, ix, probe); ix++)
+      for (struct probe *probe : probes)
 	{
 	  struct bound_probe bound;
 
@@ -907,7 +897,7 @@ probe_any_is_linespec (const char **linespecp)
 /* Dummy method used for `probe_ops_any'.  */
 
 static void
-probe_any_get_probes (VEC (probe_p) **probesp, struct objfile *objfile)
+probe_any_get_probes (std::vector<probe *> *probesp, struct objfile *objfile)
 {
   /* No probes can be provided by this dummy backend.  */
 }
diff --git a/gdb/probe.h b/gdb/probe.h
index db7f1d1..61e3031 100644
--- a/gdb/probe.h
+++ b/gdb/probe.h
@@ -64,7 +64,7 @@ struct probe_ops
 
     /* Function that should fill PROBES with known probes from OBJFILE.  */
 
-    void (*get_probes) (VEC (probe_p) **probes, struct objfile *objfile);
+    void (*get_probes) (std::vector<probe *> *probes, struct objfile *objfile);
 
     /* Compute the probe's relocated address.  OBJFILE is the objfile
        in which the probe originated.  */
diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
index c0cc662..032f796 100644
--- a/gdb/stap-probe.c
+++ b/gdb/stap-probe.c
@@ -1472,7 +1472,7 @@ stap_clear_semaphore (struct probe *probe_generic, struct objfile *objfile,
 
 static void
 handle_stap_probe (struct objfile *objfile, struct sdt_note *el,
-		   VEC (probe_p) **probesp, CORE_ADDR base)
+		   std::vector<probe *> *probesp, CORE_ADDR base)
 {
   bfd *abfd = objfile->obfd;
   int size = bfd_get_arch_size (abfd) / 8;
@@ -1543,7 +1543,7 @@ handle_stap_probe (struct objfile *objfile, struct sdt_note *el,
   ret->args_u.text = probe_args;
 
   /* Successfully created probe.  */
-  VEC_safe_push (probe_p, *probesp, (struct probe *) ret);
+  probesp->push_back ((struct probe *) ret);
 }
 
 /* Helper function which tries to find the base address of the SystemTap
@@ -1588,7 +1588,7 @@ get_stap_base_address (bfd *obfd, bfd_vma *base)
    SystemTap probes from OBJFILE.  */
 
 static void
-stap_get_probes (VEC (probe_p) **probesp, struct objfile *objfile)
+stap_get_probes (std::vector<probe *> *probesp, struct objfile *objfile)
 {
   /* If we are here, then this is the first time we are parsing the
      SystemTap probe's information.  We basically have to count how many
@@ -1597,7 +1597,7 @@ stap_get_probes (VEC (probe_p) **probesp, struct objfile *objfile)
   bfd *obfd = objfile->obfd;
   bfd_vma base;
   struct sdt_note *iter;
-  unsigned save_probesp_len = VEC_length (probe_p, *probesp);
+  unsigned save_probesp_len = probesp->size ();
 
   if (objfile->separate_debug_objfile_backlink != NULL)
     {
@@ -1628,7 +1628,7 @@ stap_get_probes (VEC (probe_p) **probesp, struct objfile *objfile)
       handle_stap_probe (objfile, iter, probesp, base);
     }
 
-  if (save_probesp_len == VEC_length (probe_p, *probesp))
+  if (save_probesp_len == probesp->size ())
     {
       /* If we are here, it means we have failed to parse every known
 	 probe.  */
diff --git a/gdb/symfile-debug.c b/gdb/symfile-debug.c
index e7890c9..32dafa8 100644
--- a/gdb/symfile-debug.c
+++ b/gdb/symfile-debug.c
@@ -384,20 +384,20 @@ static const struct quick_symbol_functions debug_sym_quick_functions =
 \f
 /* Debugging version of struct sym_probe_fns.  */
 
-static VEC (probe_p) *
+static const std::vector<probe *> &
 debug_sym_get_probes (struct objfile *objfile)
 {
   const struct debug_sym_fns_data *debug_data
     = ((const struct debug_sym_fns_data *)
        objfile_data (objfile, symfile_debug_objfile_data_key));
-  VEC (probe_p) *retval;
 
-  retval = debug_data->real_sf->sym_probe_fns->sym_get_probes (objfile);
+  const std::vector<probe *> &retval
+    = debug_data->real_sf->sym_probe_fns->sym_get_probes (objfile);
 
   fprintf_filtered (gdb_stdlog,
 		    "probes->sym_get_probes (%s) = %s\n",
 		    objfile_debug_name (objfile),
-		    host_address_to_string (retval));
+		    host_address_to_string (retval.data ()));
 
   return retval;
 }
diff --git a/gdb/symfile.h b/gdb/symfile.h
index bb47fdf..1f4460c 100644
--- a/gdb/symfile.h
+++ b/gdb/symfile.h
@@ -314,11 +314,8 @@ struct quick_symbol_functions
 
 struct sym_probe_fns
 {
-  /* If non-NULL, return an array of probe objects.
-
-     The returned value does not have to be freed and it has lifetime of the
-     OBJFILE.  */
-  VEC (probe_p) *(*sym_get_probes) (struct objfile *);
+  /* If non-NULL, return a reference to vector of probe objects.  */
+  const std::vector<probe *> &(*sym_get_probes) (struct objfile *);
 };
 
 /* Structure to keep track of symbol reading functions for various
-- 
2.7.4

Re: [PATCH 1/2] Make probe_ops::get_probes fill an std::vector

[Sergio Durigan Junior]

On Sunday, September 10 2017, Simon Marchi wrote:

> This patch changes one usage of VEC to std::vector.  It is a relatively
> straightforward 1:1 change.  The implementations of
> sym_probe_fns::sym_get_probes return a borrowed reference to their probe
> vectors, meaning that the caller should not free it.  In the new code, I
> made them return a const reference to the vector.
>
> This patch and the following one were tested by the buildbot.  I didn't
> see any failures that looked related to this one.

Thanks for the patch, Simon.  A few comments below.

> gdb/ChangeLog:
>
> 	* probe.h (struct probe_ops) <get_probes>: Change parameter from
> 	vec to std::vector.
> 	* probe.c (parse_probes_in_pspace): Update.
> 	(find_probes_in_objfile): Update.
> 	(find_probe_by_pc): Update.
> 	(collect_probes): Update.
> 	(probe_any_get_probes): Update.
> 	* symfile.h (struct sym_probe_fns) <sym_get_probes> Change
> 	return type to reference to std::vector.
> 	* dtrace-probe.c (dtrace_process_dof_probe): Change parameter to
> 	std::vector and update.
> 	(dtrace_process_dof): Likewise.
> 	(dtrace_get_probes): Likewise.
> 	* elfread.c (elf_get_probes): Change return type to std::vector,
> 	store an std::vector in bfd_data.
> 	(probe_key_free): Update to std::vector.
> 	* stap-probe.c (handle_stap_probe): Change parameter to
> 	std::vector and update.
> 	(stap_get_probes): Likewise.
> 	* symfile-debug.c (debug_sym_get_probes): Change return type to
> 	std::vector and update.
> ---
>  gdb/dtrace-probe.c  |  9 +++++----
>  gdb/elfread.c       | 27 ++++++++++-----------------
>  gdb/probe.c         | 38 ++++++++++++++------------------------
>  gdb/probe.h         |  2 +-
>  gdb/stap-probe.c    | 10 +++++-----
>  gdb/symfile-debug.c |  8 ++++----
>  gdb/symfile.h       |  7 ++-----
>  7 files changed, 41 insertions(+), 60 deletions(-)
>
> diff --git a/gdb/dtrace-probe.c b/gdb/dtrace-probe.c
> index c1a8cb0..f9209ec 100644
> --- a/gdb/dtrace-probe.c
> +++ b/gdb/dtrace-probe.c
> @@ -313,7 +313,8 @@ struct dtrace_dof_probe
>  
>  static void
>  dtrace_process_dof_probe (struct objfile *objfile,
> -			  struct gdbarch *gdbarch, VEC (probe_p) **probesp,
> +			  struct gdbarch *gdbarch,
> +			  std::vector<probe *> *probesp,

Since you're doing this, what do you think about const-fying these
occurrences of "std::vector<probe *>"?

>  			  struct dtrace_dof_hdr *dof,
>  			  struct dtrace_dof_probe *probe,
>  			  struct dtrace_dof_provider *provider,
> @@ -448,7 +449,7 @@ dtrace_process_dof_probe (struct objfile *objfile,
>        ret->enablers = VEC_copy (dtrace_probe_enabler_s, enablers);
>  
>        /* Successfully created probe.  */
> -      VEC_safe_push (probe_p, *probesp, (struct probe *) ret);
> +      probesp->push_back ((struct probe *) ret);
>      }
>  
>    do_cleanups (cleanup);
> @@ -461,7 +462,7 @@ dtrace_process_dof_probe (struct objfile *objfile,
>  
>  static void
>  dtrace_process_dof (asection *sect, struct objfile *objfile,
> -		    VEC (probe_p) **probesp, struct dtrace_dof_hdr *dof)
> +		    std::vector<probe *> *probesp, struct dtrace_dof_hdr *dof)
>  {
>    struct gdbarch *gdbarch = get_objfile_arch (objfile);
>    struct dtrace_dof_sect *section;
> @@ -620,7 +621,7 @@ dtrace_get_arg (struct dtrace_probe *probe, unsigned n,
>  /* Implementation of the get_probes method.  */
>  
>  static void
> -dtrace_get_probes (VEC (probe_p) **probesp, struct objfile *objfile)
> +dtrace_get_probes (std::vector<probe *> *probesp, struct objfile *objfile)
>  {
>    bfd *abfd = objfile->obfd;
>    asection *sect = NULL;
> diff --git a/gdb/elfread.c b/gdb/elfread.c
> index f3d4641..8a64865 100644
> --- a/gdb/elfread.c
> +++ b/gdb/elfread.c
> @@ -1309,35 +1309,30 @@ elf_symfile_init (struct objfile *objfile)
>  
>  /* Implementation of `sym_get_probes', as documented in symfile.h.  */
>  
> -static VEC (probe_p) *
> +static const std::vector<probe *> &
>  elf_get_probes (struct objfile *objfile)
>  {
> -  VEC (probe_p) *probes_per_bfd;
> +  std::vector<probe *> *probes_per_bfd;
>  
>    /* Have we parsed this objfile's probes already?  */
> -  probes_per_bfd = (VEC (probe_p) *) bfd_data (objfile->obfd, probe_key);
> +  probes_per_bfd = (std::vector<probe *> *) bfd_data (objfile->obfd, probe_key);
>  
> -  if (!probes_per_bfd)
> +  if (probes_per_bfd == NULL)
>      {
>        int ix;
>        const struct probe_ops *probe_ops;
> +      probes_per_bfd = new std::vector<probe *>;
>  
>        /* Here we try to gather information about all types of probes from the
>  	 objfile.  */
>        for (ix = 0; VEC_iterate (probe_ops_cp, all_probe_ops, ix, probe_ops);
>  	   ix++)
> -	probe_ops->get_probes (&probes_per_bfd, objfile);
> -
> -      if (probes_per_bfd == NULL)
> -	{
> -	  VEC_reserve (probe_p, probes_per_bfd, 1);
> -	  gdb_assert (probes_per_bfd != NULL);
> -	}
> +	probe_ops->get_probes (probes_per_bfd, objfile);
>  
>        set_bfd_data (objfile->obfd, probe_key, probes_per_bfd);
>      }
>  
> -  return probes_per_bfd;
> +  return *probes_per_bfd;
>  }
>  
>  /* Helper function used to free the space allocated for storing SystemTap
> @@ -1346,14 +1341,12 @@ elf_get_probes (struct objfile *objfile)
>  static void
>  probe_key_free (bfd *abfd, void *d)
>  {
> -  int ix;
> -  VEC (probe_p) *probes = (VEC (probe_p) *) d;
> -  struct probe *probe;
> +  std::vector<probe *> *probes = (std::vector<probe *> *) d;
>  
> -  for (ix = 0; VEC_iterate (probe_p, probes, ix, probe); ix++)
> +  for (struct probe *probe : *probes)
>      probe->pops->destroy (probe);
>  
> -  VEC_free (probe_p, probes);
> +  delete probes;
>  }
>  
>  \f
> diff --git a/gdb/probe.c b/gdb/probe.c
> index 686e90e..2d68437 100644
> --- a/gdb/probe.c
> +++ b/gdb/probe.c
> @@ -58,10 +58,6 @@ parse_probes_in_pspace (const struct probe_ops *probe_ops,
>  
>    ALL_PSPACE_OBJFILES (search_pspace, objfile)
>      {
> -      VEC (probe_p) *probes;
> -      struct probe *probe;
> -      int ix;
> -
>        if (!objfile->sf || !objfile->sf->sym_probe_fns)
>  	continue;
>  
> @@ -71,9 +67,10 @@ parse_probes_in_pspace (const struct probe_ops *probe_ops,
>  			   objfile_namestr) != 0)
>  	continue;
>  
> -      probes = objfile->sf->sym_probe_fns->sym_get_probes (objfile);
> +      const std::vector<probe *> &probes
> +	= objfile->sf->sym_probe_fns->sym_get_probes (objfile);
>  
> -      for (ix = 0; VEC_iterate (probe_p, probes, ix, probe); ix++)
> +      for (struct probe *probe : probes)
>  	{
>  	  if (probe_ops != &probe_ops_any && probe->pops != probe_ops)
>  	    continue;
> @@ -211,15 +208,14 @@ VEC (probe_p) *
>  find_probes_in_objfile (struct objfile *objfile, const char *provider,
>  			const char *name)

Any reason for not converting this function's return as well?

>  {
> -  VEC (probe_p) *probes, *result = NULL;
> -  int ix;
> -  struct probe *probe;
> +  VEC (probe_p) *result = NULL;
>  
>    if (!objfile->sf || !objfile->sf->sym_probe_fns)
>      return NULL;
>  
> -  probes = objfile->sf->sym_probe_fns->sym_get_probes (objfile);
> -  for (ix = 0; VEC_iterate (probe_p, probes, ix, probe); ix++)
> +  const std::vector<probe *> &probes
> +    = objfile->sf->sym_probe_fns->sym_get_probes (objfile);
> +  for (struct probe *probe : probes)
>      {
>        if (strcmp (probe->provider, provider) != 0)
>  	continue;
> @@ -246,17 +242,14 @@ find_probe_by_pc (CORE_ADDR pc)
>  
>    ALL_OBJFILES (objfile)
>    {
> -    VEC (probe_p) *probes;
> -    int ix;
> -    struct probe *probe;
> -
>      if (!objfile->sf || !objfile->sf->sym_probe_fns
>  	|| objfile->sect_index_text == -1)
>        continue;
>  
>      /* If this proves too inefficient, we can replace with a hash.  */
> -    probes = objfile->sf->sym_probe_fns->sym_get_probes (objfile);
> -    for (ix = 0; VEC_iterate (probe_p, probes, ix, probe); ix++)
> +    const std::vector<probe *> &probes
> +      = objfile->sf->sym_probe_fns->sym_get_probes (objfile);
> +    for (struct probe *probe : probes)
>        if (get_probe_address (probe, objfile) == pc)
>  	{
>  	  result.objfile = objfile;
> @@ -294,10 +287,6 @@ collect_probes (char *objname, char *provider, char *probe_name,
>  
>    ALL_OBJFILES (objfile)
>      {
> -      VEC (probe_p) *probes;
> -      struct probe *probe;
> -      int ix;
> -
>        if (! objfile->sf || ! objfile->sf->sym_probe_fns)
>  	continue;
>  
> @@ -307,9 +296,10 @@ collect_probes (char *objname, char *provider, char *probe_name,
>  	    continue;
>  	}
>  
> -      probes = objfile->sf->sym_probe_fns->sym_get_probes (objfile);
> +      const std::vector<probe *> &probes
> +	= objfile->sf->sym_probe_fns->sym_get_probes (objfile);
>  
> -      for (ix = 0; VEC_iterate (probe_p, probes, ix, probe); ix++)
> +      for (struct probe *probe : probes)
>  	{
>  	  struct bound_probe bound;
>  
> @@ -907,7 +897,7 @@ probe_any_is_linespec (const char **linespecp)
>  /* Dummy method used for `probe_ops_any'.  */
>  
>  static void
> -probe_any_get_probes (VEC (probe_p) **probesp, struct objfile *objfile)
> +probe_any_get_probes (std::vector<probe *> *probesp, struct objfile *objfile)
>  {
>    /* No probes can be provided by this dummy backend.  */
>  }
> diff --git a/gdb/probe.h b/gdb/probe.h
> index db7f1d1..61e3031 100644
> --- a/gdb/probe.h
> +++ b/gdb/probe.h
> @@ -64,7 +64,7 @@ struct probe_ops
>  
>      /* Function that should fill PROBES with known probes from OBJFILE.  */
>  
> -    void (*get_probes) (VEC (probe_p) **probes, struct objfile *objfile);
> +    void (*get_probes) (std::vector<probe *> *probes, struct objfile *objfile);
>  
>      /* Compute the probe's relocated address.  OBJFILE is the objfile
>         in which the probe originated.  */
> diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
> index c0cc662..032f796 100644
> --- a/gdb/stap-probe.c
> +++ b/gdb/stap-probe.c
> @@ -1472,7 +1472,7 @@ stap_clear_semaphore (struct probe *probe_generic, struct objfile *objfile,
>  
>  static void
>  handle_stap_probe (struct objfile *objfile, struct sdt_note *el,
> -		   VEC (probe_p) **probesp, CORE_ADDR base)
> +		   std::vector<probe *> *probesp, CORE_ADDR base)
>  {
>    bfd *abfd = objfile->obfd;
>    int size = bfd_get_arch_size (abfd) / 8;
> @@ -1543,7 +1543,7 @@ handle_stap_probe (struct objfile *objfile, struct sdt_note *el,
>    ret->args_u.text = probe_args;
>  
>    /* Successfully created probe.  */
> -  VEC_safe_push (probe_p, *probesp, (struct probe *) ret);
> +  probesp->push_back ((struct probe *) ret);
>  }
>  
>  /* Helper function which tries to find the base address of the SystemTap
> @@ -1588,7 +1588,7 @@ get_stap_base_address (bfd *obfd, bfd_vma *base)
>     SystemTap probes from OBJFILE.  */
>  
>  static void
> -stap_get_probes (VEC (probe_p) **probesp, struct objfile *objfile)
> +stap_get_probes (std::vector<probe *> *probesp, struct objfile *objfile)
>  {
>    /* If we are here, then this is the first time we are parsing the
>       SystemTap probe's information.  We basically have to count how many
> @@ -1597,7 +1597,7 @@ stap_get_probes (VEC (probe_p) **probesp, struct objfile *objfile)
>    bfd *obfd = objfile->obfd;
>    bfd_vma base;
>    struct sdt_note *iter;
> -  unsigned save_probesp_len = VEC_length (probe_p, *probesp);
> +  unsigned save_probesp_len = probesp->size ();
>  
>    if (objfile->separate_debug_objfile_backlink != NULL)
>      {
> @@ -1628,7 +1628,7 @@ stap_get_probes (VEC (probe_p) **probesp, struct objfile *objfile)
>        handle_stap_probe (objfile, iter, probesp, base);
>      }
>  
> -  if (save_probesp_len == VEC_length (probe_p, *probesp))
> +  if (save_probesp_len == probesp->size ())
>      {
>        /* If we are here, it means we have failed to parse every known
>  	 probe.  */
> diff --git a/gdb/symfile-debug.c b/gdb/symfile-debug.c
> index e7890c9..32dafa8 100644
> --- a/gdb/symfile-debug.c
> +++ b/gdb/symfile-debug.c
> @@ -384,20 +384,20 @@ static const struct quick_symbol_functions debug_sym_quick_functions =
>  \f
>  /* Debugging version of struct sym_probe_fns.  */
>  
> -static VEC (probe_p) *
> +static const std::vector<probe *> &
>  debug_sym_get_probes (struct objfile *objfile)
>  {
>    const struct debug_sym_fns_data *debug_data
>      = ((const struct debug_sym_fns_data *)
>         objfile_data (objfile, symfile_debug_objfile_data_key));
> -  VEC (probe_p) *retval;
>  
> -  retval = debug_data->real_sf->sym_probe_fns->sym_get_probes (objfile);
> +  const std::vector<probe *> &retval
> +    = debug_data->real_sf->sym_probe_fns->sym_get_probes (objfile);
>  
>    fprintf_filtered (gdb_stdlog,
>  		    "probes->sym_get_probes (%s) = %s\n",
>  		    objfile_debug_name (objfile),
> -		    host_address_to_string (retval));
> +		    host_address_to_string (retval.data ()));
>  
>    return retval;
>  }
> diff --git a/gdb/symfile.h b/gdb/symfile.h
> index bb47fdf..1f4460c 100644
> --- a/gdb/symfile.h
> +++ b/gdb/symfile.h
> @@ -314,11 +314,8 @@ struct quick_symbol_functions
>  
>  struct sym_probe_fns
>  {
> -  /* If non-NULL, return an array of probe objects.
> -
> -     The returned value does not have to be freed and it has lifetime of the
> -     OBJFILE.  */
> -  VEC (probe_p) *(*sym_get_probes) (struct objfile *);
> +  /* If non-NULL, return a reference to vector of probe objects.  */
> +  const std::vector<probe *> &(*sym_get_probes) (struct objfile *);
>  };
>  
>  /* Structure to keep track of symbol reading functions for various
> -- 
> 2.7.4

I know this patch is supposed to touch only probe_ops::get_probes, but
I'd love to see the whole VEC (probe_p) replaced (a few places on
breakpoint.c are also using it).  Well, maybe on another patch :-).

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/

Re: [PATCH 1/2] Make probe_ops::get_probes fill an std::vector

[Simon Marchi]

On 2017-09-10 19:40, Sergio Durigan Junior wrote:
> On Sunday, September 10 2017, Simon Marchi wrote:
> 
>> This patch changes one usage of VEC to std::vector.  It is a 
>> relatively
>> straightforward 1:1 change.  The implementations of
>> sym_probe_fns::sym_get_probes return a borrowed reference to their 
>> probe
>> vectors, meaning that the caller should not free it.  In the new code, 
>> I
>> made them return a const reference to the vector.
>> 
>> This patch and the following one were tested by the buildbot.  I 
>> didn't
>> see any failures that looked related to this one.
> 
> Thanks for the patch, Simon.  A few comments below.
> 
>> gdb/ChangeLog:
>> 
>> 	* probe.h (struct probe_ops) <get_probes>: Change parameter from
>> 	vec to std::vector.
>> 	* probe.c (parse_probes_in_pspace): Update.
>> 	(find_probes_in_objfile): Update.
>> 	(find_probe_by_pc): Update.
>> 	(collect_probes): Update.
>> 	(probe_any_get_probes): Update.
>> 	* symfile.h (struct sym_probe_fns) <sym_get_probes> Change
>> 	return type to reference to std::vector.
>> 	* dtrace-probe.c (dtrace_process_dof_probe): Change parameter to
>> 	std::vector and update.
>> 	(dtrace_process_dof): Likewise.
>> 	(dtrace_get_probes): Likewise.
>> 	* elfread.c (elf_get_probes): Change return type to std::vector,
>> 	store an std::vector in bfd_data.
>> 	(probe_key_free): Update to std::vector.
>> 	* stap-probe.c (handle_stap_probe): Change parameter to
>> 	std::vector and update.
>> 	(stap_get_probes): Likewise.
>> 	* symfile-debug.c (debug_sym_get_probes): Change return type to
>> 	std::vector and update.
>> ---
>>  gdb/dtrace-probe.c  |  9 +++++----
>>  gdb/elfread.c       | 27 ++++++++++-----------------
>>  gdb/probe.c         | 38 ++++++++++++++------------------------
>>  gdb/probe.h         |  2 +-
>>  gdb/stap-probe.c    | 10 +++++-----
>>  gdb/symfile-debug.c |  8 ++++----
>>  gdb/symfile.h       |  7 ++-----
>>  7 files changed, 41 insertions(+), 60 deletions(-)
>> 
>> diff --git a/gdb/dtrace-probe.c b/gdb/dtrace-probe.c
>> index c1a8cb0..f9209ec 100644
>> --- a/gdb/dtrace-probe.c
>> +++ b/gdb/dtrace-probe.c
>> @@ -313,7 +313,8 @@ struct dtrace_dof_probe
>> 
>>  static void
>>  dtrace_process_dof_probe (struct objfile *objfile,
>> -			  struct gdbarch *gdbarch, VEC (probe_p) **probesp,
>> +			  struct gdbarch *gdbarch,
>> +			  std::vector<probe *> *probesp,
> 
> Since you're doing this, what do you think about const-fying these
> occurrences of "std::vector<probe *>"?

The job of this function is actually to modify (append to) the vector, 
so I don't think it would make sense to make the vector const.  Or did I 
misunderstand what you meant?

>> @@ -71,9 +67,10 @@ parse_probes_in_pspace (const struct probe_ops 
>> *probe_ops,
>>  			   objfile_namestr) != 0)
>>  	continue;
>> 
>> -      probes = objfile->sf->sym_probe_fns->sym_get_probes (objfile);
>> +      const std::vector<probe *> &probes
>> +	= objfile->sf->sym_probe_fns->sym_get_probes (objfile);
>> 
>> -      for (ix = 0; VEC_iterate (probe_p, probes, ix, probe); ix++)
>> +      for (struct probe *probe : probes)
>>  	{
>>  	  if (probe_ops != &probe_ops_any && probe->pops != probe_ops)
>>  	    continue;
>> @@ -211,15 +208,14 @@ VEC (probe_p) *
>>  find_probes_in_objfile (struct objfile *objfile, const char 
>> *provider,
>>  			const char *name)
> 
> Any reason for not converting this function's return as well?

Yes: the return value of this function ends up assigned to, for example, 
breakpoint_objfile_data::longjmp_probes.  It would make sense to convert 
that longjmp_probes to the same std::vector type.  However, we then have 
to look at how breakpoint_objfile_data is allocated.  It's currently 
allocated with XOBNEW on the objfile obstack, so I'm not too sure how to 
handle this (I haven't really looked into it).  Since it was starting to 
be a bit too far-reaching, I preferred to take it as a separate step.

But now that we're talking about it, do you know what's the advantage of 
using obstacks?  It looks like we can just change that XOBNEW with a 
new, and put the corresponding delete in free_breakpoint_probes (which 
could probably get renamed to free_breakpoint_objfile_data).  Is there 
something I'm missing here?

> I know this patch is supposed to touch only probe_ops::get_probes, but
> I'd love to see the whole VEC (probe_p) replaced (a few places on
> breakpoint.c are also using it).  Well, maybe on another patch :-).

Indeed, I'll look into it as a follow-up.

Thanks for reviewing.

Simon

Re: [PATCH 1/2] Make probe_ops::get_probes fill an std::vector

[Sergio Durigan Junior]

On Sunday, September 10 2017, Simon Marchi wrote:

> On 2017-09-10 19:40, Sergio Durigan Junior wrote:
>> On Sunday, September 10 2017, Simon Marchi wrote:
>>
>>> This patch changes one usage of VEC to std::vector.  It is a
>>> relatively
>>> straightforward 1:1 change.  The implementations of
>>> sym_probe_fns::sym_get_probes return a borrowed reference to their
>>> probe
>>> vectors, meaning that the caller should not free it.  In the new
>>> code, I
>>> made them return a const reference to the vector.
>>>
>>> This patch and the following one were tested by the buildbot.  I
>>> didn't
>>> see any failures that looked related to this one.
>>
>> Thanks for the patch, Simon.  A few comments below.
>>
>>> gdb/ChangeLog:
>>>
>>> 	* probe.h (struct probe_ops) <get_probes>: Change parameter from
>>> 	vec to std::vector.
>>> 	* probe.c (parse_probes_in_pspace): Update.
>>> 	(find_probes_in_objfile): Update.
>>> 	(find_probe_by_pc): Update.
>>> 	(collect_probes): Update.
>>> 	(probe_any_get_probes): Update.
>>> 	* symfile.h (struct sym_probe_fns) <sym_get_probes> Change
>>> 	return type to reference to std::vector.
>>> 	* dtrace-probe.c (dtrace_process_dof_probe): Change parameter to
>>> 	std::vector and update.
>>> 	(dtrace_process_dof): Likewise.
>>> 	(dtrace_get_probes): Likewise.
>>> 	* elfread.c (elf_get_probes): Change return type to std::vector,
>>> 	store an std::vector in bfd_data.
>>> 	(probe_key_free): Update to std::vector.
>>> 	* stap-probe.c (handle_stap_probe): Change parameter to
>>> 	std::vector and update.
>>> 	(stap_get_probes): Likewise.
>>> 	* symfile-debug.c (debug_sym_get_probes): Change return type to
>>> 	std::vector and update.
>>> ---
>>>  gdb/dtrace-probe.c  |  9 +++++----
>>>  gdb/elfread.c       | 27 ++++++++++-----------------
>>>  gdb/probe.c         | 38 ++++++++++++++------------------------
>>>  gdb/probe.h         |  2 +-
>>>  gdb/stap-probe.c    | 10 +++++-----
>>>  gdb/symfile-debug.c |  8 ++++----
>>>  gdb/symfile.h       |  7 ++-----
>>>  7 files changed, 41 insertions(+), 60 deletions(-)
>>>
>>> diff --git a/gdb/dtrace-probe.c b/gdb/dtrace-probe.c
>>> index c1a8cb0..f9209ec 100644
>>> --- a/gdb/dtrace-probe.c
>>> +++ b/gdb/dtrace-probe.c
>>> @@ -313,7 +313,8 @@ struct dtrace_dof_probe
>>>
>>>  static void
>>>  dtrace_process_dof_probe (struct objfile *objfile,
>>> -			  struct gdbarch *gdbarch, VEC (probe_p) **probesp,
>>> +			  struct gdbarch *gdbarch,
>>> +			  std::vector<probe *> *probesp,
>>
>> Since you're doing this, what do you think about const-fying these
>> occurrences of "std::vector<probe *>"?
>
> The job of this function is actually to modify (append to) the vector,
> so I don't think it would make sense to make the vector const.  Or did
> I misunderstand what you meant?

What I was proposing was to make the pointer to the vector constant.
But you're right, I've read more about it and it makes sense to leave it
as is.  Thanks.

>>> @@ -71,9 +67,10 @@ parse_probes_in_pspace (const struct probe_ops
>>> *probe_ops,
>>>  			   objfile_namestr) != 0)
>>>  	continue;
>>>
>>> -      probes = objfile->sf->sym_probe_fns->sym_get_probes (objfile);
>>> +      const std::vector<probe *> &probes
>>> +	= objfile->sf->sym_probe_fns->sym_get_probes (objfile);
>>>
>>> -      for (ix = 0; VEC_iterate (probe_p, probes, ix, probe); ix++)
>>> +      for (struct probe *probe : probes)
>>>  	{
>>>  	  if (probe_ops != &probe_ops_any && probe->pops != probe_ops)
>>>  	    continue;
>>> @@ -211,15 +208,14 @@ VEC (probe_p) *
>>>  find_probes_in_objfile (struct objfile *objfile, const char
>>> *provider,
>>>  			const char *name)
>>
>> Any reason for not converting this function's return as well?
>
> Yes: the return value of this function ends up assigned to, for
> example, breakpoint_objfile_data::longjmp_probes.  It would make sense
> to convert that longjmp_probes to the same std::vector type.  However,
> we then have to look at how breakpoint_objfile_data is allocated.
> It's currently allocated with XOBNEW on the objfile obstack, so I'm
> not too sure how to handle this (I haven't really looked into it).
> Since it was starting to be a bit too far-reaching, I preferred to
> take it as a separate step.

Hm, I see.

> But now that we're talking about it, do you know what's the advantage
> of using obstacks?  It looks like we can just change that XOBNEW with
> a new, and put the corresponding delete in free_breakpoint_probes
> (which could probably get renamed to free_breakpoint_objfile_data).
> Is there something I'm missing here?

It doesn't seem there is any clear/direct advantage.  The commit that
added this specific code was discussed here:

  https://sourceware.org/ml/gdb-patches/2011-02/msg00054.html

But there isn't any mention about the use of obstacks.  I'd say you can
go ahead and replace it new/delete.

>> I know this patch is supposed to touch only probe_ops::get_probes, but
>> I'd love to see the whole VEC (probe_p) replaced (a few places on
>> breakpoint.c are also using it).  Well, maybe on another patch :-).
>
> Indeed, I'll look into it as a follow-up.

Thanks, this is fine by me then.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

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

[Sergio Durigan Junior]

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
http://sergiodj.net/

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

[Mark Wielaard]

Hi,

On Mon, 2017-09-11 at 20:17 -0400, Sergio Durigan Junior wrote:
> On Sunday, September 10 2017, Simon Marchi wrote:
> > 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?

It has been upgraded to gcc (GCC) 6.4.1 20170727 (Red Hat 6.4.1-1).
marist-fedora-s390x is currently running Fedora 25. It will be updated
in the future to Fedora 26 (which will bring an upgrade to GCC 7).

Cheers,

Mark

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

[Simon Marchi]

On 2017-09-12 11:56, Mark Wielaard wrote:
> Hi,
> 
> On Mon, 2017-09-11 at 20:17 -0400, Sergio Durigan Junior wrote:
>> On Sunday, September 10 2017, Simon Marchi wrote:
>> > 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?
> 
> It has been upgraded to gcc (GCC) 6.4.1 20170727 (Red Hat 6.4.1-1).
> marist-fedora-s390x is currently running Fedora 25. It will be updated
> in the future to Fedora 26 (which will bring an upgrade to GCC 7).
> 
> Cheers,
> 
> Mark

I have sent my patch to get tested on this builder, and it seems to 
build fine now.  Thanks to you both!

Simon

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

[Simon Marchi]

On 2017-09-12 02:17, Sergio Durigan Junior wrote:
>> 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?

Ok, I'll do that before pushing.

>> 
>> -    /* 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,

Thanks,

Simon

Re: [PATCH 1/2] Make probe_ops::get_probes fill an std::vector

[Simon Marchi]

On 2017-09-12 02:00, Sergio Durigan Junior wrote:
> On Sunday, September 10 2017, Simon Marchi wrote:
> 
>> On 2017-09-10 19:40, Sergio Durigan Junior wrote:
>>> On Sunday, September 10 2017, Simon Marchi wrote:
>>> 
>>>> This patch changes one usage of VEC to std::vector.  It is a
>>>> relatively
>>>> straightforward 1:1 change.  The implementations of
>>>> sym_probe_fns::sym_get_probes return a borrowed reference to their
>>>> probe
>>>> vectors, meaning that the caller should not free it.  In the new
>>>> code, I
>>>> made them return a const reference to the vector.
>>>> 
>>>> This patch and the following one were tested by the buildbot.  I
>>>> didn't
>>>> see any failures that looked related to this one.
>>> 
>>> Thanks for the patch, Simon.  A few comments below.
>>> 
>>>> gdb/ChangeLog:
>>>> 
>>>> 	* probe.h (struct probe_ops) <get_probes>: Change parameter from
>>>> 	vec to std::vector.
>>>> 	* probe.c (parse_probes_in_pspace): Update.
>>>> 	(find_probes_in_objfile): Update.
>>>> 	(find_probe_by_pc): Update.
>>>> 	(collect_probes): Update.
>>>> 	(probe_any_get_probes): Update.
>>>> 	* symfile.h (struct sym_probe_fns) <sym_get_probes> Change
>>>> 	return type to reference to std::vector.
>>>> 	* dtrace-probe.c (dtrace_process_dof_probe): Change parameter to
>>>> 	std::vector and update.
>>>> 	(dtrace_process_dof): Likewise.
>>>> 	(dtrace_get_probes): Likewise.
>>>> 	* elfread.c (elf_get_probes): Change return type to std::vector,
>>>> 	store an std::vector in bfd_data.
>>>> 	(probe_key_free): Update to std::vector.
>>>> 	* stap-probe.c (handle_stap_probe): Change parameter to
>>>> 	std::vector and update.
>>>> 	(stap_get_probes): Likewise.
>>>> 	* symfile-debug.c (debug_sym_get_probes): Change return type to
>>>> 	std::vector and update.
>>>> ---
>>>>  gdb/dtrace-probe.c  |  9 +++++----
>>>>  gdb/elfread.c       | 27 ++++++++++-----------------
>>>>  gdb/probe.c         | 38 ++++++++++++++------------------------
>>>>  gdb/probe.h         |  2 +-
>>>>  gdb/stap-probe.c    | 10 +++++-----
>>>>  gdb/symfile-debug.c |  8 ++++----
>>>>  gdb/symfile.h       |  7 ++-----
>>>>  7 files changed, 41 insertions(+), 60 deletions(-)
>>>> 
>>>> diff --git a/gdb/dtrace-probe.c b/gdb/dtrace-probe.c
>>>> index c1a8cb0..f9209ec 100644
>>>> --- a/gdb/dtrace-probe.c
>>>> +++ b/gdb/dtrace-probe.c
>>>> @@ -313,7 +313,8 @@ struct dtrace_dof_probe
>>>> 
>>>>  static void
>>>>  dtrace_process_dof_probe (struct objfile *objfile,
>>>> -			  struct gdbarch *gdbarch, VEC (probe_p) **probesp,
>>>> +			  struct gdbarch *gdbarch,
>>>> +			  std::vector<probe *> *probesp,
>>> 
>>> Since you're doing this, what do you think about const-fying these
>>> occurrences of "std::vector<probe *>"?
>> 
>> The job of this function is actually to modify (append to) the vector,
>> so I don't think it would make sense to make the vector const.  Or did
>> I misunderstand what you meant?
> 
> What I was proposing was to make the pointer to the vector constant.
> But you're right, I've read more about it and it makes sense to leave 
> it
> as is.  Thanks.
> 
>>>> @@ -71,9 +67,10 @@ parse_probes_in_pspace (const struct probe_ops
>>>> *probe_ops,
>>>>  			   objfile_namestr) != 0)
>>>>  	continue;
>>>> 
>>>> -      probes = objfile->sf->sym_probe_fns->sym_get_probes 
>>>> (objfile);
>>>> +      const std::vector<probe *> &probes
>>>> +	= objfile->sf->sym_probe_fns->sym_get_probes (objfile);
>>>> 
>>>> -      for (ix = 0; VEC_iterate (probe_p, probes, ix, probe); ix++)
>>>> +      for (struct probe *probe : probes)
>>>>  	{
>>>>  	  if (probe_ops != &probe_ops_any && probe->pops != probe_ops)
>>>>  	    continue;
>>>> @@ -211,15 +208,14 @@ VEC (probe_p) *
>>>>  find_probes_in_objfile (struct objfile *objfile, const char
>>>> *provider,
>>>>  			const char *name)
>>> 
>>> Any reason for not converting this function's return as well?
>> 
>> Yes: the return value of this function ends up assigned to, for
>> example, breakpoint_objfile_data::longjmp_probes.  It would make sense
>> to convert that longjmp_probes to the same std::vector type.  However,
>> we then have to look at how breakpoint_objfile_data is allocated.
>> It's currently allocated with XOBNEW on the objfile obstack, so I'm
>> not too sure how to handle this (I haven't really looked into it).
>> Since it was starting to be a bit too far-reaching, I preferred to
>> take it as a separate step.
> 
> Hm, I see.
> 
>> But now that we're talking about it, do you know what's the advantage
>> of using obstacks?  It looks like we can just change that XOBNEW with
>> a new, and put the corresponding delete in free_breakpoint_probes
>> (which could probably get renamed to free_breakpoint_objfile_data).
>> Is there something I'm missing here?
> 
> It doesn't seem there is any clear/direct advantage.  The commit that
> added this specific code was discussed here:
> 
>   https://sourceware.org/ml/gdb-patches/2011-02/msg00054.html
> 
> But there isn't any mention about the use of obstacks.  I'd say you can
> go ahead and replace it new/delete.
> 
>>> I know this patch is supposed to touch only probe_ops::get_probes, 
>>> but
>>> I'd love to see the whole VEC (probe_p) replaced (a few places on
>>> breakpoint.c are also using it).  Well, maybe on another patch :-).
>> 
>> Indeed, I'll look into it as a follow-up.
> 
> Thanks, this is fine by me then.

Thanks, both patches are now pushed.

Simon