[3/4] Support 'info proc' for native FreeBSD processes.

  - Command line arguments are fetched via the kern.proc.args.<pid>
  sysctl.
- The 'cwd' and 'exe' values are obtained from the per-process
  file descriptor table returned by kinfo_getfile() from libutil.
- 'mappings' is implemented by walking the array of VM map entries
  returned by kinfo_getvmmap() from libutil.
- 'stat' and 'status' output is generated by outputting fields from
  the structure returned by the kern.proc.pid.<pid> sysctl.

gdb/ChangeLog:

	* configure.ac: Check for kinfo_getfile in libutil.
	* configure: Regenerate.
	* config.in: Regenerate.
	* fbsd-nat.c: Include "fbsd-tdep.h".
	(fbsd_fetch_cmdline): New.
	(fbsd_fetch_kinfo_proc): Move earlier and change to return a bool
	rather than calling error.
	(fbsd_info_proc): New.
	(fbsd_thread_name): Report error if fbsd_fetch_kinfo_proc fails.
	(fbsd_wait): Report warning if fbsd_fetch_kinfo_proc fails.
	(fbsd_nat_add_target): Set "to_info_proc" to "fbsd_info_proc".
---
 gdb/ChangeLog    |  14 ++
 gdb/config.in    |   3 +
 gdb/configure    |  60 +++++++++
 gdb/configure.ac |   5 +
 gdb/fbsd-nat.c   | 404 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 5 files changed, 462 insertions(+), 24 deletions(-)
  

On 2017-12-22 05:05 PM, John Baldwin wrote:
> - Command line arguments are fetched via the kern.proc.args.<pid>
>   sysctl.
> - The 'cwd' and 'exe' values are obtained from the per-process
>   file descriptor table returned by kinfo_getfile() from libutil.
> - 'mappings' is implemented by walking the array of VM map entries
>   returned by kinfo_getvmmap() from libutil.
> - 'stat' and 'status' output is generated by outputting fields from
>   the structure returned by the kern.proc.pid.<pid> sysctl.
> 
> gdb/ChangeLog:
> 
> 	* configure.ac: Check for kinfo_getfile in libutil.
> 	* configure: Regenerate.
> 	* config.in: Regenerate.
> 	* fbsd-nat.c: Include "fbsd-tdep.h".
> 	(fbsd_fetch_cmdline): New.
> 	(fbsd_fetch_kinfo_proc): Move earlier and change to return a bool
> 	rather than calling error.
> 	(fbsd_info_proc): New.
> 	(fbsd_thread_name): Report error if fbsd_fetch_kinfo_proc fails.
> 	(fbsd_wait): Report warning if fbsd_fetch_kinfo_proc fails.
> 	(fbsd_nat_add_target): Set "to_info_proc" to "fbsd_info_proc".
> ---
>  gdb/ChangeLog    |  14 ++
>  gdb/config.in    |   3 +
>  gdb/configure    |  60 +++++++++
>  gdb/configure.ac |   5 +
>  gdb/fbsd-nat.c   | 404 +++++++++++++++++++++++++++++++++++++++++++++++++++----
>  5 files changed, 462 insertions(+), 24 deletions(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 2311749de7..3d580c1c9e 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,17 @@
> +2017-12-22  John Baldwin  <jhb@FreeBSD.org>
> +
> +	* configure.ac: Check for kinfo_getfile in libutil.
> +	* configure: Regenerate.
> +	* config.in: Regenerate.
> +	* fbsd-nat.c: Include "fbsd-tdep.h".
> +	(fbsd_fetch_cmdline): New.
> +	(fbsd_fetch_kinfo_proc): Move earlier and change to return a bool
> +	rather than calling error.
> +	(fbsd_info_proc): New.
> +	(fbsd_thread_name): Report error if fbsd_fetch_kinfo_proc fails.
> +	(fbsd_wait): Report warning if fbsd_fetch_kinfo_proc fails.
> +	(fbsd_nat_add_target): Set "to_info_proc" to "fbsd_info_proc".
> +
>  2017-12-22  John Baldwin  <jhb@FreeBSD.org>
>  
>  	* fbsd-tdep.c (KVE_STRUCTSIZE, KVE_START, KVE_END, KVE_OFFSET)
> diff --git a/gdb/config.in b/gdb/config.in
> index 1d11a97080..ad2cc1754e 100644
> --- a/gdb/config.in
> +++ b/gdb/config.in
> @@ -219,6 +219,9 @@
>  /* Define to 1 if you have the <inttypes.h> header file. */
>  #undef HAVE_INTTYPES_H
>  
> +/* Define to 1 if your system has the kinfo_getfile function. */
> +#undef HAVE_KINFO_GETFILE
> +
>  /* Define to 1 if your system has the kinfo_getvmmap function. */
>  #undef HAVE_KINFO_GETVMMAP
>  
> diff --git a/gdb/configure b/gdb/configure
> index 7b250079de..a0baa7a53a 100755
> --- a/gdb/configure
> +++ b/gdb/configure
> @@ -7927,6 +7927,66 @@ $as_echo "#define HAVE_KINFO_GETVMMAP 1" >>confdefs.h
>  fi
>  
>  
> +# fbsd-nat.c can also use kinfo_getfile.
> +{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for library containing kinfo_getfile" >&5
> +$as_echo_n "checking for library containing kinfo_getfile... " >&6; }
> +if test "${ac_cv_search_kinfo_getfile+set}" = set; then :
> +  $as_echo_n "(cached) " >&6
> +else
> +  ac_func_search_save_LIBS=$LIBS
> +cat confdefs.h - <<_ACEOF >conftest.$ac_ext
> +/* end confdefs.h.  */
> +
> +/* Override any GCC internal prototype to avoid an error.
> +   Use char because int might match the return type of a GCC
> +   builtin and then its argument prototype would still apply.  */
> +#ifdef __cplusplus
> +extern "C"
> +#endif
> +char kinfo_getfile ();
> +int
> +main ()
> +{
> +return kinfo_getfile ();
> +  ;
> +  return 0;
> +}
> +_ACEOF
> +for ac_lib in '' util util-freebsd; do
> +  if test -z "$ac_lib"; then
> +    ac_res="none required"
> +  else
> +    ac_res=-l$ac_lib
> +    LIBS="-l$ac_lib  $ac_func_search_save_LIBS"
> +  fi
> +  if ac_fn_c_try_link "$LINENO"; then :
> +  ac_cv_search_kinfo_getfile=$ac_res
> +fi
> +rm -f core conftest.err conftest.$ac_objext \
> +    conftest$ac_exeext
> +  if test "${ac_cv_search_kinfo_getfile+set}" = set; then :
> +  break
> +fi
> +done
> +if test "${ac_cv_search_kinfo_getfile+set}" = set; then :
> +
> +else
> +  ac_cv_search_kinfo_getfile=no
> +fi
> +rm conftest.$ac_ext
> +LIBS=$ac_func_search_save_LIBS
> +fi
> +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_search_kinfo_getfile" >&5
> +$as_echo "$ac_cv_search_kinfo_getfile" >&6; }
> +ac_res=$ac_cv_search_kinfo_getfile
> +if test "$ac_res" != no; then :
> +  test "$ac_res" = "none required" || LIBS="$ac_res $LIBS"
> +
> +$as_echo "#define HAVE_KINFO_GETFILE 1" >>confdefs.h
> +
> +fi
> +
> +
>  
>        if test "X$prefix" = "XNONE"; then
>      acl_final_prefix="$ac_default_prefix"
> diff --git a/gdb/configure.ac b/gdb/configure.ac
> index 8e706b6e27..96ec77e0af 100644
> --- a/gdb/configure.ac
> +++ b/gdb/configure.ac
> @@ -523,6 +523,11 @@ AC_SEARCH_LIBS(kinfo_getvmmap, util util-freebsd,
>    [AC_DEFINE(HAVE_KINFO_GETVMMAP, 1,
>              [Define to 1 if your system has the kinfo_getvmmap function. ])])
>  
> +# fbsd-nat.c can also use kinfo_getfile.
> +AC_SEARCH_LIBS(kinfo_getfile, util util-freebsd,
> +  [AC_DEFINE(HAVE_KINFO_GETFILE, 1,
> +            [Define to 1 if your system has the kinfo_getfile function. ])])
> +
>  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 29b7ee5e33..e460c23d95 100644
> --- a/gdb/fbsd-nat.c
> +++ b/gdb/fbsd-nat.c
> @@ -32,14 +32,16 @@
>  #include <sys/signal.h>
>  #include <sys/sysctl.h>
>  #include <sys/user.h>
> -#ifdef HAVE_KINFO_GETVMMAP
> +#if defined(HAVE_KINFO_GETFILE) || defined(HAVE_KINFO_GETVMMAP)
>  #include <libutil.h>
> -#else
> +#endif
> +#if !defined(HAVE_KINFO_GETVMMAP)
>  #include "filestuff.h"
>  #endif
>  
>  #include "elf-bfd.h"
>  #include "fbsd-nat.h"
> +#include "fbsd-tdep.h"
>  
>  #include <list>
>  
> @@ -77,7 +79,7 @@ fbsd_pid_to_exec_file (struct target_ops *self, int pid)
>    return NULL;
>  }
>  
> -#ifdef HAVE_KINFO_GETVMMAP
> +#if defined(HAVE_KINFO_GETFILE) || defined(HAVE_KINFO_GETVMMAP)
>  /* Deleter for std::unique_ptr that invokes free.  */
>  
>  template <typename T>
> @@ -85,7 +87,9 @@ struct free_deleter
>  {
>    void operator() (T *ptr) const { free (ptr); }
>  };
> +#endif
>  
> +#ifdef HAVE_KINFO_GETVMMAP
>  /* 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.  */
> @@ -210,6 +214,369 @@ fbsd_find_memory_regions (struct target_ops *self,
>  }
>  #endif
>  
> +/* Fetch the command line for a running process.  */
> +
> +static gdb::unique_xmalloc_ptr<char>
> +fbsd_fetch_cmdline (pid_t pid)
> +{
> +  size_t len;
> +  int mib[4];
> +
> +  len = 0;
> +  mib[0] = CTL_KERN;
> +  mib[1] = KERN_PROC;
> +  mib[2] = KERN_PROC_ARGS;
> +  mib[3] = pid;
> +  if (sysctl (mib, 4, NULL, &len, NULL, 0) == -1)
> +    return nullptr;
> +
> +  gdb::unique_xmalloc_ptr<char> cmdline ((char *) xmalloc (len));
> +  if (sysctl (mib, 4, cmdline.get (), &len, NULL, 0) == -1)
> +    return nullptr;
> +
> +  return cmdline;
> +}
> +
> +/* Fetch the external variant of the kernel's internal process
> +   structure for the process PID into KP.  */
> +
> +static bool
> +fbsd_fetch_kinfo_proc (pid_t pid, struct kinfo_proc *kp)
> +{
> +  size_t len;
> +  int mib[4];
> +
> +  len = sizeof *kp;
> +  mib[0] = CTL_KERN;
> +  mib[1] = KERN_PROC;
> +  mib[2] = KERN_PROC_PID;
> +  mib[3] = pid;
> +  return (sysctl (mib, 4, kp, &len, NULL, 0) == 0);
> +}
> +
> +/* Implement the "to_info_proc target_ops" method.  */
> +
> +static void
> +fbsd_info_proc (struct target_ops *ops, const char *args,
> +		enum info_proc_what what)
> +{
> +#ifdef HAVE_KINFO_GETFILE
> +  std::unique_ptr<struct kinfo_file, free_deleter<struct kinfo_file>> fdtbl;
> +  int nfd = 0;
> +#endif
> +  struct kinfo_proc kp;
> +  char *tmp;
> +  pid_t pid;
> +  bool do_cmdline = false;
> +  bool do_cwd = false;
> +  bool do_exe = false;
> +#ifdef HAVE_KINFO_GETVMMAP
> +  bool do_mappings = false;
> +#endif
> +  bool do_status = false;
> +  bool do_stat = false;
> +  bool kp_valid = false;
> +
> +  switch (what)
> +    {
> +    case IP_MINIMAL:
> +      do_cmdline = true;
> +      do_cwd = true;
> +      do_exe = true;
> +      break;
> +#ifdef HAVE_KINFO_GETVMMAP
> +    case IP_MAPPINGS:
> +      do_mappings = true;
> +      break;
> +#endif
> +    case IP_STATUS:
> +      do_status = true;
> +      break;
> +    case IP_STAT:
> +      do_stat = true;
> +      break;
> +    case IP_CMDLINE:
> +      do_cmdline = true;
> +      break;
> +    case IP_EXE:
> +      do_exe = true;
> +      break;
> +    case IP_CWD:
> +      do_cwd = true;
> +      break;
> +    case IP_ALL:
> +      do_cmdline = true;
> +      do_cwd = true;
> +      do_exe = true;
> +#ifdef HAVE_KINFO_GETVMMAP
> +      do_mappings = true;
> +#endif
> +      do_status = true;
> +      do_stat = true;
> +      break;
> +    default:
> +      error (_("Not supported on this target."));
> +    }
> +
> +  gdb_argv built_argv (args);
> +  if (built_argv.count () == 0)
> +    {
> +      pid = ptid_get_pid (inferior_ptid);
> +      if (pid == 0)
> +	error (_("No current process: you must name one."));
> +    }
> +  else
> +    {
> +      pid = strtol (built_argv[0], NULL, 10);
> +    }

Unnecessary curly braces.

> +
> +  printf_filtered (_("process %d\n"), pid);
> +#ifdef HAVE_KINFO_GETFILE
> +  if (do_cwd || do_exe)
> +    fdtbl.reset (kinfo_getfile (pid, &nfd));
> +#endif
> +  if (do_stat || do_status)
> +    {
> +      kp_valid = fbsd_fetch_kinfo_proc(pid, &kp);
> +      if (!kp_valid)
> +	warning (_("Failed to fetch process information"));
> +    }
> +
> +  if (do_cmdline)
> +    {
> +      gdb::unique_xmalloc_ptr<char> cmdline = fbsd_fetch_cmdline (pid);
> +      if (cmdline)
> +	printf_filtered ("cmdline = '%s'\n", cmdline.get ());
> +      else
> +	warning (_("unable to fetch command line"));
> +    }
> +  if (do_cwd)
> +    {
> +      const char *cwd = NULL;
> +#ifdef HAVE_KINFO_GETFILE
> +      struct kinfo_file *kf = fdtbl.get ();
> +      for (int i = 0; i < nfd; i++, kf++)
> +	{
> +	  if (kf->kf_type == KF_TYPE_VNODE && kf->kf_fd == KF_FD_TYPE_CWD)
> +	    {
> +	      cwd = kf->kf_path;
> +	      break;
> +	    }
> +	}
> +#endif
> +      if (cwd)
> +	printf_filtered ("cwd = '%s'\n", cwd);
> +      else
> +	warning (_("unable to fetch current working directory"));
> +    }
> +  if (do_exe)
> +    {
> +      const char *exe = NULL;
> +#ifdef HAVE_KINFO_GETFILE
> +      struct kinfo_file *kf = fdtbl.get ();
> +      for (int i = 0; i < nfd; i++, kf++)
> +	{
> +	  if (kf->kf_type == KF_TYPE_VNODE && kf->kf_fd == KF_FD_TYPE_TEXT)
> +	    {
> +	      exe = kf->kf_path;
> +	      break;
> +	    }
> +	}
> +#endif
> +      if (exe == NULL)
> +	exe = fbsd_pid_to_exec_file (ops, pid);
> +      if (exe)
> +	printf_filtered ("exe = '%s'\n", exe);
> +      else
> +	warning (_("unable to fetch executable path name"));
> +    }
> +#ifdef HAVE_KINFO_GETVMMAP
> +  if (do_mappings)
> +    {
> +      int nvment;
> +      std::unique_ptr<struct kinfo_vmentry, free_deleter<struct kinfo_vmentry>>

Is there a reason to have and use free_deleter rather than gdb::unique_xmalloc_ptr?

> +	vmentl (kinfo_getvmmap (pid, &nvment));
> +
> +      if (vmentl)

vmentl != NULL

There are a few other instances of if (ptr) that should be changed to if (ptr != NULL).

> +	{
> +	  printf_filtered (_("Mapped address spaces:\n\n"));
> +#ifdef __LP64__
> +	  printf_filtered ("  %18s %18s %10s %10s %9s %s\n",
> +			   "Start Addr",
> +			   "  End Addr",
> +			   "      Size", "    Offset", "Flags  ", "File");
> +#else
> +	  printf_filtered ("\t%10s %10s %10s %10s %9s %s\n",
> +			   "Start Addr",
> +			   "  End Addr",
> +			   "      Size", "    Offset", "Flags  ", "File");
> +#endif
> +
> +	  struct kinfo_vmentry *kve = vmentl.get ();
> +	  for (int i = 0; i < nvment; i++, kve++)
> +	    {
> +	      ULONGEST start, end;
> +
> +	      start = kve->kve_start;
> +	      end = kve->kve_end;
> +#ifdef __LP64__
> +	      printf_filtered ("  %18s %18s %10s %10s %9s %s\n",
> +			       hex_string (start),
> +			       hex_string (end),
> +			       hex_string (end - start),
> +			       hex_string (kve->kve_offset),
> +			       fbsd_vm_map_entry_flags (kve->kve_flags,
> +							kve->kve_protection),
> +			       kve->kve_path);
> +#else
> +	      printf_filtered ("\t%10s %10s %10s %10s %9s %s\n",
> +			       hex_string (start),
> +			       hex_string (end),
> +			       hex_string (end - start),
> +			       hex_string (kve->kve_offset),
> +			       fbsd_vm_map_entry_flags (kve->kve_flags,
> +							kve->kve_protection),
> +			       kve->kve_path);
> +#endif
> +	    }
> +	}
> +      else
> +	warning (_("unable to fetch virtual memory map"));
> +    }
> +#endif
> +  if (do_status && kp_valid)
> +    {
> +      const char *state;
> +      int pgtok;
> +
> +      printf_filtered ("Name:\t%s\n", kp.ki_comm);
> +      switch (kp.ki_stat)
> +	{
> +	case SIDL:
> +	  state = "I (idle)";
> +	  break;
> +	case SRUN:
> +	  state = "R (running)";
> +	  break;
> +	case SSTOP:
> +	  state = "T (stopped)";
> +	  break;
> +	case SZOMB:
> +	  state = "Z (zombie)";
> +	  break;
> +	case SSLEEP:
> +	  state = "S (sleeping)";
> +	  break;
> +	case SWAIT:
> +	  state = "W (interrupt wait)";
> +	  break;
> +	case SLOCK:
> +	  state = "L (blocked on lock)";
> +	  break;
> +	default:
> +	  state = "? (unknown)";
> +	  break;
> +	}
> +      printf_filtered ("State:\t%s\n", state);
> +      printf_filtered ("Pid:\t%d\n", kp.ki_pid);
> +      printf_filtered ("PPid:\t%d\n", kp.ki_ppid);
> +      printf_filtered ("Uid:\t%d %d %d\n", kp.ki_ruid, kp.ki_uid, kp.ki_svuid);
> +      printf_filtered ("Gid:\t%d %d %d\n", kp.ki_rgid, kp.ki_groups[0],
> +		       kp.ki_svgid);
> +      printf_filtered ("Groups:\t");
> +      for (int i = 0; i < kp.ki_ngroups; i++)
> +	printf_filtered ("%d ", kp.ki_groups[i]);
> +      printf_filtered ("\n");
> +      pgtok = getpagesize () / 1024;
> +      printf_filtered ("VmSize:\t%8ju kB\n", (uintmax_t) kp.ki_size / 1024);
> +      printf_filtered ("VmRSS:\t%8ju kB\n", (uintmax_t) kp.ki_rssize * pgtok);
> +      printf_filtered ("VmData:\t%8ju kB\n", (uintmax_t) kp.ki_dsize * pgtok);
> +      printf_filtered ("VmStk:\t%8ju kB\n", (uintmax_t) kp.ki_ssize * pgtok);
> +      printf_filtered ("VmExe:\t%8ju kB\n", (uintmax_t) kp.ki_tsize * pgtok);
> +      printf_filtered ("SigPnd:\t");
> +      for (int i = 0; i < _SIG_WORDS; i++)
> +	printf_filtered ("%08x ", kp.ki_siglist.__bits[i]);
> +      printf_filtered ("\n");
> +      printf_filtered ("SigIgn:\t");
> +      for (int i = 0; i < _SIG_WORDS; i++)
> +	printf_filtered ("%08x ", kp.ki_sigignore.__bits[i]);
> +      printf_filtered ("\n");
> +      printf_filtered ("SigCgt:\t");
> +      for (int i = 0; i < _SIG_WORDS; i++)
> +	printf_filtered ("%08x ", kp.ki_sigcatch.__bits[i]);
> +      printf_filtered ("\n");
> +    }
> +  if (do_stat && kp_valid)
> +    {
> +      char state;
> +      int pgtok;
> +
> +      printf_filtered ("Exec file: %s\n", kp.ki_comm);
> +      switch (kp.ki_stat)
> +	{
> +	case SIDL:
> +	  state = 'I';
> +	  break;
> +	case SRUN:
> +	  state = 'R';
> +	  break;
> +	case SSTOP:
> +	  state = 'T';
> +	  break;
> +	case SZOMB:
> +	  state = 'Z';
> +	  break;
> +	case SSLEEP:
> +	  state = 'S';
> +	  break;
> +	case SWAIT:
> +	  state = 'W';
> +	  break;
> +	case SLOCK:
> +	  state = 'L';
> +	  break;
> +	default:
> +	  state = '?';
> +	  break;
> +	}
> +      printf_filtered ("State: %c\n", state);
> +      printf_filtered ("Parent process: %d\n", kp.ki_ppid);
> +      printf_filtered ("Process group: %d\n", kp.ki_pgid);
> +      printf_filtered ("Session id: %d\n", kp.ki_sid);
> +      printf_filtered ("TTY: %ju\n", (uintmax_t) kp.ki_tdev);
> +      printf_filtered ("TTY owner process group: %d\n", kp.ki_tpgid);
> +      printf_filtered ("Minor faults (no memory page): %ld\n",
> +		       kp.ki_rusage.ru_minflt);
> +      printf_filtered ("Minor faults, children: %ld\n",
> +		       kp.ki_rusage_ch.ru_minflt);
> +      printf_filtered ("Major faults (memory page faults): %ld\n",
> +		       kp.ki_rusage.ru_majflt);
> +      printf_filtered ("Major faults, children: %ld\n",
> +		       kp.ki_rusage_ch.ru_majflt);
> +      printf_filtered ("utime: %jd.%06ld\n",
> +		       (intmax_t) kp.ki_rusage.ru_utime.tv_sec,
> +		       kp.ki_rusage.ru_utime.tv_usec);
> +      printf_filtered ("stime: %jd.%06ld\n",
> +		       (intmax_t) kp.ki_rusage.ru_stime.tv_sec,
> +		       kp.ki_rusage.ru_stime.tv_usec);
> +      printf_filtered ("utime, children: %jd.%06ld\n",
> +		       (intmax_t) kp.ki_rusage_ch.ru_utime.tv_sec,
> +		       kp.ki_rusage_ch.ru_utime.tv_usec);
> +      printf_filtered ("stime, children: %jd.%06ld\n",
> +		       (intmax_t) kp.ki_rusage_ch.ru_stime.tv_sec,
> +		       kp.ki_rusage_ch.ru_stime.tv_usec);
> +      printf_filtered ("'nice' value: %d\n", kp.ki_nice);
> +      printf_filtered ("start time: %jd.%06ld\n", kp.ki_start.tv_sec,
> +		       kp.ki_start.tv_usec);
> +      pgtok = getpagesize () / 1024;
> +      printf_filtered ("Virtual memory size: %ju kB\n",
> +		       (uintmax_t) kp.ki_size / 1024);
> +      printf_filtered ("Resident set size: %ju kB\n",
> +		       (uintmax_t) kp.ki_rssize * pgtok);
> +      printf_filtered ("rlim: %ju kB\n", (uintmax_t) kp.ki_rusage.ru_maxrss);
> +    }
> +}
> +
>  #ifdef KERN_PROC_AUXV
>  static enum target_xfer_status (*super_xfer_partial) (struct target_ops *ops,
>  						      enum target_object object,
> @@ -461,23 +828,6 @@ show_fbsd_lwp_debug (struct ui_file *file, int from_tty,
>  }
>  
>  #if defined(TDP_RFPPWAIT) || defined(HAVE_STRUCT_PTRACE_LWPINFO_PL_TDNAME)
> -/* Fetch the external variant of the kernel's internal process
> -   structure for the process PID into KP.  */
> -
> -static void
> -fbsd_fetch_kinfo_proc (pid_t pid, struct kinfo_proc *kp)
> -{
> -  size_t len;
> -  int mib[4];
> -
> -  len = sizeof *kp;
> -  mib[0] = CTL_KERN;
> -  mib[1] = KERN_PROC;
> -  mib[2] = KERN_PROC_PID;
> -  mib[3] = pid;
> -  if (sysctl (mib, 4, kp, &len, NULL, 0) == -1)
> -    perror_with_name (("sysctl"));
> -}
>  #endif

This leaves an empty #if/#endif.  Should it be removed or moved with the function?

>  
>  /*
> @@ -565,7 +915,8 @@ fbsd_thread_name (struct target_ops *self, struct thread_info *thr)
>    /* Note that ptrace_lwpinfo returns the process command in pl_tdname
>       if a name has not been set explicitly.  Return a NULL name in
>       that case.  */
> -  fbsd_fetch_kinfo_proc (pid, &kp);
> +  if (!fbsd_fetch_kinfo_proc (pid, &kp))
> +    perror_with_name (_("Failed to fetch process information"));
>    if (ptrace (PT_LWPINFO, lwp, (caddr_t) &pl, sizeof pl) == -1)
>      perror_with_name (("ptrace"));
>    if (strcmp (kp.ki_comm, pl.pl_tdname) == 0)
> @@ -975,9 +1326,13 @@ fbsd_wait (struct target_ops *ops,
>  #ifndef PTRACE_VFORK
>  	      /* For vfork, the child process will have the P_PPWAIT
>  		 flag set.  */
> -	      fbsd_fetch_kinfo_proc (child, &kp);
> -	      if (kp.ki_flag & P_PPWAIT)
> -		ourstatus->kind = TARGET_WAITKIND_VFORKED;
> +	      if (fbsd_fetch_kinfo_proc (child, &kp))
> +		{
> +		  if (kp.ki_flag & P_PPWAIT)
> +		    ourstatus->kind = TARGET_WAITKIND_VFORKED;
> +		}
> +	      else
> +		warning (_("Failed to fetch process information"));
>  #endif
>  	      ourstatus->value.related_pid = child_ptid;
>  
> @@ -1181,6 +1536,7 @@ fbsd_nat_add_target (struct target_ops *t)
>  {
>    t->to_pid_to_exec_file = fbsd_pid_to_exec_file;
>    t->to_find_memory_regions = fbsd_find_memory_regions;
> +  t->to_info_proc = fbsd_info_proc;
>  #ifdef KERN_PROC_AUXV
>    super_xfer_partial = t->to_xfer_partial;
>    t->to_xfer_partial = fbsd_xfer_partial;
> 

Thanks,

Simon
  

On Tuesday, December 26, 2017 09:22:52 PM Simon Marchi wrote:
> On 2017-12-22 05:05 PM, John Baldwin wrote:
> > +  else
> > +    {
> > +      pid = strtol (built_argv[0], NULL, 10);
> > +    }
> 
> Unnecessary curly braces.

Fixed.

> > +#ifdef HAVE_KINFO_GETVMMAP
> > +  if (do_mappings)
> > +    {
> > +      int nvment;
> > +      std::unique_ptr<struct kinfo_vmentry, free_deleter<struct kinfo_vmentry>>
> 
> Is there a reason to have and use free_deleter rather than gdb::unique_xmalloc_ptr?
> 
> > +	vmentl (kinfo_getvmmap (pid, &nvment));

This function (kinfo_getvmmap) which is defined in the libutil library included in
FreeBSD's base system calls malloc() internally, so the memory returned must be
freed with free() rather than xfree().  This deleter is already used earlier in
fbsd_find_memory_regions() for another call to kinfo_getvmmap() for the same reason.

> > +
> > +      if (vmentl)
> 
> vmentl != NULL
> 
> There are a few other instances of if (ptr) that should be changed to if (ptr != NULL).

Ok.  The style in GDB doesn't seem consistent in this regard (I largely based this
code on the 'info proc' implementation in linux-tdep.c which doesn't explicitly
compare against NULL/nullptr (though I prefer the explicit comparison myself)).
Also, I feel like we should use nullptr rather than NULL when working with "smart"
pointer types like unique_ptr<> at least?

> > @@ -461,23 +828,6 @@ show_fbsd_lwp_debug (struct ui_file *file, int from_tty,
> >  }
> >  
> >  #if defined(TDP_RFPPWAIT) || defined(HAVE_STRUCT_PTRACE_LWPINFO_PL_TDNAME)
> > -/* Fetch the external variant of the kernel's internal process
> > -   structure for the process PID into KP.  */
> > -
> > -static void
> > -fbsd_fetch_kinfo_proc (pid_t pid, struct kinfo_proc *kp)
> > -{
> > -  size_t len;
> > -  int mib[4];
> > -
> > -  len = sizeof *kp;
> > -  mib[0] = CTL_KERN;
> > -  mib[1] = KERN_PROC;
> > -  mib[2] = KERN_PROC_PID;
> > -  mib[3] = pid;
> > -  if (sysctl (mib, 4, kp, &len, NULL, 0) == -1)
> > -    perror_with_name (("sysctl"));
> > -}
> >  #endif
> 
> This leaves an empty #if/#endif.  Should it be removed or moved with the function?

Removed, good catch.
  

Hi John,

>> > +#ifdef HAVE_KINFO_GETVMMAP
>> > +  if (do_mappings)
>> > +    {
>> > +      int nvment;
>> > +      std::unique_ptr<struct kinfo_vmentry, free_deleter<struct kinfo_vmentry>>
>> 
>> Is there a reason to have and use free_deleter rather than 
>> gdb::unique_xmalloc_ptr?
>> 
>> > +	vmentl (kinfo_getvmmap (pid, &nvment));
> 
> This function (kinfo_getvmmap) which is defined in the libutil library
> included in
> FreeBSD's base system calls malloc() internally, so the memory returned 
> must be
> freed with free() rather than xfree().  This deleter is already used 
> earlier in
> fbsd_find_memory_regions() for another call to kinfo_getvmmap() for
> the same reason.

But isn't xfree just a wrapper around free?

>> > +
>> > +      if (vmentl)
>> 
>> vmentl != NULL
>> 
>> There are a few other instances of if (ptr) that should be changed to 
>> if (ptr != NULL).
> 
> Ok.  The style in GDB doesn't seem consistent in this regard (I
> largely based this
> code on the 'info proc' implementation in linux-tdep.c which doesn't 
> explicitly
> compare against NULL/nullptr (though I prefer the explicit comparison 
> myself)).
> Also, I feel like we should use nullptr rather than NULL when working
> with "smart"
> pointer types like unique_ptr<> at least?

You are right, there is code that comes from an era where the coding 
style wasn't as strictly enforced, it seems.  I don't think we should go 
fix coding style issues just for fun, but when we modify or copy 
existing code, we should make it respect the style.

Thanks,

Simon
  

On Wednesday, January 03, 2018 02:13:28 PM Simon Marchi wrote:
> Hi John,
> 
> >> > +#ifdef HAVE_KINFO_GETVMMAP
> >> > +  if (do_mappings)
> >> > +    {
> >> > +      int nvment;
> >> > +      std::unique_ptr<struct kinfo_vmentry, free_deleter<struct kinfo_vmentry>>
> >> 
> >> Is there a reason to have and use free_deleter rather than 
> >> gdb::unique_xmalloc_ptr?
> >> 
> >> > +	vmentl (kinfo_getvmmap (pid, &nvment));
> > 
> > This function (kinfo_getvmmap) which is defined in the libutil library
> > included in
> > FreeBSD's base system calls malloc() internally, so the memory returned 
> > must be
> > freed with free() rather than xfree().  This deleter is already used 
> > earlier in
> > fbsd_find_memory_regions() for another call to kinfo_getvmmap() for
> > the same reason.
> 
> But isn't xfree just a wrapper around free?

Ah, for some reason I thought that xmalloc/xfree were a matched pair that could in
some cases refer to a separate malloc implementation, not always a wrapper around
normal malloc() / free().  I'll remove the deleter altogether (and it's one
existing use) in a future change.