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

[Mark Wielaard <mjw at redhat dot com>]

On Tue, 2013-12-17 at 19:05 +0100, Jan Kratochvil wrote:
> 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.

Aha. And svc is how one enters the kernel and 119 is the syscall number
for sigreturn on s390 (and 173 rt_sigreturn) I see. And sigreturn is
magic inserted by the kernel as return call frame. Tricky indeed...

> I did not file it anywhere, though.

I would have expected this tricky "frame" to be part of the vdso and so
that should carry the CFI, but apparently it is more tricky than that.

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

Thanks, that explains it nicely.
Little typo: s/stock/stack/

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

Yes. And your hook documentation now explains it nicely.

> > backends/
> > 2013-12-17  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	unwinder: s390 and s390x
> > 	* Makefile.am (s390_SRCS): Add s390_initreg.c and s390_unwind.c.
> > 	* s390_corenote.c (prstatus_regs): Set PC_REGISTER.  Reindent all the
> > 	entries.
> > 	* s390_init.c (s390_init): Initialize frame_nregs,
> > 	set_initial_registers_tid, normalize_pc and unwind.
> > 	* s390_initreg.c: New file.
> > 	* s390_unwind.c: New file.
> > 
> > libdwfl/
> > 2013-12-17  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.
> > 	(setfunc, getfunc, readfunc): New functions.
> > 	(__libdwfl_frame_unwind): Call ebl_unwind with those functions.
> > 	* linux-core-attach.c (core_set_initial_registers): Always iterate
> > 	through the Ebl_Register_Location loop.  Call
> > 	dwfl_thread_state_register_pc there.
> > 
> > libebl/
> > 2013-12-17  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_Register_Location): Add field pc_register.
> > 	(ebl_normalize_pc): New declaration.
> > 	(ebl_tid_registers_get_t, ebl_pid_memory_read_t): New definitions.
> > 	(ebl_unwind): New declaration.
> > 
> > tests/
> > 2013-12-17  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	unwinder: s390 and s390x
> > 	* Makefile.am (TESTS): Add run-backtrace-core-s390x.sh and
> > 	run-backtrace-core-s390.sh.
> > 	(EXTRA_DIST): Add backtrace.s390x.core.bz2, backtrace.s390x.exec.bz2,
> > 	backtrace.s390.core.bz2, backtrace.s390.exec.bz2,
> > 	run-backtrace-core-s390x.sh and run-backtrace-core-s390.sh.
> > 	* 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-core-s390.sh: New file.
> > 	* run-backtrace-core-s390x.sh: New file.

Looks good to me.

Thanks,

Mark