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

[John Baldwin <jhb@freebsd.org>]

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.

-- 
John Baldwin