[2/3] Replace remaining cleanups in fbsd-nat.c.
Commit Message
- 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(-)
Comments
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
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.
@@ -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.
@@ -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. */