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

Message ID ZSfA0oISlP0CEsBaJ2ceAUxBvQcq7LXW7SgPhz97Q4bU03Dacg-cDMiyXvgtvQj8F97BHdw4bufNjnemgNP2BnQrdc5XsKTSeRBVAvE7cTE=@emersion.fr
State New, archived
Headers

Commit Message

Simon Ser Aug. 21, 2018, 2:45 p.m. UTC
  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.

 gdb/fbsd-nat.c  | 50 ++++++++++++++++++++++++++++++++++++++++++
 gdb/fbsd-tdep.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/target.h    |  4 ++++
 3 files changed, 112 insertions(+)
  

Comments

John Baldwin Aug. 23, 2018, 10:40 a.m. UTC | #1
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, ...  */
>  };
>  
>
  

Patch

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;
+        }
+
+	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;
+	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)
+	  {
+	    buflen += sizeof (struct_size);
+	    if (buflen > offset)
+	      {
+		buflen -= offset;
+		memcpy (readbuf, buf + offset, buflen);
+	      }
+	    else
+	      buflen = 0;
+	    *xfered_len = buflen;
+	    return (buflen == 0) ? TARGET_XFER_EOF : TARGET_XFER_OK;
+	  }
+	return TARGET_XFER_E_IO;
+      }
     default:
       return inf_ptrace_target::xfer_partial (object, annex,
 					      readbuf, writebuf, offset,
diff --git a/gdb/fbsd-tdep.c b/gdb/fbsd-tdep.c
index 9cea0098..659e64b6 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,23 @@  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 {};
+
+  if (structsize == 0)
+    return buf;
+
+  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 +606,44 @@  fbsd_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, int *note_size)
 
   note_data = thread_args.note_data;
 
+  pid_t pid = inferior_ptid.pid ();
+
+  /* Auxiliary vector.  */
+  uint32_t structsize = gdbarch_ptr_bit (gdbarch) / 4; /* Elf_Auxinfo  */
+  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  */
+  note_desc = fbsd_make_note_desc (TARGET_OBJECT_FREEBSD_VMMAP, 0);
+  if (note_desc && !note_desc->empty ())
+    {
+      note_data = elfcore_write_note (obfd, note_data, note_size,
+                                      "FreeBSD", NT_FREEBSD_PROCSTAT_VMMAP,
+                                      note_desc->data (), note_desc->size ());
+      if (!note_data)
+        return NULL;
+    }
+
+  note_desc =
+    fbsd_make_note_desc (TARGET_OBJECT_FREEBSD_PS_STRINGS, 0);
+  if (note_desc && !note_desc->empty ())
+    {
+      note_data = elfcore_write_note (obfd, note_data, note_size,
+                                      "FreeBSD",
+                                      NT_FREEBSD_PROCSTAT_PSSTRINGS,
+                                      note_desc->data (), note_desc->size ());
+      if (!note_data)
+        return NULL;
+    }
+
   return note_data;
 }
 
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,
+  /* FreeBSD process strings  */
+  TARGET_OBJECT_FREEBSD_PS_STRINGS,
   /* Possible future objects: TARGET_OBJECT_FILE, ...  */
 };