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

login
register
mail settings
Submitter John Baldwin
Date Aug. 9, 2017, 5:47 p.m.
Message ID <20170809174754.49166-3-jhb@FreeBSD.org>
Download mbox | patch
Permalink /patch/22052/
State New
Headers show

Comments

John Baldwin - Aug. 9, 2017, 5:47 p.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::byte_vector 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::byte_vector.
	(fbsd_add_threads): Likewise.
---
 gdb/ChangeLog  |  9 +++++++++
 gdb/fbsd-nat.c | 58 +++++++++++++++++++++++++++-------------------------------
 2 files changed, 36 insertions(+), 31 deletions(-)
Pedro Alves - Aug. 10, 2017, 1:24 p.m.
On 08/09/2017 06:47 PM, John Baldwin wrote:
> +  gdb::unique_xmalloc_ptr<lwpid_t> lwps (XCNEWVEC (lwpid_t, nlwps));

> +      lwpid_t lwp = lwps.get ()[i];

I was going to say:

~~~
Note that std::unique_ptr has an array version that avoids the
get() in "lwps.get ()[i]".  I.e., with:

  gdb::unique_xmalloc_ptr<lwpid_t[]> lwps (XCNEWVEC (lwpid_t, nlwps));
                                 ^^
you can then write the more natural:

  lwpid_t lwp = lwps[i];
~~~

... but realized it doesn't work currently, because I missed adding
an xfree_deleter array specialization.  Fixed now:

 https://sourceware.org/ml/gdb-patches/2017-08/msg00212.html

Thanks,
Pedro Alves
John Baldwin - Aug. 10, 2017, 2:37 p.m.
On Thursday, August 10, 2017 02:24:52 PM Pedro Alves wrote:
> On 08/09/2017 06:47 PM, John Baldwin wrote:
> > +  gdb::unique_xmalloc_ptr<lwpid_t> lwps (XCNEWVEC (lwpid_t, nlwps));
> 
> > +      lwpid_t lwp = lwps.get ()[i];
> 
> I was going to say:
> 
> ~~~
> Note that std::unique_ptr has an array version that avoids the
> get() in "lwps.get ()[i]".  I.e., with:
> 
>   gdb::unique_xmalloc_ptr<lwpid_t[]> lwps (XCNEWVEC (lwpid_t, nlwps));
>                                  ^^
> you can then write the more natural:
> 
>   lwpid_t lwp = lwps[i];
> ~~~
> 
> ... but realized it doesn't work currently, because I missed adding
> an xfree_deleter array specialization.  Fixed now:
> 
>  https://sourceware.org/ml/gdb-patches/2017-08/msg00212.html

Thanks, I'll change this to use that instead.

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 133fbaf3bd..52cdc60546 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::byte_vector.
+	(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..721e72b85c 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -18,6 +18,7 @@ 
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "defs.h"
+#include "byte-vector.h"
 #include "gdbcore.h"
 #include "inferior.h"
 #include "regcache.h"
@@ -75,6 +76,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 +93,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 +134,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 +167,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 +204,6 @@  fbsd_find_memory_regions (struct target_ops *self,
       func (start, size, read, write, exec, 1, obfd);
     }
 
-  do_cleanups (cleanup);
   return 0;
 }
 #endif
@@ -392,8 +393,8 @@  fbsd_xfer_partial (struct target_ops *ops, enum target_object object,
 #endif
     case TARGET_OBJECT_AUXV:
       {
-	struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
-	unsigned char *buf;
+	gdb::byte_vector buf_storage;
+	gdb_byte *buf;
 	size_t buflen;
 	int mib[4];
 
@@ -411,8 +412,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.resize (buflen);
+	    buf = buf_storage.data ();
 	  }
 	if (sysctl (mib, 4, buf, &buflen, NULL, 0) == 0)
 	  {
@@ -426,11 +427,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 +623,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 +630,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 +648,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 +656,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.  */