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

[John Baldwin <jhb@freebsd.org>]

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.

-- 
John Baldwin