Re: [PATCH v2] xtensa: implement NPTL helpers

[Pedro Alves <palves at redhat dot com>]

On 08/21/2015 06:15 AM, Max Filippov wrote:
> These changes allow debugging multithreaded NPTL xtensa applications.
> 
> 2015-08-20  Max Filippov  <jcmvbkbc@gmail.com>
> gdb/gdbserver/
> 	* configure.srv (xtensa*-*-linux*): Add srv_linux_thread_db=yes.
> 	* linux-xtensa-low.c (xtensa-tdep.h gdb_proc_service.h): New
> 	#includes.
> 	(ps_get_thread_area): New function.
> 
> 2015-08-20  Max Filippov  <jcmvbkbc@gmail.com>
> gdb/
> 	* xtensa-linux-nat.c (gdb_proc_service.h): New #include.
> 	(ps_get_thread_area): New function.
> 	* xtensa-linux-tdep.c (xtensa_linux_init_abi): Add call to
> 	set_gdbarch_fetch_tls_load_module_address to enable TLS support.
> 	* xtensa-tdep.c (osabi.h): New #include.
> 	(xtensa_gdbarch_init): Call gdbarch_init_osabi to register
> 	xtensa-specific hooks.
> 	* xtensa-tdep.h (struct xtensa_elf_gregset_t): Add threadptr
> 	member.
> ---
> Changes v1->v2:
> - add missing #include "osabi.h" to xtensa-tdep.c
> - add ps_get_thread_area to gdbserver/linux-xtensa-low.c
> - enable thread_db use in gdbserver/configure.srv

Thanks.

> 
>  gdb/gdbserver/configure.srv      |  1 +
>  gdb/gdbserver/linux-xtensa-low.c | 18 ++++++++++++++++++
>  gdb/xtensa-linux-nat.c           | 19 +++++++++++++++++++
>  gdb/xtensa-linux-tdep.c          |  4 ++++
>  gdb/xtensa-tdep.c                |  4 ++++
>  gdb/xtensa-tdep.h                |  3 ++-
>  6 files changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/gdbserver/configure.srv b/gdb/gdbserver/configure.srv
> index 0b18d1d..320c26a 100644
> --- a/gdb/gdbserver/configure.srv
> +++ b/gdb/gdbserver/configure.srv
> @@ -352,6 +352,7 @@ case "${target}" in
>    xtensa*-*-linux*)	srv_regobj=reg-xtensa.o
>  			srv_tgtobj="$srv_linux_obj linux-xtensa-low.o"
>  			srv_linux_regsets=yes
> +			srv_linux_thread_db=yes
>  			;;
>    tilegx-*-linux*)	srv_regobj=reg-tilegx.o
>  			srv_regobj="${srv_regobj} reg-tilegx32.o"
> diff --git a/gdb/gdbserver/linux-xtensa-low.c b/gdb/gdbserver/linux-xtensa-low.c
> index 4daccee..a515f97 100644
> --- a/gdb/gdbserver/linux-xtensa-low.c
> +++ b/gdb/gdbserver/linux-xtensa-low.c
> @@ -25,7 +25,9 @@ void init_registers_xtensa (void);
>  extern const struct target_desc *tdesc_xtensa;
>  
>  #include <asm/ptrace.h>
> +#include <xtensa-tdep.h>

Sorry, but gdbserver must not include gdb-specific files.

Maybe move xtensa_elf_gregset_t to a new gdb/arch/xtensa.h
file.

>  #include <xtensa-config.h>
> +#include "gdb_proc_service.h"
>  
>  #include "xtensa-xtregs.c"
>  
> @@ -179,6 +181,22 @@ xtensa_breakpoint_at (CORE_ADDR where)
>  		  xtensa_breakpoint, xtensa_breakpoint_len) == 0;
>  }
>  
> +ps_err_e
> +ps_get_thread_area (const struct ps_prochandle *ph,
> +                    lwpid_t lwpid, int idx, void **base)

Missed this in v1, but please add an intro comment.  As simple
as

  /* Called by libthread_db.  */

is enough.

> +{
> +  xtensa_elf_gregset_t regs;
> +  if (ptrace (PTRACE_GETREGS, lwpid, NULL, &regs) != 0)

Missing empty line after declaration.

> +    return PS_ERR;
> +
> +  /* IDX is the bias from the thread pointer to the beginning of the
> +     thread descriptor.  It has to be subtracted due to implementation
> +     quirks in libthread_db.  */
> +  *base = (void *) ((char *)regs.threadptr - idx);
> +

Space after the cast.  "(char *) regs".

> +  return PS_OK;
> +}
> +
>  static struct regsets_info xtensa_regsets_info =
>    {
>      xtensa_regsets, /* regsets */
> diff --git a/gdb/xtensa-linux-nat.c b/gdb/xtensa-linux-nat.c
> index 77ad3e0..3e5ad5c 100644
> --- a/gdb/xtensa-linux-nat.c
> +++ b/gdb/xtensa-linux-nat.c
> @@ -37,6 +37,9 @@
>  #include "gregset.h"
>  #include "xtensa-tdep.h"
>  
> +/* Defines ps_err_e, struct ps_prochandle.  */
> +#include "gdb_proc_service.h"
> +
>  /* Extended register set depends on hardware configs.
>     Keeping these definitions separately allows to introduce
>     hardware-specific overlays.  */
> @@ -280,6 +283,22 @@ xtensa_linux_store_inferior_registers (struct target_ops *ops,
>      store_xtregs (regcache, regnum);
>  }
>  
> +ps_err_e
> +ps_get_thread_area (const struct ps_prochandle *ph,
> +                    lwpid_t lwpid, int idx, void **base)

Intro comment here too.

> +{
> +  xtensa_elf_gregset_t regs;
> +  if (ptrace (PTRACE_GETREGS, lwpid, NULL, &regs) != 0)
> +    return PS_ERR;
> +
> +  /* IDX is the bias from the thread pointer to the beginning of the
> +     thread descriptor.  It has to be subtracted due to implementation
> +     quirks in libthread_db.  */
> +  *base = (void *) ((char *)regs.threadptr - idx);

Missing space.

> +
> +  return PS_OK;
> +}
> +
>  void _initialize_xtensa_linux_nat (void);

Thanks,
Pedro Alves