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

  Display the LWP ID of each thread in a FreeBSD core.  Extract thread names
from the per-thread THRMISC note.

gdb/ChangeLog:

	* fbsd_tdep.c (fbsd_core_pid_to_str): New function.
	(fbsd_init_abi): Add "core_pid_to_str" gdbarch method.
---
 gdb/ChangeLog   |  5 +++++
 gdb/fbsd-tdep.c | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)
  

On 01/13/2016 09:45 PM, John Baldwin wrote:
> Display the LWP ID of each thread in a FreeBSD core.  Extract thread names
> from the per-thread THRMISC note.
> 
> gdb/ChangeLog:
> 
> 	* fbsd_tdep.c (fbsd_core_pid_to_str): New function.
> 	(fbsd_init_abi): Add "core_pid_to_str" gdbarch method.
> ---
>  gdb/ChangeLog   |  5 +++++
>  gdb/fbsd-tdep.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 471d02b..93760ba 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,8 @@
> +2016-01-09  John Baldwin  <jhb@FreeBSD.org>
> +
> +	* fbsd_tdep.c (fbsd_core_pid_to_str): New function.
> +	(fbsd_init_abi): Add "core_pid_to_str" gdbarch method.
> +
>  2016-01-08  Yao Qi  <yao.qi@linaro.org>
>  
>  	* extension.c: Include target.h.
> diff --git a/gdb/fbsd-tdep.c b/gdb/fbsd-tdep.c
> index 0ef94d6..6851cc1 100644
> --- a/gdb/fbsd-tdep.c
> +++ b/gdb/fbsd-tdep.c
> @@ -28,6 +28,46 @@
>  #include "fbsd-tdep.h"
>  
>  
> +/* 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.

> +  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.

> +      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.


> +	    {
> +	      name[size] = '\0';
> +	      if (strcmp(name, elf_tdata (core_bfd)->core->program) != 0)

Missing space after strcmp.

Is this ".thrmisc" section documented somewhere?  Could you add a
small comment on what this entry you're skipping means?

> +		{
> +		  snprintf (buf, sizeof buf, "LWP %ld \"%s\"",
> +			    ptid_get_lwp (ptid), name);

xsnprintf.

> +		  return buf;
> +		}
> +	    }
> +	}
> +      snprintf (buf, sizeof buf, "LWP %ld", ptid_get_lwp (ptid));

xsnprintf.

> +      return buf;
> +    }
> +
> +  return normal_pid_to_str (ptid);
> +}
> +
>  static int
>  find_signalled_thread (struct thread_info *info, void *data)
>  {
> @@ -132,5 +172,6 @@ fbsd_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, int *note_size)
>  void
>  fbsd_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
>  {
> +  set_gdbarch_core_pid_to_str (gdbarch, fbsd_core_pid_to_str);
>    set_gdbarch_make_corefile_notes (gdbarch, fbsd_make_corefile_notes);
>  }
> 


Thanks,
Pedro Alves
  

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.

A somewhat related question: would it be reasonable to dump additional
notes in 'gcore' only when native?  For example, FreeBSD kernels dump
several additional notes including things like auxv, open files, etc.
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.
  

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
  

On Monday, January 18, 2016 12:27:21 PM Pedro Alves wrote:
> On 01/15/2016 08:13 PM, John Baldwin wrote:
> > On Thursday, January 14, 2016 03:03:50 PM Pedro Alves wrote:
> >> 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.

Ok.

> 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" ?

Hmm, would this work for cross-debugging a core file?  fbsd_thread_name is
currently only defined for native debugging, whereas core_pid_to_str is a
gdbarch method on the ABI so it can be used to extract thread names from
non-native cores.

Alternatively, would it be best if the core target defined a to_thread_name
method that invoked a new gdbarch method (gdbarch_core_thread_name or the
like) to fetch the thread name?
  

On 01/18/2016 05:05 PM, John Baldwin wrote:

> Alternatively, would it be best if the core target defined a to_thread_name
> method that invoked a new gdbarch method (gdbarch_core_thread_name or the
> like) to fetch the thread name?

Yes, that's the right way to go.

Thanks,
Pedro Alves