From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11447 invoked by alias); 30 Oct 2017 02:33:56 -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 11410 invoked by uid 89); 30 Oct 2017 02:33:53 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No 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,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 30 Oct 2017 02:33:51 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id v9U2XjXK003241 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Sun, 29 Oct 2017 22:33:49 -0400 Received: by simark.ca (Postfix, from userid 112) id ED6E61E533; Sun, 29 Oct 2017 22:33:44 -0400 (EDT) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id B41CC1E4C4; Sun, 29 Oct 2017 22:33:42 -0400 (EDT) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Mon, 30 Oct 2017 02:33:00 -0000 From: Simon Marchi To: Simon Marchi Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] Make encode_actions_rsp use std::vector In-Reply-To: <1509330588-25109-1-git-send-email-simon.marchi@ericsson.com> References: <1509330588-25109-1-git-send-email-simon.marchi@ericsson.com> Message-ID: X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.0 X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Mon, 30 Oct 2017 02:33:45 +0000 X-IsSubscribed: yes X-SW-Source: 2017-10/txt/msg00875.txt.bz2 On 2017-10-29 22:29, Simon Marchi wrote: > Currently, encode_actions_rsp returns two malloc'ed arrays of malloc'ed > strings (char *) by pointer. Change this to use > std::vector. This eliminates some cleanups in remote.c. > > Regtested on the buildbot. > > gdb/ChangeLog: > > * tracepoint.h (class collection_list) : Return > std::vector. > (encode_actions_rsp): Change parameters to > std::vector *. > * tracepoint.c (collection_list::stringify): Return > std::vector and adjust accordingly. > (encode_actions_rsp): Changee parameters to > std::vector and adjust accordingly. > * remote.c (free_actions_list), > free_actions_list_cleanup_wrapper): Remove. > (remote_download_tracepoint): Adjust to std::vector. > --- > gdb/remote.c | 100 > ++++++++++++++++++++----------------------------------- > gdb/tracepoint.c | 40 ++++++---------------- > gdb/tracepoint.h | 5 +-- > 3 files changed, 51 insertions(+), 94 deletions(-) > > diff --git a/gdb/remote.c b/gdb/remote.c > index ed2a9ec..d29f163 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -12297,28 +12297,6 @@ remote_trace_init (struct target_ops *self) > error (_("Target does not support this command.")); > } > > -static void free_actions_list (char **actions_list); > -static void free_actions_list_cleanup_wrapper (void *); > -static void > -free_actions_list_cleanup_wrapper (void *al) > -{ > - free_actions_list ((char **) al); > -} > - > -static void > -free_actions_list (char **actions_list) > -{ > - int ndx; > - > - if (actions_list == 0) > - return; > - > - for (ndx = 0; actions_list[ndx]; ndx++) > - xfree (actions_list[ndx]); > - > - xfree (actions_list); > -} > - > /* Recursive routine to walk through command list including loops, and > download packets for each command. */ > > @@ -12367,20 +12345,14 @@ remote_download_tracepoint (struct > target_ops *self, struct bp_location *loc) > CORE_ADDR tpaddr; > char addrbuf[40]; > char buf[BUF_SIZE]; > - char **tdp_actions; > - char **stepping_actions; > - int ndx; > - struct cleanup *old_chain = NULL; > + std::vector tdp_actions; > + std::vector stepping_actions; > char *pkt; > struct breakpoint *b = loc->owner; > struct tracepoint *t = (struct tracepoint *) b; > struct remote_state *rs = get_remote_state (); > > encode_actions_rsp (loc, &tdp_actions, &stepping_actions); > - old_chain = make_cleanup (free_actions_list_cleanup_wrapper, > - tdp_actions); > - (void) make_cleanup (free_actions_list_cleanup_wrapper, > - stepping_actions); > > tpaddr = loc->address; > sprintf_vma (addrbuf, tpaddr); > @@ -12446,7 +12418,7 @@ remote_download_tracepoint (struct target_ops > *self, struct bp_location *loc) > xsnprintf (buf + strlen (buf), BUF_SIZE - strlen (buf), ":X%x,", > aexpr->len); > pkt = buf + strlen (buf); > - for (ndx = 0; ndx < aexpr->len; ++ndx) > + for (int ndx = 0; ndx < aexpr->len; ++ndx) > pkt = pack_hex_byte (pkt, aexpr->buf[ndx]); > *pkt = '\0'; > } > @@ -12463,39 +12435,43 @@ remote_download_tracepoint (struct > target_ops *self, struct bp_location *loc) > error (_("Target does not support tracepoints.")); > > /* do_single_steps (t); */ > - if (tdp_actions) > + for (auto action_it = tdp_actions.begin (); > + action_it != tdp_actions.end (); action_it++) > { > - for (ndx = 0; tdp_actions[ndx]; ndx++) > - { > - QUIT; /* Allow user to bail out with ^C. */ > - xsnprintf (buf, BUF_SIZE, "QTDP:-%x:%s:%s%c", > - b->number, addrbuf, /* address */ > - tdp_actions[ndx], > - ((tdp_actions[ndx + 1] || stepping_actions) > - ? '-' : 0)); > - putpkt (buf); > - remote_get_noisy_reply (); > - if (strcmp (rs->buf, "OK")) > - error (_("Error on target while setting tracepoints.")); > - } > - } > - if (stepping_actions) > - { > - for (ndx = 0; stepping_actions[ndx]; ndx++) > - { > - QUIT; /* Allow user to bail out with ^C. */ > - xsnprintf (buf, BUF_SIZE, "QTDP:-%x:%s:%s%s%s", > - b->number, addrbuf, /* address */ > - ((ndx == 0) ? "S" : ""), > - stepping_actions[ndx], > - (stepping_actions[ndx + 1] ? "-" : "")); > - putpkt (buf); > - remote_get_noisy_reply (); > - if (strcmp (rs->buf, "OK")) > - error (_("Error on target while setting tracepoints.")); > - } > + QUIT; /* Allow user to bail out with ^C. */ > + > + bool has_more = (action_it != tdp_actions.end () > + || !stepping_actions.empty ()); > + > + xsnprintf (buf, BUF_SIZE, "QTDP:-%x:%s:%s%c", > + b->number, addrbuf, /* address */ > + action_it->c_str (), > + has_more ? '-' : 0); > + putpkt (buf); > + remote_get_noisy_reply (); > + if (strcmp (rs->buf, "OK")) > + error (_("Error on target while setting tracepoints.")); > } > > + for (auto action_it = stepping_actions.begin (); > + action_it != stepping_actions.end (); action_it++) > + { > + QUIT; /* Allow user to bail out with ^C. */ > + > + bool is_first = action_it == stepping_actions.begin (); > + bool has_more = action_it != stepping_actions.end (); > + > + xsnprintf (buf, BUF_SIZE, "QTDP:-%x:%s:%s%s%s", > + b->number, addrbuf, /* address */ > + is_first ? "S" : "", > + action_it->c_str (), > + has_more ? "-" : ""); > + putpkt (buf); > + remote_get_noisy_reply (); > + if (strcmp (rs->buf, "OK")) > + error (_("Error on target while setting tracepoints.")); > + } > + > if (packet_support (PACKET_TracepointSource) == PACKET_ENABLE) > { > if (b->location != NULL) > @@ -12523,8 +12499,6 @@ remote_download_tracepoint (struct target_ops > *self, struct bp_location *loc) > remote_download_command_source (b->number, loc->address, > breakpoint_commands (b)); > } > - > - do_cleanups (old_chain); > } > > static int > diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c > index fdc3b38..302c4c8 100644 > --- a/gdb/tracepoint.c > +++ b/gdb/tracepoint.c > @@ -1118,18 +1118,14 @@ collection_list::collection_list () > > /* Reduce a collection list to string form (for gdb protocol). */ > > -char ** > +std::vector > collection_list::stringify () > { > char temp_buf[2048]; > int count; > - int ndx = 0; > - char *(*str_list)[]; > char *end; > long i; > - > - count = 1 + 1 + m_memranges.size () + m_aexprs.size () + 1; > - str_list = (char *(*)[]) xmalloc (count * sizeof (char *)); > + std::vector str_list2; > > if (m_strace_data) > { > @@ -1137,8 +1133,7 @@ collection_list::stringify () > printf_filtered ("\nCollecting static trace data\n"); > end = temp_buf; > *end++ = 'L'; > - (*str_list)[ndx] = savestring (temp_buf, end - temp_buf); > - ndx++; > + str_list2.emplace_back (temp_buf, end - temp_buf); > } > > for (i = sizeof (m_regs_mask) - 1; i > 0; i--) > @@ -1158,8 +1153,7 @@ collection_list::stringify () > sprintf (end, "%02X", m_regs_mask[i]); > end += 2; > } > - (*str_list)[ndx] = xstrdup (temp_buf); > - ndx++; > + str_list2.emplace_back (temp_buf); > } > if (info_verbose) > printf_filtered ("\n"); > @@ -1179,8 +1173,7 @@ collection_list::stringify () > } > if (count + 27 > MAX_AGENT_EXPR_LEN) > { > - (*str_list)[ndx] = savestring (temp_buf, count); > - ndx++; > + str_list2.emplace_back (temp_buf, count); > count = 0; > end = temp_buf; > } > @@ -1210,8 +1203,7 @@ collection_list::stringify () > QUIT; /* Allow user to bail out with ^C. */ > if ((count + 10 + 2 * m_aexprs[i]->len) > MAX_AGENT_EXPR_LEN) > { > - (*str_list)[ndx] = savestring (temp_buf, count); > - ndx++; > + str_list2.emplace_back (temp_buf, count); > count = 0; > end = temp_buf; > } > @@ -1225,20 +1217,12 @@ collection_list::stringify () > > if (count != 0) > { > - (*str_list)[ndx] = savestring (temp_buf, count); > - ndx++; > + str_list2.emplace_back (temp_buf, count); > count = 0; > end = temp_buf; > } > - (*str_list)[ndx] = NULL; > > - if (ndx == 0) > - { > - xfree (str_list); > - return NULL; > - } > - else > - return *str_list; > + return str_list2; > } > > /* Add the printed expression EXP to *LIST. */ > @@ -1513,14 +1497,12 @@ encode_actions (struct bp_location *tloc, > /* Render all actions into gdb protocol. */ > > void > -encode_actions_rsp (struct bp_location *tloc, char ***tdp_actions, > - char ***stepping_actions) > +encode_actions_rsp (struct bp_location *tloc, > + std::vector *tdp_actions, > + std::vector *stepping_actions) > { > struct collection_list tracepoint_list, stepping_list; > > - *tdp_actions = NULL; > - *stepping_actions = NULL; > - > encode_actions (tloc, &tracepoint_list, &stepping_list); > > *tdp_actions = tracepoint_list.stringify (); > diff --git a/gdb/tracepoint.h b/gdb/tracepoint.h > index 8364d38..9416e9e 100644 > --- a/gdb/tracepoint.h > +++ b/gdb/tracepoint.h > @@ -269,7 +269,7 @@ public: > > void finish (); > > - char **stringify (); > + std::vector stringify (); > > const std::vector &wholly_collected () > { return m_wholly_collected; } > @@ -328,7 +328,8 @@ extern void encode_actions (struct bp_location > *tloc, > struct collection_list *stepping_list); > > extern void encode_actions_rsp (struct bp_location *tloc, > - char ***tdp_actions, char ***stepping_actions); > + std::vector *tdp_actions, > + std::vector *stepping_actions); > > extern void validate_actionline (const char *, struct breakpoint *); > extern void validate_trace_state_variable_name (const char *name); Woops, I left the local vector name "str_list2", a temporary name I gave it. I renamed it to "str_list" locally. Simon