Re: [PATCH] x86/dumpstack: Walk frames when built with frame pointers

[Ingo Molnar <mingo@kernel.org>]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> On Sun, Apr 27, 2014 at 02:08:20PM +0200, Ingo Molnar wrote:
> > 
> > * Richard Yao <ryao@gentoo.org> wrote:
> > 
> > > Stack traces are generated by scanning the stack and interpeting 
> > > anything that looks like it could be a pointer to something. We do 
> > > not need to do this when we have frame pointers, but we do it 
> > > anyway, with the distinction that we use the return pointers to mark 
> > > actual frames by the absence of a question mark.
> > > 
> > > The additional verbosity of stack scanning tends to bombard us with 
> > > walls of text for no gain in practice, so lets switch to printing 
> > > only stack frames when frame pointers are available. That we can 
> > > spend less time reading stack traces and more time looking at code.
> > > 
> > > Signed-off-by: Richard Yao <ryao@gentoo.org>
> > > ---
> > >  arch/x86/kernel/dumpstack.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
> > > index d9c12d3..94ffe06 100644
> > > --- a/arch/x86/kernel/dumpstack.c
> > > +++ b/arch/x86/kernel/dumpstack.c
> > > @@ -162,7 +162,11 @@ static void print_trace_address(void *data, unsigned long addr, int reliable)
> > >  static const struct stacktrace_ops print_trace_ops = {
> > >  	.stack			= print_trace_stack,
> > >  	.address		= print_trace_address,
> > > +#ifdef CONFIG_FRAME_POINTER
> > > +	.walk_stack		= print_context_stack_bp,
> > > +#else
> > >  	.walk_stack		= print_context_stack,
> > > +#endif
> > >  };
> 
> Besides the complementary informations brought by the full stack 
> walk, another big argument toward keeping full stack walk is that if 
> your frame pointer is screwed for whatever reason, you still have a 
> useful stack trace.
> 
> I have seen and fixed several broken frame links in x86-64 by the 
> past. Those are very subtle and often hardly visible issues because, 
> if they are easily spotted on common frame scenarios like : task > 
> irq, they are much harder to find on trickier, rarer frame scenarios 
> such as: task -> softirq -> irq -> nmi -> debug exception ->....
> 
> For example before a2bbe75089d5eb9a3a46d50dd5c215e213790288 ("x86: 
> Don't use frame pointer to save old stack on irq entry"), we were 
> missing entire stack frames on nesting irqs (hardirq on softirqs) 
> while using pure frame pointer based unwinding.
> 
> Who knows if we have other remaining issues like this? Especially 
> given the high possible number of frame combinations between task, 
> irq, softirq, nmi and exceptions. Multiply the contexts possibility 
> by the number of possible archs out there and their stack switch 
> implementations.
> 
> Also further frame links breakages, we have many other possibilities 
> to end up with misleading frame pointers. Relying on that source 
> alone definetly reduce the reliability of our stacktraces.
> 
> So this goes way beyond just missing complementary informations. 
> Debugging robustness itself is actually very concerned here if we 
> remove the full stack walk.

Agreed, that's a very good point.

Also, consider the following holistic argument, what is easier to 
achieve, when looking at an oops and not seeing the bug:

  - if only I had more information
  - if only I had less information

we cannot put in information that we cut out, but it's not 
particularly hard to skip overly verbose information in most cases.
 
Yes, there's a line to be drawn with verbosity: scroll-off is a 
concern when the oops does not make it to a log file.

So I don't really know.

Thanks,

	Ingo