Re: [PATCH] Use kinfo_getvmmap () on FreeBSD to enumerate memory regions.

[John Baldwin <jhb at freebsd dot org>]

On Friday, March 06, 2015 07:49:23 PM Pedro Alves wrote:
> Hi John,
> 
> Looks good to me, with a few nits/suggestions below.
> 
> On 02/26/2015 09:44 PM, John Baldwin wrote:
> > Use kinfo_getvmmap () from libutil on FreeBSD to enumerate memory
> > regions in a running process instead of /proc/<pid>/map.  FreeBSD systems
> > do not mount procfs by default, but kinfo_getvmmap () uses a sysctl that
> > is always available.
> > 
> > Skip memory regions for devices as well as regions an application has
> > requested to not be dumped via the MAP_NOCORE flag to mmap () or
> > MADV_NOCORE advice to madvise ().
> 
> Note that GNU's coding conventions tell us to refer to functions by
> name without the ()'s.

Ok.

> > gdb/ChangeLog:
> > 	* configure.ac: AC_CHECK_LIB(util, kinfo_getvmmap).
> > 	* configure: Regenerate.
> > 	* config.in: Regenerate.
> > 	* fbsd-nat.c (fbsd_find_memory_regions): Use kinfo_getvmmap to
> > 	enumerate memory regions if present.
> 
>  	* fbsd-nat.c [!HAVE_KINFO_GETVMMAP] (fbsd_read_mapping): Don't
> 	define.
>  	(fbsd_find_memory_regions): Use kinfo_getvmmap to
>  	enumerate memory regions if present.

Ok.

> > ---
> > 
> >  gdb/configure.ac |  5 +++++
> >  gdb/fbsd-nat.c   | 56
> >  ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files
> >  changed, 61 insertions(+)
> > 
> > diff --git a/gdb/configure.ac b/gdb/configure.ac
> > index 6ac8adb..b094164 100644
> > --- a/gdb/configure.ac
> > +++ b/gdb/configure.ac
> > @@ -532,6 +532,11 @@ AM_ZLIB
> > 
> >  # On HP/UX we may need libxpdl for dlgetmodinfo (used by solib-pa64.c).
> >  AC_SEARCH_LIBS(dlgetmodinfo, [dl xpdl])
> > 
> > +# On FreeBSD we may need libutil for kinfo_getvmmap (used by fbsd-nat.c).
> > +AC_CHECK_LIB(util, kinfo_getvmmap,
> > +  [AC_DEFINE(HAVE_KINFO_GETVMMAP, 1,
> > +            [Define to 1 if your system has the kinfo_getvmmap function.
> > ])]) +
> 
> Isn't
> 
>  AC_SEARCH_LIBS(kinfo_getvmmap, [util])
> 
> pretty much the same?

Might be, I wasn't sure from reading the autoconf docs that this would add the 
appropriate define.  I'll certainly try it and if it works I'm happier to use 
the simpler form.

> (Note: please make sure to use pristine GNU autoconf 2.64 when generating
> the files, to avoid spurious odd differences coming out.  Some distros
> carry local patches that result in that, dunno about FreeBSD.)

Yes, I had to download my own copy to use when I tested this locally.

> > +  vmentl = kinfo_getvmmap (pid, &nitems);
> > +  if (vmentl == NULL)
> > +    perror_with_name (_("Couldn't fetch VM map entries."));
> > +  cleanup = make_cleanup (free, vmentl);
> 
> s/free/xfree/g.

I wasn't sure about this one.  kinfo_getvmmap() calls malloc() from libc 
internally (so it isn't using xmalloc() to allocate the memory that is 
returned).  Is it still correct to use xfree() instead of free() in that case?

 > @@ -136,4 +191,5 @@ fbsd_find_memory_regions (struct target_ops *self,
> > 
> >    do_cleanups (cleanup);
> >    return 0;
> > 
> > +#endif
> > 
> >  }
> 
> I'd suggest splitting fbsd_find_memory_regions in two instead of the
> big #if/#else/#endif, but that's just personal preference.

Do you mean an #if/#else/#endif around the entire function vs just the body or 
do you mean something else?  If the former, I can easily do that (and collapse 
down to on #if/#else/#endif with the prior conditionally-defined function).

-- 
John Baldwin