[RFA] Remove VEC from breakpoint

[RFA] Remove VEC from breakpoint

[Tom Tromey]

This removes a use of VEC from breakpoint.h, also removing the
now-unnecessary breakpoint_p typedef.

This patch fixes a latent memory leak in
find_matching_tracepoint_location, which neglected to free the vector
returned by all_tracepoints.

Tested by the buildbot.

gdb/ChangeLog
2018-06-05  Tom Tromey  <tom@tromey.com>

	* tracepoint.c (process_tracepoint_on_disconnect, start_tracing)
	(stop_tracing, tstatus_command)
	(find_matching_tracepoint_location, merge_uploaded_tracepoints)
	(print_one_static_tracepoint_marker): Update.
	* breakpoint.c (static_tracepoints_here, all_tracepoints): Return
	std::vector.
	* breakpoint.h (breakpoint_p): Remove typedef.  Don't declare
	VEC.
	(all_tracepoints, static_tracepoints_here): Return std::vector.
---
 gdb/ChangeLog    | 12 +++++++++
 gdb/breakpoint.c | 12 ++++-----
 gdb/breakpoint.h | 13 +++------
 gdb/tracepoint.c | 81 +++++++++++++++-----------------------------------------
 4 files changed, 44 insertions(+), 74 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 00c538e989e..64925ff719a 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1145,11 +1145,11 @@ validate_commands_for_breakpoint (struct breakpoint *b,
 /* Return a vector of all the static tracepoints set at ADDR.  The
    caller is responsible for releasing the vector.  */
 
-VEC(breakpoint_p) *
+std::vector<breakpoint *>
 static_tracepoints_here (CORE_ADDR addr)
 {
   struct breakpoint *b;
-  VEC(breakpoint_p) *found = 0;
+  std::vector<breakpoint *> found;
   struct bp_location *loc;
 
   ALL_BREAKPOINTS (b)
@@ -1157,7 +1157,7 @@ static_tracepoints_here (CORE_ADDR addr)
       {
 	for (loc = b->loc; loc; loc = loc->next)
 	  if (loc->address == addr)
-	    VEC_safe_push(breakpoint_p, found, b);
+	    found.push_back (b);
       }
 
   return found;
@@ -15164,15 +15164,15 @@ save_tracepoints_command (const char *args, int from_tty)
 
 /* Create a vector of all tracepoints.  */
 
-VEC(breakpoint_p) *
+std::vector<breakpoint *>
 all_tracepoints (void)
 {
-  VEC(breakpoint_p) *tp_vec = 0;
+  std::vector<breakpoint *> tp_vec;
   struct breakpoint *tp;
 
   ALL_TRACEPOINTS (tp)
   {
-    VEC_safe_push (breakpoint_p, tp_vec, tp);
+    tp_vec.push_back (tp);
   }
 
   return tp_vec;
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 4223158fbc0..74db377a090 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -903,8 +903,6 @@ struct tracepoint : public breakpoint
   int static_trace_marker_id_idx;
 };
 
-typedef struct breakpoint *breakpoint_p;
-DEF_VEC_P(breakpoint_p);
 \f
 /* The following stuff is an abstract data type "bpstat" ("breakpoint
    status").  This provides the ability to determine whether we have
@@ -1625,16 +1623,13 @@ extern struct tracepoint *
   get_tracepoint_by_number (const char **arg,
 			    number_or_range_parser *parser);
 
-/* Return a vector of all tracepoints currently defined.  The vector
-   is newly allocated; the caller should free when done with it.  */
-extern VEC(breakpoint_p) *all_tracepoints (void);
+/* Return a vector of all tracepoints currently defined.  */
+extern std::vector<breakpoint *> all_tracepoints (void);
 
 extern int is_tracepoint (const struct breakpoint *b);
 
-/* Return a vector of all static tracepoints defined at ADDR.  The
-   vector is newly allocated; the caller should free when done with
-   it.  */
-extern VEC(breakpoint_p) *static_tracepoints_here (CORE_ADDR addr);
+/* Return a vector of all static tracepoints defined at ADDR.  */
+extern std::vector<breakpoint *> static_tracepoints_here (CORE_ADDR addr);
 
 /* Create an instance of this to start registering breakpoint numbers
    for a later "commands" command.  */
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index d99d663895f..63c25499554 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -1495,15 +1495,11 @@ collection_list::add_aexpr (agent_expr_up aexpr)
 static void
 process_tracepoint_on_disconnect (void)
 {
-  VEC(breakpoint_p) *tp_vec = NULL;
-  int ix;
-  struct breakpoint *b;
   int has_pending_p = 0;
 
   /* Check whether we still have pending tracepoint.  If we have, warn the
      user that pending tracepoint will no longer work.  */
-  tp_vec = all_tracepoints ();
-  for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, b); ix++)
+  for (breakpoint *b : all_tracepoints ())
     {
       if (b->loc == NULL)
 	{
@@ -1527,7 +1523,6 @@ process_tracepoint_on_disconnect (void)
 	    break;
 	}
     }
-  VEC_free (breakpoint_p, tp_vec);
 
   if (has_pending_p)
     warning (_("Pending tracepoints will not be resolved while"
@@ -1548,23 +1543,17 @@ trace_reset_local_state (void)
 void
 start_tracing (const char *notes)
 {
-  VEC(breakpoint_p) *tp_vec = NULL;
-  int ix;
-  struct breakpoint *b;
   struct trace_state_variable *tsv;
   int any_enabled = 0, num_to_download = 0;
   int ret;
 
-  tp_vec = all_tracepoints ();
+  std::vector<breakpoint *> tp_vec = all_tracepoints ();
 
   /* No point in tracing without any tracepoints...  */
-  if (VEC_length (breakpoint_p, tp_vec) == 0)
-    {
-      VEC_free (breakpoint_p, tp_vec);
-      error (_("No tracepoints defined, not starting trace"));
-    }
+  if (tp_vec.empty ())
+    error (_("No tracepoints defined, not starting trace"));
 
-  for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, b); ix++)
+  for (breakpoint *b : tp_vec)
     {
       if (b->enable_state == bp_enabled)
 	any_enabled = 1;
@@ -1586,20 +1575,16 @@ start_tracing (const char *notes)
 	{
 	  /* No point in tracing with only disabled tracepoints that
 	     cannot be re-enabled.  */
-	  VEC_free (breakpoint_p, tp_vec);
 	  error (_("No tracepoints enabled, not starting trace"));
 	}
     }
 
   if (num_to_download <= 0)
-    {
-      VEC_free (breakpoint_p, tp_vec);
-      error (_("No tracepoints that may be downloaded, not starting trace"));
-    }
+    error (_("No tracepoints that may be downloaded, not starting trace"));
 
   target_trace_init ();
 
-  for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, b); ix++)
+  for (breakpoint *b : tp_vec)
     {
       struct tracepoint *t = (struct tracepoint *) b;
       struct bp_location *loc;
@@ -1638,7 +1623,6 @@ start_tracing (const char *notes)
       if (bp_location_downloaded)
 	gdb::observers::breakpoint_modified.notify (b);
     }
-  VEC_free (breakpoint_p, tp_vec);
 
   /* Send down all the trace state variables too.  */
   for (const trace_state_variable &tsv : tvariables)
@@ -1705,14 +1689,10 @@ void
 stop_tracing (const char *note)
 {
   int ret;
-  VEC(breakpoint_p) *tp_vec = NULL;
-  int ix;
-  struct breakpoint *t;
 
   target_trace_stop ();
 
-  tp_vec = all_tracepoints ();
-  for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, t); ix++)
+  for (breakpoint *t : all_tracepoints ())
     {
       struct bp_location *loc;
 
@@ -1733,8 +1713,6 @@ stop_tracing (const char *note)
 	}
     }
 
-  VEC_free (breakpoint_p, tp_vec);
-
   if (!note)
     note = trace_stop_notes;
   ret = target_set_trace_notes (NULL, NULL, note);
@@ -1751,9 +1729,7 @@ static void
 tstatus_command (const char *args, int from_tty)
 {
   struct trace_status *ts = current_trace_status ();
-  int status, ix;
-  VEC(breakpoint_p) *tp_vec = NULL;
-  struct breakpoint *t;
+  int status;
   
   status = target_get_trace_status (ts);
 
@@ -1896,12 +1872,8 @@ tstatus_command (const char *args, int from_tty)
 		     (long int) (ts->stop_time % 1000000));
 
   /* Now report any per-tracepoint status available.  */
-  tp_vec = all_tracepoints ();
-
-  for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, t); ix++)
+  for (breakpoint *t : all_tracepoints ())
     target_get_tracepoint_status (t, NULL);
-
-  VEC_free (breakpoint_p, tp_vec);
 }
 
 /* Report the trace status to uiout, in a way suitable for MI, and not
@@ -3058,12 +3030,9 @@ cond_string_is_same (char *str1, char *str2)
 static struct bp_location *
 find_matching_tracepoint_location (struct uploaded_tp *utp)
 {
-  VEC(breakpoint_p) *tp_vec = all_tracepoints ();
-  int ix;
-  struct breakpoint *b;
   struct bp_location *loc;
 
-  for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, b); ix++)
+  for (breakpoint *b : all_tracepoints ())
     {
       struct tracepoint *t = (struct tracepoint *) b;
 
@@ -3094,9 +3063,8 @@ merge_uploaded_tracepoints (struct uploaded_tp **uploaded_tps)
 {
   struct uploaded_tp *utp;
   /* A set of tracepoints which are modified.  */
-  VEC(breakpoint_p) *modified_tp = NULL;
+  std::vector<breakpoint *> modified_tp;
   int ix;
-  struct breakpoint *b;
 
   /* Look for GDB tracepoints that match up with our uploaded versions.  */
   for (utp = *uploaded_tps; utp; utp = utp->next)
@@ -3122,16 +3090,14 @@ merge_uploaded_tracepoints (struct uploaded_tp **uploaded_tps)
 	     MODIFIED_TP if not there yet.  The 'breakpoint-modified'
 	     observers will be notified later once for each tracepoint
 	     saved in MODIFIED_TP.  */
-	  for (ix = 0;
-	       VEC_iterate (breakpoint_p, modified_tp, ix, b);
-	       ix++)
+	  for (breakpoint *b : modified_tp)
 	    if (b == loc->owner)
 	      {
 		found = 1;
 		break;
 	      }
 	  if (!found)
-	    VEC_safe_push (breakpoint_p, modified_tp, loc->owner);
+	    modified_tp.push_back (loc->owner);
 	}
       else
 	{
@@ -3156,10 +3122,9 @@ merge_uploaded_tracepoints (struct uploaded_tp **uploaded_tps)
 
   /* Notify 'breakpoint-modified' observer that at least one of B's
      locations was changed.  */
-  for (ix = 0; VEC_iterate (breakpoint_p, modified_tp, ix, b); ix++)
+  for (breakpoint *b : modified_tp)
     gdb::observers::breakpoint_modified.notify (b);
 
-  VEC_free (breakpoint_p, modified_tp);
   free_uploaded_tps (uploaded_tps);
 }
 
@@ -3643,12 +3608,12 @@ print_one_static_tracepoint_marker (int count,
   char wrap_indent[80];
   char extra_field_indent[80];
   struct ui_out *uiout = current_uiout;
-  VEC(breakpoint_p) *tracepoints;
 
   symtab_and_line sal;
   sal.pc = marker.address;
 
-  tracepoints = static_tracepoints_here (marker.address);
+  std::vector<breakpoint *> tracepoints
+    = static_tracepoints_here (marker.address);
 
   ui_out_emit_tuple tuple_emitter (uiout, "marker");
 
@@ -3659,7 +3624,7 @@ print_one_static_tracepoint_marker (int count,
   uiout->field_string ("marker-id", marker.str_id.c_str ());
 
   uiout->field_fmt ("enabled", "%c",
-		    !VEC_empty (breakpoint_p, tracepoints) ? 'y' : 'n');
+		    !tracepoints.empty () ? 'y' : 'n');
   uiout->spaces (2);
 
   strcpy (wrap_indent, "                                   ");
@@ -3715,7 +3680,7 @@ print_one_static_tracepoint_marker (int count,
   uiout->field_string ("extra-data", marker.extra.c_str ());
   uiout->text ("\"\n");
 
-  if (!VEC_empty (breakpoint_p, tracepoints))
+  if (!tracepoints.empty ())
     {
       int ix;
       struct breakpoint *b;
@@ -3725,22 +3690,20 @@ print_one_static_tracepoint_marker (int count,
 
 	uiout->text (extra_field_indent);
 	uiout->text (_("Probed by static tracepoints: "));
-	for (ix = 0; VEC_iterate(breakpoint_p, tracepoints, ix, b); ix++)
+	for (ix = 0; ix < tracepoints.size (); ix++)
 	  {
 	    if (ix > 0)
 	      uiout->text (", ");
 	    uiout->text ("#");
-	    uiout->field_int ("tracepoint-id", b->number);
+	    uiout->field_int ("tracepoint-id", tracepoints[ix]->number);
 	  }
       }
 
       if (uiout->is_mi_like_p ())
-	uiout->field_int ("number-of-tracepoints",
-			  VEC_length(breakpoint_p, tracepoints));
+	uiout->field_int ("number-of-tracepoints", tracepoints.size ());
       else
 	uiout->text ("\n");
     }
-  VEC_free (breakpoint_p, tracepoints);
 }
 
 static void
-- 
2.13.6

Re: [RFA] Remove VEC from breakpoint

[Andrew Burgess]

* Tom Tromey <tom@tromey.com> [2018-06-05 13:23:16 -0600]:

> This removes a use of VEC from breakpoint.h, also removing the
> now-unnecessary breakpoint_p typedef.
> 
> This patch fixes a latent memory leak in
> find_matching_tracepoint_location, which neglected to free the vector
> returned by all_tracepoints.
> 
> Tested by the buildbot.
> 
> gdb/ChangeLog
> 2018-06-05  Tom Tromey  <tom@tromey.com>
> 
> 	* tracepoint.c (process_tracepoint_on_disconnect, start_tracing)
> 	(stop_tracing, tstatus_command)
> 	(find_matching_tracepoint_location, merge_uploaded_tracepoints)
> 	(print_one_static_tracepoint_marker): Update.
> 	* breakpoint.c (static_tracepoints_here, all_tracepoints): Return
> 	std::vector.
> 	* breakpoint.h (breakpoint_p): Remove typedef.  Don't declare
> 	VEC.
> 	(all_tracepoints, static_tracepoints_here): Return std::vector.

FWIW this looks fine to me, except for one nit below...

> ---
>  gdb/ChangeLog    | 12 +++++++++
>  gdb/breakpoint.c | 12 ++++-----
>  gdb/breakpoint.h | 13 +++------
>  gdb/tracepoint.c | 81 +++++++++++++++-----------------------------------------
>  4 files changed, 44 insertions(+), 74 deletions(-)
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 00c538e989e..64925ff719a 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -1145,11 +1145,11 @@ validate_commands_for_breakpoint (struct breakpoint *b,
>  /* Return a vector of all the static tracepoints set at ADDR.  The
>     caller is responsible for releasing the vector.  */
>  
> -VEC(breakpoint_p) *
> +std::vector<breakpoint *>
>  static_tracepoints_here (CORE_ADDR addr)
>  {
>    struct breakpoint *b;
> -  VEC(breakpoint_p) *found = 0;
> +  std::vector<breakpoint *> found;
>    struct bp_location *loc;
>  
>    ALL_BREAKPOINTS (b)
> @@ -1157,7 +1157,7 @@ static_tracepoints_here (CORE_ADDR addr)
>        {
>  	for (loc = b->loc; loc; loc = loc->next)
>  	  if (loc->address == addr)
> -	    VEC_safe_push(breakpoint_p, found, b);
> +	    found.push_back (b);
>        }
>  
>    return found;
> @@ -15164,15 +15164,15 @@ save_tracepoints_command (const char *args, int from_tty)
>  
>  /* Create a vector of all tracepoints.  */
>  
> -VEC(breakpoint_p) *
> +std::vector<breakpoint *>
>  all_tracepoints (void)
>  {
> -  VEC(breakpoint_p) *tp_vec = 0;
> +  std::vector<breakpoint *> tp_vec;
>    struct breakpoint *tp;
>  
>    ALL_TRACEPOINTS (tp)
>    {
> -    VEC_safe_push (breakpoint_p, tp_vec, tp);
> +    tp_vec.push_back (tp);
>    }
>  
>    return tp_vec;
> diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
> index 4223158fbc0..74db377a090 100644
> --- a/gdb/breakpoint.h
> +++ b/gdb/breakpoint.h
> @@ -903,8 +903,6 @@ struct tracepoint : public breakpoint
>    int static_trace_marker_id_idx;
>  };
>  
> -typedef struct breakpoint *breakpoint_p;
> -DEF_VEC_P(breakpoint_p);
>  \f
>  /* The following stuff is an abstract data type "bpstat" ("breakpoint
>     status").  This provides the ability to determine whether we have
> @@ -1625,16 +1623,13 @@ extern struct tracepoint *
>    get_tracepoint_by_number (const char **arg,
>  			    number_or_range_parser *parser);
>  
> -/* Return a vector of all tracepoints currently defined.  The vector
> -   is newly allocated; the caller should free when done with it.  */
> -extern VEC(breakpoint_p) *all_tracepoints (void);
> +/* Return a vector of all tracepoints currently defined.  */
> +extern std::vector<breakpoint *> all_tracepoints (void);
>  
>  extern int is_tracepoint (const struct breakpoint *b);
>  
> -/* Return a vector of all static tracepoints defined at ADDR.  The
> -   vector is newly allocated; the caller should free when done with
> -   it.  */
> -extern VEC(breakpoint_p) *static_tracepoints_here (CORE_ADDR addr);
> +/* Return a vector of all static tracepoints defined at ADDR.  */
> +extern std::vector<breakpoint *> static_tracepoints_here (CORE_ADDR addr);
>  
>  /* Create an instance of this to start registering breakpoint numbers
>     for a later "commands" command.  */
> diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
> index d99d663895f..63c25499554 100644
> --- a/gdb/tracepoint.c
> +++ b/gdb/tracepoint.c
> @@ -1495,15 +1495,11 @@ collection_list::add_aexpr (agent_expr_up aexpr)
>  static void
>  process_tracepoint_on_disconnect (void)
>  {
> -  VEC(breakpoint_p) *tp_vec = NULL;
> -  int ix;
> -  struct breakpoint *b;
>    int has_pending_p = 0;
>  
>    /* Check whether we still have pending tracepoint.  If we have, warn the
>       user that pending tracepoint will no longer work.  */
> -  tp_vec = all_tracepoints ();
> -  for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, b); ix++)
> +  for (breakpoint *b : all_tracepoints ())
>      {
>        if (b->loc == NULL)
>  	{
> @@ -1527,7 +1523,6 @@ process_tracepoint_on_disconnect (void)
>  	    break;
>  	}
>      }
> -  VEC_free (breakpoint_p, tp_vec);
>  
>    if (has_pending_p)
>      warning (_("Pending tracepoints will not be resolved while"
> @@ -1548,23 +1543,17 @@ trace_reset_local_state (void)
>  void
>  start_tracing (const char *notes)
>  {
> -  VEC(breakpoint_p) *tp_vec = NULL;
> -  int ix;
> -  struct breakpoint *b;
>    struct trace_state_variable *tsv;
>    int any_enabled = 0, num_to_download = 0;
>    int ret;
>  
> -  tp_vec = all_tracepoints ();
> +  std::vector<breakpoint *> tp_vec = all_tracepoints ();
>  
>    /* No point in tracing without any tracepoints...  */
> -  if (VEC_length (breakpoint_p, tp_vec) == 0)
> -    {
> -      VEC_free (breakpoint_p, tp_vec);
> -      error (_("No tracepoints defined, not starting trace"));
> -    }
> +  if (tp_vec.empty ())
> +    error (_("No tracepoints defined, not starting trace"));
>  
> -  for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, b); ix++)
> +  for (breakpoint *b : tp_vec)
>      {
>        if (b->enable_state == bp_enabled)
>  	any_enabled = 1;
> @@ -1586,20 +1575,16 @@ start_tracing (const char *notes)
>  	{
>  	  /* No point in tracing with only disabled tracepoints that
>  	     cannot be re-enabled.  */
> -	  VEC_free (breakpoint_p, tp_vec);
>  	  error (_("No tracepoints enabled, not starting trace"));
>  	}
>      }
>  
>    if (num_to_download <= 0)
> -    {
> -      VEC_free (breakpoint_p, tp_vec);
> -      error (_("No tracepoints that may be downloaded, not starting trace"));
> -    }
> +    error (_("No tracepoints that may be downloaded, not starting trace"));
>  
>    target_trace_init ();
>  
> -  for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, b); ix++)
> +  for (breakpoint *b : tp_vec)
>      {
>        struct tracepoint *t = (struct tracepoint *) b;
>        struct bp_location *loc;
> @@ -1638,7 +1623,6 @@ start_tracing (const char *notes)
>        if (bp_location_downloaded)
>  	gdb::observers::breakpoint_modified.notify (b);
>      }
> -  VEC_free (breakpoint_p, tp_vec);
>  
>    /* Send down all the trace state variables too.  */
>    for (const trace_state_variable &tsv : tvariables)
> @@ -1705,14 +1689,10 @@ void
>  stop_tracing (const char *note)
>  {
>    int ret;
> -  VEC(breakpoint_p) *tp_vec = NULL;
> -  int ix;
> -  struct breakpoint *t;
>  
>    target_trace_stop ();
>  
> -  tp_vec = all_tracepoints ();
> -  for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, t); ix++)
> +  for (breakpoint *t : all_tracepoints ())
>      {
>        struct bp_location *loc;
>  
> @@ -1733,8 +1713,6 @@ stop_tracing (const char *note)
>  	}
>      }
>  
> -  VEC_free (breakpoint_p, tp_vec);
> -
>    if (!note)
>      note = trace_stop_notes;
>    ret = target_set_trace_notes (NULL, NULL, note);
> @@ -1751,9 +1729,7 @@ static void
>  tstatus_command (const char *args, int from_tty)
>  {
>    struct trace_status *ts = current_trace_status ();
> -  int status, ix;
> -  VEC(breakpoint_p) *tp_vec = NULL;
> -  struct breakpoint *t;
> +  int status;
>    
>    status = target_get_trace_status (ts);
>  
> @@ -1896,12 +1872,8 @@ tstatus_command (const char *args, int from_tty)
>  		     (long int) (ts->stop_time % 1000000));
>  
>    /* Now report any per-tracepoint status available.  */
> -  tp_vec = all_tracepoints ();
> -
> -  for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, t); ix++)
> +  for (breakpoint *t : all_tracepoints ())
>      target_get_tracepoint_status (t, NULL);
> -
> -  VEC_free (breakpoint_p, tp_vec);
>  }
>  
>  /* Report the trace status to uiout, in a way suitable for MI, and not
> @@ -3058,12 +3030,9 @@ cond_string_is_same (char *str1, char *str2)
>  static struct bp_location *
>  find_matching_tracepoint_location (struct uploaded_tp *utp)
>  {
> -  VEC(breakpoint_p) *tp_vec = all_tracepoints ();
> -  int ix;
> -  struct breakpoint *b;
>    struct bp_location *loc;
>  
> -  for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, b); ix++)
> +  for (breakpoint *b : all_tracepoints ())
>      {
>        struct tracepoint *t = (struct tracepoint *) b;
>  
> @@ -3094,9 +3063,8 @@ merge_uploaded_tracepoints (struct uploaded_tp **uploaded_tps)
>  {
>    struct uploaded_tp *utp;
>    /* A set of tracepoints which are modified.  */
> -  VEC(breakpoint_p) *modified_tp = NULL;
> +  std::vector<breakpoint *> modified_tp;
>    int ix;
> -  struct breakpoint *b;

The 'ix' is no longer used and could be deleted.

While I was double checking this I also spotted another 'ix' in
merge_uploaded_trace_state_variables that's not used... :)

Thanks,
Andrew



>  
>    /* Look for GDB tracepoints that match up with our uploaded versions.  */
>    for (utp = *uploaded_tps; utp; utp = utp->next)
> @@ -3122,16 +3090,14 @@ merge_uploaded_tracepoints (struct uploaded_tp **uploaded_tps)
>  	     MODIFIED_TP if not there yet.  The 'breakpoint-modified'
>  	     observers will be notified later once for each tracepoint
>  	     saved in MODIFIED_TP.  */
> -	  for (ix = 0;
> -	       VEC_iterate (breakpoint_p, modified_tp, ix, b);
> -	       ix++)
> +	  for (breakpoint *b : modified_tp)
>  	    if (b == loc->owner)
>  	      {
>  		found = 1;
>  		break;
>  	      }
>  	  if (!found)
> -	    VEC_safe_push (breakpoint_p, modified_tp, loc->owner);
> +	    modified_tp.push_back (loc->owner);
>  	}
>        else
>  	{
> @@ -3156,10 +3122,9 @@ merge_uploaded_tracepoints (struct uploaded_tp **uploaded_tps)
>  
>    /* Notify 'breakpoint-modified' observer that at least one of B's
>       locations was changed.  */
> -  for (ix = 0; VEC_iterate (breakpoint_p, modified_tp, ix, b); ix++)
> +  for (breakpoint *b : modified_tp)
>      gdb::observers::breakpoint_modified.notify (b);
>  
> -  VEC_free (breakpoint_p, modified_tp);
>    free_uploaded_tps (uploaded_tps);
>  }
>  
> @@ -3643,12 +3608,12 @@ print_one_static_tracepoint_marker (int count,
>    char wrap_indent[80];
>    char extra_field_indent[80];
>    struct ui_out *uiout = current_uiout;
> -  VEC(breakpoint_p) *tracepoints;
>  
>    symtab_and_line sal;
>    sal.pc = marker.address;
>  
> -  tracepoints = static_tracepoints_here (marker.address);
> +  std::vector<breakpoint *> tracepoints
> +    = static_tracepoints_here (marker.address);
>  
>    ui_out_emit_tuple tuple_emitter (uiout, "marker");
>  
> @@ -3659,7 +3624,7 @@ print_one_static_tracepoint_marker (int count,
>    uiout->field_string ("marker-id", marker.str_id.c_str ());
>  
>    uiout->field_fmt ("enabled", "%c",
> -		    !VEC_empty (breakpoint_p, tracepoints) ? 'y' : 'n');
> +		    !tracepoints.empty () ? 'y' : 'n');
>    uiout->spaces (2);
>  
>    strcpy (wrap_indent, "                                   ");
> @@ -3715,7 +3680,7 @@ print_one_static_tracepoint_marker (int count,
>    uiout->field_string ("extra-data", marker.extra.c_str ());
>    uiout->text ("\"\n");
>  
> -  if (!VEC_empty (breakpoint_p, tracepoints))
> +  if (!tracepoints.empty ())
>      {
>        int ix;
>        struct breakpoint *b;
> @@ -3725,22 +3690,20 @@ print_one_static_tracepoint_marker (int count,
>  
>  	uiout->text (extra_field_indent);
>  	uiout->text (_("Probed by static tracepoints: "));
> -	for (ix = 0; VEC_iterate(breakpoint_p, tracepoints, ix, b); ix++)
> +	for (ix = 0; ix < tracepoints.size (); ix++)
>  	  {
>  	    if (ix > 0)
>  	      uiout->text (", ");
>  	    uiout->text ("#");
> -	    uiout->field_int ("tracepoint-id", b->number);
> +	    uiout->field_int ("tracepoint-id", tracepoints[ix]->number);
>  	  }
>        }
>  
>        if (uiout->is_mi_like_p ())
> -	uiout->field_int ("number-of-tracepoints",
> -			  VEC_length(breakpoint_p, tracepoints));
> +	uiout->field_int ("number-of-tracepoints", tracepoints.size ());
>        else
>  	uiout->text ("\n");
>      }
> -  VEC_free (breakpoint_p, tracepoints);
>  }
>  
>  static void
> -- 
> 2.13.6
> 

Re: [RFA] Remove VEC from breakpoint

[Simon Marchi]

On 2018-07-03 10:33 AM, Andrew Burgess wrote:
> * Tom Tromey <tom@tromey.com> [2018-06-05 13:23:16 -0600]:
> 
>> This removes a use of VEC from breakpoint.h, also removing the
>> now-unnecessary breakpoint_p typedef.
>>
>> This patch fixes a latent memory leak in
>> find_matching_tracepoint_location, which neglected to free the vector
>> returned by all_tracepoints.
>>
>> Tested by the buildbot.
>>
>> gdb/ChangeLog
>> 2018-06-05  Tom Tromey  <tom@tromey.com>
>>
>> 	* tracepoint.c (process_tracepoint_on_disconnect, start_tracing)
>> 	(stop_tracing, tstatus_command)
>> 	(find_matching_tracepoint_location, merge_uploaded_tracepoints)
>> 	(print_one_static_tracepoint_marker): Update.
>> 	* breakpoint.c (static_tracepoints_here, all_tracepoints): Return
>> 	std::vector.
>> 	* breakpoint.h (breakpoint_p): Remove typedef.  Don't declare
>> 	VEC.
>> 	(all_tracepoints, static_tracepoints_here): Return std::vector.
> 
> FWIW this looks fine to me, except for one nit below...

[snip]

>> @@ -3094,9 +3063,8 @@ merge_uploaded_tracepoints (struct uploaded_tp **uploaded_tps)
>>  {
>>    struct uploaded_tp *utp;
>>    /* A set of tracepoints which are modified.  */
>> -  VEC(breakpoint_p) *modified_tp = NULL;
>> +  std::vector<breakpoint *> modified_tp;
>>    int ix;
>> -  struct breakpoint *b;
> 
> The 'ix' is no longer used and could be deleted.
> 
> While I was double checking this I also spotted another 'ix' in
> merge_uploaded_trace_state_variables that's not used... :)

Agreed, the patch LGTM in any case.  Compiling tracepoint.c with -Wunused shows a
few more:


/home/simark/src/binutils-gdb/gdb/tracepoint.c:1546:32: error: unused variable 'tsv' [-Werror,-Wunused-variable]
  struct trace_state_variable *tsv;
                               ^
/home/simark/src/binutils-gdb/gdb/tracepoint.c:2755:28: error: unused variable 'default_collect_action' [-Werror,-Wunused-variable]
      struct command_line *default_collect_action;
                           ^
/home/simark/src/binutils-gdb/gdb/tracepoint.c:3067:7: error: unused variable 'ix' [-Werror,-Wunused-variable]
  int ix;
      ^
/home/simark/src/binutils-gdb/gdb/tracepoint.c:3183:7: error: unused variable 'ix' [-Werror,-Wunused-variable]
  int ix;
      ^
/home/simark/src/binutils-gdb/gdb/tracepoint.c:3686:26: error: unused variable 'b' [-Werror,-Wunused-variable]
      struct breakpoint *b;
                         ^

It looks like an obvious patch to clean these up is in order, so it doesn't really
matter if this patch or an eventual cleanup patch removes these ix variables.

Thanks!

Simon

Re: [RFA] Remove VEC from breakpoint

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> It looks like an obvious patch to clean these up is in order, so it doesn't really
Simon> matter if this patch or an eventual cleanup patch removes these ix variables.

I wrote a patch to enable -Wunused-variable.  This actually caught a
couple of bugs.  I'm testing it now but I will submit it soon.

The main risk of it is that I can't compile all the nat-* files.

Tom

Re: [RFA] Remove VEC from breakpoint

[Tom Tromey]

Tom> The main risk of it is that I can't compile all the nat-* files.

... but one of the buildbot builders failed like this:

../../binutils-gdb/gdb/bsd-uthread.c: In member function virtual void bsd_uthread_target::fetch_registers(regcache*, int):
../../binutils-gdb/gdb/bsd-uthread.c:326:18: error: unused variable save_inferior_ptid [-Werror=unused-variable]
   scoped_restore save_inferior_ptid = make_scoped_restore (&inferior_ptid);
                  ^


However this object is used -- it is used for the side effects of its
destructor.

Presumably this is a gcc bug, fixed since then.  But I don't know if
there's a reasonable way to work around it.  So maybe -Wunused-variable
can't be used yet.

I will see about submitting the bug fixes as separate fixes.  Though
it's a shame not to then get the systematic improvement as well.

Tom

Re: [RFA] Remove VEC from breakpoint

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> The main risk of it is that I can't compile all the nat-* files.
Tom> ... but one of the buildbot builders failed like this:

Tom> ../../binutils-gdb/gdb/bsd-uthread.c: In member function virtual void bsd_uthread_target::fetch_registers(regcache*, int):
Tom> ../../binutils-gdb/gdb/bsd-uthread.c:326:18: error: unused variable save_inferior_ptid [-Werror=unused-variable]
Tom>    scoped_restore save_inferior_ptid = make_scoped_restore (&inferior_ptid);

[...]

Tom> Presumably this is a gcc bug, fixed since then.  But I don't know if
Tom> there's a reasonable way to work around it.  So maybe -Wunused-variable
Tom> can't be used yet.

I took another look at this.

The GCC bug was https://gcc.gnu.org/bugzilla/show_bug.cgi?id=38958,
fixed in GCC 4.9.  That seems pretty old...

I updated my warning.m4 patch to try to disable -Wunused-variable when
the compiler fails a test for this bug.  However, my try runs still
fail.  I'm going to take another attempt at it, using a hack to get the
configure logging output into the file that's visible in buildbot, since
I didn't see a way to fetch the config.log.

Tom

Re: [RFA] Remove VEC from breakpoint

[Tom Tromey]

Tom> I'm going to take another attempt at it, using a hack to get the
Tom> configure logging output into the file that's visible in buildbot,
Tom> since I didn't see a way to fetch the config.log.

That didn't take long:

https://gdb-build.sergiodj.net/builders/Debian-s390x-native-gdbserver-m64/builds/5859/steps/compile%20gdb/logs/stdio

I don't know what is going on here, but basically the test case passes
fine in configure, but causes problems in the build.

configure's test is:

    struct Bar { virtual ~Bar() {} }; Bar getbar();
    int
    main ()
    {
    const Bar& b = getbar();
      ;
      return 0;
    }

But the real code is:

  scoped_restore save_inferior_ptid = make_scoped_restore (&inferior_ptid);

If you have any ideas, I'd appreciate it.  Perhaps it's really some
different bug.  This only seems to affect the s390 builders.

Tom

Re: [RFA] Remove VEC from breakpoint

[Simon Marchi]

On 2018-07-09 12:21 AM, Tom Tromey wrote:
> Tom> I'm going to take another attempt at it, using a hack to get the
> Tom> configure logging output into the file that's visible in buildbot,
> Tom> since I didn't see a way to fetch the config.log.
> 
> That didn't take long:
> 
> https://gdb-build.sergiodj.net/builders/Debian-s390x-native-gdbserver-m64/builds/5859/steps/compile%20gdb/logs/stdio
> 
> I don't know what is going on here, but basically the test case passes
> fine in configure, but causes problems in the build.
> 
> configure's test is:
> 
>     struct Bar { virtual ~Bar() {} }; Bar getbar();
>     int
>     main ()
>     {
>     const Bar& b = getbar();
>       ;
>       return 0;
>     }
> 
> But the real code is:
> 
>   scoped_restore save_inferior_ptid = make_scoped_restore (&inferior_ptid);
> 
> If you have any ideas, I'd appreciate it.  Perhaps it's really some
> different bug.  This only seems to affect the s390 builders.
> 
> Tom
> 

Hi Tom,

I reduced the case to this:

  struct scoped_restore_base {};

  struct scoped_restore_tmpl : public scoped_restore_base {
    ~scoped_restore_tmpl() {}
  };

  void func() {
    const scoped_restore_base & save_inferior_ptid = scoped_restore_tmpl();
  }

The type of "save_inferior_ptid" seems important.  If it's a reference to the
child class, we don't get the warning with g++ 4.9.  So your configure test would
need to have that.  g++ 4.9 is available in Debian 8, which you can easily run in
a docker container.

root@9305ed73237c:/gdb/gdb# g++ --version
g++ (Debian 4.9.2-10+deb8u1) 4.9.2
Copyright (C) 2014 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

root@9305ed73237c:/gdb/gdb# g++ -std=gnu++11 -Werror -c -o repro.o -Wunused-variable  repro2.cpp
repro2.cpp: In function 'void func()':
repro2.cpp:8:31: error: unused variable 'save_inferior_ptid' [-Werror=unused-variable]
   const scoped_restore_base & save_inferior_ptid = scoped_restore_tmpl();
                               ^
cc1plus: all warnings being treated as errors

Simon