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

[John Baldwin <jhb at freebsd dot org>]

On Thursday, January 04, 2018 10:20:05 PM Simon Marchi wrote:
> On 2018-01-03 08:49 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.
> > - 'status' output is generated by outputting fields from the structure
> >   returned by the kern.proc.pid.<pid> sysctl.
> > - 'stat' is aliased to 'status'.
> 
> Hi John,
> 
> The patch LGTM in general, I noted 2 comments, the first one could be done
> as a separate patch (after this series), if you agree with it.  The second,
> I'm fine if you just fix it before pushing.
> 
> > +	  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
> > +	    }
> 
> It might be a good idea to factor out the printing of vm map entries from here
> and the core code, to make sure that they will always be printed the same way.
> It's up to you.

So I thought about that (and that's why the fbsd_vm_map_entry_flags is shared
in fbsd-tdep.c).  I would perhaps need some way to tell the common code which
layout to use as it's based on gdbarch_addr_bit() in the tdep code but uses
an #ifdef in this version.  I also considered doing this for the 'status'
implementation, but I would probably need to export the 'kinfo_proc_layout'
structures from the tdep file and use an #ifdef in the native target to choose
which layout to use.  The 'status' is also a bit trickier as some fields are
printed for native but not for cores, and the native version uses getpagesize()
to convert page counts to Kb that the core version can't do.  Merging the
'mappings' probably is more doable though than 'status' as it really just
needs 'addr_bit' passed in.  (I believe I can't quite pass in 'target_gdbarch()'
since I might be attached to a 32-bit process but be running 'info proc mappings'
with the PID of a 64-bit processes for which target_gdbarch() would be wrong.)

> > +	}
> > +      else
> > +	warning (_("unable to fetch virtual memory map"));
> > +    }
> > +#endif
> > +  if (do_status)
> > +    {
> > +      if (!fbsd_fetch_kinfo_proc(pid, &kp))
> 
> Missing space here.  But didn't we fetch it already (line 329)?
> IIUC, we only need it in this scope, so you could move the
> relevant variables here, and only call fbsd_fetch_kinfo_proc here.
> I suppose it needed to be this way when stat and status were not
> merged.

Oh, whoops, I missed finishing that cleanup when collapsing down to a single
'status'. :-/  Yes, the intention was to remove 'kp_valid' entirely.

-- 
John Baldwin