Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1665728 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC PATCH 10/10] x86/unwind: add undwarf unwinder Jiri Slaby <jslaby@suse.cz> - 2017-06-14 13:50 +0200
    Re: [RFC PATCH 10/10] x86/unwind: add undwarf unwinder Josh Poimboeuf <jpoimboe@redhat.com> - 2017-06-14 15:50 +0200

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

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?

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

> +		state->ip = frame->ret_addr;
> +		state->sp = task->thread.sp;
> +		state->bp = frame->bp;
> +		state->regs = NULL;
> +	}

thanks,
-- 
js
suse labs

[toc] | [next] | [standalone]

#1665849

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web