Re: [PATCH v2 1/5] Support 'info proc' for FreeBSD process core dumps.

[Simon Marchi <simark@simark.ca>]

Hi John,

I have two little comments, but otherwise it LGTM.

On 2018-01-03 08:49 PM, John Baldwin wrote:
> +/* Implement "info proc status" for a corefile.  */
> +
> +static void
> +fbsd_core_info_proc_status (struct gdbarch *gdbarch)
> +{
> +  const struct kinfo_proc_layout *kp;
> +  asection *section;
> +  const char *state;
> +  unsigned char *descdata, *descend;

I get this:

/home/simark/src/binutils-gdb/gdb/fbsd-tdep.c: In function ‘void fbsd_core_info_proc_status(gdbarch*)’:
/home/simark/src/binutils-gdb/gdb/fbsd-tdep.c:791:29: error: variable ‘descend’ set but not used [-Werror=unused-but-set-variable]
   unsigned char *descdata, *descend;
                             ^~~~~~~

> +  int addr_bit, long_bit;
> +  size_t note_size;
> +  ULONGEST value;
> +  LONGEST sec;
> +
> +  section = bfd_get_section_by_name (core_bfd, ".note.freebsdcore.proc");
> +  if (section == NULL)
> +    {
> +      warning (_("unable to find process info in core file"));
> +      return;
> +    }
> +
> +  addr_bit = gdbarch_addr_bit (gdbarch);
> +  if (addr_bit == 64)
> +    kp = &kinfo_proc_layout_64;
> +  else if (bfd_get_arch (core_bfd) == bfd_arch_i386)
> +    kp = &kinfo_proc_layout_i386;
> +  else
> +    kp = &kinfo_proc_layout_32;
> +
> +  note_size = bfd_get_section_size (section);
> +  if (note_size < 4 + kp->ki_rusage_ch + kp->ru_majflt + addr_bit)

What's the rationale behind that computation?  The field ru_majflt in kp->ki_rusage_ch
is the furthest field we'll actually need in this function?  And addr_bit is the size of
that field?  And 4 is because there's a field at the beginning containing the size of
the structure that comes after?

Maybe a small comment would be good to remove the magic.

Thanks,

Simon