[RFA 0/2] more cleanup removal

[RFA 0/2] more cleanup removal

[Tom Tromey]

This removes some more cleanups -- first converting target_read_string
to use unique_xmalloc_ptr, and then removing further cleanups from
solib-svr4.c.  The second patch depends on the first in order to fully
remove all cleanups from one function.

Regression tested by the buildbot.

Tom

[RFA 2/2] Remove some clanups from solib-svr4.c

[Tom Tromey]

This removes a few cleanups from solib-svr4.c in a straightforward
way.

gdb/ChangeLog
2018-03-26  Tom Tromey  <tom@tromey.com>

	* solib-svr4.c (lm_info_read): Use gdb::byte_vector.
	(svr4_keep_data_in_core): Use gdb::unique_xmalloc_ptr.
---
 gdb/ChangeLog    |  5 +++++
 gdb/solib-svr4.c | 47 ++++++++++-------------------------------------
 2 files changed, 15 insertions(+), 37 deletions(-)

diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index d8d047d394..c2170891e5 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -172,14 +172,11 @@ static lm_info_svr4 *
 lm_info_read (CORE_ADDR lm_addr)
 {
   struct link_map_offsets *lmo = svr4_fetch_link_map_offsets ();
-  gdb_byte *lm;
   lm_info_svr4 *lm_info;
-  struct cleanup *back_to;
 
-  lm = (gdb_byte *) xmalloc (lmo->link_map_size);
-  back_to = make_cleanup (xfree, lm);
+  gdb::byte_vector lm (lmo->link_map_size);
 
-  if (target_read_memory (lm_addr, lm, lmo->link_map_size) != 0)
+  if (target_read_memory (lm_addr, lm.data (), lmo->link_map_size) != 0)
     {
       warning (_("Error reading shared library list entry at %s"),
 	       paddress (target_gdbarch (), lm_addr)),
@@ -203,8 +200,6 @@ lm_info_read (CORE_ADDR lm_addr)
 					       ptr_type);
     }
 
-  do_cleanups (back_to);
-
   return lm_info;
 }
 
@@ -958,8 +953,6 @@ svr4_keep_data_in_core (CORE_ADDR vaddr, unsigned long size)
 {
   struct svr4_info *info;
   CORE_ADDR ldsomap;
-  struct so_list *newobj;
-  struct cleanup *old_chain;
   CORE_ADDR name_lm;
 
   info = get_svr4_info ();
@@ -973,13 +966,8 @@ svr4_keep_data_in_core (CORE_ADDR vaddr, unsigned long size)
   if (!ldsomap)
     return 0;
 
-  newobj = XCNEW (struct so_list);
-  old_chain = make_cleanup (xfree, newobj);
-  lm_info_svr4 *li = lm_info_read (ldsomap);
-  newobj->lm_info = li;
-  make_cleanup (xfree, newobj->lm_info);
+  gdb::unique_xmalloc_ptr<lm_info_svr4> li (lm_info_read (ldsomap));
   name_lm = li != NULL ? li->l_name : 0;
-  do_cleanups (old_chain);
 
   return (name_lm >= vaddr && name_lm < vaddr + size);
 }
@@ -995,8 +983,7 @@ open_symbol_file_object (int from_tty)
   struct link_map_offsets *lmo = svr4_fetch_link_map_offsets ();
   struct type *ptr_type = builtin_type (target_gdbarch ())->builtin_data_ptr;
   int l_name_size = TYPE_LENGTH (ptr_type);
-  gdb_byte *l_name_buf = (gdb_byte *) xmalloc (l_name_size);
-  struct cleanup *cleanups = make_cleanup (xfree, l_name_buf);
+  gdb::byte_vector l_name_buf (l_name_size);
   struct svr4_info *info = get_svr4_info ();
   symfile_add_flags add_flags = 0;
 
@@ -1005,38 +992,26 @@ open_symbol_file_object (int from_tty)
 
   if (symfile_objfile)
     if (!query (_("Attempt to reload symbols from process? ")))
-      {
-	do_cleanups (cleanups);
-	return 0;
-      }
+      return 0;
 
   /* Always locate the debug struct, in case it has moved.  */
   info->debug_base = 0;
   if (locate_base (info) == 0)
-    {
-      do_cleanups (cleanups);
-      return 0;	/* failed somehow...  */
-    }
+    return 0;	/* failed somehow...  */
 
   /* First link map member should be the executable.  */
   lm = solib_svr4_r_map (info);
   if (lm == 0)
-    {
-      do_cleanups (cleanups);
-      return 0;	/* failed somehow...  */
-    }
+    return 0;	/* failed somehow...  */
 
   /* Read address of name from target memory to GDB.  */
-  read_memory (lm + lmo->l_name_offset, l_name_buf, l_name_size);
+  read_memory (lm + lmo->l_name_offset, l_name_buf.data (), l_name_size);
 
   /* Convert the address to host format.  */
-  l_name = extract_typed_address (l_name_buf, ptr_type);
+  l_name = extract_typed_address (l_name_buf.data (), ptr_type);
 
   if (l_name == 0)
-    {
-      do_cleanups (cleanups);
-      return 0;		/* No filename.  */
-    }
+    return 0;		/* No filename.  */
 
   /* Now fetch the filename from target memory.  */
   target_read_string (l_name, &filename, SO_NAME_MAX_PATH_SIZE - 1, &errcode);
@@ -1045,14 +1020,12 @@ open_symbol_file_object (int from_tty)
     {
       warning (_("failed to read exec filename from attached file: %s"),
 	       safe_strerror (errcode));
-      do_cleanups (cleanups);
       return 0;
     }
 
   /* Have a pathname: read the symbol file.  */
   symbol_file_add_main (filename.get (), add_flags);
 
-  do_cleanups (cleanups);
   return 1;
 }
 
-- 
2.13.6

[RFA 1/2] Change target_read_string to use unique_xmalloc_ptr

[Tom Tromey]

This changes the out parameter of target_read_string to be a
unique_xmalloc_ptr.  This avoids a cleanup and sets the stage for more
cleanup removals.

This patch also removes a seemingly needless alloca from
print_subexp_standard.

gdb/ChangeLog
2018-03-27  Tom Tromey  <tom@tromey.com>

	* windows-nat.c (handle_output_debug_string, handle_exception):
	Update.
	* target.h (target_read_string): Update.
	* target.c (target_read_string): Change "string" to
	unique_xmalloc_ptr.
	* solib-svr4.c (open_symbol_file_object, svr4_read_so_list):
	Update.
	* solib-frv.c (frv_current_sos): Update.
	* solib-dsbt.c (dsbt_current_sos): Update.
	* solib-darwin.c (darwin_current_sos): Update.
	* linux-thread-db.c (inferior_has_bug): Update.
	* expprint.c (print_subexp_standard) <case OP_OBJC_MSGCALL>:
	Update.  Remove alloca.
	* ada-lang.c (ada_main_name): Update.
---
 gdb/ChangeLog         | 17 +++++++++++++++++
 gdb/ada-lang.c        |  5 ++---
 gdb/expprint.c        |  9 +++------
 gdb/linux-thread-db.c |  7 +++----
 gdb/solib-darwin.c    |  5 ++---
 gdb/solib-dsbt.c      |  7 +++----
 gdb/solib-frv.c       |  8 ++++----
 gdb/solib-svr4.c      | 10 ++++------
 gdb/target.c          |  5 +++--
 gdb/target.h          |  3 ++-
 gdb/windows-nat.c     | 28 ++++++++++++----------------
 11 files changed, 55 insertions(+), 49 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 505d059244..11939d7798 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -913,7 +913,7 @@ char *
 ada_main_name (void)
 {
   struct bound_minimal_symbol msym;
-  static char *main_program_name = NULL;
+  static gdb::unique_xmalloc_ptr<char> main_program_name;
 
   /* For Ada, the name of the main procedure is stored in a specific
      string constant, generated by the binder.  Look for that symbol,
@@ -931,13 +931,12 @@ ada_main_name (void)
       if (main_program_name_addr == 0)
         error (_("Invalid address for Ada main program name."));
 
-      xfree (main_program_name);
       target_read_string (main_program_name_addr, &main_program_name,
                           1024, &err_code);
 
       if (err_code != 0)
         return NULL;
-      return main_program_name;
+      return main_program_name.get ();
     }
 
   /* The main procedure doesn't seem to be in Ada.  */
diff --git a/gdb/expprint.c b/gdb/expprint.c
index 9d1884f290..c906904599 100644
--- a/gdb/expprint.c
+++ b/gdb/expprint.c
@@ -240,7 +240,7 @@ print_subexp_standard (struct expression *exp, int *pos,
 
     case OP_OBJC_MSGCALL:
       {			/* Objective C message (method) call.  */
-	char *selector;
+	gdb::unique_xmalloc_ptr<char> selector;
 
 	(*pos) += 3;
 	nargs = longest_to_int (exp->elts[pc + 2].longconst);
@@ -256,8 +256,7 @@ print_subexp_standard (struct expression *exp, int *pos,
 	  {
 	    char *s, *nextS;
 
-	    s = (char *) alloca (strlen (selector) + 1);
-	    strcpy (s, selector);
+	    s = selector.get ();
 	    for (tem = 0; tem < nargs; tem++)
 	      {
 		nextS = strchr (s, ':');
@@ -270,11 +269,9 @@ print_subexp_standard (struct expression *exp, int *pos,
 	  }
 	else
 	  {
-	    fprintf_unfiltered (stream, " %s", selector);
+	    fprintf_unfiltered (stream, " %s", selector.get ());
 	  }
 	fprintf_unfiltered (stream, "]");
-	/* "selector" was malloc'd by target_read_string.  Free it.  */
-	xfree (selector);
 	return;
       }
 
diff --git a/gdb/linux-thread-db.c b/gdb/linux-thread-db.c
index cfedd98bd8..08e3cfbc8b 100644
--- a/gdb/linux-thread-db.c
+++ b/gdb/linux-thread-db.c
@@ -413,7 +413,7 @@ inferior_has_bug (const char *ver_symbol, int ver_major_min, int ver_minor_min)
 {
   struct bound_minimal_symbol version_msym;
   CORE_ADDR version_addr;
-  char *version;
+  gdb::unique_xmalloc_ptr<char> version;
   int err, got, retval = 0;
 
   version_msym = lookup_minimal_symbol (ver_symbol, NULL, NULL);
@@ -422,15 +422,14 @@ inferior_has_bug (const char *ver_symbol, int ver_major_min, int ver_minor_min)
 
   version_addr = BMSYMBOL_VALUE_ADDRESS (version_msym);
   got = target_read_string (version_addr, &version, 32, &err);
-  if (err == 0 && memchr (version, 0, got) == &version[got -1])
+  if (err == 0 && memchr (version.get (), 0, got) == version.get () + got - 1)
     {
       int major, minor;
 
-      retval = (sscanf (version, "%d.%d", &major, &minor) == 2
+      retval = (sscanf (version.get (), "%d.%d", &major, &minor) == 2
 		&& (major < ver_major_min
 		    || (major == ver_major_min && minor < ver_minor_min)));
     }
-  xfree (version);
 
   return retval;
 }
diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c
index cf15148c36..d66938fb90 100644
--- a/gdb/solib-darwin.c
+++ b/gdb/solib-darwin.c
@@ -261,7 +261,7 @@ darwin_current_sos (void)
       CORE_ADDR path_addr;
       struct mach_o_header_external hdr;
       unsigned long hdr_val;
-      char *file_path;
+      gdb::unique_xmalloc_ptr<char> file_path;
       int errcode;
       struct so_list *newobj;
       struct cleanup *old_chain;
@@ -299,10 +299,9 @@ darwin_current_sos (void)
       lm_info_darwin *li = new lm_info_darwin;
       newobj->lm_info = li;
 
-      strncpy (newobj->so_name, file_path, SO_NAME_MAX_PATH_SIZE - 1);
+      strncpy (newobj->so_name, file_path.get (), SO_NAME_MAX_PATH_SIZE - 1);
       newobj->so_name[SO_NAME_MAX_PATH_SIZE - 1] = '\0';
       strcpy (newobj->so_original_name, newobj->so_name);
-      xfree (file_path);
       li->lm_addr = load_addr;
 
       if (head == NULL)
diff --git a/gdb/solib-dsbt.c b/gdb/solib-dsbt.c
index d038cccb2f..b3b66baffc 100644
--- a/gdb/solib-dsbt.c
+++ b/gdb/solib-dsbt.c
@@ -696,7 +696,7 @@ dsbt_current_sos (void)
       if (dsbt_index != 0)
 	{
 	  int errcode;
-	  char *name_buf;
+	  gdb::unique_xmalloc_ptr<char> name_buf;
 	  struct int_elf32_dsbt_loadmap *loadmap;
 	  struct so_list *sop;
 	  CORE_ADDR addr;
@@ -727,11 +727,10 @@ dsbt_current_sos (void)
 	    {
 	      if (solib_dsbt_debug)
 		fprintf_unfiltered (gdb_stdlog, "current_sos: name = %s\n",
-				    name_buf);
+				    name_buf.get ());
 
-	      strncpy (sop->so_name, name_buf, SO_NAME_MAX_PATH_SIZE - 1);
+	      strncpy (sop->so_name, name_buf.get (), SO_NAME_MAX_PATH_SIZE - 1);
 	      sop->so_name[SO_NAME_MAX_PATH_SIZE - 1] = '\0';
-	      xfree (name_buf);
 	      strcpy (sop->so_original_name, sop->so_name);
 	    }
 
diff --git a/gdb/solib-frv.c b/gdb/solib-frv.c
index 7929efd786..e772da6115 100644
--- a/gdb/solib-frv.c
+++ b/gdb/solib-frv.c
@@ -377,7 +377,7 @@ frv_current_sos (void)
       if (got_addr != mgot)
 	{
 	  int errcode;
-	  char *name_buf;
+	  gdb::unique_xmalloc_ptr<char> name_buf;
 	  struct int_elf32_fdpic_loadmap *loadmap;
 	  struct so_list *sop;
 	  CORE_ADDR addr;
@@ -409,16 +409,16 @@ frv_current_sos (void)
 
 	  if (solib_frv_debug)
 	    fprintf_unfiltered (gdb_stdlog, "current_sos: name = %s\n",
-	                        name_buf);
+	                        name_buf.get ());
 	  
 	  if (errcode != 0)
 	    warning (_("Can't read pathname for link map entry: %s."),
 		     safe_strerror (errcode));
 	  else
 	    {
-	      strncpy (sop->so_name, name_buf, SO_NAME_MAX_PATH_SIZE - 1);
+	      strncpy (sop->so_name, name_buf.get (),
+		       SO_NAME_MAX_PATH_SIZE - 1);
 	      sop->so_name[SO_NAME_MAX_PATH_SIZE - 1] = '\0';
-	      xfree (name_buf);
 	      strcpy (sop->so_original_name, sop->so_name);
 	    }
 
diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index 29e74daa51..d8d047d394 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -990,7 +990,7 @@ static int
 open_symbol_file_object (int from_tty)
 {
   CORE_ADDR lm, l_name;
-  char *filename;
+  gdb::unique_xmalloc_ptr<char> filename;
   int errcode;
   struct link_map_offsets *lmo = svr4_fetch_link_map_offsets ();
   struct type *ptr_type = builtin_type (target_gdbarch ())->builtin_data_ptr;
@@ -1040,7 +1040,6 @@ open_symbol_file_object (int from_tty)
 
   /* Now fetch the filename from target memory.  */
   target_read_string (l_name, &filename, SO_NAME_MAX_PATH_SIZE - 1, &errcode);
-  make_cleanup (xfree, filename);
 
   if (errcode)
     {
@@ -1051,7 +1050,7 @@ open_symbol_file_object (int from_tty)
     }
 
   /* Have a pathname: read the symbol file.  */
-  symbol_file_add_main (filename, add_flags);
+  symbol_file_add_main (filename.get (), add_flags);
 
   do_cleanups (cleanups);
   return 1;
@@ -1339,7 +1338,7 @@ svr4_read_so_list (CORE_ADDR lm, CORE_ADDR prev_lm,
   for (; lm != 0; prev_lm = lm, lm = next_lm)
     {
       int errcode;
-      char *buffer;
+      gdb::unique_xmalloc_ptr<char> buffer;
 
       so_list_up newobj (XCNEW (struct so_list));
 
@@ -1387,10 +1386,9 @@ svr4_read_so_list (CORE_ADDR lm, CORE_ADDR prev_lm,
 	  continue;
 	}
 
-      strncpy (newobj->so_name, buffer, SO_NAME_MAX_PATH_SIZE - 1);
+      strncpy (newobj->so_name, buffer.get (), SO_NAME_MAX_PATH_SIZE - 1);
       newobj->so_name[SO_NAME_MAX_PATH_SIZE - 1] = '\0';
       strcpy (newobj->so_original_name, newobj->so_name);
-      xfree (buffer);
 
       /* If this entry has no name, or its name matches the name
 	 for the main executable, don't include it in the list.  */
diff --git a/gdb/target.c b/gdb/target.c
index 84f5228919..b9b2e757dd 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -993,7 +993,8 @@ target_xfer_status_to_string (enum target_xfer_status status)
    read.  */
 
 int
-target_read_string (CORE_ADDR memaddr, char **string, int len, int *errnop)
+target_read_string (CORE_ADDR memaddr, gdb::unique_xmalloc_ptr<char> *string,
+		    int len, int *errnop)
 {
   int tlen, offset, i;
   gdb_byte buf[4];
@@ -1053,7 +1054,7 @@ target_read_string (CORE_ADDR memaddr, char **string, int len, int *errnop)
       nbytes_read += tlen;
     }
 done:
-  *string = buffer;
+  string->reset (buffer);
   if (errnop != NULL)
     *errnop = errcode;
   return nbytes_read;
diff --git a/gdb/target.h b/gdb/target.h
index bfe9b826b2..51ac884e01 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1430,7 +1430,8 @@ int target_supports_disable_randomization (void);
 #define target_can_run_breakpoint_commands() \
   (*current_target.to_can_run_breakpoint_commands) (&current_target)
 
-extern int target_read_string (CORE_ADDR, char **, int, int *);
+extern int target_read_string (CORE_ADDR, gdb::unique_xmalloc_ptr<char> *,
+			       int, int *);
 
 /* For target_read_memory see target/target.h.  */
 
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 430cc60993..95e3c5816b 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -883,25 +883,25 @@ signal_event_command (const char *args, int from_tty)
 static int
 handle_output_debug_string (struct target_waitstatus *ourstatus)
 {
-  char *s = NULL;
+  gdb::unique_xmalloc_ptr<char> s;
   int retval = 0;
 
   if (!target_read_string
 	((CORE_ADDR) (uintptr_t) current_event.u.DebugString.lpDebugStringData,
-	&s, 1024, 0)
-      || !s || !*s)
+	 &s, 1024, 0)
+      || !s || !*(s.get ()))
     /* nothing to do */;
-  else if (!startswith (s, _CYGWIN_SIGNAL_STRING))
+  else if (!startswith (s.get (), _CYGWIN_SIGNAL_STRING))
     {
 #ifdef __CYGWIN__
-      if (!startswith (s, "cYg"))
+      if (!startswith (s.get (), "cYg"))
 #endif
 	{
-	  char *p = strchr (s, '\0');
+	  char *p = strchr (s.get (), '\0');
 
-	  if (p > s && *--p == '\n')
+	  if (p > s.get () && *--p == '\n')
 	    *p = '\0';
-	  warning (("%s"), s);
+	  warning (("%s"), s.get ());
 	}
     }
 #ifdef __CYGWIN__
@@ -915,7 +915,7 @@ handle_output_debug_string (struct target_waitstatus *ourstatus)
 	 to be stored at the given address in the inferior.  Tell gdb
 	 to treat this like a real signal.  */
       char *p;
-      int sig = strtol (s + sizeof (_CYGWIN_SIGNAL_STRING) - 1, &p, 0);
+      int sig = strtol (s.get () + sizeof (_CYGWIN_SIGNAL_STRING) - 1, &p, 0);
       gdb_signal gotasig = gdb_signal_from_host (sig);
 
       ourstatus->value.sig = gotasig;
@@ -938,8 +938,6 @@ handle_output_debug_string (struct target_waitstatus *ourstatus)
     }
 #endif
 
-  if (s)
-    xfree (s);
   return retval;
 }
 
@@ -1193,18 +1191,16 @@ handle_exception (struct target_waitstatus *ourstatus)
 	  if (named_thread != NULL)
 	    {
 	      int thread_name_len;
-	      char *thread_name;
+	      gdb::unique_xmalloc_ptr<char> thread_name;
 
 	      thread_name_len = target_read_string (thread_name_target,
 						    &thread_name, 1025, NULL);
 	      if (thread_name_len > 0)
 		{
-		  thread_name[thread_name_len - 1] = '\0';
+		  thread_name.get ()[thread_name_len - 1] = '\0';
 		  xfree (named_thread->name);
-		  named_thread->name = thread_name;
+		  named_thread->name = thread_name.release ();
 		}
-	      else
-		xfree (thread_name);
 	    }
 	  ourstatus->value.sig = GDB_SIGNAL_TRAP;
 	  result = HANDLE_EXCEPTION_IGNORED;
-- 
2.13.6

Re: [RFA 1/2] Change target_read_string to use unique_xmalloc_ptr

[Simon Marchi]

On 2018-03-28 00:24, Tom Tromey wrote:
> This changes the out parameter of target_read_string to be a
> unique_xmalloc_ptr.  This avoids a cleanup and sets the stage for more
> cleanup removals.
> 
> This patch also removes a seemingly needless alloca from
> print_subexp_standard.

That looks fine to me.

Simon

Re: [RFA 2/2] Remove some clanups from solib-svr4.c

[Simon Marchi]

"clanups" in the title.

On 2018-03-28 00:24, Tom Tromey wrote:
> This removes a few cleanups from solib-svr4.c in a straightforward
> way.
> 
> gdb/ChangeLog
> 2018-03-26  Tom Tromey  <tom@tromey.com>
> 
> 	* solib-svr4.c (lm_info_read): Use gdb::byte_vector.
> 	(svr4_keep_data_in_core): Use gdb::unique_xmalloc_ptr.
> ---
>  gdb/ChangeLog    |  5 +++++
>  gdb/solib-svr4.c | 47 ++++++++++-------------------------------------
>  2 files changed, 15 insertions(+), 37 deletions(-)
> 
> diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
> index d8d047d394..c2170891e5 100644
> --- a/gdb/solib-svr4.c
> +++ b/gdb/solib-svr4.c
> @@ -172,14 +172,11 @@ static lm_info_svr4 *
>  lm_info_read (CORE_ADDR lm_addr)
>  {
>    struct link_map_offsets *lmo = svr4_fetch_link_map_offsets ();
> -  gdb_byte *lm;
>    lm_info_svr4 *lm_info;
> -  struct cleanup *back_to;
> 
> -  lm = (gdb_byte *) xmalloc (lmo->link_map_size);
> -  back_to = make_cleanup (xfree, lm);
> +  gdb::byte_vector lm (lmo->link_map_size);
> 
> -  if (target_read_memory (lm_addr, lm, lmo->link_map_size) != 0)
> +  if (target_read_memory (lm_addr, lm.data (), lmo->link_map_size) != 
> 0)
>      {
>        warning (_("Error reading shared library list entry at %s"),
>  	       paddress (target_gdbarch (), lm_addr)),
> @@ -203,8 +200,6 @@ lm_info_read (CORE_ADDR lm_addr)
>  					       ptr_type);
>      }
> 
> -  do_cleanups (back_to);
> -
>    return lm_info;
>  }
> 
> @@ -958,8 +953,6 @@ svr4_keep_data_in_core (CORE_ADDR vaddr, unsigned 
> long size)
>  {
>    struct svr4_info *info;
>    CORE_ADDR ldsomap;
> -  struct so_list *newobj;
> -  struct cleanup *old_chain;
>    CORE_ADDR name_lm;
> 
>    info = get_svr4_info ();
> @@ -973,13 +966,8 @@ svr4_keep_data_in_core (CORE_ADDR vaddr, unsigned
> long size)
>    if (!ldsomap)
>      return 0;
> 
> -  newobj = XCNEW (struct so_list);
> -  old_chain = make_cleanup (xfree, newobj);
> -  lm_info_svr4 *li = lm_info_read (ldsomap);
> -  newobj->lm_info = li;
> -  make_cleanup (xfree, newobj->lm_info);
> +  gdb::unique_xmalloc_ptr<lm_info_svr4> li (lm_info_read (ldsomap));

This is not an error because lm_info_svr4 is trivially destructible, but 
since we allocate it with "new", we might as well use a unique_ptr, it's 
more future-proof.  At least, with the xfree poisoning, adding something 
that does not destruct trivially to lm_info_svr4 would warn us not to 
use unique_xmalloc_ptr here.

How about making lm_info_read return an std::unique_ptr<lm_info_svr4>?

The patch LGTM with or without those changes.

Simon

Re: [RFA 2/2] Remove some clanups from solib-svr4.c

[Tom Tromey]

>> -  newobj = XCNEW (struct so_list);
>> -  old_chain = make_cleanup (xfree, newobj);
>> -  lm_info_svr4 *li = lm_info_read (ldsomap);
>> -  newobj->lm_info = li;
>> -  make_cleanup (xfree, newobj->lm_info);
>> +  gdb::unique_xmalloc_ptr<lm_info_svr4> li (lm_info_read (ldsomap));

Simon> This is not an error because lm_info_svr4 is trivially destructible,
Simon> but since we allocate it with "new", we might as well use a
Simon> unique_ptr, it's more future-proof.

Good catch, thanks.

Simon> How about making lm_info_read return an std::unique_ptr<lm_info_svr4>?

Simon> The patch LGTM with or without those changes.

How's this?

I re-ran it through the buildbot.

Tom

commit 0662ad7b81c11c4b23e74c1d2fcf847ca63e3a8d
Author: Tom Tromey <tom@tromey.com>
Date:   Tue Mar 27 14:42:55 2018 -0600

    Remove some cleanups from solib-svr4.c
    
    This removes a few cleanups from solib-svr4.c in a straightforward
    way.
    
    gdb/ChangeLog
    2018-03-30  Tom Tromey  <tom@tromey.com>
    
            * solib-svr4.c (lm_info_read): Use gdb::byte_vector.  Return
            std::unique_ptr.
            (svr4_keep_data_in_core): Update.
            (svr4_read_so_list): Update.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index d2fe40d0a7..7add93ed63 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2018-03-30  Tom Tromey  <tom@tromey.com>
+
+	* solib-svr4.c (lm_info_read): Use gdb::byte_vector.  Return
+	std::unique_ptr.
+	(svr4_keep_data_in_core): Update.
+	(svr4_read_so_list): Update.
+
 2018-03-27  Tom Tromey  <tom@tromey.com>
 
 	* windows-nat.c (handle_output_debug_string, handle_exception):
diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index d8d047d394..7644c9f4a9 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -168,28 +168,24 @@ svr4_same (struct so_list *gdb, struct so_list *inferior)
   return (svr4_same_1 (gdb->so_original_name, inferior->so_original_name));
 }
 
-static lm_info_svr4 *
+static std::unique_ptr<lm_info_svr4>
 lm_info_read (CORE_ADDR lm_addr)
 {
   struct link_map_offsets *lmo = svr4_fetch_link_map_offsets ();
-  gdb_byte *lm;
-  lm_info_svr4 *lm_info;
-  struct cleanup *back_to;
+  std::unique_ptr<lm_info_svr4> lm_info;
 
-  lm = (gdb_byte *) xmalloc (lmo->link_map_size);
-  back_to = make_cleanup (xfree, lm);
+  gdb::byte_vector lm (lmo->link_map_size);
 
-  if (target_read_memory (lm_addr, lm, lmo->link_map_size) != 0)
+  if (target_read_memory (lm_addr, lm.data (), lmo->link_map_size) != 0)
     {
       warning (_("Error reading shared library list entry at %s"),
-	       paddress (target_gdbarch (), lm_addr)),
-      lm_info = NULL;
+	       paddress (target_gdbarch (), lm_addr));
     }
   else
     {
       struct type *ptr_type = builtin_type (target_gdbarch ())->builtin_data_ptr;
 
-      lm_info = new lm_info_svr4;
+      lm_info.reset (new lm_info_svr4);
       lm_info->lm_addr = lm_addr;
 
       lm_info->l_addr_inferior = extract_typed_address (&lm[lmo->l_addr_offset],
@@ -203,8 +199,6 @@ lm_info_read (CORE_ADDR lm_addr)
 					       ptr_type);
     }
 
-  do_cleanups (back_to);
-
   return lm_info;
 }
 
@@ -958,8 +952,6 @@ svr4_keep_data_in_core (CORE_ADDR vaddr, unsigned long size)
 {
   struct svr4_info *info;
   CORE_ADDR ldsomap;
-  struct so_list *newobj;
-  struct cleanup *old_chain;
   CORE_ADDR name_lm;
 
   info = get_svr4_info ();
@@ -973,13 +965,8 @@ svr4_keep_data_in_core (CORE_ADDR vaddr, unsigned long size)
   if (!ldsomap)
     return 0;
 
-  newobj = XCNEW (struct so_list);
-  old_chain = make_cleanup (xfree, newobj);
-  lm_info_svr4 *li = lm_info_read (ldsomap);
-  newobj->lm_info = li;
-  make_cleanup (xfree, newobj->lm_info);
+  std::unique_ptr<lm_info_svr4> li (lm_info_read (ldsomap));
   name_lm = li != NULL ? li->l_name : 0;
-  do_cleanups (old_chain);
 
   return (name_lm >= vaddr && name_lm < vaddr + size);
 }
@@ -995,8 +982,7 @@ open_symbol_file_object (int from_tty)
   struct link_map_offsets *lmo = svr4_fetch_link_map_offsets ();
   struct type *ptr_type = builtin_type (target_gdbarch ())->builtin_data_ptr;
   int l_name_size = TYPE_LENGTH (ptr_type);
-  gdb_byte *l_name_buf = (gdb_byte *) xmalloc (l_name_size);
-  struct cleanup *cleanups = make_cleanup (xfree, l_name_buf);
+  gdb::byte_vector l_name_buf (l_name_size);
   struct svr4_info *info = get_svr4_info ();
   symfile_add_flags add_flags = 0;
 
@@ -1005,38 +991,26 @@ open_symbol_file_object (int from_tty)
 
   if (symfile_objfile)
     if (!query (_("Attempt to reload symbols from process? ")))
-      {
-	do_cleanups (cleanups);
-	return 0;
-      }
+      return 0;
 
   /* Always locate the debug struct, in case it has moved.  */
   info->debug_base = 0;
   if (locate_base (info) == 0)
-    {
-      do_cleanups (cleanups);
-      return 0;	/* failed somehow...  */
-    }
+    return 0;	/* failed somehow...  */
 
   /* First link map member should be the executable.  */
   lm = solib_svr4_r_map (info);
   if (lm == 0)
-    {
-      do_cleanups (cleanups);
-      return 0;	/* failed somehow...  */
-    }
+    return 0;	/* failed somehow...  */
 
   /* Read address of name from target memory to GDB.  */
-  read_memory (lm + lmo->l_name_offset, l_name_buf, l_name_size);
+  read_memory (lm + lmo->l_name_offset, l_name_buf.data (), l_name_size);
 
   /* Convert the address to host format.  */
-  l_name = extract_typed_address (l_name_buf, ptr_type);
+  l_name = extract_typed_address (l_name_buf.data (), ptr_type);
 
   if (l_name == 0)
-    {
-      do_cleanups (cleanups);
-      return 0;		/* No filename.  */
-    }
+    return 0;		/* No filename.  */
 
   /* Now fetch the filename from target memory.  */
   target_read_string (l_name, &filename, SO_NAME_MAX_PATH_SIZE - 1, &errcode);
@@ -1045,14 +1019,12 @@ open_symbol_file_object (int from_tty)
     {
       warning (_("failed to read exec filename from attached file: %s"),
 	       safe_strerror (errcode));
-      do_cleanups (cleanups);
       return 0;
     }
 
   /* Have a pathname: read the symbol file.  */
   symbol_file_add_main (filename.get (), add_flags);
 
-  do_cleanups (cleanups);
   return 1;
 }
 
@@ -1342,7 +1314,7 @@ svr4_read_so_list (CORE_ADDR lm, CORE_ADDR prev_lm,
 
       so_list_up newobj (XCNEW (struct so_list));
 
-      lm_info_svr4 *li = lm_info_read (lm);
+      lm_info_svr4 *li = lm_info_read (lm).release ();
       newobj->lm_info = li;
       if (li == NULL)
 	return 0;

Re: [RFA 2/2] Remove some clanups from solib-svr4.c

[Simon Marchi]

On 2018-03-30 15:04, Tom Tromey wrote:
>>> -  newobj = XCNEW (struct so_list);
>>> -  old_chain = make_cleanup (xfree, newobj);
>>> -  lm_info_svr4 *li = lm_info_read (ldsomap);
>>> -  newobj->lm_info = li;
>>> -  make_cleanup (xfree, newobj->lm_info);
>>> +  gdb::unique_xmalloc_ptr<lm_info_svr4> li (lm_info_read (ldsomap));
> 
> Simon> This is not an error because lm_info_svr4 is trivially 
> destructible,
> Simon> but since we allocate it with "new", we might as well use a
> Simon> unique_ptr, it's more future-proof.
> 
> Good catch, thanks.
> 
> Simon> How about making lm_info_read return an 
> std::unique_ptr<lm_info_svr4>?
> 
> Simon> The patch LGTM with or without those changes.
> 
> How's this?

LGTM, I just noted two nits:

> @@ -168,28 +168,24 @@ svr4_same (struct so_list *gdb, struct so_list 
> *inferior)
>    return (svr4_same_1 (gdb->so_original_name, 
> inferior->so_original_name));
>  }
> 
> -static lm_info_svr4 *
> +static std::unique_ptr<lm_info_svr4>
>  lm_info_read (CORE_ADDR lm_addr)
>  {
>    struct link_map_offsets *lmo = svr4_fetch_link_map_offsets ();
> -  gdb_byte *lm;
> -  lm_info_svr4 *lm_info;
> -  struct cleanup *back_to;
> +  std::unique_ptr<lm_info_svr4> lm_info;
> 
> -  lm = (gdb_byte *) xmalloc (lmo->link_map_size);
> -  back_to = make_cleanup (xfree, lm);
> +  gdb::byte_vector lm (lmo->link_map_size);
> 
> -  if (target_read_memory (lm_addr, lm, lmo->link_map_size) != 0)
> +  if (target_read_memory (lm_addr, lm.data (), lmo->link_map_size) != 
> 0)
>      {
>        warning (_("Error reading shared library list entry at %s"),
> -	       paddress (target_gdbarch (), lm_addr)),
> -      lm_info = NULL;
> +	       paddress (target_gdbarch (), lm_addr));
>      }

Curly braces can be removed here.

> @@ -973,13 +965,8 @@ svr4_keep_data_in_core (CORE_ADDR vaddr, unsigned
> long size)
>    if (!ldsomap)
>      return 0;
> 
> -  newobj = XCNEW (struct so_list);
> -  old_chain = make_cleanup (xfree, newobj);
> -  lm_info_svr4 *li = lm_info_read (ldsomap);
> -  newobj->lm_info = li;
> -  make_cleanup (xfree, newobj->lm_info);
> +  std::unique_ptr<lm_info_svr4> li (lm_info_read (ldsomap));

I think it's clearer to use the assignment operator in that case:

std::unique_ptr<lm_info_svr4> li = lm_info_read (ldsomap);

Simon

Re: [RFA 2/2] Remove some clanups from solib-svr4.c

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> LGTM, I just noted two nits:
[...]

Thanks, I've made these changes and I'm checking it in.

Tom