[patch 4/4] unwinder: s390 and s390x

Jan Kratochvil jan.kratochvil at redhat.com
Tue Dec 17 18:05:48 UTC 2013


Hi Mark,

jankratochvil/unwinds390

attached patch lacks the binary blobs.

On Mon, 18 Nov 2013 23:35:25 +0100, Mark Wielaard wrote:
> 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?

Because s390 does not provide CFI for its signal handler.

(gdb) up
#1  <signal handler called>
(gdb) x/i $pc
=> 0x3ffff9cea7c:	svc	119

This address is in stack, as GDB watchpoint does not catch it I guess it is
generated by Linux kernel.  Dynamically registering CFI for stack address may
not be safe.  They should rather put the instruction to some text segment.

I did not file it anywhere, though.


> OK. But see the core_pc_offset -> core_pc_regno suggestion in the ppc
> case.

This is reworked using 'pc_register', although for s390* in
Ebl_Register_Location instead of Ebl_Core_Item (as for ppc*).


> > --- /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?

Yes, fixed.


> > +  /* 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?

This second block is a leftover, deleted.  Sorry I apparently forgot to
re-test the patch on native s390* after the last changes.

Sorry if obvious but if you ask about the cast it is stated above:
	/* Avoid conversion double -> integer.  */
because:
	/usr/include/sys/user.h
	struct _user_fpregs_struct
	{
	  unsigned int fpc;
	  double fprs[16];
	};


> > --- /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.

OK, good to know that just the reference to GDB is not enough.
Still I believe to _also_ point to GDB is helpful.
In this case it is described what it does by the messages:
	/* Check for 'svc'.  */
	/* Check for 'sigreturn' or 'rt_sigreturn'.  */
I have improved them by:
	/* Check for 'svc' as the first instruction.  */
	/* Check for 'sigreturn' or 'rt_sigreturn' as the second instruction.  */


> > +  /* 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.

I left there the comment, see above.  But I have added some more comments
below.


> > +# 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.

Added this function comment:

/* s390/s390x do not annotate signal handler frame by CFI.  It would be also
   difficult as PC points into a stub built on stock.  Function below is called
   only if unwinder could not find CFI.  Function then verifies the register
   state for this frame really belongs to a signal frame.  In such case it
   fetches original registers saved by the signal frame.  */


> > @@ -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?

Checked in by the mail:
	[obv] Fix forgotten call of ebl_dwarf_to_regno


> >  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?

Currently ebl_unwind is executed only if there is no module or no dwarf or no
cfi range.

For the s390* case it would be enough to execute ebl_unwind only if there is
no module but I find it a bit too narrowly bounded generic code to s390*.

If CFI/DWARF for PC has been found but handle_cfi unwinder failed for it
ebl_unwind should not be executed and it really is not executed.

Do we agree here?


> > +/* 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.

Added:

/* Get previous frame state for an existing frame state.  Method is called only 
   if unwinder could not find CFI for current PC.  PC is for the
   existing frame.  SETFUNC sets register in the previous frame.  GETFUNC gets
[...]


> > --- 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.

Yes.  And the -1 mapping does not make sense, PC is already passed to
ebl/backend.  Therefore also deleted:

libdwfl/frame_unwind.c:
static bool
getfunc (int firstreg, unsigned nregs, Dwarf_Word *regs, void *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++;
-    }
+  assert (firstreg >= 0);

The comment is now:
	/* Callback type for ebl_unwind's parameter getfunc.  */


     /* Callback type for ebl_unwind's parameter readfunc.  */
> > +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.

Copied there the comment with added:
	Method is called only if unwinder could not find CFI for current PC.


Thanks,
Jan
-------------- next part --------------
An embedded message was scrubbed...
From: Jan Kratochvil <jan.kratochvil at redhat.com>
Subject: [PATCH] unwinder: s390 and s390x
Date: Tue, 17 Dec 2013 18:49:54 +0100
Size: 30344
URL: <https://lists.fedorahosted.org/pipermail/elfutils-devel/attachments/20131217/8ab0911e/attachment-0001.mht>


More information about the elfutils-devel mailing list