[PATCH 0/3] Some C++-ification of the FreeBSD target

[PATCH 0/3] Some C++-ification of the FreeBSD target

[John Baldwin]

Just a few C++ cleanups of the FreeBSD native target.  This should remove
the remaining cleanups from the BSD targets other than bsd-uthread.c.

John Baldwin (3):
  Fix compile in the !HAVE_KINFO_GETVMMAP case.
  Replace remaining cleanups in fbsd-nat.c.
  Replace home-grown linked-lists in FreeBSD's native target with
    std::list.

 gdb/ChangeLog  |  26 ++++++++++++
 gdb/fbsd-nat.c | 130 ++++++++++++++++++++++-----------------------------------
 2 files changed, 75 insertions(+), 81 deletions(-)

-- 
2.13.3

[PATCH 3/3] Replace home-grown linked-lists in FreeBSD's native target with std::list.

[John Baldwin]

FreeBSD's native target uses linked-lists to keep track of pending fork
events and fake vfork done events.  Replace these with instances of
std::list.

gdb/ChangeLog:

	* fbsd-nat.c (struct fbsd_fork_info): Remove.
	(fbsd_pending_children): Use std::list.
	(fbsd_remember_child): Likewise.
	(fbsd_is_child_pending): Likewise.
	(fbsd_pending_vfork_done): Likewise.
	(fbsd_add_vfork_done): Likewise.
	(fbsd_is_vfork_done_pending): Likewise.
	(fbsd_next_vfork_done): Likewise.
---
 gdb/ChangeLog  | 11 +++++++++
 gdb/fbsd-nat.c | 71 +++++++++++++++++-----------------------------------------
 2 files changed, 32 insertions(+), 50 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 8e25be490c..8030d30712 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,16 @@
 2017-08-07  John Baldwin  <jhb@FreeBSD.org>
 
+	* fbsd-nat.c (struct fbsd_fork_info): Remove.
+	(fbsd_pending_children): Use std::list.
+	(fbsd_remember_child): Likewise.
+	(fbsd_is_child_pending): Likewise.
+	(fbsd_pending_vfork_done): Likewise.
+	(fbsd_add_vfork_done): Likewise.
+	(fbsd_is_vfork_done_pending): Likewise.
+	(fbsd_next_vfork_done): Likewise.
+
+2017-08-07  John Baldwin  <jhb@FreeBSD.org>
+
 	* fbsd-nat.c [HAVE_KINFO_GETVMMAP] (struct free_deleter): New.
 	(fbsd_find_memory_regions): Use free_deleter with std::unique_ptr.
 	[!HAVE_KINFO_GETVMMAP] (fbsd_find_memory_regions): Use std::string
diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index 4f47369084..48649dc21b 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -40,6 +40,8 @@
 #include "elf-bfd.h"
 #include "fbsd-nat.h"
 
+#include <list>
+
 /* Return the name of a file that can be opened to get the symbols for
    the child process identified by PID.  */
 
@@ -711,13 +713,7 @@ fbsd_update_thread_list (struct target_ops *ops)
   sake.  FreeBSD versions newer than 9.1 contain both fixes.
 */
 
-struct fbsd_fork_info
-{
-  struct fbsd_fork_info *next;
-  ptid_t ptid;
-};
-
-static struct fbsd_fork_info *fbsd_pending_children;
+static std::list<ptid_t> fbsd_pending_children;
 
 /* Record a new child process event that is reported before the
    corresponding fork event in the parent.  */
@@ -725,11 +721,7 @@ static struct fbsd_fork_info *fbsd_pending_children;
 static void
 fbsd_remember_child (ptid_t pid)
 {
-  struct fbsd_fork_info *info = XCNEW (struct fbsd_fork_info);
-
-  info->ptid = pid;
-  info->next = fbsd_pending_children;
-  fbsd_pending_children = info;
+  fbsd_pending_children.push_front (pid);
 }
 
 /* Check for a previously-recorded new child process event for PID.
@@ -738,39 +730,26 @@ fbsd_remember_child (ptid_t pid)
 static ptid_t
 fbsd_is_child_pending (pid_t pid)
 {
-  struct fbsd_fork_info *info, *prev;
-  ptid_t ptid;
-
-  prev = NULL;
-  for (info = fbsd_pending_children; info; prev = info, info = info->next)
-    {
-      if (ptid_get_pid (info->ptid) == pid)
-	{
-	  if (prev == NULL)
-	    fbsd_pending_children = info->next;
-	  else
-	    prev->next = info->next;
-	  ptid = info->ptid;
-	  xfree (info);
-	  return ptid;
-	}
-    }
+  for (auto it = fbsd_pending_children.begin ();
+       it != fbsd_pending_children.end (); it++)
+    if (it->pid () == pid)
+      {
+	ptid_t ptid = *it;
+	fbsd_pending_children.erase (it);
+	return ptid;
+      }
   return null_ptid;
 }
 
 #ifndef PTRACE_VFORK
-static struct fbsd_fork_info *fbsd_pending_vfork_done;
+static std::list<ptid_t> fbsd_pending_vfork_done;
 
 /* Record a pending vfork done event.  */
 
 static void
 fbsd_add_vfork_done (ptid_t pid)
 {
-  struct fbsd_fork_info *info = XCNEW (struct fbsd_fork_info);
-
-  info->ptid = pid;
-  info->next = fbsd_pending_vfork_done;
-  fbsd_pending_vfork_done = info;
+  fbsd_pending_vfork_done.push_front (pid);
 }
 
 /* Check for a pending vfork done event for a specific PID.  */
@@ -778,13 +757,10 @@ fbsd_add_vfork_done (ptid_t pid)
 static int
 fbsd_is_vfork_done_pending (pid_t pid)
 {
-  struct fbsd_fork_info *info;
-
-  for (info = fbsd_pending_vfork_done; info != NULL; info = info->next)
-    {
-      if (ptid_get_pid (info->ptid) == pid)
-	return 1;
-    }
+  for (auto it = fbsd_pending_vfork_done.begin ();
+       it != fbsd_pending_vfork_done.end (); it++)
+    if (it->pid () == pid)
+      return 1;
   return 0;
 }
 
@@ -794,15 +770,10 @@ fbsd_is_vfork_done_pending (pid_t pid)
 static ptid_t
 fbsd_next_vfork_done (void)
 {
-  struct fbsd_fork_info *info;
-  ptid_t ptid;
-
-  if (fbsd_pending_vfork_done != NULL)
+  if (!fbsd_pending_vfork_done.empty ())
     {
-      info = fbsd_pending_vfork_done;
-      fbsd_pending_vfork_done = info->next;
-      ptid = info->ptid;
-      xfree (info);
+      ptid_t ptid = fbsd_pending_vfork_done.front ();
+      fbsd_pending_vfork_done.pop_front ();
       return ptid;
     }
   return null_ptid;
-- 
2.13.3

Re: [PATCH 3/3] Replace home-grown linked-lists in FreeBSD's native target with std::list.

[Simon Marchi]

Hi John,

> --- a/gdb/fbsd-nat.c
> +++ b/gdb/fbsd-nat.c
> @@ -40,6 +40,8 @@
>  #include "elf-bfd.h"
>  #include "fbsd-nat.h"
> 
> +#include <list>
> +
>  /* Return the name of a file that can be opened to get the symbols for
>     the child process identified by PID.  */
> 
> @@ -711,13 +713,7 @@ fbsd_update_thread_list (struct target_ops *ops)
>    sake.  FreeBSD versions newer than 9.1 contain both fixes.
>  */
> 
> -struct fbsd_fork_info
> -{
> -  struct fbsd_fork_info *next;
> -  ptid_t ptid;
> -};
> -
> -static struct fbsd_fork_info *fbsd_pending_children;
> +static std::list<ptid_t> fbsd_pending_children;

If you only need a singly linked list with push/pop at the front, you 
can use std::forward_list.

Otherwise, LGTM.

Simon

Re: [PATCH 3/3] Replace home-grown linked-lists in FreeBSD's native target with std::list.

[John Baldwin]

On Tuesday, August 08, 2017 11:21:16 AM Simon Marchi wrote:
> Hi John,
> 
> > --- a/gdb/fbsd-nat.c
> > +++ b/gdb/fbsd-nat.c
> > @@ -40,6 +40,8 @@
> >  #include "elf-bfd.h"
> >  #include "fbsd-nat.h"
> > 
> > +#include <list>
> > +
> >  /* Return the name of a file that can be opened to get the symbols for
> >     the child process identified by PID.  */
> > 
> > @@ -711,13 +713,7 @@ fbsd_update_thread_list (struct target_ops *ops)
> >    sake.  FreeBSD versions newer than 9.1 contain both fixes.
> >  */
> > 
> > -struct fbsd_fork_info
> > -{
> > -  struct fbsd_fork_info *next;
> > -  ptid_t ptid;
> > -};
> > -
> > -static struct fbsd_fork_info *fbsd_pending_children;
> > +static std::list<ptid_t> fbsd_pending_children;
> 
> If you only need a singly linked list with push/pop at the front, you 
> can use std::forward_list.

Oh, yes, and the old list was a singly-linked list as well.

> Otherwise, LGTM.
> 
> Simon


-- 
John Baldwin

[PATCH 1/3] Fix compile in the !HAVE_KINFO_GETVMMAP case.

[John Baldwin]

gdb/ChangeLog:

	* fbsd-nat.c: [!HAVE_KINFO_GETVMMAP]: Include <sys/user.h> and
	"filestuff.h".
	(fbsd_find_memory_regions): Fix `mapfile' initialization.
---
 gdb/ChangeLog  | 6 ++++++
 gdb/fbsd-nat.c | 6 ++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index c4ad2bf1eb..133fbaf3bd 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2017-08-07  John Baldwin  <jhb@FreeBSD.org>
+
+	* fbsd-nat.c: [!HAVE_KINFO_GETVMMAP]: Include <sys/user.h> and
+	"filestuff.h".
+	(fbsd_find_memory_regions): Fix `mapfile' initialization.
+
 2017-08-07  Maciej W. Rozycki  <macro@imgtec.com>
 
 	PR breakpoints/21886
diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index 833f460237..3d3aa3df59 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -30,9 +30,11 @@
 #include <sys/ptrace.h>
 #include <sys/signal.h>
 #include <sys/sysctl.h>
-#ifdef HAVE_KINFO_GETVMMAP
 #include <sys/user.h>
+#ifdef HAVE_KINFO_GETVMMAP
 #include <libutil.h>
+#else
+#include "filestuff.h"
 #endif
 
 #include "elf-bfd.h"
@@ -168,7 +170,7 @@ fbsd_find_memory_regions (struct target_ops *self,
 
   mapfilename = xstrprintf ("/proc/%ld/map", (long) pid);
   cleanup = make_cleanup (xfree, mapfilename);
-  gdb_file_up mapfile = fopen (mapfilename, "r");
+  gdb_file_up mapfile (fopen (mapfilename, "r"));
   if (mapfile == NULL)
     error (_("Couldn't open %s."), mapfilename);
 
-- 
2.13.3

[PATCH 2/3] Replace remaining cleanups in fbsd-nat.c.

[John Baldwin]

- Use a custom deleter with std::unique_ptr to free() memory returned
  by kinfo_getvmmap().
- Use std::string with string_printf() to generate the pathname of the
  procfs 'map' file.
- Use gdb::unique_xmalloc_ptr to manage the dynamic buffer for
  TARGET_OBJECT_AUXV and the dynamically allocated array of LWP IDs.

gdb/ChangeLog:

	* fbsd-nat.c [HAVE_KINFO_GETVMMAP] (struct free_deleter): New.
	(fbsd_find_memory_regions): Use free_deleter with std::unique_ptr.
	[!HAVE_KINFO_GETVMMAP] (fbsd_find_memory_regions): Use std::string
	for `mapfilename'.
	(fbsd_xfer_partial): Use gdb::unique_xmalloc_ptr.
	(fbsd_add_threads): Likewise.
---
 gdb/ChangeLog  |  9 +++++++++
 gdb/fbsd-nat.c | 55 +++++++++++++++++++++++++------------------------------
 2 files changed, 34 insertions(+), 30 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 133fbaf3bd..8e25be490c 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,14 @@
 2017-08-07  John Baldwin  <jhb@FreeBSD.org>
 
+	* fbsd-nat.c [HAVE_KINFO_GETVMMAP] (struct free_deleter): New.
+	(fbsd_find_memory_regions): Use free_deleter with std::unique_ptr.
+	[!HAVE_KINFO_GETVMMAP] (fbsd_find_memory_regions): Use std::string
+	for `mapfilename'.
+	(fbsd_xfer_partial): Use gdb::unique_xmalloc_ptr.
+	(fbsd_add_threads): Likewise.
+
+2017-08-07  John Baldwin  <jhb@FreeBSD.org>
+
 	* fbsd-nat.c: [!HAVE_KINFO_GETVMMAP]: Include <sys/user.h> and
 	"filestuff.h".
 	(fbsd_find_memory_regions): Fix `mapfile' initialization.
diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index 3d3aa3df59..4f47369084 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -75,6 +75,14 @@ fbsd_pid_to_exec_file (struct target_ops *self, int pid)
 }
 
 #ifdef HAVE_KINFO_GETVMMAP
+/* Deleter for std::unique_ptr that invokes free.  */
+
+template <typename T>
+struct free_deleter
+{
+  void operator() (T *ptr) const { free (ptr); }
+};
+
 /* Iterate over all the memory regions in the current inferior,
    calling FUNC for each memory region.  OBFD is passed as the last
    argument to FUNC.  */
@@ -84,20 +92,17 @@ fbsd_find_memory_regions (struct target_ops *self,
 			  find_memory_region_ftype func, void *obfd)
 {
   pid_t pid = ptid_get_pid (inferior_ptid);
-  struct kinfo_vmentry *vmentl, *kve;
+  struct kinfo_vmentry *kve;
   uint64_t size;
-  struct cleanup *cleanup;
   int i, nitems;
 
-  vmentl = kinfo_getvmmap (pid, &nitems);
+  std::unique_ptr<struct kinfo_vmentry, free_deleter<struct kinfo_vmentry>>
+    vmentl (kinfo_getvmmap (pid, &nitems));
   if (vmentl == NULL)
     perror_with_name (_("Couldn't fetch VM map entries."));
-  cleanup = make_cleanup (free, vmentl);
 
-  for (i = 0; i < nitems; i++)
+  for (i = 0, kve = vmentl.get (); i < nitems; i++, kve++)
     {
-      kve = &vmentl[i];
-
       /* Skip unreadable segments and those where MAP_NOCORE has been set.  */
       if (!(kve->kve_protection & KVME_PROT_READ)
 	  || kve->kve_flags & KVME_FLAG_NOCOREDUMP)
@@ -128,7 +133,6 @@ fbsd_find_memory_regions (struct target_ops *self,
 	    kve->kve_protection & KVME_PROT_WRITE,
 	    kve->kve_protection & KVME_PROT_EXEC, 1, obfd);
     }
-  do_cleanups (cleanup);
   return 0;
 }
 #else
@@ -162,21 +166,18 @@ fbsd_find_memory_regions (struct target_ops *self,
 			  find_memory_region_ftype func, void *obfd)
 {
   pid_t pid = ptid_get_pid (inferior_ptid);
-  char *mapfilename;
   unsigned long start, end, size;
   char protection[4];
   int read, write, exec;
-  struct cleanup *cleanup;
 
-  mapfilename = xstrprintf ("/proc/%ld/map", (long) pid);
-  cleanup = make_cleanup (xfree, mapfilename);
-  gdb_file_up mapfile (fopen (mapfilename, "r"));
+  std::string mapfilename = string_printf ("/proc/%ld/map", (long) pid);
+  gdb_file_up mapfile (fopen (mapfilename.c_str (), "r"));
   if (mapfile == NULL)
-    error (_("Couldn't open %s."), mapfilename);
+    error (_("Couldn't open %s."), mapfilename.c_str ());
 
   if (info_verbose)
     fprintf_filtered (gdb_stdout, 
-		      "Reading memory regions from %s\n", mapfilename);
+		      "Reading memory regions from %s\n", mapfilename.c_str ());
 
   /* Now iterate until end-of-file.  */
   while (fbsd_read_mapping (mapfile.get (), &start, &end, &protection[0]))
@@ -202,7 +203,6 @@ fbsd_find_memory_regions (struct target_ops *self,
       func (start, size, read, write, exec, 1, obfd);
     }
 
-  do_cleanups (cleanup);
   return 0;
 }
 #endif
@@ -392,7 +392,7 @@ fbsd_xfer_partial (struct target_ops *ops, enum target_object object,
 #endif
     case TARGET_OBJECT_AUXV:
       {
-	struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
+	gdb::unique_xmalloc_ptr<unsigned char> buf_storage(nullptr);
 	unsigned char *buf;
 	size_t buflen;
 	int mib[4];
@@ -411,8 +411,8 @@ fbsd_xfer_partial (struct target_ops *ops, enum target_object object,
 	else
 	  {
 	    buflen = offset + len;
-	    buf = XCNEWVEC (unsigned char, buflen);
-	    cleanup = make_cleanup (xfree, buf);
+	    buf_storage.reset (XCNEWVEC (unsigned char, buflen));
+	    buf = buf_storage.get ();
 	  }
 	if (sysctl (mib, 4, buf, &buflen, NULL, 0) == 0)
 	  {
@@ -426,11 +426,9 @@ fbsd_xfer_partial (struct target_ops *ops, enum target_object object,
 		else
 		  buflen = 0;
 	      }
-	    do_cleanups (cleanup);
 	    *xfered_len = buflen;
 	    return (buflen == 0) ? TARGET_XFER_EOF : TARGET_XFER_OK;
 	  }
-	do_cleanups (cleanup);
 	return TARGET_XFER_E_IO;
       }
     default:
@@ -624,8 +622,6 @@ fbsd_enable_proc_events (pid_t pid)
 static void
 fbsd_add_threads (pid_t pid)
 {
-  struct cleanup *cleanup;
-  lwpid_t *lwps;
   int i, nlwps;
 
   gdb_assert (!in_thread_list (pid_to_ptid (pid)));
@@ -633,16 +629,16 @@ fbsd_add_threads (pid_t pid)
   if (nlwps == -1)
     perror_with_name (("ptrace"));
 
-  lwps = XCNEWVEC (lwpid_t, nlwps);
-  cleanup = make_cleanup (xfree, lwps);
+  gdb::unique_xmalloc_ptr<lwpid_t> lwps (XCNEWVEC (lwpid_t, nlwps));
 
-  nlwps = ptrace (PT_GETLWPLIST, pid, (caddr_t) lwps, nlwps);
+  nlwps = ptrace (PT_GETLWPLIST, pid, (caddr_t) lwps.get (), nlwps);
   if (nlwps == -1)
     perror_with_name (("ptrace"));
 
   for (i = 0; i < nlwps; i++)
     {
-      ptid_t ptid = ptid_build (pid, lwps[i], 0);
+      lwpid_t lwp = lwps.get ()[i];
+      ptid_t ptid = ptid_build (pid, lwp, 0);
 
       if (!in_thread_list (ptid))
 	{
@@ -651,7 +647,7 @@ fbsd_add_threads (pid_t pid)
 
 	  /* Don't add exited threads.  Note that this is only called
 	     when attaching to a multi-threaded process.  */
-	  if (ptrace (PT_LWPINFO, lwps[i], (caddr_t) &pl, sizeof pl) == -1)
+	  if (ptrace (PT_LWPINFO, lwp, (caddr_t) &pl, sizeof pl) == -1)
 	    perror_with_name (("ptrace"));
 	  if (pl.pl_flags & PL_FLAG_EXITED)
 	    continue;
@@ -659,11 +655,10 @@ fbsd_add_threads (pid_t pid)
 	  if (debug_fbsd_lwp)
 	    fprintf_unfiltered (gdb_stdlog,
 				"FLWP: adding thread for LWP %u\n",
-				lwps[i]);
+				lwp);
 	  add_thread (ptid);
 	}
     }
-  do_cleanups (cleanup);
 }
 
 /* Implement the "to_update_thread_list" target_ops method.  */
-- 
2.13.3

Re: [PATCH 2/3] Replace remaining cleanups in fbsd-nat.c.

[Simon Marchi]

Hi John,

> @@ -84,20 +92,17 @@ fbsd_find_memory_regions (struct target_ops *self,
>  			  find_memory_region_ftype func, void *obfd)
>  {
>    pid_t pid = ptid_get_pid (inferior_ptid);
> -  struct kinfo_vmentry *vmentl, *kve;
> +  struct kinfo_vmentry *kve;
>    uint64_t size;
> -  struct cleanup *cleanup;
>    int i, nitems;
> 
> -  vmentl = kinfo_getvmmap (pid, &nitems);
> +  std::unique_ptr<struct kinfo_vmentry, free_deleter<struct 
> kinfo_vmentry>>
> +    vmentl (kinfo_getvmmap (pid, &nitems));

Doesn't this essentially do the same thing as gdb::unique_xmalloc_ptr, 
since xfree calls free?

> @@ -392,7 +392,7 @@ fbsd_xfer_partial (struct target_ops *ops, enum
> target_object object,
>  #endif
>      case TARGET_OBJECT_AUXV:
>        {
> -	struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
> +	gdb::unique_xmalloc_ptr<unsigned char> buf_storage(nullptr);

You don't need to initialize explicitly to nullptr, that's the default 
value.  But if you still want to, then add a space before parenthesis 
:).

Otherwise, gdb::byte_vector with a .resize() would probably be 
appropriate to use here.

Thanks,

Simon

Re: [PATCH 2/3] Replace remaining cleanups in fbsd-nat.c.

[John Baldwin]

On Tuesday, August 08, 2017 11:16:28 AM Simon Marchi wrote:
> Hi John,
> 
> > @@ -84,20 +92,17 @@ fbsd_find_memory_regions (struct target_ops *self,
> >  			  find_memory_region_ftype func, void *obfd)
> >  {
> >    pid_t pid = ptid_get_pid (inferior_ptid);
> > -  struct kinfo_vmentry *vmentl, *kve;
> > +  struct kinfo_vmentry *kve;
> >    uint64_t size;
> > -  struct cleanup *cleanup;
> >    int i, nitems;
> > 
> > -  vmentl = kinfo_getvmmap (pid, &nitems);
> > +  std::unique_ptr<struct kinfo_vmentry, free_deleter<struct 
> > kinfo_vmentry>>
> > +    vmentl (kinfo_getvmmap (pid, &nitems));
> 
> Doesn't this essentially do the same thing as gdb::unique_xmalloc_ptr, 
> since xfree calls free?

Well, this calls an API in a system library that allocates memory via libc's
malloc() and requires it to be free'd via libc's free() rather than any
interpositions.  Thus, it isn't allocated via xmalloc() and that is why
the existing code uses free() rather than xfree().

> > @@ -392,7 +392,7 @@ fbsd_xfer_partial (struct target_ops *ops, enum
> > target_object object,
> >  #endif
> >      case TARGET_OBJECT_AUXV:
> >        {
> > -	struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
> > +	gdb::unique_xmalloc_ptr<unsigned char> buf_storage(nullptr);
> 
> You don't need to initialize explicitly to nullptr, that's the default 
> value.  But if you still want to, then add a space before parenthesis 
> :).

Oh, duh.

> Otherwise, gdb::byte_vector with a .resize() would probably be 
> appropriate to use here.

Ok, I'll rework it with that.

-- 
John Baldwin