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

login
register
mail settings
Submitter John Baldwin
Date Aug. 8, 2017, 6:19 a.m.
Message ID <20170808061917.73979-3-jhb@FreeBSD.org>
Download mbox | patch
Permalink /patch/21970/
State New
Headers show

Comments

John Baldwin - Aug. 8, 2017, 6:19 a.m.
- 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(-)
Simon Marchi - Aug. 8, 2017, 9:16 a.m.
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
John Baldwin - Aug. 8, 2017, 3 p.m.
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.

Patch

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.  */