Re: [PATCH v2 30/44] x86/unwind: add new unwind interface and implementations

[Josh Poimboeuf]

On Thu, Aug 11, 2016 at 07:58:34AM -0700, Andy Lutomirski wrote:
> On Thu, Aug 11, 2016 at 7:28 AM, Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
> > On Thu, Aug 11, 2016 at 12:18:54AM -0700, Andy Lutomirski wrote:
> >> > > I admit I don't fully understand the use case.  If someone wants to
> >> > > start a trace in the middle, shouldn't they just pass regs in?
> >> >
> >> > The regs aren't always available.  Some callers just want to skip the
> >> > first few frames so the stack dump code itself isn't traced.
> >>
> >> I suspect that all users are okay with your algorithm simply because
> >> they don't switch stacks.  Maybe the thing to do is to stop advancing
> >> when sp is passed or if the stack switches at all.
> >
> > There are actually some cases which could be broken by the sloppy "while
> > state->bp < sp" check.  When starting with sp from 'regs->sp', sp often
> > points to a different stack than the current one.  It seemed to work in
> > my testing, but I guess I got lucky, with irq percpu stack addresses
> > being smaller than the thread stack addresses.
> >
> >> Could you point me at a user that passes anything other than regs->sp
> >> here?  On brief inspection, I haven't found any at all.
> >
> > For example, see show_stack().
> 
> Is that a non-trivial case?  show_stack() is generating sp *and* bp.
> Why not just pass both of them all the way in to show_trace_log_lvl
> (which the existing code already does) and then pass it into
> unwind_start?
> 
> Alternatively, since that risks causing a bit of loss if you implement
> DWARF, you could add an unwind_start_here() function that captures the
> state in the calling function and pass the result all the way through.
> Or you could write a silly asm helper that literally fills in a struct
> pt_regs for the current context (although that could get a bit awkward
> for 32-bit, since pt_regs doesn't really contain sp there).
> 
> If there are genuinely zero non-trivial cases, then this should fully
> solve the problem, no?

Hm, what do you mean by non-trivial cases?

As far as I can tell, they're all trivial, and we don't need the caller
to provide bp.  Just start with the current frame's bp and unwind until
bp and sp are on the same stack and bp > sp.

> Aside: the current code looks a bit silly to me:
> 
>     unsigned long stack;
>     ...
>         sp = &stack;
>         bp = stack_frame(current, NULL);
> 
> Why not just force a frame pointer and read out sp and bp using asm?

Due to the above-discussed algorithm, we no longer need to get bp in
show_stack() (though I forgot to remove the get_frame_pointer() call in
my patches).

And I changed the 'sp = &stack' to a call to get_stack_pointer().

-- 
Josh