This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH v2 3/6] Display per-thread information for threads in FreeBSD cores.
- From: Pedro Alves <palves at redhat dot com>
- To: John Baldwin <jhb at freebsd dot org>
- Cc: gdb-patches at sourceware dot org, binutils at sourceware dot org
- Date: Mon, 18 Jan 2016 12:27:21 +0000
- Subject: Re: [PATCH v2 3/6] Display per-thread information for threads in FreeBSD cores.
- Authentication-results: sourceware.org; auth=none
- References: <1452721551-657-1-git-send-email-jhb at FreeBSD dot org> <1452721551-657-4-git-send-email-jhb at FreeBSD dot org> <5697B8D6 dot 9010301 at redhat dot com> <2320320 dot x23qbZCVbY at ralph dot baldwin dot cx>
On 01/15/2016 08:13 PM, John Baldwin wrote:
> On Thursday, January 14, 2016 03:03:50 PM Pedro Alves wrote:
>> On 01/13/2016 09:45 PM, John Baldwin wrote:
>>>
>>>
>>> +/* This is how we want PTIDs from core files to be printed. */
>>> +
>>> +static char *
>>> +fbsd_core_pid_to_str (struct gdbarch *gdbarch, ptid_t ptid)
>>> +{
>>> + static char buf[80], name[64];
>>
>> This static "name" buffer appears unused, and then masked
>> by the "name" pointer below.
>
> Yes, it was from an earlier rev of the patch before I switched to
> alloca(). Removed.
>>
>>> + struct bfd_section *section;
>>> + bfd_size_type size;
>>> + char sectionstr[32];
>>> +
>>> + if (ptid_get_lwp (ptid) != 0)
>>> + {
>>> + snprintf (sectionstr, sizeof sectionstr, ".thrmisc/%ld",
>>> + ptid_get_lwp (ptid));
>>
>> xsnprintf.
>
> Fixed here and throughout.
>
>>> + section = bfd_get_section_by_name (core_bfd, sectionstr);
>>> + if (section != NULL)
>>> + {
>>> + char *name;
>>> +
>>> + size = bfd_section_size (core_bfd, section);
>>> + name = alloca (size + 1);
>>> + if (bfd_get_section_contents (core_bfd, section, name, (file_ptr) 0,
>>> + size) && name[0] != '\0')
>>
>> This indentation / line break reads unusual to me. I think breaking
>> before the && would be clearer:
>>
>> if (bfd_get_section_contents (core_bfd, section, name,
>> (file_ptr) 0, size)
>> && name[0] != '\0')
>>
>> Guess this should check size > 0 as well, otherwise name[0] contains
>> garbage.
>
> Ok. I decided to check the size in the earlier if statement to avoid
> the alloca(), etc. when the size is zero.
>
>>> + {
>>> + name[size] = '\0';
>>> + if (strcmp(name, elf_tdata (core_bfd)->core->program) != 0)
>>
>> Missing space after strcmp.
>
> Fixed.
>
>> Is this ".thrmisc" section documented somewhere? Could you add a
>> small comment on what this entry you're skipping means?
>
> It is not documented aside from the code unfortunately. :( If I had my
> way we would be dumping the full ptrace_lwpinfo structure that contains
> per-LWP info used by the 'nat' target instead (it has other potentially
> useful information such as the siginfo_t for each thread). At the moment
> I don't generate a NT_THRMISC note in 'gcore' because 'struct thrmisc' is
> currently only present in FreeBSD's headers.
I think just having a comment mentioning that this note contains
the "struct thrmisc" structure, which contains the thread name, would
be helpful.
But, if this is about the thread name, is there a reason you're hacking it
through fbsd_core_pid_to_str, rather than hooking this up
through "thread name" ?
>
> A somewhat related question: would it be reasonable to dump additional
> notes in 'gcore' only when native?
That's OK, as long as you go through the target vector, through some target
method or by reading some target object.
If there was a FreeBSD gdbserver port I'd push you into defining whatever
packets are missing to make it work in gdbserver too, though...
> For example, FreeBSD kernels dump
> several additional notes including things like auxv, open files, etc.
See the TARGET_OBJECT_AUXV dumping in linux-tdep.c. When native, that goes
to linux-nat.c. When cross/remote, it goes through remote.c/qXfer:auxv:read.
> The "gross" way I can think of for doing that is to simply place that
> additional logic under #ifdef __FreeBSD__ in fbsd-tdep.c, but that seems
> quite hackish given the otherwise clean split between tdep.c and nat.c.
Yeah, I'd rather not. Best go through the target vector instead.
Thanks,
Pedro Alves