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

[Pedro Alves <palves@redhat.com>]

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