Re: [RFC PATCH 10/10] x86/unwind: add undwarf unwinder

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Wed, Jun 14, 2017 at 01:45:41PM +0200, Jiri Slaby wrote:
> On 06/01/2017, 07:44 AM, Josh Poimboeuf wrote:
> > --- /dev/null
> > +++ b/arch/x86/kernel/unwind_undwarf.c
> > @@ -0,0 +1,402 @@
> ...
> > +void __unwind_start(struct unwind_state *state, struct task_struct *task,
> > +		    struct pt_regs *regs, unsigned long *first_frame)
> > +{
> > +	memset(state, 0, sizeof(*state));
> > +	state->task = task;
> > +
> > +	if (regs) {
> > +		if (user_mode(regs)) {
> > +			state->stack_info.type = STACK_TYPE_UNKNOWN;
> > +			return;
> > +		}
> > +
> > +		state->ip = regs->ip;
> > +		state->sp = kernel_stack_pointer(regs);
> > +		state->bp = regs->bp;
> > +		state->regs = regs;
> > +
> > +	} else if (task == current) {
> > +		register void *__sp asm(_ASM_SP);
> > +
> > +		asm volatile("lea (%%rip), %0\n\t"
> > +			     "mov %%rsp, %1\n\t"
> > +			     "mov %%rbp, %2\n\t"
> > +			     : "=r" (state->ip), "=r" (state->sp),
> > +			       "=r" (state->bp), "+r" (__sp));
> 
> Maybe I don't understand this completely, but what is __sp used for here?

This tells gcc "if this function saves the frame pointer, make sure it's
saved before inserting this inline asm."

But on second thought, it shouldn't be needed.  Either way it can use
the undwarf data to find the previous bp.  I'm struggling to remember
why I thought this was needed in the first place...

> > +		state->regs = NULL;
> > +
> > +	} else {
> 
> In DWARF unwinder, we also used to do here:
> 
> +#ifdef CONFIG_SMP
> +       } else if (task->on_cpu) {
> +               return;
> +#endif
>         } else {
> 
> > +		struct inactive_task_frame *frame = (void *)task->thread.sp;
> 
> Since there is no inactive_task_frame for tasks currently running (on
> other CPUs). At least this always held in the past.
> 
> Though, the test is indeed racy.

Yeah, it's indeed racy, but it's probably a good idea to add the check
anyway.  There are other checks to prevent going off the rails, but we
should try to detect it early when we can.  The frame pointer unwinder
could probably use a similar check.

-- 
Josh