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

[John Baldwin <jhb@FreeBSD.org>]

On 7/16/18 6:38 AM, 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-07-16  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
> ---
> 
> This patch uses a different approach than the previous one: it adds two
> FreeBSD-specific target objects. I chose this approach because there
> were already some platform-specific target objects (e.g. for Darwin).
> This should fix the issue John pointed out.
> 
>  gdb/fbsd-nat.c  | 17 ++++++++++++++-
>  gdb/fbsd-tdep.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++
>  gdb/target.h    |  4 ++++
>  3 files changed, 77 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
> index 115deac0..2d056676 100644
> --- a/gdb/fbsd-nat.c
> +++ b/gdb/fbsd-nat.c
> @@ -711,17 +711,32 @@ fbsd_nat_target::xfer_partial (enum target_object object,
>        }
>  #endif
>      case TARGET_OBJECT_AUXV:
> +    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;
> +        switch (object) {
> +        case TARGET_OBJECT_AUXV:
> +          proc_target = KERN_PROC_AUXV;
> +          break;
> +        case TARGET_OBJECT_FREEBSD_VMMAP:
> +          proc_target = KERN_PROC_VMMAP;
> +          break;
> +        case TARGET_OBJECT_FREEBSD_PS_STRINGS:
> +          proc_target = KERN_PROC_PS_STRINGS;
> +          break;
> +        }
> +
>  	if (writebuf != NULL)
>  	  return TARGET_XFER_E_IO;
>  	mib[0] = CTL_KERN;
>  	mib[1] = KERN_PROC;
> -	mib[2] = KERN_PROC_AUXV;
> +	mib[2] = proc_target;
>  	mib[3] = pid;
>  	if (offset == 0)
>  	  {

This won't work correctly for VMMAP.  The sysctl returns a "packed" representation
whereas the core dump note uses an unpacked format.  src/lib/libutil/kinfo_getvmmap.c
in FreeBSD's source tree has an example of the unpacking.  This means that you will
have to use a separate case for VMMAP I think (and would be nice to use for FILE in
the future which has the same packing "feature")) where you read the sysctl into
a buffer and then unpack it into the destination.  You could size the temporary
buffer as 'offset + len' as the unpacking can only grow the buffer.

> diff --git a/gdb/fbsd-tdep.c b/gdb/fbsd-tdep.c
> index 9cea0098..2ea76e66 100644
> --- a/gdb/fbsd-tdep.c
> +++ b/gdb/fbsd-tdep.c
> @@ -25,6 +25,9 @@
>  #include "regset.h"
>  #include "gdbthread.h"
>  #include "xml-syscall.h"
> +#include <sys/user.h>
> +#include <sys/sysctl.h>
> +#include <sys/types.h>
>  
>  #include "elf-bfd.h"
>  #include "fbsd-tdep.h"
> @@ -512,6 +515,20 @@ fbsd_corefile_thread (struct thread_info *info,
>       args->note_size, args->stop_signal);
>  }
>  
> +static gdb::optional<gdb::byte_vector>
> +fbsd_make_note_desc (enum target_object object, uint32_t structsize)
> +{
> +  gdb::optional<gdb::byte_vector> buf =
> +    target_read_alloc (current_top_target (), object, NULL);
> +  if (!buf || buf->empty ())
> +    return {};
> +
> +  gdb::byte_vector desc (sizeof (structsize) + buf->size ());
> +  memcpy (desc.data (), &structsize, sizeof (structsize));
> +  memcpy (desc.data () + sizeof (structsize), buf->data (), buf->size ());
> +  return desc;
> +}
> +
>  /* Create appropriate note sections for a corefile, returning them in
>     allocated memory.  */
>  
> @@ -586,6 +603,46 @@ fbsd_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, int *note_size)
>  
>    note_data = thread_args.note_data;
>  
> +  pid_t pid = inferior_ptid.pid ();
> +
> +  /* Auxillary vector.  */

Typo here: Auxiliary.

> +  uint32_t structsize = gdbarch_addr_bit (gdbarch) / 4; /* Elf_Auxinfo  */

I think this should probably use gdbarch_pointer_bit rather than
gdbarch_addr_bit.

> +  gdb::optional<gdb::byte_vector> note_desc =
> +    fbsd_make_note_desc (TARGET_OBJECT_AUXV, structsize);
> +  if (note_desc && !note_desc->empty ())
> +    {
> +      note_data = elfcore_write_note (obfd, note_data, note_size,
> +                                      "FreeBSD", NT_FREEBSD_PROCSTAT_AUXV,
> +                                      note_desc->data (), note_desc->size ());
> +      if (!note_data)
> +        return NULL;
> +    }
> +  
> +  /* File mappings  */
> +  structsize = 0x488; /* struct kinfo_vmentry  */

A bare constant feels a bit gross.  I actually think it would be cleanest if
the TARGET_OBJECT_FREEBSD_* streams included the struct size in the data
stream just as the core dump notes do.  This provides flexibility in case
newer kernels extend the structure size in the future.

-- 
John Baldwin