On Sun, 2013-11-10 at 21:22 +0100, Jan Kratochvil wrote: > jankratochvil/ynonx86base-attach-firstreg-ppc-s390 > > I have left ebl API extensions together with s390 and s390x all in one.
As with ppc, a quick overview of why the extensions are needed would be helpful. normalize_pc is clear from the 31-bitness. I understand what the new unwind hook is supposed to do. It gives the backend a fallback for unwinding if DWARF unwinding fails. Which seems a good thing to have in general. But why is it needed specifically for s390? > backends/ > 2013-11-10 Jan Kratochvil <[email protected]> > > unwinder: s390 and s390x > * Makefile.am (s390_SRCS): Add s390_initreg.c and s390_unwind.c. > * s390_init.c (s390_init): Initialize frame_nregs, core_pc_offset, > set_initial_registers_tid, normalize_pc and unwind. > * s390_initreg.c: New file. > * s390_unwind.c: New file. > > libdwfl/ > 2013-11-10 Jan Kratochvil <[email protected]> > > unwinder: s390 and s390x > * dwfl_frame_pc.c (dwfl_frame_pc): Call ebl_normalize_pc. > * frame_unwind.c (new_unwound): New function from ... > (handle_cfi): ... here. Call it. > Call also ebl_dwarf_to_regno. > (setfunc, getfunc, readfunc): New functions. > (__libdwfl_frame_unwind): Call ebl_unwind with those functions. > > libebl/ > 2013-11-10 Jan Kratochvil <[email protected]> > > unwinder: s390 and s390x > * Makefile.am (gen_SOURCES): Add eblnormalizepc.c and eblunwind.c. > * ebl-hooks.h (normalize_pc, unwind): New. > * eblnormalizepc.c: New file. > * eblunwind.c: New file. > * libebl.h (ebl_normalize_pc): New declaration. > (ebl_tid_registers_get_t, ebl_pid_memory_read_t): New definitions. > (ebl_unwind): New declaration. > > tests/ > 2013-11-10 Jan Kratochvil <[email protected]> > > unwinder: s390 and s390x > * Makefile.am (EXTRA_DIST): Add backtrace.s390.core.bz2, > backtrace.s390.exec.bz2, backtrace.s390x.core.bz2 and > backtrace.s390x.exec.bz2. > * backtrace.s390.core.bz2: New file. > * backtrace.s390.exec.bz2: New file. > * backtrace.s390x.core.bz2: New file. > * backtrace.s390x.exec.bz2: New file. > * run-backtrace.sh: Test also s390 and s390x. > > Signed-off-by: Jan Kratochvil <[email protected]> > > --- a/backends/Makefile.am > +++ b/backends/Makefile.am > @@ -99,7 +99,8 @@ libebl_ppc64_pic_a_SOURCES = $(ppc64_SRCS) > am_libebl_ppc64_pic_a_OBJECTS = $(ppc64_SRCS:.c=.os) > > s390_SRCS = s390_init.c s390_symbol.c s390_regs.c s390_retval.c \ > - s390_corenote.c s390x_corenote.c s390_cfi.c > + s390_corenote.c s390x_corenote.c s390_cfi.c s390_initreg.c \ > + s390_unwind.c OK. > --- a/backends/s390_init.c > +++ b/backends/s390_init.c > @@ -62,6 +62,16 @@ s390_init (elf, machine, eh, ehlen) > else > HOOK (eh, core_note); > HOOK (eh, abi_cfi); > + /* gcc/config/ #define DWARF_FRAME_REGISTERS 34. > + But from the gcc/config/s390/s390.h "Register usage." comment it looks > as > + if #32 (Argument pointer) and #33 (Condition code) are not used for > + unwinding. */ > + eh->frame_nregs = 32; > + eh->core_pc_offset = eh->class == ELFCLASS64 ? 0x78 : 0x4c; > + HOOK (eh, set_initial_registers_tid); > + if (eh->class == ELFCLASS32) > + HOOK (eh, normalize_pc); > + HOOK (eh, unwind); OK. But see the core_pc_offset -> core_pc_regno suggestion in the ppc case. > --- /dev/null > +++ b/backends/s390_initreg.c > [...] > +bool > +s390_set_initial_registers_tid (pid_t tid __attribute__ ((unused)), > + ebl_tid_registers_t *setfunc __attribute__ ((unused)), > + void *arg __attribute__ ((unused))) > +{ > +#ifndef __s390__ > + return false; > +#else /* __s390__ */ > + struct user user_regs; > + ptrace_area parea; > + parea.process_addr = (uintptr_t) &user_regs; > + parea.kernel_addr = 0; > + parea.len = sizeof (user_regs); > + if (ptrace (PTRACE_PEEKUSR_AREA, tid, &parea, NULL) != 0) > + return false; > + /* If we run as s390x we get the 64-bit registers of tid. > + But -m31 executable seems to use only the 32-bit parts of its > + registers so we ignore the upper half. */ > + Dwarf_Word dwarf_regs[16]; > + for (unsigned u = 0; u < 16; u++) > + dwarf_regs[u] = user_regs.regs.gprs[u]); > + return setfunc (0, 16, dwarf_regs, arg); Should that be: if (! setfunc ...) return false? > + /* Avoid conversion double -> integer. */ > + eu_static_assert (sizeof user_regs.regs.fp_regs.fprs[0] > + == sizeof state->regs[0]); > + for (unsigned u = 0; u < 16; u++) > + dwarf_regs[u] = *((const __typeof (*state->regs) *) > + &user_regs.regs.fp_regs.fprs[u])); > + if (! setfunc (0, 16, dwarf_regs, arg)) > + return false; > + for (unsigned u = 0; u < 16; u++) > + dwarf_frame_state_reg_set (state, 16 + u, > + *((const __typeof (*state->regs) *) > + &user_regs.regs.fp_regs.fprs[u])); I don't understand this. What is the intention? > + if (! setfunc (16, 16, dwarf_regs, arg)) > + return false; > + dwarf_regs[0] = user_regs.regs.psw.addr; > + return setfunc (-1, 1, dwarf_regs, arg)) > + return false; > +#endif /* __s390__ */ > +} OK. > +void > +s390_normalize_pc (Ebl *ebl __attribute__ ((unused)), Dwarf_Addr *pc) > +{ > + assert (ebl->class == ELFCLASS32); > + > + /* Clear S390 bit 31. */ > + *pc &= (1U << 31) - 1; > +} OK. Lovely :) > --- /dev/null > +++ b/backends/s390_unwind.c > [...] > +bool > +s390_unwind (Ebl *ebl, Dwarf_Addr pc, ebl_tid_registers_t *setfunc, > + ebl_tid_registers_get_t *getfunc, ebl_pid_memory_read_t *readfunc, > + void *arg, bool *signal_framep) > +{ > + /* Caller already assumed caller adjustment but S390 instructions are 4 > bytes > + long. Undo it. */ > + if ((pc & 0x3) != 0x3) > + return false; > + pc++; > + /* We can assume big-endian read here. */ > + Dwarf_Word instr; > + if (! readfunc (pc, &instr, arg)) > + return false; > + /* Fetch only the very first two bytes. */ > + instr = (instr >> (ebl->class == ELFCLASS64 ? 48 : 16)) & 0xffff; > + /* See GDB s390_sigtramp_frame_sniffer. */ That is not a good comment. You have to explain here what the code is trying to do. > + /* Check for 'svc'. */ > + if (((instr >> 8) & 0xff) != 0x0a) > + return false; > + /* Check for 'sigreturn' or 'rt_sigreturn'. */ > + if ((instr & 0xff) != 119 && (instr & 0xff) != 173) > + return false; > + /* See GDB s390_sigtramp_frame_unwind_cache. */ Again. > +# define S390_SP_REGNUM (0 + 15) /* S390_R15_REGNUM */ > + Dwarf_Word this_sp; > + if (! getfunc (S390_SP_REGNUM, 1, &this_sp, arg)) > + return false; > + unsigned word_size = ebl->class == ELFCLASS64 ? 8 : 4; > + Dwarf_Addr next_cfa = this_sp + 16 * word_size + 32; > + /* "New-style RT frame" is not supported, > + assuming "Old-style RT frame and all non-RT frames". */ > + Dwarf_Word sigreg_ptr; > + if (! readfunc (next_cfa + 8, &sigreg_ptr, arg)) > + return false; > + /* Skip PSW mask. */ > + sigreg_ptr += word_size; > + /* Read PSW address. */ > + Dwarf_Word val; > + if (! readfunc (sigreg_ptr, &val, arg)) > + return false; > + if (! setfunc (-1, 1, &val, arg)) > + return false; > + sigreg_ptr += word_size; > + /* Then the GPRs. */ > + Dwarf_Word gprs[16]; > + for (int i = 0; i < 16; i++) > + { > + if (! readfunc (sigreg_ptr, &gprs[i], arg)) > + return false; > + sigreg_ptr += word_size; > + } > + /* Then the ACRs. Skip them, they are not used in CFI. */ > + for (int i = 0; i < 16; i++) > + sigreg_ptr += 4; > + /* The floating-point control word. */ > + sigreg_ptr += 8; > + /* And finally the FPRs. */ > + Dwarf_Word fprs[16]; > + for (int i = 0; i < 16; i++) > + { > + if (! readfunc (sigreg_ptr, &val, arg)) > + return false; > + if (ebl->class == ELFCLASS32) > + { > + Dwarf_Addr val_low; > + if (! readfunc (sigreg_ptr + 4, &val_low, arg)) > + return false; > + val = (val << 32) | val_low; > + } > + fprs[i] = val; > + sigreg_ptr += 8; > + } > + /* If we have them, the GPR upper halves are appended at the end. */ > + if (ebl->class == ELFCLASS32) > + { > + unsigned sigreg_high_off = 4; > + sigreg_ptr += sigreg_high_off; > + for (int i = 0; i < 16; i++) > + { > + if (! readfunc (sigreg_ptr, &val, arg)) > + return false; > + Dwarf_Word val_low = gprs[i]; > + val = (val << 32) | val_low; > + gprs[i] = val; > + sigreg_ptr += 4; > + } > + } > + if (! setfunc (0, 16, gprs, arg)) > + return false; > + if (! setfunc (16, 16, fprs, arg)) > + return false; > + *signal_framep = true; > + return true; > +} This code is a little too magic. Please add a comment at the top in which situations it works and how. > --- a/libdwfl/dwfl_frame_pc.c > +++ b/libdwfl/dwfl_frame_pc.c > @@ -37,6 +37,7 @@ dwfl_frame_pc (Dwfl_Frame *state, Dwarf_Addr *pc, bool > *isactivation) > { > assert (state->pc_state == DWFL_FRAME_STATE_PC_SET); > *pc = state->pc; > + ebl_normalize_pc (state->thread->process->ebl, pc); OK. > --- a/libdwfl/frame_unwind.c > +++ b/libdwfl/frame_unwind.c > @@ -494,6 +494,26 @@ expr_eval (Dwfl_Frame *state, Dwarf_Frame *frame, const > Dwarf_Op *ops, > return true; > } > > +static void > +new_unwound (Dwfl_Frame *state) > +{ > + assert (state->unwound == NULL); > + Dwfl_Thread *thread = state->thread; > + Dwfl_Process *process = thread->process; > + Ebl *ebl = process->ebl; > + size_t nregs = ebl_frame_nregs (ebl); > + assert (nregs > 0); > + Dwfl_Frame *unwound; > + unwound = malloc (sizeof (*unwound) + sizeof (*unwound->regs) * nregs); > + state->unwound = unwound; > + unwound->thread = thread; > + unwound->unwound = NULL; > + unwound->signal_frame = false; > + unwound->initial_frame = false; > + unwound->pc_state = DWFL_FRAME_STATE_ERROR; > + memset (unwound->regs_set, 0, sizeof (unwound->regs_set)); > +} > + > /* The logic is to call __libdwfl_seterrno for any CFI bytecode > interpretation > error so one can easily catch the problem with a debugger. Still there > are > archs with invalid CFI for some registers where the registers are never > used > @@ -508,20 +528,14 @@ handle_cfi (Dwfl_Frame *state, Dwarf_Addr pc, Dwarf_CFI > *cfi, Dwarf_Addr bias) > __libdwfl_seterrno (DWFL_E_LIBDW); > return; > } > + new_unwound (state); > + Dwfl_Frame *unwound = state->unwound; > + unwound->signal_frame = frame->fde->cie->signal_frame; > Dwfl_Thread *thread = state->thread; > Dwfl_Process *process = thread->process; > Ebl *ebl = process->ebl; > size_t nregs = ebl_frame_nregs (ebl); > assert (nregs > 0); > - Dwfl_Frame *unwound; > - unwound = malloc (sizeof (*unwound) + sizeof (*unwound->regs) * nregs); > - state->unwound = unwound; > - unwound->thread = thread; > - unwound->unwound = NULL; > - unwound->signal_frame = frame->fde->cie->signal_frame; > - unwound->initial_frame = false; > - unwound->pc_state = DWFL_FRAME_STATE_ERROR; > - memset (unwound->regs_set, 0, sizeof (unwound->regs_set)); OK > @@ -539,7 +553,7 @@ handle_cfi (Dwfl_Frame *state, Dwarf_Addr pc, Dwarf_CFI > *cfi, Dwarf_Addr bias) > { > /* REGNO is undefined. */ > unsigned ra = frame->fde->cie->return_address_register; > - if (regno == ra) > + if (ebl_dwarf_to_regno (ebl, &ra) && regno == ra) > unwound->pc_state = DWFL_FRAME_STATE_PC_UNDEFINED; > continue; > } OK. Shouldn't this have been part of the previous (ppc) patch? > +static bool > +setfunc (int firstreg, unsigned nregs, const Dwarf_Word *regs, void *arg) > +{ > + Dwfl_Frame *state = arg; > + Dwfl_Frame *unwound = state->unwound; > + if (firstreg < 0) > + { > + assert (firstreg == -1); > + assert (nregs > 0); > + assert (unwound->pc_state == DWFL_FRAME_STATE_PC_UNDEFINED); > + unwound->pc = *regs; > + unwound->pc_state = DWFL_FRAME_STATE_PC_SET; > + firstreg++; > + nregs--; > + regs++; > + } > + while (nregs--) > + if (! __libdwfl_frame_reg_set (unwound, firstreg++, *regs++)) > + return false; > + return true; > +} > > +static bool > +getfunc (int firstreg, unsigned nregs, Dwarf_Word *regs, void *arg) > +{ > + Dwfl_Frame *state = arg; > + if (firstreg < 0) > + { > + assert (firstreg == -1); > + assert (nregs > 0); > + assert (state->pc_state == DWFL_FRAME_STATE_PC_SET); > + *regs = state->pc; > + firstreg++; > + nregs--; > + regs++; > + } > + while (nregs--) > + if (! __libdwfl_frame_reg_get (state, firstreg++, regs++)) > + return false; > + return true; > +} > + > +static bool > +readfunc (Dwarf_Addr addr, Dwarf_Word *datap, void *arg) > +{ > + Dwfl_Frame *state = arg; > + Dwfl_Thread *thread = state->thread; > + Dwfl_Process *process = thread->process; > + return process->callbacks->memory_read (process->dwfl, addr, datap, > + process->callbacks_arg); > +} OK. > void > internal_function > __libdwfl_frame_unwind (Dwfl_Frame *state) > @@ -618,4 +684,20 @@ __libdwfl_frame_unwind (Dwfl_Frame *state) > return; > } > } > + assert (state->unwound == NULL); > + Dwfl_Thread *thread = state->thread; > + Dwfl_Process *process = thread->process; > + Ebl *ebl = process->ebl; > + new_unwound (state); > + state->unwound->pc_state = DWFL_FRAME_STATE_PC_UNDEFINED; > + // &Dwfl_Frame.signal_frame cannot be passed as it is a bitfield. > + bool signal_frame = false; > + if (! ebl_unwind (ebl, pc, setfunc, getfunc, readfunc, state, > &signal_frame)) > + { > + state->unwound->pc_state = DWFL_FRAME_STATE_ERROR; > + // __libdwfl_seterrno has been called above. > + return; > + } > + assert (state->unwound->pc_state == DWFL_FRAME_STATE_PC_SET); > + state->unwound->signal_frame = signal_frame; > } Should this always be done? Or only if there is no dwarf or no cfi range for the given address? > --- a/libebl/Makefile.am > +++ b/libebl/Makefile.am > @@ -54,7 +54,8 @@ 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 ebldwarftoregno.c > + eblstother.c eblinitreg.c eblgetsymbol.c ebldwarftoregno.c \ > + eblnormalizepc.c eblunwind.c OK > --- a/libebl/ebl-hooks.h > +++ b/libebl/ebl-hooks.h > @@ -184,5 +184,21 @@ const char *EBLHOOK(get_symbol) (Ebl *ebl, size_t ndx, > GElf_Sym *symp, > Dwarf_Frame->REGS indexing. */ > bool EBLHOOK(dwarf_to_regno) (Ebl *ebl, unsigned *regno); > > +/* Optionally modify *PC as fetched from inferior data into valid PC > + instruction pointer. */ > +void EBLHOOK(normalize_pc) (Ebl *ebl, Dwarf_Addr *pc); OK. > +/* Get previous frame state for an existing frame state. PC is for the > + existing frame. SETFUNC sets register in the previous frame. GETFUNC > gets > + register from the existing frame. Note that GETFUNC vs. SETFUNC act on > + a disjunct set of registers. READFUNC reads memory. ARG has to be passed > + for SETFUNC, GETFUNC and READFUNC. *SIGNAL_FRAMEP is initialized to > false, > + it can be set to true if existing frame is a signal frame. SIGNAL_FRAMEP > is > + never NULL. */ > +bool EBLHOOK(unwind) (Ebl *ebl, Dwarf_Addr pc, ebl_tid_registers_t *setfunc, > + ebl_tid_registers_get_t *getfunc, > + ebl_pid_memory_read_t *readfunc, void *arg, > + bool *signal_framep); This should explain when it is called. > /* Destructor for ELF backend handle. */ > void EBLHOOK(destr) (struct ebl *); > --- /dev/null > +++ b/libebl/eblnormalizepc.c > [...] > +void > +ebl_normalize_pc (Ebl *ebl, Dwarf_Addr *pc) > +{ > + if (ebl != NULL && ebl->normalize_pc != NULL) > + ebl->normalize_pc (ebl, pc); > +} > --- /dev/null > +++ b/libebl/eblunwind.c > [...] > +bool > +ebl_unwind (Ebl *ebl, Dwarf_Addr pc, ebl_tid_registers_t *setfunc, > + ebl_tid_registers_get_t *getfunc, ebl_pid_memory_read_t *readfunc, > + void *arg, bool *signal_framep) > +{ > + if (ebl == NULL || ebl->unwind == NULL) > + return false; > + return ebl->unwind (ebl, pc, setfunc, getfunc, readfunc, arg, > signal_framep); > +} OK. > --- a/libebl/libebl.h > +++ b/libebl/libebl.h > @@ -433,6 +433,33 @@ extern const char *ebl_get_symbol (Ebl *ebl, size_t ndx, > GElf_Sym *symp, > extern bool ebl_dwarf_to_regno (Ebl *ebl, unsigned *regno) > __nonnull_attribute__ (1, 2); > > +/* Modify PC as fetched from inferior data into valid PC. */ > +extern void ebl_normalize_pc (Ebl *ebl, Dwarf_Addr *pc) > + __nonnull_attribute__ (1, 2); > + > +/* Callback type for FIXME. > + Register -1 is mapped to PC (if arch PC has no DWARF number). */ > +typedef bool (ebl_tid_registers_get_t) (int firstreg, unsigned nregs, > + Dwarf_Word *regs, void *arg) > + __nonnull_attribute__ (3); FIXME should be ebl_unwind I assume. > +typedef bool (ebl_pid_memory_read_t) (Dwarf_Addr addr, Dwarf_Word *data, > + void *arg) > + __nonnull_attribute__ (3); > + > +/* Get previous frame state for an existing frame state. PC is for the > + existing frame. SETFUNC sets register in the previous frame. GETFUNC > gets > + register from the existing frame. Note that GETFUNC vs. SETFUNC act on > + a disjunct set of registers. READFUNC reads memory. ARG has to be passed > + for SETFUNC, GETFUNC and READFUNC. *SIGNAL_FRAMEP is initialized to > false, > + it can be set to true if existing frame is a signal frame. SIGNAL_FRAMEP > is > + never NULL. */ > +extern bool ebl_unwind (Ebl *ebl, Dwarf_Addr pc, ebl_tid_registers_t > *setfunc, > + ebl_tid_registers_get_t *getfunc, > + ebl_pid_memory_read_t *readfunc, void *arg, > + bool *signal_framep) > + __nonnull_attribute__ (1, 3, 4, 5, 7); > + > #ifdef __cplusplus > } > #endif See above about comment explanation. Thanks, Mark
