This is the mail archive of the elfutils-devel@sourceware.org mailing list for the elfutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch 3/4] unwinder: ppc and ppc64


On Sun, 2013-11-10 at 21:21 +0100, Jan Kratochvil wrote:
> jankratochvil/ynonx86base-attach-firstreg-ppc
> 
> I have left ebl API extensions together with ppc and ppc64 all in one.

Please do add a short description on why the extensions were needed in
this case to the commit message (or in the intro of an email). It really
makes things easier to review and might be useful for people looking
back on this code/commit later.

Given the "interesting" DWARF register numbering on ppc, I see why we
want a dwarf_to_regno () to flatten things out. The firstreg -1
indicating PC seems reasonable, if the architecture doesn't define a
PC/return address DWARF register number. I had some trouble
understanding the core_pc_offset one at first though.

> backends/
> 2013-11-10  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	unwinder: ppc and ppc64
> 	* Makefile.am (ppc_SRCS, ppc64_SRCS): Add ppc_initreg.c.
> 	* ppc64_init.c (ppc64_init): Initialize also frame_nregs,
> 	core_pc_offset, set_initial_registers_tid and dwarf_to_regno.
> 	* ppc_init.c (ppc64_init): Initialize also frame_nregs,
> 	core_pc_offset, set_initial_registers_tid and dwarf_to_regno.
> 	* ppc_initreg.c: New file.
> 
> libdwfl/
> 2013-11-10  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	unwinder: ppc and ppc64
> 	* frame_unwind.c (__libdwfl_frame_reg_get, __libdwfl_frame_reg_set):
> 	Call ebl_dwarf_to_regno.
> 	* linux-core-attach.c (core_set_initial_registers): Keep DESC intact.
> 	Implement ebl_core_pc_offset support.
> 	* linux-pid-attach.c (pid_thread_state_registers_cb): Implement
> 	FIRSTREG -1.
> 
> libebl/
> 2013-11-10  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	unwinder: ppc and ppc64
> 	* Makefile.am (gen_SOURCES): Add ebldwarftoregno.c.
> 	* ebl-hooks.h (dwarf_to_regno): New.
> 	* ebldwarftoregno.c: New file.
> 	* eblinitreg.c (ebl_core_pc_offset): New.
> 	* libebl.h (ebl_tid_registers_t): Add FIRSTREG -1 to the comment.
> 	(ebl_core_pc_offset, ebl_dwarf_to_regno): New.
> 	* libeblP.h (struct ebl): New field core_pc_offset.
> 
> tests/
> 2013-11-10  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* Makefile.am (EXTRA_DIST): Add backtrace.ppc.core.bz2,
> 	backtrace.ppc.exec.bz2, backtrace.ppc64.core.bz2 and
> 	backtrace.ppc64.exec.bz2.
> 	* backtrace.ppc.core.bz2: New file.
> 	* backtrace.ppc.exec.bz2: New file.
> 	* backtrace.ppc64.core.bz2: New file.
> 	* backtrace.ppc64.exec.bz2: New file.
> 	* run-backtrace.sh: Add tests for ppc and ppc64.
> 
> Signed-off-by: Jan Kratochvil <jan.kratochvil@redhat.com>
> 
> --- a/backends/Makefile.am
> +++ b/backends/Makefile.am
> @@ -88,13 +88,13 @@ am_libebl_sparc_pic_a_OBJECTS = $(sparc_SRCS:.c=.os)
>  
>  ppc_SRCS = ppc_init.c ppc_symbol.c ppc_retval.c ppc_regs.c \
>  	   ppc_corenote.c ppc_auxv.c ppc_attrs.c ppc_syscall.c \
> -	   ppc_cfi.c
> +	   ppc_cfi.c ppc_initreg.c
>  libebl_ppc_pic_a_SOURCES = $(ppc_SRCS)
>  am_libebl_ppc_pic_a_OBJECTS = $(ppc_SRCS:.c=.os)
>  
>  ppc64_SRCS = ppc64_init.c ppc64_symbol.c ppc64_retval.c \
>  	     ppc64_corenote.c ppc_regs.c ppc_auxv.c ppc_attrs.c ppc_syscall.c \
> -	     ppc_cfi.c ppc64_get_symbol.c
> +	     ppc_cfi.c ppc64_get_symbol.c ppc_initreg.c
>  libebl_ppc64_pic_a_SOURCES = $(ppc64_SRCS)
>  am_libebl_ppc64_pic_a_OBJECTS = $(ppc64_SRCS:.c=.os)

OK.

> --- a/backends/ppc64_init.c
> +++ b/backends/ppc64_init.c
> @@ -68,6 +68,11 @@ ppc64_init (elf, machine, eh, ehlen)
>    HOOK (eh, init_symbols);
>    HOOK (eh, get_symbol);
>    HOOK (eh, destr);
> +  /* gcc/config/ #define DWARF_FRAME_REGISTERS.  */
> +  eh->frame_nregs = (114 - 1) + 32;
> +  eh->core_pc_offset = 0x170;
> +  HOOK (eh, set_initial_registers_tid);
> +  HOOK (eh, dwarf_to_regno);

OK, but the 0x170 magic number really should have a comment where it was
specified.

>    return MODVERSION;
>  }
> --- a/backends/ppc_init.c
> +++ b/backends/ppc_init.c
> @@ -65,6 +65,11 @@ ppc_init (elf, machine, eh, ehlen)
>    HOOK (eh, auxv_info);
>    HOOK (eh, check_object_attribute);
>    HOOK (eh, abi_cfi);
> +  /* gcc/config/ #define DWARF_FRAME_REGISTERS.  */
> +  eh->frame_nregs = (114 - 1) + 32;
> +  eh->core_pc_offset = 0xc8;
> +  HOOK (eh, set_initial_registers_tid);
> +  HOOK (eh, dwarf_to_regno);
>  
>    return MODVERSION;
>  }

OK, but again do add a comment for the core_pc_offset magic value.

> +++ b/backends/ppc_initreg.c
> [...]
> +bool
> +ppc_dwarf_to_regno (Ebl *ebl __attribute__ ((unused)), unsigned *regno)
> +{
> +  switch (*regno)
> +  {
> +    case 108:
> +      *regno = 65;
> +      return true;
> +    case 0 ... 107:
> +    case 109 ... (114 - 1) -1:
> +      return true;
> +    case 1200 ... 1231:
> +      *regno = *regno - 1200 + (114 - 1);
> +      return true;
> +    default:
> +      return false;
> +  }
> +  abort ();
> +}
> +__typeof (ppc_dwarf_to_regno)
> +     ppc64_dwarf_to_regno
> +     __attribute__ ((alias ("ppc_dwarf_to_regno")));

OK. The abort () really means unreachable here.

> +bool
> +ppc_set_initial_registers_tid (pid_t tid __attribute__ ((unused)),
> +			  ebl_tid_registers_t *setfunc __attribute__ ((unused)),
> +			       void *arg __attribute__ ((unused)))
> +{
> +#ifndef __powerpc__
> +  return false;
> +#else /* __powerpc__ */
> +  union
> +    {
> +      struct pt_regs r;
> +      long l[sizeof (struct pt_regs) / sizeof (long)];
> +    }
> +  user_regs;
> +  eu_static_assert (sizeof (struct pt_regs) % sizeof (long) == 0);
> +  /* PTRACE_GETREGS is EIO on kernel-2.6.18-308.el5.ppc64.  */

You could try it anyway as optimization and fallback on the iteration
over PTRACE_PEEKUSER if it fails. But if you feel that is just a
micro-optimization don't. The comment shows we could do it later if we
really care.

> +  errno = 0;
> +  for (unsigned regno = 0; regno < sizeof (user_regs) / sizeof (long);
> +       regno++)
> +    {
> +      user_regs.l[regno] = ptrace (PTRACE_PEEKUSER, tid,
> +				   (void *) (uintptr_t) (regno
> +							 * sizeof (long)),
> +				   NULL);
> +      if (errno != 0)
> +	return false;
> +    }
> +  const size_t gprs = sizeof (user_regs.r.gpr) / sizeof (*user_regs.r.gpr);
> +  Dwarf_Word dwarf_regs[gprs];
> +  for (unsigned gpr = 0; gpr < gprs; gpr++)
> +    dwarf_regs[gprs] = user_regs.r.gpr[gpr];
> +  if (! setfunc (0, gprs, dwarf_regs, arg))
> +    return false;
> +  dwarf_regs[0] = user_regs.r.link;
> +  if (! setfunc (65, 1, dwarf_regs, arg)) // or 108
> +    return false;

This might need an extra comment. If I remember right 108 is the
official DWARF register number, but GCC always produces 65?

> +  /* Registers like msr, ctr, xer, dar, dsisr etc. are probably irrelevant
> +     for CFI.  */
> +  dwarf_regs[0] = user_regs.r.nip;
> +  return setfunc (-1, 1, dwarf_regs, arg);
> +#endif /* __powerpc__ */
> +}

Nitpick. You could probably use &user_regs.r.link and &user_regs.r.nip
instead of reusing dwarf_regs[0]. Though maybe the compiler will
complain about that. But again a micro optimization that is probably not
worth it.

> +__typeof (ppc_set_initial_registers_tid)
> +     ppc64_set_initial_registers_tid
> +     __attribute__ ((alias ("ppc_set_initial_registers_tid")));
> --- a/libdwfl/frame_unwind.c
> +++ b/libdwfl/frame_unwind.c
> @@ -52,6 +52,8 @@ internal_function
>  __libdwfl_frame_reg_get (Dwfl_Frame *state, unsigned regno, Dwarf_Addr *val)
>  {
>    Ebl *ebl = state->thread->process->ebl;
> +  if (! ebl_dwarf_to_regno (ebl, &regno))
> +    return false;
>    if (regno >= ebl_frame_nregs (ebl))
>      return false;
>    if ((state->regs_set[regno / sizeof (*state->regs_set) / 8]
> @@ -67,6 +69,8 @@ internal_function
>  __libdwfl_frame_reg_set (Dwfl_Frame *state, unsigned regno, Dwarf_Addr val)
>  {
>    Ebl *ebl = state->thread->process->ebl;
> +  if (! ebl_dwarf_to_regno (ebl, &regno))
> +    return false;
>    if (regno >= ebl_frame_nregs (ebl))
>      return false;
>    /* For example i386 user_regs_struct has signed fields.  */

OK.

> --- a/libdwfl/linux-core-attach.c
> +++ b/libdwfl/linux-core-attach.c
> @@ -198,14 +198,13 @@ core_set_initial_registers (Dwfl_Thread *thread, void *thread_arg_voidp)
>    }
>    /* core_next_thread already found this TID there.  */
>    assert (tid == INTUSE(dwfl_thread_tid) (thread));
> -  desc += regs_offset;
>    for (size_t regloci = 0; regloci < nregloc; regloci++)
>      {
>        const Ebl_Register_Location *regloc = reglocs + regloci;
>        if (regloc->regno >= nregs)
>  	continue;
>        assert (regloc->bits == 32 || regloc->bits == 64);
> -      const char *reg_desc = desc + regloc->offset;
> +      const char *reg_desc = desc + regs_offset + regloc->offset;
>        for (unsigned regno = regloc->regno;
>  	   regno < MIN (regloc->regno + (regloc->count ?: 1U), nregs);
>  	   regno++)
> @@ -244,6 +243,30 @@ core_set_initial_registers (Dwfl_Thread *thread, void *thread_arg_voidp)
>  	  reg_desc += regloc->pad;
>  	}
>      }

OK.

> +  size_t core_pc_offset = ebl_core_pc_offset (core_arg->ebl);
> +  if (core_pc_offset)
> +    {
> +      Dwarf_Word pc;
> +      switch (gelf_getclass (core) == ELFCLASS32 ? 32 : 64)
> +      {
> +	case 32:;
> +	  uint32_t val32 = *(const uint32_t *) (desc + core_pc_offset);
> +	  val32 = (elf_getident (core, NULL)[EI_DATA] == ELFDATA2MSB
> +		   ? be32toh (val32) : le32toh (val32));
> +	  /* Do a host width conversion.  */
> +	  pc = val32;
> +	  break;
> +	case 64:;
> +	  uint64_t val64 = *(const uint64_t *) (desc + core_pc_offset);
> +	  val64 = (elf_getident (core, NULL)[EI_DATA] == ELFDATA2MSB
> +		   ? be64toh (val64) : le64toh (val64));
> +	  pc = val64;
> +	  break;
> +	default:
> +	  abort ();
> +      }
> +      INTUSE(dwfl_thread_state_register_pc) (thread, pc);
> +    }
>    return true;
>  }

hmmm. I think I now understand the core_pc_offset.
It is the offset into the PRSTATUS note where to find the register that
represents the pc. But you reparse that location here instead of using
Ebl_Register_Location which you have just iterated through just above.

Why don't you use the loop above and check the regloc->offset there
before calling either dwfl_thread_state_registers or
dwfl_thread_state_register_pc on the value?

> --- a/libdwfl/linux-pid-attach.c
> +++ b/libdwfl/linux-pid-attach.c
> @@ -202,6 +202,17 @@ pid_thread_state_registers_cb (int firstreg, unsigned nregs,
>  			       const Dwarf_Word *regs, void *arg)
>  {
>    Dwfl_Thread *thread = (Dwfl_Thread *) arg;
> +  if (firstreg < 0)
> +    {
> +      assert (firstreg == -1);
> +      assert (nregs > 0);
> +      INTUSE(dwfl_thread_state_register_pc) (thread, *regs);
> +      firstreg++;
> +      nregs--;
> +      regs++;
> +    }
> +  if (! nregs)
> +    return true;
>    return INTUSE(dwfl_thread_state_registers) (thread, firstreg, nregs, regs);
>  }

OK. Though in practice I assume it will always be firstreg == -1 &&
nregs == 1. Which would result it slightly simpler code.

> --- a/libebl/Makefile.am
> +++ b/libebl/Makefile.am
> @@ -54,7 +54,7 @@ gen_SOURCES = eblopenbackend.c eblclosebackend.c eblstrtab.c \
>  	      eblreginfo.c eblnonerelocp.c eblrelativerelocp.c \
>  	      eblsysvhashentrysize.c eblauxvinfo.c eblcheckobjattr.c \
>  	      ebl_check_special_section.c ebl_syscall_abi.c eblabicfi.c \
> -	      eblstother.c eblinitreg.c eblgetsymbol.c
> +	      eblstother.c eblinitreg.c eblgetsymbol.c ebldwarftoregno.c

OK.

> --- a/libebl/ebl-hooks.h
> +++ b/libebl/ebl-hooks.h
> @@ -180,5 +180,9 @@ void EBLHOOK(init_symbols) (Ebl *ebl, size_t syments, int first_global,
>  const char *EBLHOOK(get_symbol) (Ebl *ebl, size_t ndx, GElf_Sym *symp,
>  				 GElf_Word *shndxp, void **filep);
>  
> +/* Convert *REGNO as is in DWARF to a lower range suitable for
> +   Dwarf_Frame->REGS indexing.  */
> +bool EBLHOOK(dwarf_to_regno) (Ebl *ebl, unsigned *regno);
> +
>  /* Destructor for ELF backend handle.  */
>  void EBLHOOK(destr) (struct ebl *);
> --- /dev/null
> +++ b/libebl/ebldwarftoregno.c
> [...]
> +bool
> +ebl_dwarf_to_regno (Ebl *ebl, unsigned *regno)
> +{
> +  if (ebl == NULL)
> +    return false;
> +  return ebl->dwarf_to_regno == NULL ? true : ebl->dwarf_to_regno (ebl, regno);
> +}

OK.

> --- a/libebl/eblinitreg.c
> +++ b/libebl/eblinitreg.c
> @@ -49,3 +49,9 @@ ebl_frame_nregs (Ebl *ebl)
>  {
>    return ebl == NULL ? 0 : ebl->frame_nregs;
>  }
> +
> +size_t
> +ebl_core_pc_offset (Ebl *ebl)
> +{
> +  return ebl == NULL ? 0 : ebl->core_pc_offset;
> +}

See below...

> --- a/libebl/libebl.h
> +++ b/libebl/libebl.h
> @@ -383,7 +383,8 @@ extern int ebl_auxv_info (Ebl *ebl, GElf_Xword a_type,
>  			  const char **name, const char **format)
>    __nonnull_attribute__ (1, 3, 4);
>  
> -/* Callback type for ebl_set_initial_registers_tid.  */
> +/* Callback type for ebl_set_initial_registers_tid.
> +   Register -1 is mapped to PC (if arch PC has no DWARF number).  */
>  typedef bool (ebl_tid_registers_t) (int firstreg, unsigned nregs,
>  				    const Dwarf_Word *regs, void *arg)

OK. I would suggest to just say that if firstreg == -1 then nregs == 1.
To simplify the code. If that makes sense.

> +/* PC register location stored in core file.  It is a number of bytes from
> +   descriptor of NT_PRSTATUS core note.  It is 0 if unused.  */
> +extern size_t ebl_core_pc_offset (Ebl *ebl)
> +  __nonnull_attribute__ (1);

It really should mention that the offset is one of the valid offsets of
the Ebl_Register_Location returned by ebl_core_note.

Or better. It should be named ebl_core_pc_regno. If not zero, it is the
Ebl_Register_Location regno that (double) as initial pc value.

> +/* Convert *REGNO as is in DWARF to a lower range suitable for
> +   Dwarf_Frame->REGS indexing.  */
> +extern bool ebl_dwarf_to_regno (Ebl *ebl, unsigned *regno)
> +  __nonnull_attribute__ (1, 2);

OK.

>  #ifdef __cplusplus
>  }
>  #endif
> --- a/libebl/libeblP.h
> +++ b/libebl/libeblP.h
> @@ -64,6 +64,10 @@ struct ebl
>       Ebl architecture can unwind iff FRAME_NREGS > 0.  */
>    size_t frame_nregs;
>  
> +  /* PC register location stored in core file.  It is a number of bytes from
> +     descriptor of NT_PRSTATUS core note.  It is 0 if unused.  */
> +  size_t core_pc_offset;

See above. Would it make sense to rename/define it as core_pc_regno?

> --- a/tests/run-backtrace.sh
> +++ b/tests/run-backtrace.sh
> @@ -91,4 +91,12 @@ for child in backtrace-child{,-biarch}; do
>  
>  done
>  
> +for arch in ppc ppc64; do
> +  testfiles backtrace.$arch.{exec,core}
> +  tempfiles backtrace.$arch.{bt,err}
> +  echo ./backtrace ./backtrace.$arch.{exec,core}
> +  testrun ${abs_builddir}/backtrace ./backtrace.$arch.{exec,core} 1>backtrace.$arch.bt 2>backtrace.$arch.err || true
> +  check_all backtrace.$arch.{bt,err} backtrace.$arch.core
> +done

Do see the comments I made about the tests in an earlier email.

Thanks,

Mark


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]