This is the mail archive of the
elfutils-devel@sourceware.org
mailing list for the elfutils project.
Re: [patchv2 3/4] unwinder: ppc and ppc64
- From: Mark Wielaard <mjw at redhat dot com>
- To: elfutils-devel at lists dot fedorahosted dot org
- Date: Fri, 29 Nov 2013 11:32:08 +0100
- Subject: Re: [patchv2 3/4] unwinder: ppc and ppc64
On Sat, 2013-11-23 at 22:22 +0100, Jan Kratochvil wrote:
> On Mon, 18 Nov 2013 22:40:18 +0100, Mark Wielaard wrote:
> > > + /* 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.
>
> It could be useful but I find it OK to add only later.
OK, the comment will remind us later.
> > > + 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?
>
> I do not know what is the real reason. I tried to read some docs but 65 vs.
> 108 seems to me just as a bug/mess. I can add
>
> // LR uses both 65 and 108 numbers, there is no consistency for it.
Yes that would be good.
I tried to find the history, but it is all just messy :{ At one point it
seemed that the wrong (internal gcc) register mapping was used. And
there were some attempts to fix it up, but they were only partial
(maybe .debug_info and .debug_frame only, but not .eh_frame?). Sigh.
This thread seems to have some of the history (it is almost 10 years
old): http://gcc.gnu.org/ml/gcc/2004-01/msg00025.html
> > > + /* 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.
>
> Yes, I do not find it somehow clean to mix types when passing a pointer.
OK.
In this case I don't mind the "type mixing", but it is fine as is too.
> > 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?
>
> One cannot use regloc, backends/ppc_corenote.c does not list PC in
> Ebl_Register_Location prstatus_regs[]. But you are right it is listed in
> PRSTATUS_REGSET_ITEMS so I use it now. And the whole ebl->core_pc_offset
> could be removed.
Aha, yes of course, "nip" isn't associated with any regno. I think I
would have used the "nip" name as marker or maybe introduced a
"fake/special" regno for it. Instead of adding a new pc_register boolean
field to every register to save a little space, but I guess this works
too and there aren't so many registers that it really matters.
> unwinder: ppc and ppc64
>
> 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,
> set_initial_registers_tid and dwarf_to_regno.
> * ppc_corenote.c (PRSTATUS_REGSET_ITEMS) <nip>: Set PC_REGISTER.
> * ppc_init.c (ppc64_init): Initialize also frame_nregs,
> 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): Implement
> pc_register 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.
> * libebl.h (Ebl_Core_Item): New field pc_register.
> (ebl_tid_registers_t): Add FIRSTREG -1 to the comment.
> (ebl_dwarf_to_regno): New.
>
> tests/
> 2013-11-10 Jan Kratochvil <jan.kratochvil@redhat.com>
>
> * Makefile.am (TESTS): Add run-backtrace-core-ppc64.sh and
> run-backtrace-core-ppc.sh.
> (EXTRA_DIST): Add backtrace.ppc.core.bz2,
> backtrace.ppc.exec.bz2, backtrace.ppc64.core.bz2,
> backtrace.ppc64.exec.bz2, run-backtrace-core-ppc64.sh and
> run-backtrace-core-ppc.sh.
> * 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-core-ppc.sh: New file.
> * run-backtrace-core-ppc64.sh: New file.
The tests depend on the other test code going in. But I think you can
just push the rest already master.
> diff --git a/backends/Makefile.am b/backends/Makefile.am
> index 0e00329..653b56e 100644
> --- 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.
> diff --git a/backends/ppc64_init.c b/backends/ppc64_init.c
> index 3ed882b..8fd4575 100644
> --- a/backends/ppc64_init.c
> +++ b/backends/ppc64_init.c
> @@ -68,6 +68,10 @@ 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;
> + HOOK (eh, set_initial_registers_tid);
> + HOOK (eh, dwarf_to_regno);
OK.
> diff --git a/backends/ppc_corenote.c b/backends/ppc_corenote.c
> index 707a395..9ac8871 100644
> --- a/backends/ppc_corenote.c
> +++ b/backends/ppc_corenote.c
> @@ -123,7 +123,7 @@ static const Ebl_Register_Location spe_regs[] =
> { \
> .name = "nip", .type = ELF_T_ADDR, .format = 'x', \
> .offset = offsetof (struct EBLHOOK(prstatus), pr_reg[32]), \
> - .group = "register" \
> + .group = "register", .pc_register = true \
> }, \
> { \
> .name = "orig_gpr3", .type = TYPE_LONG, .format = 'd', \
OK. If you go with the extra pc_register field.
> diff --git a/backends/ppc_init.c b/backends/ppc_init.c
> index 004c601..ad92765 100644
> --- a/backends/ppc_init.c
> +++ b/backends/ppc_init.c
> @@ -65,6 +65,10 @@ 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;
> + HOOK (eh, set_initial_registers_tid);
> + HOOK (eh, dwarf_to_regno);
OK.
> diff --git a/backends/ppc_initreg.c 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 ();
> +}
OK. I would also have added a comment here about the 108/65 regno mess.
> +__typeof (ppc_dwarf_to_regno)
> + ppc64_dwarf_to_regno
> + __attribute__ ((alias ("ppc_dwarf_to_regno")));
OK.
> +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. */
> + 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[gpr] = 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;
> + /* 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__ */
> +}
OK. Maybe extend to comment for 65/108 to refer to ppc_dwarf_to_regno.
> +__typeof (ppc_set_initial_registers_tid)
> + ppc64_set_initial_registers_tid
> + __attribute__ ((alias ("ppc_set_initial_registers_tid")));
OK.
> diff --git a/libdwfl/frame_unwind.c b/libdwfl/frame_unwind.c
> index 1aed8cb..2973f95 100644
> --- 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, ®no))
> + return false;
> if (regno >= ebl_frame_nregs (ebl))
> return false;
> if ((state->regs_set[regno / sizeof (*state->regs_set) / 8]
OK.
> @@ -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, ®no))
> + return false;
> if (regno >= ebl_frame_nregs (ebl))
> return false;
> /* For example i386 user_regs_struct has signed fields. */
OK.
> diff --git a/libdwfl/linux-core-attach.c b/libdwfl/linux-core-attach.c
> index b2d703f..2853c3c 100644
> --- a/libdwfl/linux-core-attach.c
> +++ b/libdwfl/linux-core-attach.c
> @@ -198,6 +198,32 @@ 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));
> + for (item = items; item < items + nitems; item++)
> + if (item->pc_register)
> + break;
> + if (item < items + nitems)
> + {
> + Dwarf_Word pc;
> + switch (gelf_getclass (core) == ELFCLASS32 ? 32 : 64)
> + {
> + case 32:;
> + uint32_t val32 = *(const uint32_t *) (desc + item->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 + item->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);
> + }
OK, but just below this code is another loop over the items/regs which
contains some FIXMEs that you seem to have fixed now. Can't these loops
be combined? It currently seems to duplicate the value extraction code
and seems to depend on the order of notes/items. It would be better if
it was just one loop that only depended on the regno (and/or pc_register
flag or special PC name).
> diff --git a/libdwfl/linux-pid-attach.c b/libdwfl/linux-pid-attach.c
> index e4eb621..45a0732 100644
> --- a/libdwfl/linux-pid-attach.c
> +++ b/libdwfl/linux-pid-attach.c
> @@ -205,6 +205,14 @@ 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 == 1);
> + INTUSE(dwfl_thread_state_register_pc) (thread, *regs);
> + return true;
> + }
> + assert (nregs > 0);
> return INTUSE(dwfl_thread_state_registers) (thread, firstreg, nregs, regs);
> }
OK.
> diff --git a/libebl/Makefile.am b/libebl/Makefile.am
> index 1fb3da3..6c03af8 100644
> --- 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.
> diff --git a/libebl/ebl-hooks.h b/libebl/ebl-hooks.h
> index 2ad18ec..f2b3857 100644
> --- a/libebl/ebl-hooks.h
> +++ b/libebl/ebl-hooks.h
> @@ -172,5 +172,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);
>
> +/* 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);
OK.
> diff --git a/libebl/ebldwarftoregno.c 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.
> diff --git a/libebl/libebl.h b/libebl/libebl.h
> index 2446cc9..7fe5456 100644
> --- a/libebl/libebl.h
> +++ b/libebl/libebl.h
> @@ -368,6 +368,7 @@ typedef struct
> Elf_Type type;
> char format;
> bool thread_identifier;
> + bool pc_register;
> } Ebl_Core_Item;
OK.
> -/* 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).
> + If FIRSTREG is -1 then NREGS has to be 1. */
> typedef bool (ebl_tid_registers_t) (int firstreg, unsigned nregs,
> const Dwarf_Word *regs, void *arg)
> __nonnull_attribute__ (3);
> @@ -428,6 +431,11 @@ extern const char *ebl_get_symbol (Ebl *ebl, size_t ndx, GElf_Sym *symp,
> GElf_Word *shndxp)
> __nonnull_attribute__ (1, 3);
>
> +/* 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.
Thanks,
Mark