Re: [PATCH] Make encode_actions_rsp use std::vector

[Simon Marchi <simon.marchi@polymtl.ca>]

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