Re: [PATCH v2 3/6] Display per-thread information for threads in FreeBSD cores.

Pedro Alves
2016-01-18T12:27:25Z

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