From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18234 invoked by alias); 3 Jul 2018 14:33:15 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 18085 invoked by uid 89); 3 Jul 2018 14:33:14 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-24.9 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=releasing, H*r:sk:k7-v6so X-HELO: mail-wr0-f180.google.com Received: from mail-wr0-f180.google.com (HELO mail-wr0-f180.google.com) (209.85.128.180) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 03 Jul 2018 14:33:06 +0000 Received: by mail-wr0-f180.google.com with SMTP id k7-v6so2232163wrq.0 for ; Tue, 03 Jul 2018 07:33:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=mpVk0E04xHIStOT/Qp+lbo4CfyuRcYx5YSBRHPLczFY=; b=AjNtiBrfA0afyKhs0BiZGPG4ouBv4vMp13rH+KVitRaPSZXJv9z7yTLVmwC1ZP+XXt LixDgGsuYLs83Lw56zCYE9t3sCGJp2h/b7yzEkvAWo5Kw6RFkyO7q/VrkPBIb3UfIqgO wVzGubpyB9MwtARcpnRX4TX0fasJ6s4xziiJpDRtJgSDXjvnuzT8kJsjdbhlvAGF7t8k Z5aGLcjBTBm3M0FeN6aGerFUT3YmWfhEj/6Tfvvgd66MVKrjGeDGNaIHPV7r2CHiNWT9 DNpZ/zJUgwhx0fJry9hcrpE6QI8R7UETcXiGjN9996upcPi19bsOdKr43CIUpqMMlkFN s20w== Return-Path: Received: from localhost (cust64-dsl91-135-5.idnet.net. [91.135.5.64]) by smtp.gmail.com with ESMTPSA id l7-v6sm2553575wrg.95.2018.07.03.07.33.02 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 03 Jul 2018 07:33:02 -0700 (PDT) Date: Tue, 03 Jul 2018 14:33:00 -0000 From: Andrew Burgess To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [RFA] Remove VEC from breakpoint Message-ID: <20180703143301.GA2675@embecosm.com> References: <20180605192316.5423-1-tom@tromey.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180605192316.5423-1-tom@tromey.com> X-Fortune: You should go home. X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes X-SW-Source: 2018-07/txt/msg00051.txt.bz2 * Tom Tromey [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 > > * 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 > static_tracepoints_here (CORE_ADDR addr) > { > struct breakpoint *b; > - VEC(breakpoint_p) *found = 0; > + std::vector 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 > all_tracepoints (void) > { > - VEC(breakpoint_p) *tp_vec = 0; > + std::vector 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 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 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 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 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 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 >