Re: [patch 4/4] unwinder: s390 and s390x

[Mark Wielaard <mjw at redhat dot com>]

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  <jan.kratochvil@redhat.com>
> 
> 	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  <jan.kratochvil@redhat.com>
> 
> 	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  <jan.kratochvil@redhat.com>
> 
> 	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  <jan.kratochvil@redhat.com>
> 
> 	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 <jan.kratochvil@redhat.com>
> 
> --- 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