[RFA 0/6] more cleanup removals

[RFA 0/6] more cleanup removals

[Tom Tromey]

This series removes more cleanups, using the by-now familiar
techniques of replacing data structures with variants that have
destructors.

Regression tested by the buildbot; which by the way caught a problem
in an early version of patch #3... having a builder for your host of
choice really helps.

Tom

[RFA 2/6] Remove some cleanups from probe.c

[Tom Tromey]

This removes some cleanups from parse_probes by using std::string; and
removes some unnecessary cleanups from elsewhere in probe.c.

ChangeLog
2017-10-15  Tom Tromey  <tom@tromey.com>

	* probe.c (parse_probes): Use std::string.
	(info_probes_for_ops, enable_probes_command)
	(disable_probes_command): Remove cleanups.
---
 gdb/ChangeLog |  6 ++++++
 gdb/probe.c   | 24 ++++--------------------
 2 files changed, 10 insertions(+), 20 deletions(-)

diff --git a/gdb/probe.c b/gdb/probe.c
index b3dbf896b9..e5a09211cb 100644
--- a/gdb/probe.c
+++ b/gdb/probe.c
@@ -98,7 +98,6 @@ parse_probes (const struct event_location *location,
 {
   char *arg_end, *arg;
   char *objfile_namestr = NULL, *provider = NULL, *name, *p;
-  struct cleanup *cleanup;
   const struct probe_ops *probe_ops;
   const char *arg_start, *cs;
 
@@ -118,8 +117,8 @@ parse_probes (const struct event_location *location,
   arg_end = skip_to_space (arg);
 
   /* We make a copy here so we can write over parts with impunity.  */
-  arg = savestring (arg, arg_end - arg);
-  cleanup = make_cleanup (xfree, arg);
+  std::string copy (arg, arg_end - arg);
+  arg = &copy[0];
 
   /* Extract each word from the argument, separated by ":"s.  */
   p = strchr (arg, ':');
@@ -183,17 +182,12 @@ parse_probes (const struct event_location *location,
 
   if (canonical)
     {
-      char *canon;
-
-      canon = savestring (arg_start, arg_end - arg_start);
-      make_cleanup (xfree, canon);
+      std::string canon (arg_start, arg_end - arg_start);
       canonical->special_display = 1;
       canonical->pre_expanded = 1;
-      canonical->location = new_probe_location (canon);
+      canonical->location = new_probe_location (canon.c_str ());
     }
 
-  do_cleanups (cleanup);
-
   return result;
 }
 
@@ -548,7 +542,6 @@ info_probes_for_ops (const char *arg, int from_tty,
 		     const struct probe_ops *pops)
 {
   std::string provider, probe_name, objname;
-  struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
   int any_found;
   int ui_out_extra_fields = 0;
   size_t size_addr;
@@ -657,7 +650,6 @@ info_probes_for_ops (const char *arg, int from_tty,
 
     any_found = !probes.empty ();
   }
-  do_cleanups (cleanup);
 
   if (!any_found)
     current_uiout->message (_("No probes matched.\n"));
@@ -677,7 +669,6 @@ static void
 enable_probes_command (const char *arg, int from_tty)
 {
   std::string provider, probe_name, objname;
-  struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
 
   parse_probe_linespec ((const char *) arg, &provider, &probe_name, &objname);
 
@@ -686,7 +677,6 @@ enable_probes_command (const char *arg, int from_tty)
   if (probes.empty ())
     {
       current_uiout->message (_("No probes matched.\n"));
-      do_cleanups (cleanup);
       return;
     }
 
@@ -706,8 +696,6 @@ enable_probes_command (const char *arg, int from_tty)
 	current_uiout->message (_("Probe %s:%s cannot be enabled.\n"),
 				probe.probe->provider, probe.probe->name);
     }
-
-  do_cleanups (cleanup);
 }
 
 /* Implementation of the `disable probes' command.  */
@@ -716,7 +704,6 @@ static void
 disable_probes_command (const char *arg, int from_tty)
 {
   std::string provider, probe_name, objname;
-  struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
 
   parse_probe_linespec ((const char *) arg, &provider, &probe_name, &objname);
 
@@ -725,7 +712,6 @@ disable_probes_command (const char *arg, int from_tty)
   if (probes.empty ())
     {
       current_uiout->message (_("No probes matched.\n"));
-      do_cleanups (cleanup);
       return;
     }
 
@@ -745,8 +731,6 @@ disable_probes_command (const char *arg, int from_tty)
 	current_uiout->message (_("Probe %s:%s cannot be disabled.\n"),
 				probe.probe->provider, probe.probe->name);
     }
-
-  do_cleanups (cleanup);
 }
 
 /* See comments in probe.h.  */
-- 
2.13.6

Re: [RFA 2/6] Remove some cleanups from probe.c

[Simon Marchi]

On 2017-10-15 11:04 PM, Tom Tromey wrote:
> This removes some cleanups from parse_probes by using std::string; and
> removes some unnecessary cleanups from elsewhere in probe.c.
> 
> ChangeLog
> 2017-10-15  Tom Tromey  <tom@tromey.com>
> 
> 	* probe.c (parse_probes): Use std::string.
> 	(info_probes_for_ops, enable_probes_command)
> 	(disable_probes_command): Remove cleanups.
> ---
>  gdb/ChangeLog |  6 ++++++
>  gdb/probe.c   | 24 ++++--------------------
>  2 files changed, 10 insertions(+), 20 deletions(-)
> 
> diff --git a/gdb/probe.c b/gdb/probe.c
> index b3dbf896b9..e5a09211cb 100644
> --- a/gdb/probe.c
> +++ b/gdb/probe.c
> @@ -98,7 +98,6 @@ parse_probes (const struct event_location *location,
>  {
>    char *arg_end, *arg;
>    char *objfile_namestr = NULL, *provider = NULL, *name, *p;
> -  struct cleanup *cleanup;
>    const struct probe_ops *probe_ops;
>    const char *arg_start, *cs;
>  
> @@ -118,8 +117,8 @@ parse_probes (const struct event_location *location,
>    arg_end = skip_to_space (arg);
>  
>    /* We make a copy here so we can write over parts with impunity.  */
> -  arg = savestring (arg, arg_end - arg);
> -  cleanup = make_cleanup (xfree, arg);
> +  std::string copy (arg, arg_end - arg);
> +  arg = &copy[0];
>  
>    /* Extract each word from the argument, separated by ":"s.  */
>    p = strchr (arg, ':');
> @@ -183,17 +182,12 @@ parse_probes (const struct event_location *location,
>  
>    if (canonical)
>      {
> -      char *canon;
> -
> -      canon = savestring (arg_start, arg_end - arg_start);
> -      make_cleanup (xfree, canon);
> +      std::string canon (arg_start, arg_end - arg_start);
>        canonical->special_display = 1;
>        canonical->pre_expanded = 1;
> -      canonical->location = new_probe_location (canon);
> +      canonical->location = new_probe_location (canon.c_str ());
>      }
>  
> -  do_cleanups (cleanup);
> -
>    return result;
>  }
>  
> @@ -548,7 +542,6 @@ info_probes_for_ops (const char *arg, int from_tty,
>  		     const struct probe_ops *pops)
>  {
>    std::string provider, probe_name, objname;
> -  struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
>    int any_found;
>    int ui_out_extra_fields = 0;
>    size_t size_addr;
> @@ -657,7 +650,6 @@ info_probes_for_ops (const char *arg, int from_tty,
>  
>      any_found = !probes.empty ();
>    }
> -  do_cleanups (cleanup);
>  
>    if (!any_found)
>      current_uiout->message (_("No probes matched.\n"));
> @@ -677,7 +669,6 @@ static void
>  enable_probes_command (const char *arg, int from_tty)
>  {
>    std::string provider, probe_name, objname;
> -  struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
>  
>    parse_probe_linespec ((const char *) arg, &provider, &probe_name, &objname);
>  
> @@ -686,7 +677,6 @@ enable_probes_command (const char *arg, int from_tty)
>    if (probes.empty ())
>      {
>        current_uiout->message (_("No probes matched.\n"));
> -      do_cleanups (cleanup);
>        return;
>      }
>  
> @@ -706,8 +696,6 @@ enable_probes_command (const char *arg, int from_tty)
>  	current_uiout->message (_("Probe %s:%s cannot be enabled.\n"),
>  				probe.probe->provider, probe.probe->name);
>      }
> -
> -  do_cleanups (cleanup);
>  }
>  
>  /* Implementation of the `disable probes' command.  */
> @@ -716,7 +704,6 @@ static void
>  disable_probes_command (const char *arg, int from_tty)
>  {
>    std::string provider, probe_name, objname;
> -  struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
>  
>    parse_probe_linespec ((const char *) arg, &provider, &probe_name, &objname);
>  
> @@ -725,7 +712,6 @@ disable_probes_command (const char *arg, int from_tty)
>    if (probes.empty ())
>      {
>        current_uiout->message (_("No probes matched.\n"));
> -      do_cleanups (cleanup);
>        return;
>      }
>  
> @@ -745,8 +731,6 @@ disable_probes_command (const char *arg, int from_tty)
>  	current_uiout->message (_("Probe %s:%s cannot be disabled.\n"),
>  				probe.probe->provider, probe.probe->name);
>      }
> -
> -  do_cleanups (cleanup);
>  }
>  
>  /* See comments in probe.h.  */
> 


LGTM.

[RFA 5/6] Return unique_xmalloc_ptr from target_read_stralloc

[Tom Tromey]

This changes target_read_stralloc to return a unique_xmalloc_ptr, and
then fixes all the callers.  unique_xmalloc_ptr is used, rather than
std::string, because target_read_stralloc gives a special meaning to a
NULL return.

ChangeLog
2017-10-15  Tom Tromey  <tom@tromey.com>

	* xml-syscall.c (xml_init_syscalls_info): Update.
	* xml-support.c (xinclude_start_include): Update.
	(xml_fetch_content_from_file): Return unique_xmalloc_ptr.
	* xml-support.h (xml_fetch_another): Return unique_xmalloc_ptr.
	(xml_fetch_content_from_file): Likewise.
	* osdata.c (get_osdata): Update.
	* target.h (target_read_stralloc, target_get_osdata): Return
	unique_xmalloc_ptr.
	* solib-aix.c (solib_aix_get_library_list): Update.
	* solib-target.c (solib_target_current_sos): Update.
	* solib-svr4.c (svr4_current_sos_via_xfer_libraries): Update.
	* xml-tdesc.c (fetch_available_features_from_target): Update.
	(target_fetch_description_xml): Update.
	(file_read_description_xml): Update.
	* remote.c (remote_get_threads_with_qxfer, remote_memory_map)
	(remote_traceframe_info, btrace_read_config, remote_read_btrace)
	(remote_pid_to_exec_file): Update.
	* target.c (target_read_stralloc): Return unique_xmalloc_ptr.
	(target_get_osdata): Likewise.
---
 gdb/ChangeLog      | 22 +++++++++++++++++++
 gdb/osdata.c       | 10 +++------
 gdb/remote.c       | 64 +++++++++++++++---------------------------------------
 gdb/solib-aix.c    | 15 +++++--------
 gdb/solib-svr4.c   | 16 ++++----------
 gdb/solib-target.c | 15 +++----------
 gdb/target.c       | 16 +++++++-------
 gdb/target.h       |  7 +++---
 gdb/xml-support.c  | 28 +++++++++---------------
 gdb/xml-support.h  |  7 +++---
 gdb/xml-syscall.c  | 18 +++++----------
 gdb/xml-tdesc.c    | 37 ++++++++++---------------------
 12 files changed, 96 insertions(+), 159 deletions(-)

diff --git a/gdb/osdata.c b/gdb/osdata.c
index 9da3d07cdb..276d22427a 100644
--- a/gdb/osdata.c
+++ b/gdb/osdata.c
@@ -245,13 +245,11 @@ struct osdata *
 get_osdata (const char *type)
 {
   struct osdata *osdata = NULL;
-  char *xml = target_get_osdata (type);
+  gdb::unique_xmalloc_ptr<char> xml = target_get_osdata (type);
 
   if (xml)
     {
-      struct cleanup *old_chain = make_cleanup (xfree, xml);
-
-      if (xml[0] == '\0')
+      if (xml.get ()[0] == '\0')
 	{
 	  if (type)
 	    warning (_("Empty data returned by target.  Wrong osdata type?"));
@@ -259,9 +257,7 @@ get_osdata (const char *type)
 	    warning (_("Empty type list returned by target.  No type data?"));
 	}
       else
-	osdata = osdata_parse (xml);
-
-      do_cleanups (old_chain);
+	osdata = osdata_parse (xml.get ());
     }
 
   if (!osdata)
diff --git a/gdb/remote.c b/gdb/remote.c
index 43cc661daf..b333d76a6a 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -3203,16 +3203,15 @@ remote_get_threads_with_qxfer (struct target_ops *ops,
 #if defined(HAVE_LIBEXPAT)
   if (packet_support (PACKET_qXfer_threads) == PACKET_ENABLE)
     {
-      char *xml = target_read_stralloc (ops, TARGET_OBJECT_THREADS, NULL);
-      struct cleanup *back_to = make_cleanup (xfree, xml);
+      gdb::unique_xmalloc_ptr<char> xml
+	= target_read_stralloc (ops, TARGET_OBJECT_THREADS, NULL);
 
       if (xml != NULL && *xml != '\0')
 	{
 	  gdb_xml_parse_quick (_("threads"), "threads.dtd",
-			       threads_elements, xml, context);
+			       threads_elements, xml.get (), context);
 	}
 
-      do_cleanups (back_to);
       return 1;
     }
 #endif
@@ -10902,16 +10901,11 @@ static VEC(mem_region_s) *
 remote_memory_map (struct target_ops *ops)
 {
   VEC(mem_region_s) *result = NULL;
-  char *text = target_read_stralloc (&current_target,
-				     TARGET_OBJECT_MEMORY_MAP, NULL);
+  gdb::unique_xmalloc_ptr<char> text
+    = target_read_stralloc (&current_target, TARGET_OBJECT_MEMORY_MAP, NULL);
 
   if (text)
-    {
-      struct cleanup *back_to = make_cleanup (xfree, text);
-
-      result = parse_memory_map (text);
-      do_cleanups (back_to);
-    }
+    result = parse_memory_map (text.get ());
 
   return result;
 }
@@ -13049,18 +13043,11 @@ remote_set_circular_trace_buffer (struct target_ops *self, int val)
 static traceframe_info_up
 remote_traceframe_info (struct target_ops *self)
 {
-  char *text;
-
-  text = target_read_stralloc (&current_target,
-			       TARGET_OBJECT_TRACEFRAME_INFO, NULL);
+  gdb::unique_xmalloc_ptr<char> text
+    = target_read_stralloc (&current_target, TARGET_OBJECT_TRACEFRAME_INFO,
+			    NULL);
   if (text != NULL)
-    {
-      struct cleanup *back_to = make_cleanup (xfree, text);
-      traceframe_info_up info = parse_traceframe_info (text);
-
-      do_cleanups (back_to);
-      return info;
-    }
+    return parse_traceframe_info (text.get ());
 
   return NULL;
 }
@@ -13319,18 +13306,10 @@ btrace_sync_conf (const struct btrace_config *conf)
 static void
 btrace_read_config (struct btrace_config *conf)
 {
-  char *xml;
-
-  xml = target_read_stralloc (&current_target,
-			      TARGET_OBJECT_BTRACE_CONF, "");
+  gdb::unique_xmalloc_ptr<char> xml
+    = target_read_stralloc (&current_target, TARGET_OBJECT_BTRACE_CONF, "");
   if (xml != NULL)
-    {
-      struct cleanup *cleanup;
-
-      cleanup = make_cleanup (xfree, xml);
-      parse_xml_btrace_conf (conf, xml);
-      do_cleanups (cleanup);
-    }
+    parse_xml_btrace_conf (conf, xml.get ());
 }
 
 /* Maybe reopen target btrace.  */
@@ -13501,9 +13480,7 @@ remote_read_btrace (struct target_ops *self,
 		    enum btrace_read_type type)
 {
   struct packet_config *packet = &remote_protocol_packets[PACKET_qXfer_btrace];
-  struct cleanup *cleanup;
   const char *annex;
-  char *xml;
 
   if (packet_config_support (packet) != PACKET_ENABLE)
     error (_("Target does not support branch tracing."));
@@ -13529,14 +13506,12 @@ remote_read_btrace (struct target_ops *self,
 		      (unsigned int) type);
     }
 
-  xml = target_read_stralloc (&current_target,
-			      TARGET_OBJECT_BTRACE, annex);
+  gdb::unique_xmalloc_ptr<char> xml
+    = target_read_stralloc (&current_target, TARGET_OBJECT_BTRACE, annex);
   if (xml == NULL)
     return BTRACE_ERR_UNKNOWN;
 
-  cleanup = make_cleanup (xfree, xml);
-  parse_xml_btrace (btrace, xml);
-  do_cleanups (cleanup);
+  parse_xml_btrace (btrace, xml.get ());
 
   return BTRACE_ERR_NONE;
 }
@@ -13570,16 +13545,13 @@ remote_load (struct target_ops *self, const char *name, int from_tty)
 static char *
 remote_pid_to_exec_file (struct target_ops *self, int pid)
 {
-  static char *filename = NULL;
+  static gdb::unique_xmalloc_ptr<char> filename;
   struct inferior *inf;
   char *annex = NULL;
 
   if (packet_support (PACKET_qXfer_exec_file) != PACKET_ENABLE)
     return NULL;
 
-  if (filename != NULL)
-    xfree (filename);
-
   inf = find_inferior_pid (pid);
   if (inf == NULL)
     internal_error (__FILE__, __LINE__,
@@ -13596,7 +13568,7 @@ remote_pid_to_exec_file (struct target_ops *self, int pid)
   filename = target_read_stralloc (&current_target,
 				   TARGET_OBJECT_EXEC_FILE, annex);
 
-  return filename;
+  return filename.get ();
 }
 
 /* Implement the to_can_do_single_step target_ops method.  */
diff --git a/gdb/solib-aix.c b/gdb/solib-aix.c
index 9233e7815c..de238e597b 100644
--- a/gdb/solib-aix.c
+++ b/gdb/solib-aix.c
@@ -271,39 +271,34 @@ static VEC (lm_info_aix_p) *
 solib_aix_get_library_list (struct inferior *inf, const char *warning_msg)
 {
   struct solib_aix_inferior_data *data;
-  char *library_document;
-  struct cleanup *cleanup;
 
   /* If already computed, return the cached value.  */
   data = get_solib_aix_inferior_data (inf);
   if (data->library_list != NULL)
     return data->library_list;
 
-  library_document = target_read_stralloc (&current_target,
-                                           TARGET_OBJECT_LIBRARIES_AIX,
-                                           NULL);
+  gdb::unique_xmalloc_ptr<char> library_document
+    = target_read_stralloc (&current_target, TARGET_OBJECT_LIBRARIES_AIX,
+			    NULL);
   if (library_document == NULL && warning_msg != NULL)
     {
       warning (_("%s (failed to read TARGET_OBJECT_LIBRARIES_AIX)"),
 	       warning_msg);
       return NULL;
     }
-  cleanup = make_cleanup (xfree, library_document);
 
   if (solib_aix_debug)
     fprintf_unfiltered (gdb_stdlog,
 			"DEBUG: TARGET_OBJECT_LIBRARIES_AIX = \n%s\n",
-			library_document);
+			library_document.get ());
 
-  data->library_list = solib_aix_parse_libraries (library_document);
+  data->library_list = solib_aix_parse_libraries (library_document.get ());
   if (data->library_list == NULL && warning_msg != NULL)
     {
       warning (_("%s (missing XML support?)"), warning_msg);
-      do_cleanups (cleanup);
       return NULL;
     }
 
-  do_cleanups (cleanup);
   return data->library_list;
 }
 
diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index d33479128e..bf2577a433 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -1269,24 +1269,16 @@ static int
 svr4_current_sos_via_xfer_libraries (struct svr4_library_list *list,
 				     const char *annex)
 {
-  char *svr4_library_document;
-  int result;
-  struct cleanup *back_to;
-
   gdb_assert (annex == NULL || target_augmented_libraries_svr4_read ());
 
   /* Fetch the list of shared libraries.  */
-  svr4_library_document = target_read_stralloc (&current_target,
-						TARGET_OBJECT_LIBRARIES_SVR4,
-						annex);
+  gdb::unique_xmalloc_ptr<char> svr4_library_document
+    = target_read_stralloc (&current_target, TARGET_OBJECT_LIBRARIES_SVR4,
+			    annex);
   if (svr4_library_document == NULL)
     return 0;
 
-  back_to = make_cleanup (xfree, svr4_library_document);
-  result = svr4_parse_libraries (svr4_library_document, list);
-  do_cleanups (back_to);
-
-  return result;
+  return svr4_parse_libraries (svr4_library_document.get (), list);
 }
 
 #else
diff --git a/gdb/solib-target.c b/gdb/solib-target.c
index cf569ec3e6..01171f85dd 100644
--- a/gdb/solib-target.c
+++ b/gdb/solib-target.c
@@ -248,27 +248,18 @@ static struct so_list *
 solib_target_current_sos (void)
 {
   struct so_list *new_solib, *start = NULL, *last = NULL;
-  char *library_document;
-  struct cleanup *old_chain;
   VEC(lm_info_target_p) *library_list;
   lm_info_target *info;
   int ix;
 
   /* Fetch the list of shared libraries.  */
-  library_document = target_read_stralloc (&current_target,
-					   TARGET_OBJECT_LIBRARIES,
-					   NULL);
+  gdb::unique_xmalloc_ptr<char> library_document
+    = target_read_stralloc (&current_target, TARGET_OBJECT_LIBRARIES, NULL);
   if (library_document == NULL)
     return NULL;
 
-  /* solib_target_parse_libraries may throw, so we use a cleanup.  */
-  old_chain = make_cleanup (xfree, library_document);
-
   /* Parse the list.  */
-  library_list = solib_target_parse_libraries (library_document);
-
-  /* library_document string is not needed behind this point.  */
-  do_cleanups (old_chain);
+  library_list = solib_target_parse_libraries (library_document.get ());
 
   if (library_list == NULL)
     return NULL;
diff --git a/gdb/target.c b/gdb/target.c
index 4a7589d445..9d65b496f1 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1925,12 +1925,12 @@ target_read_alloc (struct target_ops *ops, enum target_object object,
 }
 
 /* Read OBJECT/ANNEX using OPS.  The result is NUL-terminated and
-   returned as a string, allocated using xmalloc.  If an error occurs
-   or the transfer is unsupported, NULL is returned.  Empty objects
-   are returned as allocated but empty strings.  A warning is issued
-   if the result contains any embedded NUL bytes.  */
+   returned as a string.  If an error occurs or the transfer is
+   unsupported, NULL is returned.  Empty objects are returned as
+   allocated but empty strings.  A warning is issued if the result
+   contains any embedded NUL bytes.  */
 
-char *
+gdb::unique_xmalloc_ptr<char>
 target_read_stralloc (struct target_ops *ops, enum target_object object,
 		      const char *annex)
 {
@@ -1945,7 +1945,7 @@ target_read_stralloc (struct target_ops *ops, enum target_object object,
     return NULL;
 
   if (transferred == 0)
-    return xstrdup ("");
+    return gdb::unique_xmalloc_ptr<char> (xstrdup (""));
 
   bufstr[transferred] = 0;
 
@@ -1959,7 +1959,7 @@ target_read_stralloc (struct target_ops *ops, enum target_object object,
 	break;
       }
 
-  return bufstr;
+  return gdb::unique_xmalloc_ptr<char> (bufstr);
 }
 
 /* Memory transfer methods.  */
@@ -2654,7 +2654,7 @@ target_supports_multi_process (void)
   return (*current_target.to_supports_multi_process) (&current_target);
 }
 
-char *
+gdb::unique_xmalloc_ptr<char>
 target_get_osdata (const char *type)
 {
   struct target_ops *t;
diff --git a/gdb/target.h b/gdb/target.h
index 581c89be54..98bcd789e0 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -358,9 +358,8 @@ extern LONGEST target_read_alloc (struct target_ops *ops,
    are returned as allocated but empty strings.  A warning is issued
    if the result contains any embedded NUL bytes.  */
 
-extern char *target_read_stralloc (struct target_ops *ops,
-				   enum target_object object,
-				   const char *annex);
+extern gdb::unique_xmalloc_ptr<char> target_read_stralloc
+    (struct target_ops *ops, enum target_object object, const char *annex);
 
 /* See target_ops->to_xfer_partial.  */
 extern target_xfer_partial_ftype target_xfer_partial;
@@ -2401,7 +2400,7 @@ struct target_ops *find_target_at (enum strata stratum);
    unsupported, NULL is returned.  Empty objects are returned as
    allocated but empty strings.  */
 
-extern char *target_get_osdata (const char *type);
+extern gdb::unique_xmalloc_ptr<char> target_get_osdata (const char *type);
 
 \f
 /* Stuff that should be shared among the various remote targets.  */
diff --git a/gdb/xml-support.c b/gdb/xml-support.c
index 50a062a3a4..7cecaa8d4d 100644
--- a/gdb/xml-support.c
+++ b/gdb/xml-support.c
@@ -808,8 +808,7 @@ xinclude_start_include (struct gdb_xml_parser *parser,
   struct xinclude_parsing_data *data
     = (struct xinclude_parsing_data *) user_data;
   char *href = (char *) xml_find_attribute (attributes, "href")->value;
-  struct cleanup *back_to;
-  char *text, *output;
+  char *output;
 
   gdb_xml_debug (parser, _("Processing XInclude of \"%s\""), href);
 
@@ -817,19 +816,17 @@ xinclude_start_include (struct gdb_xml_parser *parser,
     gdb_xml_error (parser, _("Maximum XInclude depth (%d) exceeded"),
 		   MAX_XINCLUDE_DEPTH);
 
-  text = data->fetcher (href, data->fetcher_baton);
+  gdb::unique_xmalloc_ptr<char> text = data->fetcher (href,
+						      data->fetcher_baton);
   if (text == NULL)
     gdb_xml_error (parser, _("Could not load XML document \"%s\""), href);
-  back_to = make_cleanup (xfree, text);
 
   if (!xml_process_xincludes (data->output, parser->name (),
-			      text, data->fetcher,
+			      text.get (), data->fetcher,
 			      data->fetcher_baton,
 			      data->include_depth + 1))
     gdb_xml_error (parser, _("Parsing \"%s\" failed"), href);
 
-  do_cleanups (back_to);
-
   data->skip_depth++;
 }
 
@@ -997,13 +994,11 @@ show_debug_xml (struct ui_file *file, int from_tty,
   fprintf_filtered (file, _("XML debugging is %s.\n"), value);
 }
 
-char *
+gdb::unique_xmalloc_ptr<char>
 xml_fetch_content_from_file (const char *filename, void *baton)
 {
   const char *dirname = (const char *) baton;
   gdb_file_up file;
-  struct cleanup *back_to;
-  char *text;
   size_t len, offset;
 
   if (dirname && *dirname)
@@ -1024,19 +1019,18 @@ xml_fetch_content_from_file (const char *filename, void *baton)
   /* Read in the whole file, one chunk at a time.  */
   len = 4096;
   offset = 0;
-  text = (char *) xmalloc (len);
-  back_to = make_cleanup (free_current_contents, &text);
+  gdb::unique_xmalloc_ptr<char> text ((char *) xmalloc (len));
   while (1)
     {
       size_t bytes_read;
 
       /* Continue reading where the last read left off.  Leave at least
 	 one byte so that we can NUL-terminate the result.  */
-      bytes_read = fread (text + offset, 1, len - offset - 1, file.get ());
+      bytes_read = fread (text.get () + offset, 1, len - offset - 1,
+			  file.get ());
       if (ferror (file.get ()))
 	{
 	  warning (_("Read error from \"%s\""), filename);
-	  do_cleanups (back_to);
 	  return NULL;
 	}
 
@@ -1046,12 +1040,10 @@ xml_fetch_content_from_file (const char *filename, void *baton)
 	break;
 
       len = len * 2;
-      text = (char *) xrealloc (text, len);
+      text.reset ((char *) xrealloc (text.get (), len));
     }
 
-  discard_cleanups (back_to);
-
-  text[offset] = '\0';
+  text.get ()[offset] = '\0';
   return text;
 }
 
diff --git a/gdb/xml-support.h b/gdb/xml-support.h
index 1a1b7fd0e7..74bc8117c0 100644
--- a/gdb/xml-support.h
+++ b/gdb/xml-support.h
@@ -52,7 +52,8 @@ extern const char *xml_builtin[][2];
 
 /* Callback to fetch a new XML file, based on the provided HREF.  */
 
-typedef char *(*xml_fetch_another) (const char *href, void *baton);
+typedef gdb::unique_xmalloc_ptr<char> (*xml_fetch_another) (const char *href,
+							    void *baton);
 
 /* Append the expansion of TEXT after processing <xi:include> tags in
    RESULT.  FETCHER will be called (with FETCHER_BATON) to retrieve
@@ -230,7 +231,7 @@ ULONGEST gdb_xml_parse_ulongest (struct gdb_xml_parser *parser,
 /* Open FILENAME, read all its text into memory, close it, and return
    the text.  If something goes wrong, return NULL and warn.  */
 
-extern char *xml_fetch_content_from_file (const char *filename,
-                                          void *baton);
+extern gdb::unique_xmalloc_ptr<char> xml_fetch_content_from_file
+    (const char *filename, void *baton);
 
 #endif
diff --git a/gdb/xml-syscall.c b/gdb/xml-syscall.c
index a43641893a..fe036cbf5c 100644
--- a/gdb/xml-syscall.c
+++ b/gdb/xml-syscall.c
@@ -362,22 +362,14 @@ syscall_parse_xml (const char *document, xml_fetch_another fetcher,
 static struct syscalls_info *
 xml_init_syscalls_info (const char *filename)
 {
-  char *full_file;
-  struct syscalls_info *syscalls_info;
-  struct cleanup *back_to;
-
-  full_file = xml_fetch_content_from_file (filename, gdb_datadir);
+  gdb::unique_xmalloc_ptr<char> full_file
+    = xml_fetch_content_from_file (filename, gdb_datadir);
   if (full_file == NULL)
     return NULL;
 
-  back_to = make_cleanup (xfree, full_file);
-
-  syscalls_info = syscall_parse_xml (full_file,
-				     xml_fetch_content_from_file,
-				     (void *) ldirname (filename).c_str ());
-  do_cleanups (back_to);
-
-  return syscalls_info;
+  return syscall_parse_xml (full_file.get (),
+			    xml_fetch_content_from_file,
+			    (void *) ldirname (filename).c_str ());
 }
 
 /* Initializes the syscalls_info structure according to the
diff --git a/gdb/xml-tdesc.c b/gdb/xml-tdesc.c
index daa713f13f..5a1f459f98 100644
--- a/gdb/xml-tdesc.c
+++ b/gdb/xml-tdesc.c
@@ -673,24 +673,16 @@ tdesc_parse_xml (const char *document, xml_fetch_another fetcher,
 const struct target_desc *
 file_read_description_xml (const char *filename)
 {
-  struct target_desc *tdesc;
-  char *tdesc_str;
-  struct cleanup *back_to;
-
-  tdesc_str = xml_fetch_content_from_file (filename, NULL);
+  gdb::unique_xmalloc_ptr<char> tdesc_str
+    = xml_fetch_content_from_file (filename, NULL);
   if (tdesc_str == NULL)
     {
       warning (_("Could not open \"%s\""), filename);
       return NULL;
     }
 
-  back_to = make_cleanup (xfree, tdesc_str);
-
-  tdesc = tdesc_parse_xml (tdesc_str, xml_fetch_content_from_file,
-			   (void *) ldirname (filename).c_str ());
-  do_cleanups (back_to);
-
-  return tdesc;
+  return tdesc_parse_xml (tdesc_str.get (), xml_fetch_content_from_file,
+			  (void *) ldirname (filename).c_str ());
 }
 
 /* Read a string representation of available features from the target,
@@ -700,7 +692,7 @@ file_read_description_xml (const char *filename)
    is "target.xml".  Other calls may be performed for the DTD or
    for <xi:include>.  */
 
-static char *
+static gdb::unique_xmalloc_ptr<char>
 fetch_available_features_from_target (const char *name, void *baton_)
 {
   struct target_ops *ops = (struct target_ops *) baton_;
@@ -719,21 +711,14 @@ fetch_available_features_from_target (const char *name, void *baton_)
 const struct target_desc *
 target_read_description_xml (struct target_ops *ops)
 {
-  struct target_desc *tdesc;
-  char *tdesc_str;
-  struct cleanup *back_to;
-
-  tdesc_str = fetch_available_features_from_target ("target.xml", ops);
+  gdb::unique_xmalloc_ptr<char> tdesc_str
+    = fetch_available_features_from_target ("target.xml", ops);
   if (tdesc_str == NULL)
     return NULL;
 
-  back_to = make_cleanup (xfree, tdesc_str);
-  tdesc = tdesc_parse_xml (tdesc_str,
-			   fetch_available_features_from_target,
-			   ops);
-  do_cleanups (back_to);
-
-  return tdesc;
+  return tdesc_parse_xml (tdesc_str.get (),
+			  fetch_available_features_from_target,
+			  ops);
 }
 
 /* Fetches an XML target description using OPS,  processing
@@ -758,7 +743,7 @@ target_fetch_description_xml (struct target_ops *ops)
   struct target_desc *tdesc;
 
   gdb::unique_xmalloc_ptr<char>
-    tdesc_str (fetch_available_features_from_target ("target.xml", ops));
+    tdesc_str = fetch_available_features_from_target ("target.xml", ops);
   if (tdesc_str == NULL)
     return {};
 
-- 
2.13.6

Re: [RFA 5/6] Return unique_xmalloc_ptr from target_read_stralloc

[Simon Marchi]

On 2017-10-15 11:04 PM, Tom Tromey wrote:
> diff --git a/gdb/target.h b/gdb/target.h
> index 581c89be54..98bcd789e0 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -358,9 +358,8 @@ extern LONGEST target_read_alloc (struct target_ops *ops,
>     are returned as allocated but empty strings.  A warning is issued
>     if the result contains any embedded NUL bytes.  */
>  
> -extern char *target_read_stralloc (struct target_ops *ops,
> -				   enum target_object object,
> -				   const char *annex);
> +extern gdb::unique_xmalloc_ptr<char> target_read_stralloc
> +    (struct target_ops *ops, enum target_object object, const char *annex);

You changed the comment above target_read_stralloc in target.c.  Can you put

  /* See target.h.  */

and update this one instead?

>  
>  /* See target_ops->to_xfer_partial.  */
>  extern target_xfer_partial_ftype target_xfer_partial;
> @@ -2401,7 +2400,7 @@ struct target_ops *find_target_at (enum strata stratum);
>     unsupported, NULL is returned.  Empty objects are returned as
>     allocated but empty strings.  */
>  
> -extern char *target_get_osdata (const char *type);
> +extern gdb::unique_xmalloc_ptr<char> target_get_osdata (const char *type);

You can update the comment of this function in the same way, and add "See target.h"
in target.c.

>  
>  \f
>  /* Stuff that should be shared among the various remote targets.  */
> diff --git a/gdb/xml-support.c b/gdb/xml-support.c
> index 50a062a3a4..7cecaa8d4d 100644
> --- a/gdb/xml-support.c
> +++ b/gdb/xml-support.c
> @@ -808,8 +808,7 @@ xinclude_start_include (struct gdb_xml_parser *parser,
>    struct xinclude_parsing_data *data
>      = (struct xinclude_parsing_data *) user_data;
>    char *href = (char *) xml_find_attribute (attributes, "href")->value;
> -  struct cleanup *back_to;
> -  char *text, *output;
> +  char *output;

You can remove output too.

LGTM with those fixed.

Simon

[RFA 4/6] Simple cleanup removals in remote.c

[Tom Tromey]

This removes a few cleanups in remote.c using the usual techniques:
std::vector, unique_xmalloc_ptr, and gdb::def_vector.

ChangeLog
2017-10-15  Tom Tromey  <tom@tromey.com>

	* remote.c (remote_register_number_and_offset): Use std::vector.
	(remote_set_syscall_catchpoint): Use gdb::unique_xmalloc_ptr.
	(putpkt_binary): Use gdb::def_vector.
	(compare_sections_command): Likewise.
---
 gdb/ChangeLog |  7 ++++++
 gdb/remote.c  | 71 +++++++++++++++++++++++------------------------------------
 2 files changed, 35 insertions(+), 43 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index e2bdd115f9..43cc661daf 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -803,21 +803,15 @@ int
 remote_register_number_and_offset (struct gdbarch *gdbarch, int regnum,
 				   int *pnum, int *poffset)
 {
-  struct packet_reg *regs;
-  struct cleanup *old_chain;
-
   gdb_assert (regnum < gdbarch_num_regs (gdbarch));
 
-  regs = XCNEWVEC (struct packet_reg, gdbarch_num_regs (gdbarch));
-  old_chain = make_cleanup (xfree, regs);
+  std::vector<packet_reg> regs (gdbarch_num_regs (gdbarch));
 
-  map_regcache_remote_table (gdbarch, regs);
+  map_regcache_remote_table (gdbarch, regs.data ());
 
   *pnum = regs[regnum].pnum;
   *poffset = regs[regnum].offset;
 
-  do_cleanups (old_chain);
-
   return *pnum != -1;
 }
 
@@ -2062,7 +2056,7 @@ remote_set_syscall_catchpoint (struct target_ops *self,
 			       int pid, int needed, int any_count,
 			       int table_size, int *table)
 {
-  char *catch_packet;
+  const char *catch_packet;
   enum packet_result result;
   int n_sysno = 0;
 
@@ -2092,6 +2086,7 @@ remote_set_syscall_catchpoint (struct target_ops *self,
 			  pid, needed, any_count, n_sysno);
     }
 
+  gdb::unique_xmalloc_ptr<char> built_packet;
   if (needed)
     {
       /* Prepare a packet with the sysno list, assuming max 8+1
@@ -2099,46 +2094,45 @@ remote_set_syscall_catchpoint (struct target_ops *self,
 	 big, fallback on the non-selective packet.  */
       const int maxpktsz = strlen ("QCatchSyscalls:1") + n_sysno * 9 + 1;
 
-      catch_packet = (char *) xmalloc (maxpktsz);
-      strcpy (catch_packet, "QCatchSyscalls:1");
+      built_packet.reset ((char *) xmalloc (maxpktsz));
+      strcpy (built_packet.get (), "QCatchSyscalls:1");
       if (!any_count)
 	{
 	  int i;
 	  char *p;
 
-	  p = catch_packet;
+	  p = built_packet.get ();
 	  p += strlen (p);
 
 	  /* Add in catch_packet each syscall to be caught (table[i] != 0).  */
 	  for (i = 0; i < table_size; i++)
 	    {
 	      if (table[i] != 0)
-		p += xsnprintf (p, catch_packet + maxpktsz - p, ";%x", i);
+		p += xsnprintf (p, built_packet.get () + maxpktsz - p,
+				";%x", i);
 	    }
 	}
-      if (strlen (catch_packet) > get_remote_packet_size ())
+      if (strlen (built_packet.get ()) > get_remote_packet_size ())
 	{
 	  /* catch_packet too big.  Fallback to less efficient
 	     non selective mode, with GDB doing the filtering.  */
-	  catch_packet[sizeof ("QCatchSyscalls:1") - 1] = 0;
+	  catch_packet = "QCatchSyscalls:1";
 	}
+      else
+	catch_packet = built_packet.get ();
     }
   else
-    catch_packet = xstrdup ("QCatchSyscalls:0");
+    catch_packet = "QCatchSyscalls:0";
 
-  {
-    struct cleanup *old_chain = make_cleanup (xfree, catch_packet);
-    struct remote_state *rs = get_remote_state ();
+  struct remote_state *rs = get_remote_state ();
 
-    putpkt (catch_packet);
-    getpkt (&rs->buf, &rs->buf_size, 0);
-    result = packet_ok (rs->buf, &remote_protocol_packets[PACKET_QCatchSyscalls]);
-    do_cleanups (old_chain);
-    if (result == PACKET_OK)
-      return 0;
-    else
-      return -1;
-  }
+  putpkt (catch_packet);
+  getpkt (&rs->buf, &rs->buf_size, 0);
+  result = packet_ok (rs->buf, &remote_protocol_packets[PACKET_QCatchSyscalls]);
+  if (result == PACKET_OK)
+    return 0;
+  else
+    return -1;
 }
 
 /* If 'QProgramSignals' is supported, tell the remote stub what
@@ -8762,8 +8756,8 @@ putpkt_binary (const char *buf, int cnt)
   struct remote_state *rs = get_remote_state ();
   int i;
   unsigned char csum = 0;
-  char *buf2 = (char *) xmalloc (cnt + 6);
-  struct cleanup *old_chain = make_cleanup (xfree, buf2);
+  gdb::def_vector<char> data (cnt + 6);
+  char *buf2 = data.data ();
 
   int ch;
   int tcount = 0;
@@ -8866,7 +8860,6 @@ putpkt_binary (const char *buf, int cnt)
 	    case '+':
 	      if (remote_debug)
 		fprintf_unfiltered (gdb_stdlog, "Ack\n");
-	      do_cleanups (old_chain);
 	      return 1;
 	    case '-':
 	      if (remote_debug)
@@ -8875,10 +8868,7 @@ putpkt_binary (const char *buf, int cnt)
 	    case SERIAL_TIMEOUT:
 	      tcount++;
 	      if (tcount > 3)
-		{
-		  do_cleanups (old_chain);
-		  return 0;
-		}
+		return 0;
 	      break;		/* Retransmit buffer.  */
 	    case '$':
 	      {
@@ -8962,7 +8952,6 @@ putpkt_binary (const char *buf, int cnt)
 #endif
     }
 
-  do_cleanups (old_chain);
   return 0;
 }
 
@@ -10331,7 +10320,6 @@ static void
 compare_sections_command (const char *args, int from_tty)
 {
   asection *s;
-  struct cleanup *old_chain;
   gdb_byte *sectdata;
   const char *sectname;
   bfd_size_type size;
@@ -10372,11 +10360,10 @@ compare_sections_command (const char *args, int from_tty)
       matched = 1;		/* Do this section.  */
       lma = s->lma;
 
-      sectdata = (gdb_byte *) xmalloc (size);
-      old_chain = make_cleanup (xfree, sectdata);
-      bfd_get_section_contents (exec_bfd, s, sectdata, 0, size);
+      gdb::def_vector<gdb_byte> sectdata (size);
+      bfd_get_section_contents (exec_bfd, s, sectdata.data (), 0, size);
 
-      res = target_verify_memory (sectdata, lma, size);
+      res = target_verify_memory (sectdata.data (), lma, size);
 
       if (res == -1)
 	error (_("target memory fault, section %s, range %s -- %s"), sectname,
@@ -10393,8 +10380,6 @@ compare_sections_command (const char *args, int from_tty)
 	  printf_filtered ("MIS-MATCHED!\n");
 	  mismatched++;
 	}
-
-      do_cleanups (old_chain);
     }
   if (mismatched > 0)
     warning (_("One or more sections of the target image does not match\n\
-- 
2.13.6

Re: [RFA 4/6] Simple cleanup removals in remote.c

[Simon Marchi]

Hi Tom,

On 2017-10-15 11:04 PM, Tom Tromey wrote:
> This removes a few cleanups in remote.c using the usual techniques:
> std::vector, unique_xmalloc_ptr, and gdb::def_vector.
> 
> ChangeLog
> 2017-10-15  Tom Tromey  <tom@tromey.com>
> 
> 	* remote.c (remote_register_number_and_offset): Use std::vector.
> 	(remote_set_syscall_catchpoint): Use gdb::unique_xmalloc_ptr.
> 	(putpkt_binary): Use gdb::def_vector.
> 	(compare_sections_command): Likewise.
> ---
>  gdb/ChangeLog |  7 ++++++
>  gdb/remote.c  | 71 +++++++++++++++++++++++------------------------------------
>  2 files changed, 35 insertions(+), 43 deletions(-)
> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index e2bdd115f9..43cc661daf 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -803,21 +803,15 @@ int
>  remote_register_number_and_offset (struct gdbarch *gdbarch, int regnum,
>  				   int *pnum, int *poffset)
>  {
> -  struct packet_reg *regs;
> -  struct cleanup *old_chain;
> -
>    gdb_assert (regnum < gdbarch_num_regs (gdbarch));
>  
> -  regs = XCNEWVEC (struct packet_reg, gdbarch_num_regs (gdbarch));
> -  old_chain = make_cleanup (xfree, regs);
> +  std::vector<packet_reg> regs (gdbarch_num_regs (gdbarch));
>  
> -  map_regcache_remote_table (gdbarch, regs);
> +  map_regcache_remote_table (gdbarch, regs.data ());
>  
>    *pnum = regs[regnum].pnum;
>    *poffset = regs[regnum].offset;
>  
> -  do_cleanups (old_chain);
> -
>    return *pnum != -1;
>  }
>  
> @@ -2062,7 +2056,7 @@ remote_set_syscall_catchpoint (struct target_ops *self,
>  			       int pid, int needed, int any_count,
>  			       int table_size, int *table)
>  {
> -  char *catch_packet;
> +  const char *catch_packet;
>    enum packet_result result;
>    int n_sysno = 0;
>  
> @@ -2092,6 +2086,7 @@ remote_set_syscall_catchpoint (struct target_ops *self,
>  			  pid, needed, any_count, n_sysno);
>      }
>  
> +  gdb::unique_xmalloc_ptr<char> built_packet;
>    if (needed)
>      {
>        /* Prepare a packet with the sysno list, assuming max 8+1
> @@ -2099,46 +2094,45 @@ remote_set_syscall_catchpoint (struct target_ops *self,
>  	 big, fallback on the non-selective packet.  */
>        const int maxpktsz = strlen ("QCatchSyscalls:1") + n_sysno * 9 + 1;
>  
> -      catch_packet = (char *) xmalloc (maxpktsz);
> -      strcpy (catch_packet, "QCatchSyscalls:1");
> +      built_packet.reset ((char *) xmalloc (maxpktsz));
> +      strcpy (built_packet.get (), "QCatchSyscalls:1");
>        if (!any_count)
>  	{
>  	  int i;
>  	  char *p;
>  
> -	  p = catch_packet;
> +	  p = built_packet.get ();
>  	  p += strlen (p);
>  
>  	  /* Add in catch_packet each syscall to be caught (table[i] != 0).  */
>  	  for (i = 0; i < table_size; i++)
>  	    {
>  	      if (table[i] != 0)
> -		p += xsnprintf (p, catch_packet + maxpktsz - p, ";%x", i);
> +		p += xsnprintf (p, built_packet.get () + maxpktsz - p,
> +				";%x", i);
>  	    }
>  	}
> -      if (strlen (catch_packet) > get_remote_packet_size ())
> +      if (strlen (built_packet.get ()) > get_remote_packet_size ())
>  	{
>  	  /* catch_packet too big.  Fallback to less efficient
>  	     non selective mode, with GDB doing the filtering.  */
> -	  catch_packet[sizeof ("QCatchSyscalls:1") - 1] = 0;
> +	  catch_packet = "QCatchSyscalls:1";
>  	}
> +      else
> +	catch_packet = built_packet.get ();
>      }
>    else
> -    catch_packet = xstrdup ("QCatchSyscalls:0");
> +    catch_packet = "QCatchSyscalls:0";
>  
> -  {
> -    struct cleanup *old_chain = make_cleanup (xfree, catch_packet);
> -    struct remote_state *rs = get_remote_state ();
> +  struct remote_state *rs = get_remote_state ();
>  
> -    putpkt (catch_packet);
> -    getpkt (&rs->buf, &rs->buf_size, 0);
> -    result = packet_ok (rs->buf, &remote_protocol_packets[PACKET_QCatchSyscalls]);
> -    do_cleanups (old_chain);
> -    if (result == PACKET_OK)
> -      return 0;
> -    else
> -      return -1;
> -  }
> +  putpkt (catch_packet);
> +  getpkt (&rs->buf, &rs->buf_size, 0);
> +  result = packet_ok (rs->buf, &remote_protocol_packets[PACKET_QCatchSyscalls]);
> +  if (result == PACKET_OK)
> +    return 0;
> +  else
> +    return -1;
>  }

For this one, wouldn't it be easier to just go with a string?  Something like:

commit b8629fb5fa7e869341a745c168a5f7103ee91c1c
Author: Simon Marchi <simon.marchi@ericsson.com>
Date:   Mon Oct 16 16:36:51 2017 -0400

    foo

diff --git a/gdb/remote.c b/gdb/remote.c
index 6b77a9f..37874d3 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -2056,7 +2056,7 @@ remote_set_syscall_catchpoint (struct target_ops *self,
 			       int pid, int needed, int any_count,
 			       int table_size, int *table)
 {
-  const char *catch_packet;
+  std::string catch_packet;
   enum packet_result result;
   int n_sysno = 0;

@@ -2086,47 +2086,31 @@ remote_set_syscall_catchpoint (struct target_ops *self,
 			  pid, needed, any_count, n_sysno);
     }

-  gdb::unique_xmalloc_ptr<char> built_packet;
   if (needed)
     {
-      /* Prepare a packet with the sysno list, assuming max 8+1
-	 characters for a sysno.  If the resulting packet size is too
-	 big, fallback on the non-selective packet.  */
-      const int maxpktsz = strlen ("QCatchSyscalls:1") + n_sysno * 9 + 1;
-
-      built_packet.reset ((char *) xmalloc (maxpktsz));
-      strcpy (built_packet.get (), "QCatchSyscalls:1");
+      catch_packet = "QCatchSyscalls:1";
       if (!any_count)
 	{
-	  int i;
-	  char *p;
-
-	  p = built_packet.get ();
-	  p += strlen (p);
-
 	  /* Add in catch_packet each syscall to be caught (table[i] != 0).  */
-	  for (i = 0; i < table_size; i++)
+	  for (int i = 0; i < table_size; i++)
 	    {
 	      if (table[i] != 0)
-		p += xsnprintf (p, built_packet.get () + maxpktsz - p,
-				";%x", i);
+		catch_packet += string_printf (";%x", i);
 	    }
 	}
-      if (strlen (built_packet.get ()) > get_remote_packet_size ())
+      if (catch_packet.length () > get_remote_packet_size ())
 	{
 	  /* catch_packet too big.  Fallback to less efficient
 	     non selective mode, with GDB doing the filtering.  */
 	  catch_packet = "QCatchSyscalls:1";
 	}
-      else
-	catch_packet = built_packet.get ();
     }
   else
     catch_packet = "QCatchSyscalls:0";

   struct remote_state *rs = get_remote_state ();

-  putpkt (catch_packet);
+  putpkt (catch_packet.c_str ());
   getpkt (&rs->buf, &rs->buf_size, 0);
   result = packet_ok (rs->buf, &remote_protocol_packets[PACKET_QCatchSyscalls]);
   if (result == PACKET_OK)


>  
>  /* If 'QProgramSignals' is supported, tell the remote stub what
> @@ -8762,8 +8756,8 @@ putpkt_binary (const char *buf, int cnt)
>    struct remote_state *rs = get_remote_state ();
>    int i;
>    unsigned char csum = 0;
> -  char *buf2 = (char *) xmalloc (cnt + 6);
> -  struct cleanup *old_chain = make_cleanup (xfree, buf2);
> +  gdb::def_vector<char> data (cnt + 6);
> +  char *buf2 = data.data ();
>  
>    int ch;
>    int tcount = 0;
> @@ -8866,7 +8860,6 @@ putpkt_binary (const char *buf, int cnt)
>  	    case '+':
>  	      if (remote_debug)
>  		fprintf_unfiltered (gdb_stdlog, "Ack\n");
> -	      do_cleanups (old_chain);
>  	      return 1;
>  	    case '-':
>  	      if (remote_debug)
> @@ -8875,10 +8868,7 @@ putpkt_binary (const char *buf, int cnt)
>  	    case SERIAL_TIMEOUT:
>  	      tcount++;
>  	      if (tcount > 3)
> -		{
> -		  do_cleanups (old_chain);
> -		  return 0;
> -		}
> +		return 0;
>  	      break;		/* Retransmit buffer.  */
>  	    case '$':
>  	      {
> @@ -8962,7 +8952,6 @@ putpkt_binary (const char *buf, int cnt)
>  #endif
>      }
>  
> -  do_cleanups (old_chain);
>    return 0;
>  }
>  
> @@ -10331,7 +10320,6 @@ static void
>  compare_sections_command (const char *args, int from_tty)
>  {
>    asection *s;
> -  struct cleanup *old_chain;
>    gdb_byte *sectdata;
>    const char *sectname;
>    bfd_size_type size;
> @@ -10372,11 +10360,10 @@ compare_sections_command (const char *args, int from_tty)
>        matched = 1;		/* Do this section.  */
>        lma = s->lma;
>  
> -      sectdata = (gdb_byte *) xmalloc (size);
> -      old_chain = make_cleanup (xfree, sectdata);
> -      bfd_get_section_contents (exec_bfd, s, sectdata, 0, size);
> +      gdb::def_vector<gdb_byte> sectdata (size);

Use gdb::byte_vector.

> +      bfd_get_section_contents (exec_bfd, s, sectdata.data (), 0, size);
>  
> -      res = target_verify_memory (sectdata, lma, size);
> +      res = target_verify_memory (sectdata.data (), lma, size);
>  
>        if (res == -1)
>  	error (_("target memory fault, section %s, range %s -- %s"), sectname,
> @@ -10393,8 +10380,6 @@ compare_sections_command (const char *args, int from_tty)
>  	  printf_filtered ("MIS-MATCHED!\n");
>  	  mismatched++;
>  	}
> -
> -      do_cleanups (old_chain);
>      }
>    if (mismatched > 0)
>      warning (_("One or more sections of the target image does not match\n\
> 

Thanks,

Simon

Re: [RFA 4/6] Simple cleanup removals in remote.c

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:

Simon> For this one, wouldn't it be easier to just go with a string?  Something like:

It's easier but less efficient.
I wasn't sure if it mattered so I erred on the side of efficiency.

Tom

Re: [RFA 4/6] Simple cleanup removals in remote.c

[Simon Marchi]

On 2017-10-16 05:14 PM, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:
> 
> Simon> For this one, wouldn't it be easier to just go with a string?  Something like:
> 
> It's easier but less efficient.
> I wasn't sure if it mattered so I erred on the side of efficiency.
> 
> Tom
> 

I suppose you are talking about the += string_printf ()?  It's true that it's
not the most efficient, because the formatting is done in a separate buffer
(an std::string) and then copied, instead of directly in-place.  At least,
there's likely no dynamic memory allocation, because the string fits in
std::string's local buffer.

So since there's likely no real-world impact, I would've erred on the other
side.

In any case, I don't really mind, your version is good too and eliminates the
clean up, which is the main point.

Simon

Re: [RFA 4/6] Simple cleanup removals in remote.c

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:

Simon> In any case, I don't really mind, your version is good too and eliminates the
Simon> clean up, which is the main point.

I guess I'll leave it then, since it's already tested.
I'm not always sure what to do in this situation; I guess for me it
depends on how strong I perceive your (the reviewer's) opinions to be.

thanks,
Tom

Re: [RFA 4/6] Simple cleanup removals in remote.c

[Pedro Alves]

On 10/16/2017 10:36 PM, Simon Marchi wrote:
> On 2017-10-16 05:14 PM, Tom Tromey wrote:
>>>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:
>>
>> Simon> For this one, wouldn't it be easier to just go with a string?  Something like:
>>
>> It's easier but less efficient.
>> I wasn't sure if it mattered so I erred on the side of efficiency.
>>
>> Tom
>>
> 
> I suppose you are talking about the += string_printf ()?  It's true that it's
> not the most efficient, because the formatting is done in a separate buffer
> (an std::string) and then copied, instead of directly in-place.  At least,
> there's likely no dynamic memory allocation, because the string fits in
> std::string's local buffer.

This suggests to me that we're missing a string_printf variant that
appends to a preexisting string:

  void string_appendf (std::string &dest, const char* fmt, ...);

See (untested) patch below.

> 
> So since there's likely no real-world impact, I would've erred on the other
> side.
> 
> In any case, I don't really mind, your version is good too and eliminates the
> clean up, which is the main point.

I agree.  Tom, please go ahead.

From e9602dc6ee8eac1f0dc5b267e961f9a206ce649b Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 16 Oct 2017 23:22:27 +0100
Subject: [PATCH] string_appendf

---
 gdb/common/common-utils.c | 22 ++++++++++++++++++++++
 gdb/common/common-utils.h |  5 +++++
 gdb/remote.c              | 22 +++++++---------------
 3 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/gdb/common/common-utils.c b/gdb/common/common-utils.c
index d8c546a..8ca62f3 100644
--- a/gdb/common/common-utils.c
+++ b/gdb/common/common-utils.c
@@ -153,6 +153,28 @@ xsnprintf (char *str, size_t size, const char *format, ...)
 
 /* See documentation in common-utils.h.  */
 
+void
+string_appendf (std::string &str, const char* fmt, ...)
+{
+  va_list vp;
+  int grow_size;
+
+  va_start (vp, fmt);
+  grow_size = vsnprintf (NULL, 0, fmt, vp);
+  va_end (vp);
+
+  size_t curr_size = str.size ();
+  str.resize (curr_size + grow_size);
+
+  /* C++11 and later guarantee std::string uses contiguous memory and
+     always includes the terminating '\0'.  */
+  va_start (vp, fmt);
+  vsprintf (&str[curr_size], fmt, vp);
+  va_end (vp);
+}
+
+/* See documentation in common-utils.h.  */
+
 std::string
 string_printf (const char* fmt, ...)
 {
diff --git a/gdb/common/common-utils.h b/gdb/common/common-utils.h
index 19724f9..bf1b444 100644
--- a/gdb/common/common-utils.h
+++ b/gdb/common/common-utils.h
@@ -67,6 +67,11 @@ std::string string_printf (const char* fmt, ...)
 std::string string_vprintf (const char* fmt, va_list args)
   ATTRIBUTE_PRINTF (1, 0);
 
+/* Like string_printf, but appends to dest instead of returning a new
+   std::string.  */
+void string_appendf (std::string &dest, const char* fmt, ...)
+  ATTRIBUTE_PRINTF (2, 3);
+
 /* Make a copy of the string at PTR with LEN characters
    (and add a null character at the end in the copy).
    Uses malloc to get the space.  Returns the address of the copy.  */
diff --git a/gdb/remote.c b/gdb/remote.c
index 6b77a9f..a6cb724 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -2086,40 +2086,32 @@ remote_set_syscall_catchpoint (struct target_ops *self,
 			  pid, needed, any_count, n_sysno);
     }
 
-  gdb::unique_xmalloc_ptr<char> built_packet;
+  std::string built_packet;
   if (needed)
     {
       /* Prepare a packet with the sysno list, assuming max 8+1
 	 characters for a sysno.  If the resulting packet size is too
 	 big, fallback on the non-selective packet.  */
       const int maxpktsz = strlen ("QCatchSyscalls:1") + n_sysno * 9 + 1;
-
-      built_packet.reset ((char *) xmalloc (maxpktsz));
-      strcpy (built_packet.get (), "QCatchSyscalls:1");
+      built_packet.reserve (maxpktsz);
+      built_packet = "QCatchSyscalls:1";
       if (!any_count)
 	{
-	  int i;
-	  char *p;
-
-	  p = built_packet.get ();
-	  p += strlen (p);
-
 	  /* Add in catch_packet each syscall to be caught (table[i] != 0).  */
-	  for (i = 0; i < table_size; i++)
+	  for (int i = 0; i < table_size; i++)
 	    {
 	      if (table[i] != 0)
-		p += xsnprintf (p, built_packet.get () + maxpktsz - p,
-				";%x", i);
+		string_appendf (built_packet, ";%x", i);
 	    }
 	}
-      if (strlen (built_packet.get ()) > get_remote_packet_size ())
+      if (built_packet.size () > get_remote_packet_size ())
 	{
 	  /* catch_packet too big.  Fallback to less efficient
 	     non selective mode, with GDB doing the filtering.  */
 	  catch_packet = "QCatchSyscalls:1";
 	}
       else
-	catch_packet = built_packet.get ();
+	catch_packet = built_packet.c_str ();
     }
   else
     catch_packet = "QCatchSyscalls:0";
-- 
2.5.5

Re: [RFA 4/6] Simple cleanup removals in remote.c

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> This suggests to me that we're missing a string_printf variant
Pedro> that appends to a preexisting string:
Pedro>   void string_appendf (std::string &dest, const char* fmt, ...);
Pedro> See (untested) patch below.

Seems like a good idea FWIW.

Tom

[PATCH 1/2] Introduce string_appendf/string_vappendf (Re: [RFA 4/6] Simple cleanup removals in remote.c)

[Pedro Alves]

On 10/17/2017 12:00 AM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> This suggests to me that we're missing a string_printf variant
> Pedro> that appends to a preexisting string:
> Pedro>   void string_appendf (std::string &dest, const char* fmt, ...);
> Pedro> See (untested) patch below.
> 
> Seems like a good idea FWIW.

Alright, here's a version with unit tests, then.

From 7d51020e1f1f77d9bfc3a4a06be19d1cbb889500 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 16 Oct 2017 23:22:27 +0100
Subject: [PATCH] Introduce string_appendf/string_vappendf

string_appendf is like string_printf, but instead of allocating a new
string, it appends to an existing string.  This allows reusing a
std::string's memory buffer across several calls, for example.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* common/common-utils.c (string_appendf, string_vappendf): New
	functions.
	* common/common-utils.h (string_appendf, string_vappendf): New
	declarations.
	* remote.c (remote_set_syscall_catchpoint): Use string_append.
	* unittests/common-utils-selftests.c (string_appendf_func)
	(test_appendf_func, string_vappendf_wrapper, string_appendf_tests)
	(string_vappendf_tests): New functions.
	(_initialize_common_utils_selftests): Register "string_appendf" and
	"string_vappendf tests".
---
 gdb/common/common-utils.c              | 44 +++++++++++++++++++++++++++++++
 gdb/common/common-utils.h              |  9 +++++++
 gdb/unittests/common-utils-selftests.c | 47 ++++++++++++++++++++++++++++++++++
 3 files changed, 100 insertions(+)

diff --git a/gdb/common/common-utils.c b/gdb/common/common-utils.c
index d8c546a..942aebb 100644
--- a/gdb/common/common-utils.c
+++ b/gdb/common/common-utils.c
@@ -195,6 +195,50 @@ string_vprintf (const char* fmt, va_list args)
   return str;
 }
 
+
+/* See documentation in common-utils.h.  */
+
+void
+string_appendf (std::string &str, const char *fmt, ...)
+{
+  va_list vp;
+  int grow_size;
+
+  va_start (vp, fmt);
+  grow_size = vsnprintf (NULL, 0, fmt, vp);
+  va_end (vp);
+
+  size_t curr_size = str.size ();
+  str.resize (curr_size + grow_size);
+
+  /* C++11 and later guarantee std::string uses contiguous memory and
+     always includes the terminating '\0'.  */
+  va_start (vp, fmt);
+  vsprintf (&str[curr_size], fmt, vp);
+  va_end (vp);
+}
+
+
+/* See documentation in common-utils.h.  */
+
+void
+string_vappendf (std::string &str, const char *fmt, va_list args)
+{
+  va_list vp;
+  int grow_size;
+
+  va_copy (vp, args);
+  grow_size = vsnprintf (NULL, 0, fmt, vp);
+  va_end (vp);
+
+  size_t curr_size = str.size ();
+  str.resize (curr_size + grow_size);
+
+  /* C++11 and later guarantee std::string uses contiguous memory and
+     always includes the terminating '\0'.  */
+  vsprintf (&str[curr_size], fmt, args);
+}
+
 char *
 savestring (const char *ptr, size_t len)
 {
diff --git a/gdb/common/common-utils.h b/gdb/common/common-utils.h
index 19724f9..a32863c 100644
--- a/gdb/common/common-utils.h
+++ b/gdb/common/common-utils.h
@@ -67,6 +67,15 @@ std::string string_printf (const char* fmt, ...)
 std::string string_vprintf (const char* fmt, va_list args)
   ATTRIBUTE_PRINTF (1, 0);
 
+/* Like string_printf, but appends to DEST instead of returning a new
+   std::string.  */
+void string_appendf (std::string &dest, const char* fmt, ...)
+  ATTRIBUTE_PRINTF (2, 3);
+
+/* Like string_appendf, but takes a va_list.  */
+void string_vappendf (std::string &dest, const char* fmt, va_list args)
+  ATTRIBUTE_PRINTF (2, 0);
+
 /* Make a copy of the string at PTR with LEN characters
    (and add a null character at the end in the copy).
    Uses malloc to get the space.  Returns the address of the copy.  */
diff --git a/gdb/unittests/common-utils-selftests.c b/gdb/unittests/common-utils-selftests.c
index cf65513..9825845 100644
--- a/gdb/unittests/common-utils-selftests.c
+++ b/gdb/unittests/common-utils-selftests.c
@@ -76,6 +76,51 @@ string_vprintf_tests ()
   test_format_func (format);
 }
 
+/* Type of both 'string_appendf' and the 'string_vappendf_wrapper'
+   function below.  Used to run the same tests against both
+   string_appendf and string_vappendf.  */
+typedef void (string_appendf_func) (std::string &str, const char *fmt, ...);
+
+static void
+test_appendf_func (string_appendf_func *func)
+{
+  std::string str;
+
+  func (str, "%s", "");
+  SELF_CHECK (str == "");
+
+  func (str, "%s", "test");
+  SELF_CHECK (str == "test");
+
+  func (str, "%d", 23);
+  SELF_CHECK (str == "test23");
+
+  func (str, "%s %d %s", "foo", 45, "bar");
+  SELF_CHECK (str == "test23foo 45 bar");
+}
+
+static void
+string_vappendf_wrapper (std::string &str, const char *fmt, ...)
+{
+  va_list vp;
+
+  va_start (vp, fmt);
+  string_vappendf (str, fmt, vp);
+  va_end (vp);
+}
+
+static void
+string_appendf_tests ()
+{
+  test_appendf_func (string_appendf);
+}
+
+static void
+string_vappendf_tests ()
+{
+  test_appendf_func (string_vappendf_wrapper);
+}
+
 } /* namespace selftests */
 
 void
@@ -83,4 +128,6 @@ _initialize_common_utils_selftests ()
 {
   selftests::register_test ("string_printf", selftests::string_printf_tests);
   selftests::register_test ("string_vprintf", selftests::string_vprintf_tests);
+  selftests::register_test ("string_appendf", selftests::string_appendf_tests);
+  selftests::register_test ("string_vappendf", selftests::string_vappendf_tests);
 }
-- 
2.5.5

Re: [PATCH 1/2] Introduce string_appendf/string_vappendf (Re: [RFA 4/6] Simple cleanup removals in remote.c)

[Simon Marchi]

On 2017-10-16 19:34, Pedro Alves wrote:
> On 10/17/2017 12:00 AM, Tom Tromey wrote:
>>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>> 
>> Pedro> This suggests to me that we're missing a string_printf variant
>> Pedro> that appends to a preexisting string:
>> Pedro>   void string_appendf (std::string &dest, const char* fmt, 
>> ...);
>> Pedro> See (untested) patch below.
>> 
>> Seems like a good idea FWIW.
> 
> Alright, here's a version with unit tests, then.
> 
> From 7d51020e1f1f77d9bfc3a4a06be19d1cbb889500 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Mon, 16 Oct 2017 23:22:27 +0100
> Subject: [PATCH] Introduce string_appendf/string_vappendf
> 
> string_appendf is like string_printf, but instead of allocating a new
> string, it appends to an existing string.  This allows reusing a
> std::string's memory buffer across several calls, for example.
> 
> gdb/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
> 
> 	* common/common-utils.c (string_appendf, string_vappendf): New
> 	functions.
> 	* common/common-utils.h (string_appendf, string_vappendf): New
> 	declarations.
> 	* remote.c (remote_set_syscall_catchpoint): Use string_append.
> 	* unittests/common-utils-selftests.c (string_appendf_func)
> 	(test_appendf_func, string_vappendf_wrapper, string_appendf_tests)
> 	(string_vappendf_tests): New functions.
> 	(_initialize_common_utils_selftests): Register "string_appendf" and
> 	"string_vappendf tests".
> ---
>  gdb/common/common-utils.c              | 44 
> +++++++++++++++++++++++++++++++
>  gdb/common/common-utils.h              |  9 +++++++
>  gdb/unittests/common-utils-selftests.c | 47 
> ++++++++++++++++++++++++++++++++++
>  3 files changed, 100 insertions(+)
> 
> diff --git a/gdb/common/common-utils.c b/gdb/common/common-utils.c
> index d8c546a..942aebb 100644
> --- a/gdb/common/common-utils.c
> +++ b/gdb/common/common-utils.c
> @@ -195,6 +195,50 @@ string_vprintf (const char* fmt, va_list args)
>    return str;
>  }
> 
> +
> +/* See documentation in common-utils.h.  */
> +
> +void
> +string_appendf (std::string &str, const char *fmt, ...)
> +{
> +  va_list vp;
> +  int grow_size;
> +
> +  va_start (vp, fmt);
> +  grow_size = vsnprintf (NULL, 0, fmt, vp);
> +  va_end (vp);
> +
> +  size_t curr_size = str.size ();
> +  str.resize (curr_size + grow_size);
> +
> +  /* C++11 and later guarantee std::string uses contiguous memory and
> +     always includes the terminating '\0'.  */
> +  va_start (vp, fmt);
> +  vsprintf (&str[curr_size], fmt, vp);
> +  va_end (vp);
> +}
> +
> +
> +/* See documentation in common-utils.h.  */
> +
> +void
> +string_vappendf (std::string &str, const char *fmt, va_list args)
> +{
> +  va_list vp;
> +  int grow_size;
> +
> +  va_copy (vp, args);
> +  grow_size = vsnprintf (NULL, 0, fmt, vp);
> +  va_end (vp);
> +
> +  size_t curr_size = str.size ();
> +  str.resize (curr_size + grow_size);
> +
> +  /* C++11 and later guarantee std::string uses contiguous memory and
> +     always includes the terminating '\0'.  */
> +  vsprintf (&str[curr_size], fmt, args);
> +}
> +

string_appendf can be implemented using string_vappendf, to reduce 
duplication.  It would basically be like string_vappendf_wrapper is.  In 
the tests, we can probably just test string_appendf then.

Unless there's a good reason for them not sharing code?

>  char *
>  savestring (const char *ptr, size_t len)
>  {
> diff --git a/gdb/common/common-utils.h b/gdb/common/common-utils.h
> index 19724f9..a32863c 100644
> --- a/gdb/common/common-utils.h
> +++ b/gdb/common/common-utils.h
> @@ -67,6 +67,15 @@ std::string string_printf (const char* fmt, ...)
>  std::string string_vprintf (const char* fmt, va_list args)
>    ATTRIBUTE_PRINTF (1, 0);
> 
> +/* Like string_printf, but appends to DEST instead of returning a new
> +   std::string.  */
> +void string_appendf (std::string &dest, const char* fmt, ...)
> +  ATTRIBUTE_PRINTF (2, 3);
> +
> +/* Like string_appendf, but takes a va_list.  */
> +void string_vappendf (std::string &dest, const char* fmt, va_list 
> args)
> +  ATTRIBUTE_PRINTF (2, 0);
> +
>  /* Make a copy of the string at PTR with LEN characters
>     (and add a null character at the end in the copy).
>     Uses malloc to get the space.  Returns the address of the copy.  */
> diff --git a/gdb/unittests/common-utils-selftests.c
> b/gdb/unittests/common-utils-selftests.c
> index cf65513..9825845 100644
> --- a/gdb/unittests/common-utils-selftests.c
> +++ b/gdb/unittests/common-utils-selftests.c
> @@ -76,6 +76,51 @@ string_vprintf_tests ()
>    test_format_func (format);
>  }
> 
> +/* Type of both 'string_appendf' and the 'string_vappendf_wrapper'
> +   function below.  Used to run the same tests against both
> +   string_appendf and string_vappendf.  */
> +typedef void (string_appendf_func) (std::string &str, const char *fmt, 
> ...);
> +
> +static void
> +test_appendf_func (string_appendf_func *func)
> +{
> +  std::string str;
> +
> +  func (str, "%s", "");
> +  SELF_CHECK (str == "");
> +
> +  func (str, "%s", "test");
> +  SELF_CHECK (str == "test");
> +
> +  func (str, "%d", 23);
> +  SELF_CHECK (str == "test23");
> +
> +  func (str, "%s %d %s", "foo", 45, "bar");
> +  SELF_CHECK (str == "test23foo 45 bar");
> +}
> +
> +static void
> +string_vappendf_wrapper (std::string &str, const char *fmt, ...)
> +{
> +  va_list vp;
> +
> +  va_start (vp, fmt);
> +  string_vappendf (str, fmt, vp);
> +  va_end (vp);
> +}
> +
> +static void
> +string_appendf_tests ()
> +{
> +  test_appendf_func (string_appendf);
> +}
> +
> +static void
> +string_vappendf_tests ()
> +{
> +  test_appendf_func (string_vappendf_wrapper);
> +}
> +
>  } /* namespace selftests */
> 
>  void
> @@ -83,4 +128,6 @@ _initialize_common_utils_selftests ()
>  {
>    selftests::register_test ("string_printf", 
> selftests::string_printf_tests);
>    selftests::register_test ("string_vprintf", 
> selftests::string_vprintf_tests);
> +  selftests::register_test ("string_appendf", 
> selftests::string_appendf_tests);
> +  selftests::register_test ("string_vappendf",
> selftests::string_vappendf_tests);

The last line is too long.

>  }

Otherwise, LGTM.

Simon

Re: [PATCH 1/2] Introduce string_appendf/string_vappendf (Re: [RFA 4/6] Simple cleanup removals in remote.c)

[Simon Marchi]

On 2017-10-18 23:11, Simon Marchi wrote:
>> +
>> +/* See documentation in common-utils.h.  */
>> +
>> +void
>> +string_appendf (std::string &str, const char *fmt, ...)
>> +{
>> +  va_list vp;
>> +  int grow_size;
>> +
>> +  va_start (vp, fmt);
>> +  grow_size = vsnprintf (NULL, 0, fmt, vp);
>> +  va_end (vp);
>> +
>> +  size_t curr_size = str.size ();
>> +  str.resize (curr_size + grow_size);
>> +
>> +  /* C++11 and later guarantee std::string uses contiguous memory and
>> +     always includes the terminating '\0'.  */
>> +  va_start (vp, fmt);
>> +  vsprintf (&str[curr_size], fmt, vp);
>> +  va_end (vp);
>> +}
>> +
>> +
>> +/* See documentation in common-utils.h.  */
>> +
>> +void
>> +string_vappendf (std::string &str, const char *fmt, va_list args)
>> +{
>> +  va_list vp;
>> +  int grow_size;
>> +
>> +  va_copy (vp, args);
>> +  grow_size = vsnprintf (NULL, 0, fmt, vp);
>> +  va_end (vp);
>> +
>> +  size_t curr_size = str.size ();
>> +  str.resize (curr_size + grow_size);
>> +
>> +  /* C++11 and later guarantee std::string uses contiguous memory and
>> +     always includes the terminating '\0'.  */
>> +  vsprintf (&str[curr_size], fmt, args);
>> +}
>> +
> 
> string_appendf can be implemented using string_vappendf, to reduce
> duplication.  It would basically be like string_vappendf_wrapper is.
> In the tests, we can probably just test string_appendf then.
> 
> Unless there's a good reason for them not sharing code?

Actually the same comment would apply to string_{v,}printf.

Simon

Re: [PATCH 1/2] Introduce string_appendf/string_vappendf (Re: [RFA 4/6] Simple cleanup removals in remote.c)

[Simon Marchi]

On 2017-10-18 23:12, Simon Marchi wrote:
> On 2017-10-18 23:11, Simon Marchi wrote:
>>> +
>>> +/* See documentation in common-utils.h.  */
>>> +
>>> +void
>>> +string_appendf (std::string &str, const char *fmt, ...)
>>> +{
>>> +  va_list vp;
>>> +  int grow_size;
>>> +
>>> +  va_start (vp, fmt);
>>> +  grow_size = vsnprintf (NULL, 0, fmt, vp);
>>> +  va_end (vp);
>>> +
>>> +  size_t curr_size = str.size ();
>>> +  str.resize (curr_size + grow_size);
>>> +
>>> +  /* C++11 and later guarantee std::string uses contiguous memory 
>>> and
>>> +     always includes the terminating '\0'.  */
>>> +  va_start (vp, fmt);
>>> +  vsprintf (&str[curr_size], fmt, vp);
>>> +  va_end (vp);
>>> +}
>>> +
>>> +
>>> +/* See documentation in common-utils.h.  */
>>> +
>>> +void
>>> +string_vappendf (std::string &str, const char *fmt, va_list args)
>>> +{
>>> +  va_list vp;
>>> +  int grow_size;
>>> +
>>> +  va_copy (vp, args);
>>> +  grow_size = vsnprintf (NULL, 0, fmt, vp);
>>> +  va_end (vp);
>>> +
>>> +  size_t curr_size = str.size ();
>>> +  str.resize (curr_size + grow_size);
>>> +
>>> +  /* C++11 and later guarantee std::string uses contiguous memory 
>>> and
>>> +     always includes the terminating '\0'.  */
>>> +  vsprintf (&str[curr_size], fmt, args);
>>> +}
>>> +
>> 
>> string_appendf can be implemented using string_vappendf, to reduce
>> duplication.  It would basically be like string_vappendf_wrapper is.
>> In the tests, we can probably just test string_appendf then.
>> 
>> Unless there's a good reason for them not sharing code?
> 
> Actually the same comment would apply to string_{v,}printf.
> 
> Simon

Hi Pedro,

I came across a case where string_appendf would be useful, so I was 
wondering if you intended to push this patch.  If you don't have the 
time, I would be happy to take care of it, just tell me if you agree 
with my comments and I'll make the changes & push.

Simon

Re: [PATCH 1/2] Introduce string_appendf/string_vappendf (Re: [RFA 4/6] Simple cleanup removals in remote.c)

[Pedro Alves]

On 10/19/2017 04:11 AM, Simon Marchi wrote:

> string_appendf can be implemented using string_vappendf, to reduce
> duplication.  It would basically be like string_vappendf_wrapper is.  In
> the tests, we can probably just test string_appendf then.
> 
> Unless there's a good reason for them not sharing code?

No really good reason.  It'd save some branching, I guess.

I've done that now.  I still left string_vappendf_wrapper
in place because I think it's better to test both entry points
in case the implementation of string_appendf changes from
the trivial wrapper to something else for some reason.  At that
point, the string_vappendf_wrapper can still be left as the
simple wrapper that it is, irrespective of how string_appendf
is implemented.

>> @@ -83,4 +128,6 @@ _initialize_common_utils_selftests ()
>>  {
>>    selftests::register_test ("string_printf",
>> selftests::string_printf_tests);
>>    selftests::register_test ("string_vprintf",
>> selftests::string_vprintf_tests);
>> +  selftests::register_test ("string_appendf",
>> selftests::string_appendf_tests);
>> +  selftests::register_test ("string_vappendf",
>> selftests::string_vappendf_tests);
> 
> The last line is too long.

Fixed.  Below's what I pushed.

From 31b833b3eab69d91df67edc3e9a21792abc3f93e Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 30 Oct 2017 11:41:34 +0000
Subject: [PATCH] Introduce string_appendf/string_vappendf

string_appendf is like string_printf, but instead of allocating a new
string, it appends to an existing string.  This allows reusing a
std::string's memory buffer across several calls, for example.

gdb/ChangeLog:
2017-10-30  Pedro Alves  <palves@redhat.com>

	* common/common-utils.c (string_appendf, string_vappendf): New
	functions.
	* common/common-utils.h (string_appendf, string_vappendf): New
	declarations.
	* unittests/common-utils-selftests.c (string_appendf_func)
	(test_appendf_func, string_vappendf_wrapper, string_appendf_tests)
	(string_vappendf_tests): New functions.
	(_initialize_common_utils_selftests): Register "string_appendf" and
	"string_vappendf tests".
---
 gdb/ChangeLog                          | 12 +++++++++
 gdb/common/common-utils.c              | 34 +++++++++++++++++++++++
 gdb/common/common-utils.h              |  9 +++++++
 gdb/unittests/common-utils-selftests.c | 49 ++++++++++++++++++++++++++++++++++
 4 files changed, 104 insertions(+)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 20d78ba..f3abf16 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,17 @@
 2017-10-30  Pedro Alves  <palves@redhat.com>
 
+	* common/common-utils.c (string_appendf, string_vappendf): New
+	functions.
+	* common/common-utils.h (string_appendf, string_vappendf): New
+	declarations.
+	* unittests/common-utils-selftests.c (string_appendf_func)
+	(test_appendf_func, string_vappendf_wrapper, string_appendf_tests)
+	(string_vappendf_tests): New functions.
+	(_initialize_common_utils_selftests): Register "string_appendf" and
+	"string_vappendf tests".
+
+2017-10-30  Pedro Alves  <palves@redhat.com>
+
 	* unittests/common-utils-selftests.c (format_func): New typedef.
 	(string_printf_tests, string_vprintf_tests): Tests factored out
 	and merged to ...
diff --git a/gdb/common/common-utils.c b/gdb/common/common-utils.c
index d8c546a..7139302 100644
--- a/gdb/common/common-utils.c
+++ b/gdb/common/common-utils.c
@@ -195,6 +195,40 @@ string_vprintf (const char* fmt, va_list args)
   return str;
 }
 
+
+/* See documentation in common-utils.h.  */
+
+void
+string_appendf (std::string &str, const char *fmt, ...)
+{
+  va_list vp;
+
+  va_start (vp, fmt);
+  string_vappendf (str, fmt, vp);
+  va_end (vp);
+}
+
+
+/* See documentation in common-utils.h.  */
+
+void
+string_vappendf (std::string &str, const char *fmt, va_list args)
+{
+  va_list vp;
+  int grow_size;
+
+  va_copy (vp, args);
+  grow_size = vsnprintf (NULL, 0, fmt, vp);
+  va_end (vp);
+
+  size_t curr_size = str.size ();
+  str.resize (curr_size + grow_size);
+
+  /* C++11 and later guarantee std::string uses contiguous memory and
+     always includes the terminating '\0'.  */
+  vsprintf (&str[curr_size], fmt, args);
+}
+
 char *
 savestring (const char *ptr, size_t len)
 {
diff --git a/gdb/common/common-utils.h b/gdb/common/common-utils.h
index 19724f9..a32863c 100644
--- a/gdb/common/common-utils.h
+++ b/gdb/common/common-utils.h
@@ -67,6 +67,15 @@ std::string string_printf (const char* fmt, ...)
 std::string string_vprintf (const char* fmt, va_list args)
   ATTRIBUTE_PRINTF (1, 0);
 
+/* Like string_printf, but appends to DEST instead of returning a new
+   std::string.  */
+void string_appendf (std::string &dest, const char* fmt, ...)
+  ATTRIBUTE_PRINTF (2, 3);
+
+/* Like string_appendf, but takes a va_list.  */
+void string_vappendf (std::string &dest, const char* fmt, va_list args)
+  ATTRIBUTE_PRINTF (2, 0);
+
 /* Make a copy of the string at PTR with LEN characters
    (and add a null character at the end in the copy).
    Uses malloc to get the space.  Returns the address of the copy.  */
diff --git a/gdb/unittests/common-utils-selftests.c b/gdb/unittests/common-utils-selftests.c
index 596406e..0e1d2c6 100644
--- a/gdb/unittests/common-utils-selftests.c
+++ b/gdb/unittests/common-utils-selftests.c
@@ -77,6 +77,52 @@ string_vprintf_tests ()
   test_format_func (format);
 }
 
+/* Type of both 'string_appendf' and the 'string_vappendf_wrapper'
+   function below.  Used to run the same tests against both
+   string_appendf and string_vappendf.  */
+typedef void (string_appendf_func) (std::string &str, const char *fmt, ...)
+  ATTRIBUTE_PRINTF (2, 3);
+
+static void
+test_appendf_func (string_appendf_func *func)
+{
+  std::string str;
+
+  func (str, "%s", "");
+  SELF_CHECK (str == "");
+
+  func (str, "%s", "test");
+  SELF_CHECK (str == "test");
+
+  func (str, "%d", 23);
+  SELF_CHECK (str == "test23");
+
+  func (str, "%s %d %s", "foo", 45, "bar");
+  SELF_CHECK (str == "test23foo 45 bar");
+}
+
+static void ATTRIBUTE_PRINTF (2, 3)
+string_vappendf_wrapper (std::string &str, const char *fmt, ...)
+{
+  va_list vp;
+
+  va_start (vp, fmt);
+  string_vappendf (str, fmt, vp);
+  va_end (vp);
+}
+
+static void
+string_appendf_tests ()
+{
+  test_appendf_func (string_appendf);
+}
+
+static void
+string_vappendf_tests ()
+{
+  test_appendf_func (string_vappendf_wrapper);
+}
+
 } /* namespace selftests */
 
 void
@@ -84,4 +130,7 @@ _initialize_common_utils_selftests ()
 {
   selftests::register_test ("string_printf", selftests::string_printf_tests);
   selftests::register_test ("string_vprintf", selftests::string_vprintf_tests);
+  selftests::register_test ("string_appendf", selftests::string_appendf_tests);
+  selftests::register_test ("string_vappendf",
+			    selftests::string_vappendf_tests);
 }
-- 
2.5.5

[PATCH 2/2] remote.c, QCatchSyscalls: Build std::string instead of unique_xmalloc_ptr (Re: [RFA 4/6] Simple cleanup removals in remote.c)

[Pedro Alves]

On 10/17/2017 12:00 AM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> This suggests to me that we're missing a string_printf variant
> Pedro> that appends to a preexisting string:
> Pedro>   void string_appendf (std::string &dest, const char* fmt, ...);
> Pedro> See (untested) patch below.
> 
> Seems like a good idea FWIW.

And here's the remote.c bit making use of string_appendf, slit out as
a proper patch.

Forgot to mention in 1/2, but this applies on top of:
  https://sourceware.org/ml/gdb-patches/2017-10/msg00485.html

From 19283964911072c76ee530c58a46fd4b87dfbaae Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 17 Oct 2017 00:28:46 +0100
Subject: [PATCH] remote.c, QCatchSyscalls: Build std::string instead of
 unique_xmalloc_ptr

Simplify the code a little bit using std::string + string_appendf.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
	    Simon Marchi <simon.marchi@ericsson.com>

	* remote.c (remote_set_syscall_catchpoint): Build a std::string
	instead of a gdb::unique_xmalloc_ptr, using string_appendf.
---
 gdb/remote.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 6b77a9f..a6cb724 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -2086,40 +2086,32 @@ remote_set_syscall_catchpoint (struct target_ops *self,
 			  pid, needed, any_count, n_sysno);
     }
 
-  gdb::unique_xmalloc_ptr<char> built_packet;
+  std::string built_packet;
   if (needed)
     {
       /* Prepare a packet with the sysno list, assuming max 8+1
 	 characters for a sysno.  If the resulting packet size is too
 	 big, fallback on the non-selective packet.  */
       const int maxpktsz = strlen ("QCatchSyscalls:1") + n_sysno * 9 + 1;
-
-      built_packet.reset ((char *) xmalloc (maxpktsz));
-      strcpy (built_packet.get (), "QCatchSyscalls:1");
+      built_packet.reserve (maxpktsz);
+      built_packet = "QCatchSyscalls:1";
       if (!any_count)
 	{
-	  int i;
-	  char *p;
-
-	  p = built_packet.get ();
-	  p += strlen (p);
-
 	  /* Add in catch_packet each syscall to be caught (table[i] != 0).  */
-	  for (i = 0; i < table_size; i++)
+	  for (int i = 0; i < table_size; i++)
 	    {
 	      if (table[i] != 0)
-		p += xsnprintf (p, built_packet.get () + maxpktsz - p,
-				";%x", i);
+		string_appendf (built_packet, ";%x", i);
 	    }
 	}
-      if (strlen (built_packet.get ()) > get_remote_packet_size ())
+      if (built_packet.size () > get_remote_packet_size ())
 	{
 	  /* catch_packet too big.  Fallback to less efficient
 	     non selective mode, with GDB doing the filtering.  */
 	  catch_packet = "QCatchSyscalls:1";
 	}
       else
-	catch_packet = built_packet.get ();
+	catch_packet = built_packet.c_str ();
     }
   else
     catch_packet = "QCatchSyscalls:0";
-- 
2.5.5

Re: [PATCH 2/2] remote.c, QCatchSyscalls: Build std::string instead of unique_xmalloc_ptr (Re: [RFA 4/6] Simple cleanup removals in remote.c)

[Simon Marchi]

On 2017-10-16 19:38, Pedro Alves wrote:
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 6b77a9f..a6cb724 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -2086,40 +2086,32 @@ remote_set_syscall_catchpoint (struct 
> target_ops *self,
>  			  pid, needed, any_count, n_sysno);
>      }
> 
> -  gdb::unique_xmalloc_ptr<char> built_packet;
> +  std::string built_packet;
>    if (needed)
>      {
>        /* Prepare a packet with the sysno list, assuming max 8+1
>  	 characters for a sysno.  If the resulting packet size is too
>  	 big, fallback on the non-selective packet.  */
>        const int maxpktsz = strlen ("QCatchSyscalls:1") + n_sysno * 9 + 
> 1;
> -
> -      built_packet.reset ((char *) xmalloc (maxpktsz));
> -      strcpy (built_packet.get (), "QCatchSyscalls:1");
> +      built_packet.reserve (maxpktsz);
> +      built_packet = "QCatchSyscalls:1";
>        if (!any_count)
>  	{
> -	  int i;
> -	  char *p;
> -
> -	  p = built_packet.get ();
> -	  p += strlen (p);
> -
>  	  /* Add in catch_packet each syscall to be caught (table[i] != 0).  
> */
> -	  for (i = 0; i < table_size; i++)
> +	  for (int i = 0; i < table_size; i++)
>  	    {
>  	      if (table[i] != 0)
> -		p += xsnprintf (p, built_packet.get () + maxpktsz - p,
> -				";%x", i);
> +		string_appendf (built_packet, ";%x", i);
>  	    }
>  	}
> -      if (strlen (built_packet.get ()) > get_remote_packet_size ())
> +      if (built_packet.size () > get_remote_packet_size ())
>  	{
>  	  /* catch_packet too big.  Fallback to less efficient
>  	     non selective mode, with GDB doing the filtering.  */
>  	  catch_packet = "QCatchSyscalls:1";
>  	}
>        else
> -	catch_packet = built_packet.get ();
> +	catch_packet = built_packet.c_str ();

You can get rid of built_packet, and make catch_packet the std::string.  
And then this last else branch can be removed.

Simon

Re: [PATCH 2/2] remote.c, QCatchSyscalls: Build std::string instead of unique_xmalloc_ptr (Re: [RFA 4/6] Simple cleanup removals in remote.c)

[Pedro Alves]

On 10/19/2017 04:17 AM, Simon Marchi wrote:
> On 2017-10-16 19:38, Pedro Alves wrote:
>> diff --git a/gdb/remote.c b/gdb/remote.c
>> index 6b77a9f..a6cb724 100644
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -2086,40 +2086,32 @@ remote_set_syscall_catchpoint (struct
>> target_ops *self,
>>                pid, needed, any_count, n_sysno);
>>      }
>>
>> -  gdb::unique_xmalloc_ptr<char> built_packet;
>> +  std::string built_packet;
>>    if (needed)
>>      {
>>        /* Prepare a packet with the sysno list, assuming max 8+1
>>       characters for a sysno.  If the resulting packet size is too
>>       big, fallback on the non-selective packet.  */
>>        const int maxpktsz = strlen ("QCatchSyscalls:1") + n_sysno * 9
>> + 1;
>> -
>> -      built_packet.reset ((char *) xmalloc (maxpktsz));
>> -      strcpy (built_packet.get (), "QCatchSyscalls:1");
>> +      built_packet.reserve (maxpktsz);
>> +      built_packet = "QCatchSyscalls:1";
>>        if (!any_count)
>>      {
>> -      int i;
>> -      char *p;
>> -
>> -      p = built_packet.get ();
>> -      p += strlen (p);
>> -
>>        /* Add in catch_packet each syscall to be caught (table[i] !=
>> 0).  */
>> -      for (i = 0; i < table_size; i++)
>> +      for (int i = 0; i < table_size; i++)
>>          {
>>            if (table[i] != 0)
>> -        p += xsnprintf (p, built_packet.get () + maxpktsz - p,
>> -                ";%x", i);
>> +        string_appendf (built_packet, ";%x", i);
>>          }
>>      }
>> -      if (strlen (built_packet.get ()) > get_remote_packet_size ())
>> +      if (built_packet.size () > get_remote_packet_size ())
>>      {
>>        /* catch_packet too big.  Fallback to less efficient
>>           non selective mode, with GDB doing the filtering.  */
>>        catch_packet = "QCatchSyscalls:1";
>>      }
>>        else
>> -    catch_packet = built_packet.get ();
>> +    catch_packet = built_packet.c_str ();
> 
> You can get rid of built_packet, and make catch_packet the std::string. 
> And then this last else branch can be removed.

The reason I didn't do that is because that'd introduce an
allocation + one deep string copy in the '!needed' case:

  else
    catch_packet = "QCatchSyscalls:0";

when it's very easy to avoid.

So I pushed the patch in as is.

Thanks,
Pedro Alves

[RFA 3/6] Remove cleanup from ppc-linux-nat.c

[Tom Tromey]

This removes a cleanup from ppc-linux-nat.c, by using
unique_xmalloc_ptr.  It also slightly simplifies the code by using
XDUP rather than XNEW and memcpy.

ChangeLog
2017-10-15  Tom Tromey  <tom@tromey.com>

	* ppc-linux-nat.c (hwdebug_insert_point): Use
	gdb::unique_xmalloc_ptr, XDUP.
---
 gdb/ChangeLog       |  5 +++++
 gdb/ppc-linux-nat.c | 11 +++--------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
index 45c8903ef6..7c8ab6c67b 100644
--- a/gdb/ppc-linux-nat.c
+++ b/gdb/ppc-linux-nat.c
@@ -1541,16 +1541,13 @@ hwdebug_insert_point (struct ppc_hw_breakpoint *b, int tid)
 {
   int i;
   long slot;
-  struct ppc_hw_breakpoint *p = XNEW (struct ppc_hw_breakpoint);
+  gdb::unique_xmalloc_ptr<ppc_hw_breakpoint> p (XDUP (ppc_hw_breakpoint, b));
   struct hw_break_tuple *hw_breaks;
-  struct cleanup *c = make_cleanup (xfree, p);
   struct thread_points *t;
   struct hw_break_tuple *tuple;
 
-  memcpy (p, b, sizeof (struct ppc_hw_breakpoint));
-
   errno = 0;
-  slot = ptrace (PPC_PTRACE_SETHWDEBUG, tid, 0, p);
+  slot = ptrace (PPC_PTRACE_SETHWDEBUG, tid, 0, p.get ());
   if (slot < 0)
     perror_with_name (_("Unexpected error setting breakpoint or watchpoint"));
 
@@ -1564,13 +1561,11 @@ hwdebug_insert_point (struct ppc_hw_breakpoint *b, int tid)
     if (hw_breaks[i].hw_break == NULL)
       {
 	hw_breaks[i].slot = slot;
-	hw_breaks[i].hw_break = p;
+	hw_breaks[i].hw_break = p.release ();
 	break;
       }
 
   gdb_assert (i != max_slots_number);
-
-  discard_cleanups (c);
 }
 
 /* This function is a generic wrapper that is responsible for removing a
-- 
2.13.6

Re: [RFA 3/6] Remove cleanup from ppc-linux-nat.c

[Simon Marchi]

On 2017-10-15 11:04 PM, Tom Tromey wrote:
> This removes a cleanup from ppc-linux-nat.c, by using
> unique_xmalloc_ptr.  It also slightly simplifies the code by using
> XDUP rather than XNEW and memcpy.
> 
> ChangeLog
> 2017-10-15  Tom Tromey  <tom@tromey.com>
> 
> 	* ppc-linux-nat.c (hwdebug_insert_point): Use
> 	gdb::unique_xmalloc_ptr, XDUP.
> ---
>  gdb/ChangeLog       |  5 +++++
>  gdb/ppc-linux-nat.c | 11 +++--------
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
> index 45c8903ef6..7c8ab6c67b 100644
> --- a/gdb/ppc-linux-nat.c
> +++ b/gdb/ppc-linux-nat.c
> @@ -1541,16 +1541,13 @@ hwdebug_insert_point (struct ppc_hw_breakpoint *b, int tid)
>  {
>    int i;
>    long slot;
> -  struct ppc_hw_breakpoint *p = XNEW (struct ppc_hw_breakpoint);
> +  gdb::unique_xmalloc_ptr<ppc_hw_breakpoint> p (XDUP (ppc_hw_breakpoint, b));
>    struct hw_break_tuple *hw_breaks;
> -  struct cleanup *c = make_cleanup (xfree, p);
>    struct thread_points *t;
>    struct hw_break_tuple *tuple;
>  
> -  memcpy (p, b, sizeof (struct ppc_hw_breakpoint));
> -
>    errno = 0;
> -  slot = ptrace (PPC_PTRACE_SETHWDEBUG, tid, 0, p);
> +  slot = ptrace (PPC_PTRACE_SETHWDEBUG, tid, 0, p.get ());
>    if (slot < 0)
>      perror_with_name (_("Unexpected error setting breakpoint or watchpoint"));
>  
> @@ -1564,13 +1561,11 @@ hwdebug_insert_point (struct ppc_hw_breakpoint *b, int tid)
>      if (hw_breaks[i].hw_break == NULL)
>        {
>  	hw_breaks[i].slot = slot;
> -	hw_breaks[i].hw_break = p;
> +	hw_breaks[i].hw_break = p.release ();
>  	break;
>        }
>  
>    gdb_assert (i != max_slots_number);
> -
> -  discard_cleanups (c);
>  }
>  
>  /* This function is a generic wrapper that is responsible for removing a
> 

I was going to say, it would make more sense to use the copy constructor:

  std::unique_ptr<ppc_hw_breakpoint> p (new ppc_hw_breakpoint (*b));

but it would require other changes, and it's probably out of scope for your
current goal, so LGTM.

Simon

[RFA 1/6] Use std::vector in end_symtab_get_static_block

[Tom Tromey]

Change end_symtab_get_static_block to use std::vector.  This removes a
cleanup.

ChangeLog
2017-10-15  Tom Tromey  <tom@tromey.com>

	* buildsym.c (block_compar): Remove.
	(end_symtab_get_static_block): Use std::vector.
---
 gdb/ChangeLog  |  5 +++++
 gdb/buildsym.c | 38 ++++++++++----------------------------
 2 files changed, 15 insertions(+), 28 deletions(-)

diff --git a/gdb/buildsym.c b/gdb/buildsym.c
index cbad027d0c..c3cfc37cd6 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -86,6 +86,7 @@
 #include "cp-support.h"
 #include "dictionary.h"
 #include "addrmap.h"
+#include <algorithm>
 
 /* Ask buildsym.h to define the vars it normally declares `extern'.  */
 #define	EXTERN
@@ -1165,19 +1166,6 @@ watch_main_source_file_lossage (void)
     }
 }
 
-/* Helper function for qsort.  Parameters are `struct block *' pointers,
-   function sorts them in descending order by their BLOCK_START.  */
-
-static int
-block_compar (const void *ap, const void *bp)
-{
-  const struct block *a = *(const struct block **) ap;
-  const struct block *b = *(const struct block **) bp;
-
-  return ((BLOCK_START (b) > BLOCK_START (a))
-	  - (BLOCK_START (b) < BLOCK_START (a)));
-}
-
 /* Reset state after a successful building of a symtab.
    This exists because dbxread.c and xcoffread.c can call
    start_symtab+end_symtab multiple times after one call to buildsym_init,
@@ -1254,28 +1242,22 @@ end_symtab_get_static_block (CORE_ADDR end_addr, int expandable, int required)
 
   if ((objfile->flags & OBJF_REORDERED) && pending_blocks)
     {
-      unsigned count = 0;
       struct pending_block *pb;
-      struct block **barray, **bp;
-      struct cleanup *back_to;
-
-      for (pb = pending_blocks; pb != NULL; pb = pb->next)
-	count++;
 
-      barray = XNEWVEC (struct block *, count);
-      back_to = make_cleanup (xfree, barray);
+      std::vector<block *> barray;
 
-      bp = barray;
       for (pb = pending_blocks; pb != NULL; pb = pb->next)
-	*bp++ = pb->block;
+	barray.push_back (pb->block);
 
-      qsort (barray, count, sizeof (*barray), block_compar);
+      std::sort (barray.begin (), barray.end (),
+		 [] (const block *a, const block *b)
+		 {
+		   return BLOCK_START (a) > BLOCK_START (b);
+		 });
 
-      bp = barray;
+      int i = 0;
       for (pb = pending_blocks; pb != NULL; pb = pb->next)
-	pb->block = *bp++;
-
-      do_cleanups (back_to);
+	pb->block = barray[i++];
     }
 
   /* Cleanup any undefined types that have been left hanging around
-- 
2.13.6

Re: [RFA 1/6] Use std::vector in end_symtab_get_static_block

[Simon Marchi]

On 2017-10-15 11:04 PM, Tom Tromey wrote:
> Change end_symtab_get_static_block to use std::vector.  This removes a
> cleanup.
> 
> ChangeLog
> 2017-10-15  Tom Tromey  <tom@tromey.com>
> 
> 	* buildsym.c (block_compar): Remove.
> 	(end_symtab_get_static_block): Use std::vector.
> ---
>  gdb/ChangeLog  |  5 +++++
>  gdb/buildsym.c | 38 ++++++++++----------------------------
>  2 files changed, 15 insertions(+), 28 deletions(-)
> 
> diff --git a/gdb/buildsym.c b/gdb/buildsym.c
> index cbad027d0c..c3cfc37cd6 100644
> --- a/gdb/buildsym.c
> +++ b/gdb/buildsym.c
> @@ -86,6 +86,7 @@
>  #include "cp-support.h"
>  #include "dictionary.h"
>  #include "addrmap.h"
> +#include <algorithm>
>  
>  /* Ask buildsym.h to define the vars it normally declares `extern'.  */
>  #define	EXTERN
> @@ -1165,19 +1166,6 @@ watch_main_source_file_lossage (void)
>      }
>  }
>  
> -/* Helper function for qsort.  Parameters are `struct block *' pointers,
> -   function sorts them in descending order by their BLOCK_START.  */
> -
> -static int
> -block_compar (const void *ap, const void *bp)
> -{
> -  const struct block *a = *(const struct block **) ap;
> -  const struct block *b = *(const struct block **) bp;
> -
> -  return ((BLOCK_START (b) > BLOCK_START (a))
> -	  - (BLOCK_START (b) < BLOCK_START (a)));
> -}
> -
>  /* Reset state after a successful building of a symtab.
>     This exists because dbxread.c and xcoffread.c can call
>     start_symtab+end_symtab multiple times after one call to buildsym_init,
> @@ -1254,28 +1242,22 @@ end_symtab_get_static_block (CORE_ADDR end_addr, int expandable, int required)
>  
>    if ((objfile->flags & OBJF_REORDERED) && pending_blocks)
>      {
> -      unsigned count = 0;
>        struct pending_block *pb;
> -      struct block **barray, **bp;
> -      struct cleanup *back_to;
> -
> -      for (pb = pending_blocks; pb != NULL; pb = pb->next)
> -	count++;
>  
> -      barray = XNEWVEC (struct block *, count);
> -      back_to = make_cleanup (xfree, barray);
> +      std::vector<block *> barray;
>  
> -      bp = barray;
>        for (pb = pending_blocks; pb != NULL; pb = pb->next)
> -	*bp++ = pb->block;
> +	barray.push_back (pb->block);
>  
> -      qsort (barray, count, sizeof (*barray), block_compar);
> +      std::sort (barray.begin (), barray.end (),
> +		 [] (const block *a, const block *b)
> +		 {
> +		   return BLOCK_START (a) > BLOCK_START (b);

That made me doubt for a second, since we're more used to see "<"
in these functions.  I then saw that block_compar did sort them
in descending order.  Could you add a comment here to indicate that?

> +		 });
>  
> -      bp = barray;
> +      int i = 0;
>        for (pb = pending_blocks; pb != NULL; pb = pb->next)
> -	pb->block = *bp++;
> -
> -      do_cleanups (back_to);
> +	pb->block = barray[i++];
>      }

I thought, why don't we replace pending_blocks by a vector and sort that in place
rather than copy the pointers to a temporary vector and copy them back, but it's
not easy and may not even be what we want.

LGTM with the comment added.

Simon

Re: [RFA 1/6] Use std::vector in end_symtab_get_static_block

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:

Simon> That made me doubt for a second, since we're more used to see "<"
Simon> in these functions.  I then saw that block_compar did sort them
Simon> in descending order.  Could you add a comment here to indicate that?

Done, thanks.

Tom

Re: [RFA 1/6] Use std::vector in end_symtab_get_static_block

[Ulrich Weigand]

[Re-sent since the original seems to have gotten lost somehow.]

> >>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:
> 
> Simon> That made me doubt for a second, since we're more used to see "<"
> Simon> in these functions.  I then saw that block_compar did sort them
> Simon> in descending order.  Could you add a comment here to indicate that?
> 
> Done, thanks.

This causes a number of regressions in the gdb.opt/inline-cmds.exp
test case for me.  Not sure exactly why, but changing the std::sort
to a std::stable_sort like below fixes those regressions for me.
Maybe the logic for handling inline function blocks somehow relied
on some (undocumented) behavior of qsort for handling elements that
compare as equal?

Bye,
Ulrich

diff --git a/gdb/buildsym.c b/gdb/buildsym.c
index c556ac1..b6d4e67 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -1249,7 +1249,7 @@ end_symtab_get_static_block (CORE_ADDR end_addr, int expandable, int required)
       for (pb = pending_blocks; pb != NULL; pb = pb->next)
 	barray.push_back (pb->block);
 
-      std::sort (barray.begin (), barray.end (),
+      std::stable_sort (barray.begin (), barray.end (),
 		 [] (const block *a, const block *b)
 		 {
 		   /* Sort blocks in descending order.  */


-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [RFA 1/6] Use std::vector in end_symtab_get_static_block

[Pedro Alves]

On 10/20/2017 04:33 PM, Ulrich Weigand wrote:
> [Re-sent since the original seems to have gotten lost somehow.]
> 
>>>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:
>>
>> Simon> That made me doubt for a second, since we're more used to see "<"
>> Simon> in these functions.  I then saw that block_compar did sort them
>> Simon> in descending order.  Could you add a comment here to indicate that?
>>
>> Done, thanks.
> 
> This causes a number of regressions in the gdb.opt/inline-cmds.exp
> test case for me.  Not sure exactly why, but changing the std::sort
> to a std::stable_sort like below fixes those regressions for me.
> Maybe the logic for handling inline function blocks somehow relied
> on some (undocumented) behavior of qsort for handling elements that
> compare as equal?
> 

Sounds like we should improve the sort predicate to disambiguate
better, define a total order?  I don't really know which sorting
is assumed, but e.g., if the block start addresses are the same,
compare the blocks' end addresses.  If those match as well,
repeat but with the blocks' superblocks.  And/or sort function
blocks before non-function blocks.  Etc.

Thanks,
Pedro Alves

Re: [RFA 1/6] Use std::vector in end_symtab_get_static_block

[Ulrich Weigand]

Pedro Alves wrote:
> On 10/20/2017 04:33 PM, Ulrich Weigand wrote:
> > [Re-sent since the original seems to have gotten lost somehow.]
> > 
> >>>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:
> >>
> >> Simon> That made me doubt for a second, since we're more used to see "<"
> >> Simon> in these functions.  I then saw that block_compar did sort them
> >> Simon> in descending order.  Could you add a comment here to indicate that?
> >>
> >> Done, thanks.
> > 
> > This causes a number of regressions in the gdb.opt/inline-cmds.exp
> > test case for me.  Not sure exactly why, but changing the std::sort
> > to a std::stable_sort like below fixes those regressions for me.
> > Maybe the logic for handling inline function blocks somehow relied
> > on some (undocumented) behavior of qsort for handling elements that
> > compare as equal?
> > 
> 
> Sounds like we should improve the sort predicate to disambiguate
> better, define a total order?  I don't really know which sorting
> is assumed, but e.g., if the block start addresses are the same,
> compare the blocks' end addresses.  If those match as well,
> repeat but with the blocks' superblocks.  And/or sort function
> blocks before non-function blocks.  Etc.

I don't believe we can do this, actually.  At this point in time,
a block's superblock is not yet known -- in fact, the superblock
links are set up afterwards *based on* the order determined here.

We could compare end addresses, but that doesn't fully resolve
the issue, in case an inline function 100% overlaps its caller.

It looks like the only place where have the information which
function is the caller and which the callee is in fact the original
order of blocks in the pending_blocks list, since they were placed
in the correct order by the DWARF reader based on DWARF info.

Now, the reason why we re-sort pending_blocks here is to allow
for object code reordering (hot/cold sections and the like).
But this type of reordering never actually affects inline function
relationships.  So it may be that just using stable_sort here is
actually the *correct* fix ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [RFA 1/6] Use std::vector in end_symtab_get_static_block

[Tom Tromey]

>>>>> "Ulrich" == Ulrich Weigand <uweigand@de.ibm.com> writes:

Ulrich> Now, the reason why we re-sort pending_blocks here is to allow
Ulrich> for object code reordering (hot/cold sections and the like).
Ulrich> But this type of reordering never actually affects inline function
Ulrich> relationships.  So it may be that just using stable_sort here is
Ulrich> actually the *correct* fix ...

I tend to think so as well, for the reasons you mentioned.
glibc doesn't claim that qsort is stable, but maybe it is in practice
sometimes?

Tom

[pushed] Re: [RFA 1/6] Use std::vector in end_symtab_get_static_block

[Ulrich Weigand]

Tom Tromey wrote:
> >>>>> "Ulrich" == Ulrich Weigand <uweigand@de.ibm.com> writes:
> 
> Ulrich> Now, the reason why we re-sort pending_blocks here is to allow
> Ulrich> for object code reordering (hot/cold sections and the like).
> Ulrich> But this type of reordering never actually affects inline function
> Ulrich> relationships.  So it may be that just using stable_sort here is
> Ulrich> actually the *correct* fix ...
> 
> I tend to think so as well, for the reasons you mentioned.
> glibc doesn't claim that qsort is stable, but maybe it is in practice
> sometimes?

I just had a look at the glibc qsort implementation.  It actually uses
(in the common case) a *merge sort* into a temporary buffer, which is
always stable.  The GNU libstdc++ std::sort on the other hand seems to
use an in-place quicksort algorithm, which is generally not stable.
This would explain the difference.

I've now pushed the patch in the form below.

Thanks,
Ulrich

gdb/ChangeLog:
2017-10-24  Ulrich Weigand  <uweigand@de.ibm.com>

	* buildsym.c (end_symtab_get_static_block): Use std::stable_sort.

diff --git a/gdb/buildsym.c b/gdb/buildsym.c
index c556ac1..07bfbd5 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -1249,12 +1249,14 @@ end_symtab_get_static_block (CORE_ADDR end_addr, int expandable, int required)
       for (pb = pending_blocks; pb != NULL; pb = pb->next)
 	barray.push_back (pb->block);
 
-      std::sort (barray.begin (), barray.end (),
-		 [] (const block *a, const block *b)
-		 {
-		   /* Sort blocks in descending order.  */
-		   return BLOCK_START (a) > BLOCK_START (b);
-		 });
+      /* Sort blocks by start address in descending order.  Blocks with the
+	 same start address must remain in the original order to preserve
+	 inline function caller/callee relationships.  */
+      std::stable_sort (barray.begin (), barray.end (),
+			[] (const block *a, const block *b)
+			{
+			  return BLOCK_START (a) > BLOCK_START (b);
+			});
 
       int i = 0;
       for (pb = pending_blocks; pb != NULL; pb = pb->next)

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

[RFA 6/6] Return unique_xmalloc_ptr from target_fileio_read_stralloc

[Tom Tromey]

Change target_fileio_read_stralloc to return unique_xmalloc_ptr and
fix up the callers.  This removes a number of cleanups.

ChangeLog
2017-10-15  Tom Tromey  <tom@tromey.com>

	* linux-tdep.c (linux_info_proc, linux_find_memory_regions_full)
	(linux_fill_prpsinfo, linux_vsyscall_range_raw): Update.
	* target.c (target_fileio_read_stralloc): Update.
	* sparc64-tdep.c (adi_is_addr_mapped): Update.
	* target.h (target_fileio_read_stralloc): Return
	unique_xmalloc_ptr.
---
 gdb/ChangeLog      |   9 +++++
 gdb/linux-tdep.c   | 117 +++++++++++++++++++++--------------------------------
 gdb/sparc64-tdep.c |  12 ++----
 gdb/target.c       |   8 ++--
 gdb/target.h       |   4 +-
 5 files changed, 64 insertions(+), 86 deletions(-)

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index 8751718e84..d890083fa2 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -749,13 +749,10 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
   if (cmdline_f)
     {
       xsnprintf (filename, sizeof filename, "/proc/%ld/cmdline", pid);
-      data = target_fileio_read_stralloc (NULL, filename);
-      if (data)
-	{
-	  struct cleanup *cleanup = make_cleanup (xfree, data);
-          printf_filtered ("cmdline = '%s'\n", data);
-	  do_cleanups (cleanup);
-	}
+      gdb::unique_xmalloc_ptr<char> cmdline
+	= target_fileio_read_stralloc (NULL, filename);
+      if (cmdline)
+	printf_filtered ("cmdline = '%s'\n", cmdline.get ());
       else
 	warning (_("unable to open /proc file '%s'"), filename);
     }
@@ -788,10 +785,10 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
   if (mappings_f)
     {
       xsnprintf (filename, sizeof filename, "/proc/%ld/maps", pid);
-      data = target_fileio_read_stralloc (NULL, filename);
-      if (data)
+      gdb::unique_xmalloc_ptr<char> map
+	= target_fileio_read_stralloc (NULL, filename);
+      if (map)
 	{
-	  struct cleanup *cleanup = make_cleanup (xfree, data);
 	  char *line;
 
 	  printf_filtered (_("Mapped address spaces:\n\n"));
@@ -810,7 +807,9 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
 			   "      Size", "    Offset", "objfile");
 	    }
 
-	  for (line = strtok (data, "\n"); line; line = strtok (NULL, "\n"))
+	  for (line = strtok (map.get (), "\n");
+	       line;
+	       line = strtok (NULL, "\n"))
 	    {
 	      ULONGEST addr, endaddr, offset, inode;
 	      const char *permissions, *device, *filename;
@@ -840,8 +839,6 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
 				   *filename? filename : "");
 	        }
 	    }
-
-	  do_cleanups (cleanup);
 	}
       else
 	warning (_("unable to open /proc file '%s'"), filename);
@@ -849,24 +846,21 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
   if (status_f)
     {
       xsnprintf (filename, sizeof filename, "/proc/%ld/status", pid);
-      data = target_fileio_read_stralloc (NULL, filename);
-      if (data)
-	{
-	  struct cleanup *cleanup = make_cleanup (xfree, data);
-          puts_filtered (data);
-	  do_cleanups (cleanup);
-	}
+      gdb::unique_xmalloc_ptr<char> status
+	= target_fileio_read_stralloc (NULL, filename);
+      if (status)
+	puts_filtered (status.get ());
       else
 	warning (_("unable to open /proc file '%s'"), filename);
     }
   if (stat_f)
     {
       xsnprintf (filename, sizeof filename, "/proc/%ld/stat", pid);
-      data = target_fileio_read_stralloc (NULL, filename);
-      if (data)
+      gdb::unique_xmalloc_ptr<char> statstr
+	= target_fileio_read_stralloc (NULL, filename);
+      if (statstr)
 	{
-	  struct cleanup *cleanup = make_cleanup (xfree, data);
-	  const char *p = data;
+	  const char *p = statstr.get ();
 
 	  printf_filtered (_("Process: %s\n"),
 			   pulongest (strtoulst (p, &p, 10)));
@@ -991,7 +985,6 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
 	    printf_filtered (_("wchan (system call): %s\n"),
 			     hex_string (strtoulst (p, &p, 10)));
 #endif
-	  do_cleanups (cleanup);
 	}
       else
 	warning (_("unable to open /proc file '%s'"), filename);
@@ -1164,7 +1157,6 @@ linux_find_memory_regions_full (struct gdbarch *gdbarch,
 {
   char mapsfilename[100];
   char coredumpfilter_name[100];
-  char *data, *coredumpfilterdata;
   pid_t pid;
   /* Default dump behavior of coredump_filter (0x33), according to
      Documentation/filesystems/proc.txt from the Linux kernel
@@ -1184,20 +1176,20 @@ linux_find_memory_regions_full (struct gdbarch *gdbarch,
     {
       xsnprintf (coredumpfilter_name, sizeof (coredumpfilter_name),
 		 "/proc/%d/coredump_filter", pid);
-      coredumpfilterdata = target_fileio_read_stralloc (NULL,
-							coredumpfilter_name);
+      gdb::unique_xmalloc_ptr<char> coredumpfilterdata
+	= target_fileio_read_stralloc (NULL, coredumpfilter_name);
       if (coredumpfilterdata != NULL)
 	{
 	  unsigned int flags;
 
-	  sscanf (coredumpfilterdata, "%x", &flags);
+	  sscanf (coredumpfilterdata.get (), "%x", &flags);
 	  filterflags = (enum filter_flag) flags;
-	  xfree (coredumpfilterdata);
 	}
     }
 
   xsnprintf (mapsfilename, sizeof mapsfilename, "/proc/%d/smaps", pid);
-  data = target_fileio_read_stralloc (NULL, mapsfilename);
+  gdb::unique_xmalloc_ptr<char> data
+    = target_fileio_read_stralloc (NULL, mapsfilename);
   if (data == NULL)
     {
       /* Older Linux kernels did not support /proc/PID/smaps.  */
@@ -1207,10 +1199,9 @@ linux_find_memory_regions_full (struct gdbarch *gdbarch,
 
   if (data != NULL)
     {
-      struct cleanup *cleanup = make_cleanup (xfree, data);
       char *line, *t;
 
-      line = strtok_r (data, "\n", &t);
+      line = strtok_r (data.get (), "\n", &t);
       while (line != NULL)
 	{
 	  ULONGEST addr, endaddr, offset, inode;
@@ -1329,7 +1320,6 @@ linux_find_memory_regions_full (struct gdbarch *gdbarch,
 		  filename, obfd);
 	}
 
-      do_cleanups (cleanup);
       return 0;
     }
 
@@ -1750,15 +1740,9 @@ linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo *p)
   /* The filename which we will use to obtain some info about the process.
      We will basically use this to store the `/proc/PID/FILENAME' file.  */
   char filename[100];
-  /* The full name of the program which generated the corefile.  */
-  char *fname;
   /* The basename of the executable.  */
   const char *basename;
-  /* The arguments of the program.  */
-  char *psargs;
   char *infargs;
-  /* The contents of `/proc/PID/stat' and `/proc/PID/status' files.  */
-  char *proc_stat, *proc_status;
   /* Temporary buffer.  */
   char *tmpstr;
   /* The valid states of a process, according to the Linux kernel.  */
@@ -1775,56 +1759,54 @@ linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo *p)
   long pr_nice;
   /* The number of fields read by `sscanf'.  */
   int n_fields = 0;
-  /* Cleanups.  */
-  struct cleanup *c;
 
   gdb_assert (p != NULL);
 
   /* Obtaining PID and filename.  */
   pid = ptid_get_pid (inferior_ptid);
   xsnprintf (filename, sizeof (filename), "/proc/%d/cmdline", (int) pid);
-  fname = target_fileio_read_stralloc (NULL, filename);
+  /* The full name of the program which generated the corefile.  */
+  gdb::unique_xmalloc_ptr<char> fname
+    = target_fileio_read_stralloc (NULL, filename);
 
-  if (fname == NULL || *fname == '\0')
+  if (fname == NULL || fname.get ()[0] == '\0')
     {
       /* No program name was read, so we won't be able to retrieve more
 	 information about the process.  */
-      xfree (fname);
       return 0;
     }
 
-  c = make_cleanup (xfree, fname);
   memset (p, 0, sizeof (*p));
 
   /* Defining the PID.  */
   p->pr_pid = pid;
 
   /* Copying the program name.  Only the basename matters.  */
-  basename = lbasename (fname);
+  basename = lbasename (fname.get ());
   strncpy (p->pr_fname, basename, sizeof (p->pr_fname));
   p->pr_fname[sizeof (p->pr_fname) - 1] = '\0';
 
   infargs = get_inferior_args ();
 
-  psargs = xstrdup (fname);
+  /* The arguments of the program.  */
+  std::string psargs = fname.get ();
   if (infargs != NULL)
-    psargs = reconcat (psargs, psargs, " ", infargs, (char *) NULL);
-
-  make_cleanup (xfree, psargs);
+    psargs = psargs + " " + infargs;
 
-  strncpy (p->pr_psargs, psargs, sizeof (p->pr_psargs));
+  strncpy (p->pr_psargs, psargs.c_str (), sizeof (p->pr_psargs));
   p->pr_psargs[sizeof (p->pr_psargs) - 1] = '\0';
 
   xsnprintf (filename, sizeof (filename), "/proc/%d/stat", (int) pid);
-  proc_stat = target_fileio_read_stralloc (NULL, filename);
-  make_cleanup (xfree, proc_stat);
+  /* The contents of `/proc/PID/stat'.  */
+  gdb::unique_xmalloc_ptr<char> proc_stat_contents
+    = target_fileio_read_stralloc (NULL, filename);
+  char *proc_stat = proc_stat_contents.get ();
 
   if (proc_stat == NULL || *proc_stat == '\0')
     {
       /* Despite being unable to read more information about the
 	 process, we return 1 here because at least we have its
 	 command line, PID and arguments.  */
-      do_cleanups (c);
       return 1;
     }
 
@@ -1846,10 +1828,7 @@ linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo *p)
   /* ps command also relies on no trailing fields ever contain ')'.  */
   proc_stat = strrchr (proc_stat, ')');
   if (proc_stat == NULL)
-    {
-      do_cleanups (c);
-      return 1;
-    }
+    return 1;
   proc_stat++;
 
   proc_stat = skip_spaces (proc_stat);
@@ -1875,7 +1854,6 @@ linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo *p)
       /* Again, we couldn't read the complementary information about
 	 the process state.  However, we already have minimal
 	 information, so we just return 1 here.  */
-      do_cleanups (c);
       return 1;
     }
 
@@ -1897,13 +1875,14 @@ linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo *p)
   /* Finally, obtaining the UID and GID.  For that, we read and parse the
      contents of the `/proc/PID/status' file.  */
   xsnprintf (filename, sizeof (filename), "/proc/%d/status", (int) pid);
-  proc_status = target_fileio_read_stralloc (NULL, filename);
-  make_cleanup (xfree, proc_status);
+  /* The contents of `/proc/PID/status'.  */
+  gdb::unique_xmalloc_ptr<char> proc_status_contents
+    = target_fileio_read_stralloc (NULL, filename);
+  char *proc_status = proc_status_contents.get ();
 
   if (proc_status == NULL || *proc_status == '\0')
     {
       /* Returning 1 since we already have a bunch of information.  */
-      do_cleanups (c);
       return 1;
     }
 
@@ -1933,8 +1912,6 @@ linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo *p)
 	p->pr_gid = strtol (tmpstr, &tmpstr, 10);
     }
 
-  do_cleanups (c);
-
   return 1;
 }
 
@@ -2296,7 +2273,6 @@ linux_vsyscall_range_raw (struct gdbarch *gdbarch, struct mem_range *range)
 {
   char filename[100];
   long pid;
-  char *data;
 
   if (target_auxv_search (&current_target, AT_SYSINFO_EHDR, &range->start) <= 0)
     return 0;
@@ -2345,14 +2321,14 @@ linux_vsyscall_range_raw (struct gdbarch *gdbarch, struct mem_range *range)
      takes several seconds.  Also note that "smaps", what we read for
      determining core dump mappings, is even slower than "maps".  */
   xsnprintf (filename, sizeof filename, "/proc/%ld/task/%ld/maps", pid, pid);
-  data = target_fileio_read_stralloc (NULL, filename);
+  gdb::unique_xmalloc_ptr<char> data
+    = target_fileio_read_stralloc (NULL, filename);
   if (data != NULL)
     {
-      struct cleanup *cleanup = make_cleanup (xfree, data);
       char *line;
       char *saveptr = NULL;
 
-      for (line = strtok_r (data, "\n", &saveptr);
+      for (line = strtok_r (data.get (), "\n", &saveptr);
 	   line != NULL;
 	   line = strtok_r (NULL, "\n", &saveptr))
 	{
@@ -2366,12 +2342,9 @@ linux_vsyscall_range_raw (struct gdbarch *gdbarch, struct mem_range *range)
 		p++;
 	      endaddr = strtoulst (p, &p, 16);
 	      range->length = endaddr - addr;
-	      do_cleanups (cleanup);
 	      return 1;
 	    }
 	}
-
-      do_cleanups (cleanup);
     }
   else
     warning (_("unable to open /proc file '%s'"), filename);
diff --git a/gdb/sparc64-tdep.c b/gdb/sparc64-tdep.c
index 2fbeec0e98..6a1d9631bb 100644
--- a/gdb/sparc64-tdep.c
+++ b/gdb/sparc64-tdep.c
@@ -311,13 +311,13 @@ adi_is_addr_mapped (CORE_ADDR vaddr, size_t cnt)
 
   pid_t pid = ptid_get_pid (inferior_ptid);
   snprintf (filename, sizeof filename, "/proc/%ld/adi/maps", (long) pid);
-  char *data = target_fileio_read_stralloc (NULL, filename);
+  gdb::unique_xmalloc_ptr<char> data
+    = target_fileio_read_stralloc (NULL, filename);
   if (data)
     {
-      struct cleanup *cleanup = make_cleanup (xfree, data);
       adi_stat_t adi_stat = get_adi_info (pid);
       char *line;
-      for (line = strtok (data, "\n"); line; line = strtok (NULL, "\n"))
+      for (line = strtok (data.get (), "\n"); line; line = strtok (NULL, "\n"))
         {
           ULONGEST addr, endaddr;
 
@@ -327,13 +327,9 @@ adi_is_addr_mapped (CORE_ADDR vaddr, size_t cnt)
                  && ((vaddr + i) * adi_stat.blksize) < endaddr)
             {
               if (++i == cnt)
-                {
-                  do_cleanups (cleanup);
-                  return true;
-                }
+		return true;
             }
         }
-        do_cleanups (cleanup);
       }
     else
       warning (_("unable to open /proc file '%s'"), filename);
diff --git a/gdb/target.c b/gdb/target.c
index 9d65b496f1..5e6db31fad 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3085,7 +3085,7 @@ target_fileio_read_alloc (struct inferior *inf, const char *filename,
 
 /* See target.h.  */
 
-char *
+gdb::unique_xmalloc_ptr<char> 
 target_fileio_read_stralloc (struct inferior *inf, const char *filename)
 {
   gdb_byte *buffer;
@@ -3096,10 +3096,10 @@ target_fileio_read_stralloc (struct inferior *inf, const char *filename)
   bufstr = (char *) buffer;
 
   if (transferred < 0)
-    return NULL;
+    return gdb::unique_xmalloc_ptr<char> (nullptr);
 
   if (transferred == 0)
-    return xstrdup ("");
+    return gdb::unique_xmalloc_ptr<char> (xstrdup (""));
 
   bufstr[transferred] = 0;
 
@@ -3113,7 +3113,7 @@ target_fileio_read_stralloc (struct inferior *inf, const char *filename)
 	break;
       }
 
-  return bufstr;
+  return gdb::unique_xmalloc_ptr<char> (bufstr);
 }
 
 
diff --git a/gdb/target.h b/gdb/target.h
index 98bcd789e0..82092874f9 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -2131,8 +2131,8 @@ extern LONGEST target_fileio_read_alloc (struct inferior *inf,
    or the transfer is unsupported, NULL is returned.  Empty objects
    are returned as allocated but empty strings.  A warning is issued
    if the result contains any embedded NUL bytes.  */
-extern char *target_fileio_read_stralloc (struct inferior *inf,
-					  const char *filename);
+extern gdb::unique_xmalloc_ptr<char> target_fileio_read_stralloc
+    (struct inferior *inf, const char *filename);
 
 
 /* Tracepoint-related operations.  */
-- 
2.13.6

Re: [RFA 6/6] Return unique_xmalloc_ptr from target_fileio_read_stralloc

[Simon Marchi]

On 2017-10-15 11:04 PM, Tom Tromey wrote:
> Change target_fileio_read_stralloc to return unique_xmalloc_ptr and
> fix up the callers.  This removes a number of cleanups.
> 
> ChangeLog
> 2017-10-15  Tom Tromey  <tom@tromey.com>
> 
> 	* linux-tdep.c (linux_info_proc, linux_find_memory_regions_full)
> 	(linux_fill_prpsinfo, linux_vsyscall_range_raw): Update.
> 	* target.c (target_fileio_read_stralloc): Update.
> 	* sparc64-tdep.c (adi_is_addr_mapped): Update.
> 	* target.h (target_fileio_read_stralloc): Return
> 	unique_xmalloc_ptr.
> ---
>  gdb/ChangeLog      |   9 +++++
>  gdb/linux-tdep.c   | 117 +++++++++++++++++++++--------------------------------
>  gdb/sparc64-tdep.c |  12 ++----
>  gdb/target.c       |   8 ++--
>  gdb/target.h       |   4 +-
>  5 files changed, 64 insertions(+), 86 deletions(-)
> 
> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index 8751718e84..d890083fa2 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -749,13 +749,10 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
>    if (cmdline_f)
>      {
>        xsnprintf (filename, sizeof filename, "/proc/%ld/cmdline", pid);
> -      data = target_fileio_read_stralloc (NULL, filename);
> -      if (data)
> -	{
> -	  struct cleanup *cleanup = make_cleanup (xfree, data);
> -          printf_filtered ("cmdline = '%s'\n", data);
> -	  do_cleanups (cleanup);
> -	}
> +      gdb::unique_xmalloc_ptr<char> cmdline
> +	= target_fileio_read_stralloc (NULL, filename);
> +      if (cmdline)
> +	printf_filtered ("cmdline = '%s'\n", cmdline.get ());
>        else
>  	warning (_("unable to open /proc file '%s'"), filename);
>      }
> @@ -788,10 +785,10 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
>    if (mappings_f)
>      {
>        xsnprintf (filename, sizeof filename, "/proc/%ld/maps", pid);
> -      data = target_fileio_read_stralloc (NULL, filename);
> -      if (data)
> +      gdb::unique_xmalloc_ptr<char> map
> +	= target_fileio_read_stralloc (NULL, filename);
> +      if (map)

if (map != NULL)

Otherwise, LGTM.

Simon