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

[Simon Marchi <simon dot marchi at ericsson dot com>]

On 2018-01-05 02:21 PM, John Baldwin wrote:
> On Thursday, January 04, 2018 09:53:47 PM Simon Marchi wrote:
>> 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;
>>                              ^~~~~~~
> 
> Oops, too much copy and paste (and I've only been building with clang so far).
> 
>>> +  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?
> 
> Correct.  This is the bounds check to avoid overflowing the note buffer,
> but it does warrany a comment to explain what it is doing.  And actually,
> writing that comment made me realize a bug.  It should be 'addr_bit /
> TARGET_CHAR_BIT', not addr_bit directly.  Also, I noticed that the
> 'long_bit' variable at the start of the quoted section above also isn't
> used, but it's probably cleaner to be explicit and assign it to
> gdbarch_long_bit() and use it in place of addr_bit for structure members
> that are of type long rather than assuming addr_bit == long_bit (even
> though that is true on all of the architectures FreeBSD supports or is
> likely to support).
> 
> Ends up like this:

That LGTM, the comment you added is clear.

Simon