[RFA] Remove most cleanups from linux-thread-db.c

Message ID 20180222170248.27265-1-tom@tromey.com
State New
Headers show
Series
  • [RFA] Remove most cleanups from linux-thread-db.c
Related show

Commit Message

Tom Tromey Feb. 22, 2018, 5:02 p.m.
This removes most (but not all) cleanups from linux-thread-db.c.
std::string and std::vector are used in place of manual memory
management.

The remaining cleanup in linux-thread-db.c uses
make_cleanup_free_char_ptr_vec, which requires a somewhat bigger
change.

Regression tested by the buildbot.

gdb/ChangeLog
2018-02-21  Tom Tromey  <tom@tromey.com>

	* linux-thread-db.c (try_thread_db_load_from_pdir_1)
	(try_thread_db_load_from_dir, thread_db_load_search): Use
	std::string.
	(info_auto_load_libthread_db_compare): Return bool.  Change
	argument types.
	(info_auto_load_libthread_db): Use std::vector, std::string.
	Remove cleanups.
---
 gdb/ChangeLog         |  10 ++++
 gdb/linux-thread-db.c | 142 ++++++++++++++------------------------------------
 2 files changed, 49 insertions(+), 103 deletions(-)

-- 
2.13.6

Comments

Simon Marchi Feb. 24, 2018, 2:17 a.m. | #1
Hi Tom,

LGTM, just one nit:

On 2018-02-22 12:02 PM, Tom Tromey wrote:
> @@ -1559,22 +1528,21 @@ thread_db_resume (struct target_ops *ops,

>    beneath->to_resume (beneath, ptid, step, signo);

>  }

>  

> -/* qsort helper function for info_auto_load_libthread_db, sort the

> +/* std:;sort helper function for info_auto_load_libthread_db, sort the


std:;sort -> std::sort

info_auto_load_libthread_db is a little bit hard to follow and there's no
test for it, so I tested it by hand and it seems to work fine with the patch
applied.  In order to be able to test it using my system libthread_db, I copied
it to two directories, started two inferiors, setting "set libthread-db-search-path"
to one directory then the other.  Otherwise, "info auto-load libthread-db" doesn't
do any meaningful work.

Simon
Simon Marchi Feb. 25, 2018, 4:34 p.m. | #2
On 2018-02-22 12:02, Tom Tromey wrote:
> The remaining cleanup in linux-thread-db.c uses

> make_cleanup_free_char_ptr_vec, which requires a somewhat bigger

> change.


I had a patch for that lying around, so I posted it along with other 
cleanup patches to get rid of free_char_ptr_vec here, if you want to 
take a look:

https://sourceware.org/ml/gdb-patches/2018-02/msg00355.html

Simon
Tom Tromey Feb. 25, 2018, 6 p.m. | #3
>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:


Simon> On 2018-02-22 12:02, Tom Tromey wrote:
>> The remaining cleanup in linux-thread-db.c uses

>> make_cleanup_free_char_ptr_vec, which requires a somewhat bigger

>> change.


Simon> I had a patch for that lying around, so I posted it along with other
Simon> cleanup patches to get rid of free_char_ptr_vec here, if you want to
Simon> take a look:

Simon> https://sourceware.org/ml/gdb-patches/2018-02/msg00355.html

I skimmed this series and it looked good to me.
I definitely like the idea and I thought the particular decisions you
made, about when to use string or unique_xmalloc_ptr, made sense.

Tom

Patch

diff --git a/gdb/linux-thread-db.c b/gdb/linux-thread-db.c
index 794c97b48a..abd4ce9e14 100644
--- a/gdb/linux-thread-db.c
+++ b/gdb/linux-thread-db.c
@@ -718,11 +718,7 @@  try_thread_db_load (const char *library, int check_auto_load_safe)
 static int
 try_thread_db_load_from_pdir_1 (struct objfile *obj, const char *subdir)
 {
-  struct cleanup *cleanup;
-  char *path, *cp;
-  int result;
   const char *obj_name = objfile_name (obj);
-  int alloc_len;
 
   if (obj_name[0] != '/')
     {
@@ -731,28 +727,16 @@  try_thread_db_load_from_pdir_1 (struct objfile *obj, const char *subdir)
       return 0;
     }
 
-  alloc_len = (strlen (obj_name)
-	       + (subdir ? strlen (subdir) + 1 : 0)
-	       + 1 + strlen (LIBTHREAD_DB_SO) + 1);
-  path = (char *) xmalloc (alloc_len);
-  cleanup = make_cleanup (xfree, path);
-
-  strcpy (path, obj_name);
-  cp = strrchr (path, '/');
+  std::string path = obj_name;
+  size_t cp = path.rfind ('/');
   /* This should at minimum hit the first character.  */
-  gdb_assert (cp != NULL);
-  cp[1] = '\0';
+  gdb_assert (cp != std::string::npos);
+  path.resize (cp + 1);
   if (subdir != NULL)
-    {
-      strcat (cp, subdir);
-      strcat (cp, "/");
-    }
-  strcat (cp, LIBTHREAD_DB_SO);
+    path = path + subdir + "/";
+  path += LIBTHREAD_DB_SO;
 
-  result = try_thread_db_load (path, 1);
-
-  do_cleanups (cleanup);
-  return result;
+  return try_thread_db_load (path.c_str (), 1);
 }
 
 /* Handle $pdir in libthread-db-search-path.
@@ -804,24 +788,12 @@  try_thread_db_load_from_sdir (void)
 static int
 try_thread_db_load_from_dir (const char *dir, size_t dir_len)
 {
-  struct cleanup *cleanup;
-  char *path;
-  int result;
-
   if (!auto_load_thread_db)
     return 0;
 
-  path = (char *) xmalloc (dir_len + 1 + strlen (LIBTHREAD_DB_SO) + 1);
-  cleanup = make_cleanup (xfree, path);
-
-  memcpy (path, dir, dir_len);
-  path[dir_len] = '/';
-  strcpy (path + dir_len + 1, LIBTHREAD_DB_SO);
+  std::string path = std::string (dir, dir_len) + "/" + LIBTHREAD_DB_SO;
 
-  result = try_thread_db_load (path, 1);
-
-  do_cleanups (cleanup);
-  return result;
+  return try_thread_db_load (path.c_str (), 1);
 }
 
 /* Search libthread_db_search_path for libthread_db which "agrees"
@@ -850,18 +822,15 @@  thread_db_load_search (void)
 	  && (this_dir[pdir_len] == '\0'
 	      || this_dir[pdir_len] == '/'))
 	{
-	  char *subdir = NULL;
-	  struct cleanup *free_subdir_cleanup
-	    = make_cleanup (null_cleanup, NULL);
+	  const char *subdir = NULL;
 
+	  std::string subdir_holder;
 	  if (this_dir[pdir_len] == '/')
 	    {
-	      subdir = (char *) xmalloc (strlen (this_dir));
-	      make_cleanup (xfree, subdir);
-	      strcpy (subdir, this_dir + pdir_len + 1);
+	      subdir_holder = std::string (this_dir + pdir_len + 1);
+	      subdir = subdir_holder.c_str ();
 	    }
 	  rc = try_thread_db_load_from_pdir (subdir);
-	  do_cleanups (free_subdir_cleanup);
 	  if (rc)
 	    break;
 	}
@@ -1559,22 +1528,21 @@  thread_db_resume (struct target_ops *ops,
   beneath->to_resume (beneath, ptid, step, signo);
 }
 
-/* qsort helper function for info_auto_load_libthread_db, sort the
+/* std:;sort helper function for info_auto_load_libthread_db, sort the
    thread_db_info pointers primarily by their FILENAME and secondarily by their
    PID, both in ascending order.  */
 
-static int
-info_auto_load_libthread_db_compare (const void *ap, const void *bp)
+static bool
+info_auto_load_libthread_db_compare (const struct thread_db_info *a,
+				     const struct thread_db_info *b)
 {
-  struct thread_db_info *a = *(struct thread_db_info **) ap;
-  struct thread_db_info *b = *(struct thread_db_info **) bp;
   int retval;
 
   retval = strcmp (a->filename, b->filename);
   if (retval)
-    return retval;
+    return retval < 0;
 
-  return (a->pid > b->pid) - (a->pid - b->pid);
+  return a->pid < b->pid;
 }
 
 /* Implement 'info auto-load libthread-db'.  */
@@ -1584,43 +1552,31 @@  info_auto_load_libthread_db (const char *args, int from_tty)
 {
   struct ui_out *uiout = current_uiout;
   const char *cs = args ? args : "";
-  struct thread_db_info *info, **array;
-  unsigned info_count, unique_filenames;
-  size_t max_filename_len, max_pids_len, pids_len;
-  struct cleanup *back_to;
-  char *pids;
+  struct thread_db_info *info;
+  unsigned unique_filenames;
+  size_t max_filename_len, pids_len;
   int i;
 
   cs = skip_spaces (cs);
   if (*cs)
     error (_("'info auto-load libthread-db' does not accept any parameters"));
 
-  info_count = 0;
+  std::vector<struct thread_db_info *> array;
   for (info = thread_db_list; info; info = info->next)
     if (info->filename != NULL)
-      info_count++;
-
-  array = XNEWVEC (struct thread_db_info *, info_count);
-  back_to = make_cleanup (xfree, array);
-
-  info_count = 0;
-  for (info = thread_db_list; info; info = info->next)
-    if (info->filename != NULL)
-      array[info_count++] = info;
+      array.push_back (info);
 
   /* Sort ARRAY by filenames and PIDs.  */
-
-  qsort (array, info_count, sizeof (*array),
-	 info_auto_load_libthread_db_compare);
+  std::sort (array.begin (), array.end (),
+	     info_auto_load_libthread_db_compare);
 
   /* Calculate the number of unique filenames (rows) and the maximum string
      length of PIDs list for the unique filenames (columns).  */
 
   unique_filenames = 0;
   max_filename_len = 0;
-  max_pids_len = 0;
   pids_len = 0;
-  for (i = 0; i < info_count; i++)
+  for (i = 0; i < array.size (); i++)
     {
       int pid = array[i]->pid;
       size_t this_pid_len;
@@ -1635,23 +1591,17 @@  info_auto_load_libthread_db (const char *args, int from_tty)
 				       strlen (array[i]->filename));
 
 	  if (i > 0)
-	    {
-	      pids_len -= strlen (", ");
-	      max_pids_len = std::max (max_pids_len, pids_len);
-	    }
+	    pids_len -= strlen (", ");
 	  pids_len = 0;
 	}
       pids_len += this_pid_len + strlen (", ");
     }
   if (i)
-    {
-      pids_len -= strlen (", ");
-      max_pids_len = std::max (max_pids_len, pids_len);
-    }
+    pids_len -= strlen (", ");
 
   /* Table header shifted right by preceding "libthread-db:  " would not match
      its columns.  */
-  if (info_count > 0 && args == auto_load_info_scripts_pattern_nl)
+  if (array.size () > 0 && args == auto_load_info_scripts_pattern_nl)
     uiout->text ("\n");
 
   {
@@ -1662,45 +1612,31 @@  info_auto_load_libthread_db (const char *args, int from_tty)
     uiout->table_header (pids_len, ui_left, "PIDs", "Pids");
     uiout->table_body ();
 
-    pids = (char *) xmalloc (max_pids_len + 1);
-    make_cleanup (xfree, pids);
-
     /* Note I is incremented inside the cycle, not at its end.  */
-    for (i = 0; i < info_count;)
+    for (i = 0; i < array.size ();)
       {
 	ui_out_emit_tuple tuple_emitter (uiout, NULL);
-	char *pids_end;
 
 	info = array[i];
 	uiout->field_string ("filename", info->filename);
-	pids_end = pids;
 
-	while (i < info_count && strcmp (info->filename,
-					 array[i]->filename) == 0)
+	std::string pids;
+	while (i < array.size () && strcmp (info->filename,
+					    array[i]->filename) == 0)
 	  {
-	    if (pids_end != pids)
-	      {
-		*pids_end++ = ',';
-		*pids_end++ = ' ';
-	      }
-	    pids_end += xsnprintf (pids_end,
-				   &pids[max_pids_len + 1] - pids_end,
-				   "%u", array[i]->pid);
-	    gdb_assert (pids_end < &pids[max_pids_len + 1]);
-
+	    if (!pids.empty ())
+	      pids += ", ";
+	    string_appendf (pids, "%u", array[i]->pid);
 	    i++;
 	  }
-	*pids_end = '\0';
 
-	uiout->field_string ("pids", pids);
+	uiout->field_string ("pids", pids.c_str ());
 
 	uiout->text ("\n");
       }
   }
 
-  do_cleanups (back_to);
-
-  if (info_count == 0)
+  if (array.empty ())
     uiout->message (_("No auto-loaded libthread-db.\n"));
 }