This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA] Remove VEC from breakpoint
- From: Andrew Burgess <andrew dot burgess at embecosm dot com>
- To: Tom Tromey <tom at tromey dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Tue, 3 Jul 2018 15:33:01 +0100
- Subject: Re: [RFA] Remove VEC from breakpoint
- References: <20180605192316.5423-1-tom@tromey.com>
* 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);
>
> /* 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
>