Re: [PATCH v3] Generate NT_PROCSTAT_{AUXV,VMMAP,PS_STRINGS} in FreeBSD coredumps

[John Baldwin <jhb@FreeBSD.org>]

On 8/21/18 3:45 PM, Simon Ser wrote:
> gcore generates NT_AUXV and NT_FILE notes for Linux targets. On
> FreeBSD auxv is stored in a NT_PROCSTAT_AUXV section, file mappings
> are stored in a NT_PROCSTAT_VMMAP and both are prefixed with the
> struct size.
> 
> 2018-08-21  Simon Ser  <contact@emersion.fr>
>         * target.h (enum target_object): add FreeBSD-specific
>         TARGET_OBJECT_FREEBSD_VMMAP and TARGET_OBJECT_FREEBSD_PS_STRINGS
>         * fbsd-nat.c (fbsd_nat_target::xfer_partial): add support for
>         TARGET_OBJECT_FREEBSD_VMMAP and TARGET_OBJECT_FREEBSD_PS_STRINGS
>         * fbsd-tdep.c (fbsd_make_corefile_notes): write NT_PROCSTAT_AUXV,
>         NT_PROCSTAT_VMMAP and NT_PROCSTAT_PS_STRINGS notes
> ---
> 
> Changes from v2 to v3:
> - Remove constants, prepend struct size for FreeBSD-specific objects
> - Use gdbarch_ptr_bit instead of gdbarch_addr_bit
> - Fixed typo
> 
> This addresses all of John's comments except the VMMAP packed one.
> 
> John, you said coredumps use the unpacked format, but looking at the
> `gcore` utility source code (in usr.bin/gcore/elfcore.c) I believe it
> uses the packed format for coredumps. It also makes more sense to me
> to use the packed format for coredumps because it allows us not to
> store PATH_MAX zeros for each entry. I may be wrong though, let me know
> what you think.

Ah, yes.  We used to pad them, but now we pack them by default (the kernel
has knobs to control packing in core dumps that default to packing).

> 
>  gdb/fbsd-nat.c  | 50 ++++++++++++++++++++++++++++++++++++++++++
>  gdb/fbsd-tdep.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++
>  gdb/target.h    |  4 ++++
>  3 files changed, 112 insertions(+)
> 
> diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
> index 115deac0..7cd325e4 100644
> --- a/gdb/fbsd-nat.c
> +++ b/gdb/fbsd-nat.c
> @@ -751,6 +751,56 @@ fbsd_nat_target::xfer_partial (enum target_object object,
>  	  }
>  	return TARGET_XFER_E_IO;
>        }
> +    case TARGET_OBJECT_FREEBSD_VMMAP:
> +    case TARGET_OBJECT_FREEBSD_PS_STRINGS:
> +      {
> +	gdb::byte_vector buf_storage;
> +	gdb_byte *buf;
> +	size_t buflen;
> +	int mib[4];
> +
> +        int proc_target;
> +        uint32_t struct_size;
> +        switch (object) {
> +        case TARGET_OBJECT_FREEBSD_VMMAP:
> +          proc_target = KERN_PROC_VMMAP;
> +          struct_size = sizeof (struct kinfo_vmentry);
> +          break;
> +        case TARGET_OBJECT_FREEBSD_PS_STRINGS:
> +          proc_target = KERN_PROC_PS_STRINGS;
> +          struct_size = sizeof (void *);
> +          break;
> +        }

The indentation of this block seems off, perhaps it's not using
tabs but only spaces?

> +
> +	if (writebuf != NULL)
> +	  return TARGET_XFER_E_IO;
> +
> +	mib[0] = CTL_KERN;
> +	mib[1] = KERN_PROC;
> +	mib[2] = proc_target;
> +	mib[3] = pid;
> +
> +	buflen = sizeof (struct_size) + offset + len;

Presumably this should just be 'offset + len' as 'struct_size' is
part of the logical stream described by 'offset + len', but see
below where I think we need to rework buf_storage and buf.

> +	buf_storage.resize (buflen);
> +	buf = buf_storage.data ();
> +
> +	memcpy (buf, &struct_size, sizeof (struct_size));
> +
> +	if (sysctl (mib, 4, buf + sizeof (struct_size), &buflen, NULL, 0) == 0)
> +	  {

Hmm, if the caller asks to read a subset of the "stream", then buflen will be
too short and this will fail with ENOMEM or the like.  (It seems I failed to
handle this properly in the auxv case as well *sigh*.)  Probably you need to
follow the pattern that gcore uses in procstat_sysctl() in elfcore.c where you
first request the length via a NULL buffer, then size buf_storage to buflen +
sizeof struct size, then do the sysctl to fetch the entire buffer and finally
do the memcpy out of buf into readbuf.

> diff --git a/gdb/target.h b/gdb/target.h
> index 18c4a84c..83f1172c 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -203,6 +203,10 @@ enum target_object
>       of the process ID of the process in question, in hexadecimal
>       format.  */
>    TARGET_OBJECT_EXEC_FILE,
> +  /* FreeBSD file mappings  */
> +  TARGET_OBJECT_FREEBSD_VMMAP,

Perhaps 'virtual memory mappings'

> +  /* FreeBSD process strings  */
> +  TARGET_OBJECT_FREEBSD_PS_STRINGS,
>    /* Possible future objects: TARGET_OBJECT_FILE, ...  */
>  };
>  
> 


-- 
John Baldwin