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

[Simon Marchi <simon.marchi@ericsson.com>]

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?

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

"Constats"

> +/* 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)
> +{
> +  struct fbsd_sockaddr_in6 *sin6 =
> +    reinterpret_cast<struct fbsd_sockaddr_in6 *>(sockaddr);
> +  uint16_t words[ARRAY_SIZE(sin6->sin6_addr) / 2];
> +
> +  /* Populate the array of 16-bit words from network-order bytes.  */
> +  for (int i = 0; i < ARRAY_SIZE(words); i++)
> +    words[i] = (sin6->sin6_addr[i * 2] << 8) | sin6->sin6_addr[i * 2 + 1];
> +
> +  /* Find the longest run of zero words.  */
> +  int best, bestlen, current, len;
> +
> +  best = -1;
> +  bestlen = 0;
> +  current = -1;
> +  len = 0;
> +  for (int i = 0; i < ARRAY_SIZE(words); i++)
> +    {
> +      if (words[i] == 0)
> +	{
> +	  if (current >= 0)
> +	    len++;
> +	  else
> +	    {
> +	      current = i;
> +	      len = 1;
> +	    }
> +	}
> +      else
> +	{
> +	  if (current >= 0 && len > bestlen)
> +	    {
> +	      best = current;
> +	      bestlen = len;
> +	    }
> +	  current = -1;
> +	  len = 0;
> +	}
> +    }
> +  if (current >= 0 && len > bestlen)
> +    {
> +      best = current;
> +      bestlen = len;
> +    }
> +  if (bestlen < 2)
> +    best = -1;
> +
> +  for (int i = 0; i < ARRAY_SIZE(words); i++)
> +    {
> +      if (best >= 0 && i >= best && i < best + bestlen)
> +	{
> +	  if (i == best || i == ARRAY_SIZE(words) - 1)
> +	    printf_filtered (":");
> +	}
> +      else
> +	{
> +	  if (i != 0)
> +	    printf_filtered (":");
> +	  printf_filtered ("%x", words[i]);
> +	}
> +    }
> +  printf_filtered (".%u", (sin6->sin6_port[0] << 8) | sin6->sin6_port[1]);
> +}

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.

> +/* 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 :).
> +
> +  section = bfd_get_section_by_name (core_bfd, ".note.freebsdcore.files");
> +  if (section == NULL)
> +    {
> +      warning (_("unable to find open files in core file"));
> +      return;
> +    }
> +
> +  note_size = bfd_get_section_size (section);
> +  if (note_size < 4)
> +    error (_("malformed core note - too short for header"));
> +
> +  gdb::def_vector<unsigned char> contents (note_size);
> +  if (!bfd_get_section_contents (core_bfd, section, contents.data (),
> +				 0, note_size))
> +    error (_("could not get core note contents"));
> +
> +  descdata = contents.data ();
> +  descend = descdata + note_size;
> +
> +  /* Skip over the structure size.  */
> +  descdata += 4;
> +
> +  printf_filtered (_("Open files:\n\n"));
> +  printf_filtered ("  %6s %6s %10s %9s %s\n",
> +		   "FD", "Type", "Offset", "Flags  ", "Name");
> +
> +  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.

Simon