Re: [PATCH 3/5] Add support for 'info proc files' on FreeBSD core dumps.

[John Baldwin <jhb at FreeBSD dot org>]

On 9/8/18 3:54 PM, Simon Marchi wrote:
> On 2018-09-08 01:36 AM, John Baldwin wrote:
>> Walk the list of struct kinfo_file objects in the
>> NT_FREEBSD_PROCSTAT_FILES core dump note outputting a description of
>> each open file descriptor.  For sockets, the local and remote socket
>> addresses are displayed in place of the file name field.  For UNIX
>> local domain sockets, only a single address is displayed since most
>> UNIX sockets only have one valid address and printing both pathnames
>> could be quite long.  The output format was somewhat inspired by the
>> output of the "procstat -f" command on FreeBSD, but with a few less
>> details and some fields were condensed.
> 
> Just some nits, LGTM otherwise.
>> diff --git a/gdb/fbsd-tdep.c b/gdb/fbsd-tdep.c
>> index 9e6d7276c4..a8b5b2f146 100644
>> --- a/gdb/fbsd-tdep.c
>> +++ b/gdb/fbsd-tdep.c
>> @@ -90,18 +90,115 @@
>>  #define	KF_STRUCTSIZE		0x0
>>  #define	KF_TYPE			0x4
>>  #define	KF_FD			0x8
>> +#define	KF_FLAGS		0x10
>> +#define	KF_OFFSET		0x18
> 
> In sys/user.h, it says:
> 
>  /* XXX Hidden alignment padding here on amd64 */
> 
> Does that mean the field offsets can be different for other arches?

That comment is in the structure 'struct kinfo_ofile' which is for
an older format of this structure used in FreeBSD 7.0 kernels.  7.1
and later kernels use the 'struct kinfo_file' structure which has a
fixed layout (and includes an explicit kf_pad0 padding field).  FreeBSD
didn't add the NT_PROCSTAT_FILES note to process core dumps until FreeBSD
10.0 (also merged to 9.2), so process cores have only ever included the
newer format with a fixed layout.

If it is helpful I could add a comment.  I think I redid the same
archaeology when adding the initial 'info proc' core dump bits for
FreeBSD. :)

>> +/* Constats for socket types.  These match SOCK_* constants in
> 
> "Constats"

Oops, fixed.

>> +/* Helper function to print out an IPv6 socket address.  The address
>> +   is formatted similar to inet_ntop.  */
>> +
>> +static void
>> +fbsd_print_sockaddr_in6 (void *sockaddr)
>> +{
>> +  ...
>> +}
> 
> Hmm, I suppose we'll want to re-use this ipv6 address printing code for the Linux...
> We can take care of moving it to a common file then.

So I did hate to duplicate a lot of what is normally common code and even
POSIX (inet_ntoa() and inet_ntop()).  We currently don't use those routines
anywhere else in GDB, and I wasn't sure if we could depend on their always
existing.  gnulib doesn't seem to have existing fallbacks for those.  If
we could somehow require them or provide standard fallbacks then I'd be
happier using those instead.

>> +/* Implement "info proc files" for a corefile.  */
>> +
>> +static void
>> +fbsd_core_info_proc_files (struct gdbarch *gdbarch)
>> +{
>> +  asection *section;
>> +  unsigned char *descdata, *descend;
>> +  size_t note_size;
> 
> Don't hesitate to declare them variables at the point you use the the first time :).

Old C habits die hard (even with C99).  I've fixed for V2 though. :)

>> +  while (descdata + KF_PATH < descend)
>> +    {
>> +      LONGEST fd, flags, offset, type, vnode_type;
>> +      ULONGEST structsize;
>> +
>> +      structsize = bfd_get_32 (core_bfd, descdata + KF_STRUCTSIZE);
>> +      if (structsize < KF_PATH)
>> +	error (_("malformed core note - vmmap entry too small"));
> 
> Copy pasta?  Actually, there seems to be the same mistake in fbsd_core_vnode_path.

Yes, and I should fix the original mistake in the first patch of the series.

-- 
John Baldwin