Make encode_actions_rsp use std::vector
Commit Message
Currently, encode_actions_rsp returns two malloc'ed arrays of malloc'ed
strings (char *) by pointer. Change this to use
std::vector<std::string>. This eliminates some cleanups in remote.c.
Regtested on the buildbot.
gdb/ChangeLog:
* tracepoint.h (class collection_list) <stringify>: Return
std::vector<std::string>.
(encode_actions_rsp): Change parameters to
std::vector<std::string> *.
* tracepoint.c (collection_list::stringify): Return
std::vector<std::string> and adjust accordingly.
(encode_actions_rsp): Changee parameters to
std::vector<std::string> 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(-)
Comments
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<std::string>. This eliminates some cleanups in remote.c.
>
> Regtested on the buildbot.
>
> gdb/ChangeLog:
>
> * tracepoint.h (class collection_list) <stringify>: Return
> std::vector<std::string>.
> (encode_actions_rsp): Change parameters to
> std::vector<std::string> *.
> * tracepoint.c (collection_list::stringify): Return
> std::vector<std::string> and adjust accordingly.
> (encode_actions_rsp): Changee parameters to
> std::vector<std::string> 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<std::string> tdp_actions;
> + std::vector<std::string> 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<std::string>
> 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<std::string> 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<std::string> *tdp_actions,
> + std::vector<std::string> *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<std::string> stringify ();
>
> const std::vector<std::string> &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<std::string> *tdp_actions,
> + std::vector<std::string> *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
On 2017-10-29 22:33, Simon Marchi wrote:
> 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<std::string>. This eliminates some cleanups in remote.c.
...
> Woops, I left the local vector name "str_list2", a temporary name I
> gave it. I renamed it to "str_list" locally.
This is now pushed.
Simon
@@ -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<std::string> tdp_actions;
+ std::vector<std::string> 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
@@ -1118,18 +1118,14 @@ collection_list::collection_list ()
/* Reduce a collection list to string form (for gdb protocol). */
-char **
+std::vector<std::string>
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<std::string> 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<std::string> *tdp_actions,
+ std::vector<std::string> *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 ();
@@ -269,7 +269,7 @@ public:
void finish ();
- char **stringify ();
+ std::vector<std::string> stringify ();
const std::vector<std::string> &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<std::string> *tdp_actions,
+ std::vector<std::string> *stepping_actions);
extern void validate_actionline (const char *, struct breakpoint *);
extern void validate_trace_state_variable_name (const char *name);