From: Simon Marchi <simon.marchi@ericsson.com>
To: John Baldwin <jhb@freebsd.org>, Simon Marchi <simark@simark.ca>
Cc: <gdb-patches@sourceware.org>
Subject: Re: [PATCH v2 1/5] Support 'info proc' for FreeBSD process core dumps.
Date: Tue, 09 Jan 2018 19:29:00 -0000 [thread overview]
Message-ID: <3f3d8fd5-3106-3c1b-5a60-80ed82d65a24@ericsson.com> (raw)
In-Reply-To: <1752022.FNtOcQG6Rg@ralph.baldwin.cx>
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
next prev parent reply other threads:[~2018-01-09 19:29 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-01-04 1:50 [PATCH v2 0/5] Support for 'info proc' on FreeBSD cores and native John Baldwin
2018-01-04 1:50 ` [PATCH v2 2/5] Don't return stale data from fbsd_pid_to_exec_file for kernel processes John Baldwin
2018-01-05 2:57 ` Simon Marchi
2018-01-05 19:43 ` John Baldwin
2018-01-04 1:50 ` [PATCH v2 3/5] Use gdb::unique_xmalloc_ptr<> instead of a deleter that invokes free() John Baldwin
2018-01-05 2:58 ` Simon Marchi
2018-01-04 1:50 ` [PATCH v2 1/5] Support 'info proc' for FreeBSD process core dumps John Baldwin
2018-01-05 2:54 ` Simon Marchi
2018-01-05 19:43 ` John Baldwin
2018-01-09 19:29 ` Simon Marchi [this message]
2018-01-04 1:59 ` [PATCH v2 4/5] Support 'info proc' for native FreeBSD processes John Baldwin
2018-01-05 3:20 ` Simon Marchi
2018-01-05 19:43 ` John Baldwin
2018-01-04 1:59 ` [PATCH v2 5/5] Document support for 'info proc' on FreeBSD John Baldwin
2018-01-04 16:38 ` Eli Zaretskii
2018-01-04 21:36 ` John Baldwin
2018-01-05 6:53 ` Eli Zaretskii
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3f3d8fd5-3106-3c1b-5a60-80ed82d65a24@ericsson.com \
--to=simon.marchi@ericsson.com \
--cc=gdb-patches@sourceware.org \
--cc=jhb@freebsd.org \
--cc=simark@simark.ca \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).