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

Simon Marchi
2018-09-08T22:54:48Z

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