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

[Pedro Alves <palves@redhat.com>]

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