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

[Pedro Alves <palves at redhat dot com>]

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.

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

> 
> ---
>  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?

(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.)

>  AM_ICONV
>  
>  # GDB may fork/exec the iconv program to get the list of supported character
> diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
> index 062eede..0369a0a 100644
> --- a/gdb/fbsd-nat.c
> +++ b/gdb/fbsd-nat.c
> @@ -26,6 +26,10 @@
>  #include <sys/types.h>
>  #include <sys/procfs.h>
>  #include <sys/sysctl.h>
> +#ifdef HAVE_KINFO_GETVMMAP
> +#include <sys/user.h>
> +#include <libutil.h>
> +#endif
>  
>  #include "elf-bfd.h"
>  #include "fbsd-nat.h"
> @@ -62,6 +66,7 @@ fbsd_pid_to_exec_file (struct target_ops *self, int pid)
>    return NULL;
>  }
>  
> +#ifndef HAVE_KINFO_GETVMMAP
>  static int
>  fbsd_read_mapping (FILE *mapfile, unsigned long *start, unsigned long *end,
>  		   char *protection)
> @@ -82,6 +87,7 @@ fbsd_read_mapping (FILE *mapfile, unsigned long *start, unsigned long *end,
>  
>    return (ret != 0 && ret != EOF);
>  }
> +#endif
>  
>  /* Iterate over all the memory regions in the current inferior,
>     calling FUNC for each memory region.  OBFD is passed as the last
> @@ -91,6 +97,55 @@ int
>  fbsd_find_memory_regions (struct target_ops *self,
>  			  find_memory_region_ftype func, void *obfd)
>  {
> +#ifdef HAVE_KINFO_GETVMMAP
> +  pid_t pid = ptid_get_pid (inferior_ptid);
> +  struct kinfo_vmentry *vmentl, *kve;
> +  uint64_t size;
> +  struct cleanup *cleanup;
> +  int i, nitems;
> +
> +  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.

> +
> +  for (i = 0; i < nitems; i++)
> +    {
> +      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)
> +	continue;

|| goes at the beginning of the other line.

> +
> +      /* Skip segments with an invalid type.  */
> +      if (kve->kve_type != KVME_TYPE_DEFAULT &&
> +	  kve->kve_type != KVME_TYPE_VNODE &&
> +	  kve->kve_type != KVME_TYPE_SWAP &&
> +	  kve->kve_type != KVME_TYPE_PHYS)
> +	continue;

Likewise &&'s here.

> +
> +      size = kve->kve_end - kve->kve_start;
> +      if (info_verbose)
> +	{
> +	  fprintf_filtered (gdb_stdout, 
> +			    "Save segment, %ld bytes at %s (%c%c%c)\n",
> +			    (long)size,

Space after cast.

> +			    paddress (target_gdbarch (), kve->kve_start),
> +			    kve->kve_protection & KVME_PROT_READ ? 'r' : '-',
> +			    kve->kve_protection & KVME_PROT_WRITE ? 'w' : '-',
> +			    kve->kve_protection & KVME_PROT_EXEC ? 'x' : '-');
> +	}
> +
> +      /* Invoke the callback function to create the corefile segment.
> +	 Pass MODIFIED as true, we do not know the real modification state.  */
> +      func (kve->kve_start, size, kve->kve_protection & KVME_PROT_READ,
> +	    kve->kve_protection & KVME_PROT_WRITE,
> +	    kve->kve_protection & KVME_PROT_EXEC, 1, obfd);
> +    }
> +  do_cleanups (cleanup);
> +  return 0;
> +#else
>    pid_t pid = ptid_get_pid (inferior_ptid);
>    char *mapfilename;
>    FILE *mapfile;
> @@ -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.

Thanks,
Pedro Alves