[PATCH] Fix show_regs to give a backtrace

[PATCH] Fix show_regs to give a backtrace

[Laura Abbott]

Currently, show_regs will not give a backtrace unless frame pointer is enabled.
show_regs calls __backtrace which does not support the arm unwind tables and
returns nothing if CONFIG_FRAME_POINTER is not enabled. Having a useful
backtrace when dumping registers is helpful, so fix show_regs to call into
dump_backtrace which properly handles both the CONFIG_FRAME_POINTER and the
CONFIG_ARM_UNWIND case for backtraces.

[PATCH 1/2] arm: add dump_stack_regs to dump stack from registers

[Laura Abbott]

Currently, there is no API to be able to dump a stack
from registers. Use dump_backtrace to walk the stack
with a given set of registers.

Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
---
 arch/arm/include/asm/stacktrace.h |    4 ++++
 arch/arm/kernel/traps.c           |    7 +++++++
 2 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/stacktrace.h b/arch/arm/include/asm/stacktrace.h
index 4d0a164..614b017 100644
--- a/arch/arm/include/asm/stacktrace.h
+++ b/arch/arm/include/asm/stacktrace.h
@@ -1,6 +1,8 @@
 #ifndef __ASM_STACKTRACE_H
 #define __ASM_STACKTRACE_H
 
+#include <asm/system.h>
+
 struct stackframe {
 	unsigned long fp;
 	unsigned long sp;
@@ -12,4 +14,6 @@ extern int unwind_frame(struct stackframe *frame);
 extern void walk_stackframe(struct stackframe *frame,
 			    int (*fn)(struct stackframe *, void *), void *data);
 
+void dump_stack_regs(struct pt_regs *regs);
+
 #endif	/* __ASM_STACKTRACE_H */
diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index bc9f9da..983ba41 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -209,6 +209,13 @@ void dump_stack(void)
 
 EXPORT_SYMBOL(dump_stack);
 
+
+void dump_stack_regs(struct pt_regs *regs)
+{
+	dump_backtrace(regs, NULL);
+}
+EXPORT_SYMBOL(dump_stack_regs);
+
 void show_stack(struct task_struct *tsk, unsigned long *sp)
 {
 	dump_backtrace(NULL, tsk);
-- 
1.7.3.3

[PATCH 2/2] process: change from __backtrace to dump_stack_regs in show_regs

[Laura Abbott]

Currently, show_regs calls __backtrace which does
nothing if CONIFG_FRAME_POINTER is not set. Switch to
dump_stack_regs to show the backtrace from a given
set of registers.

Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
---
 arch/arm/kernel/process.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index 1a347f4..6f11213 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -319,7 +319,7 @@ void show_regs(struct pt_regs * regs)
 	printk("\n");
 	printk("Pid: %d, comm: %20s\n", task_pid_nr(current), current->comm);
 	__show_regs(regs);
-	__backtrace();
+	dump_stack_regs(regs);
 }
 
 ATOMIC_NOTIFIER_HEAD(thread_notify_head);
-- 
1.7.3.3

[PATCH 2/2] process: change from __backtrace to dump_stack_regs in show_regs

[Nicolas Pitre]

On Wed, 24 Aug 2011, Laura Abbott wrote:

> Currently, show_regs calls __backtrace which does
> nothing if CONIFG_FRAME_POINTER is not set. Switch to

s/CONIFG/CONFIG/

> dump_stack_regs to show the backtrace from a given
> set of registers.
> 
> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
> ---
>  arch/arm/kernel/process.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
> index 1a347f4..6f11213 100644
> --- a/arch/arm/kernel/process.c
> +++ b/arch/arm/kernel/process.c
> @@ -319,7 +319,7 @@ void show_regs(struct pt_regs * regs)
>  	printk("\n");
>  	printk("Pid: %d, comm: %20s\n", task_pid_nr(current), current->comm);
>  	__show_regs(regs);
> -	__backtrace();
> +	dump_stack_regs(regs);

This is not equivalent.  A call to __backtrace() will display a call 
trace leading to this show_regs().  Given that you defined 
dump_stack_regs(regs) as dump_backtrace(regs, NULL), the trace will 
instead contain calls leading up to the point where those regs were 
saved.

A better way to achieve what you want would be:

- remove the __backtrace entry point from lib/backtrace.S

- define __backtrace() in kernel/traps.c as a call to 
  dump_backtrace(NULL, NULL) which will have the same effect as the
  assembly code for __backtrace().

- realize that dump_stack() is already doing exactly that, therefore...

- use dump_stack() in show_regs() which is the only caller of 
  __backtrace and get rid of __backtrace entirely.

Obviously the above can be collapsed in one patch.


Nicolas

[PATCH 2/2] process: change from __backtrace to dump_stack_regs in show_regs

[Laura Abbott]

On Wed, August 24, 2011 11:57 am, Nicolas Pitre wrote:
> On Wed, 24 Aug 2011, Laura Abbott wrote:
>
>> Currently, show_regs calls __backtrace which does
>> nothing if CONIFG_FRAME_POINTER is not set. Switch to
>
> s/CONIFG/CONFIG/
>
>> dump_stack_regs to show the backtrace from a given
>> set of registers.
>>
>> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
>> ---
>>  arch/arm/kernel/process.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
>> index 1a347f4..6f11213 100644
>> --- a/arch/arm/kernel/process.c
>> +++ b/arch/arm/kernel/process.c
>> @@ -319,7 +319,7 @@ void show_regs(struct pt_regs * regs)
>>  	printk("\n");
>>  	printk("Pid: %d, comm: %20s\n", task_pid_nr(current), current->comm);
>>  	__show_regs(regs);
>> -	__backtrace();
>> +	dump_stack_regs(regs);
>
> This is not equivalent.  A call to __backtrace() will display a call
> trace leading to this show_regs().  Given that you defined
> dump_stack_regs(regs) as dump_backtrace(regs, NULL), the trace will
> instead contain calls leading up to the point where those regs were
> saved.
>

Do other architectures show the call stack to show_regs? From my reading
of other code (x86, ppc) it seems like if show_regs generates a call stack
it generates a call stack based off of the saved registers unless I'm
drastically misunderstanding the code. Generating a call stack based off
of not saved registers seems inconsistent with the rest of show_regs which
shows saved state.

> A better way to achieve what you want would be:
>
> - remove the __backtrace entry point from lib/backtrace.S
>
> - define __backtrace() in kernel/traps.c as a call to
>   dump_backtrace(NULL, NULL) which will have the same effect as the
>   assembly code for __backtrace().
>
> - realize that dump_stack() is already doing exactly that, therefore...
>
> - use dump_stack() in show_regs() which is the only caller of
>   __backtrace and get rid of __backtrace entirely.
>
> Obviously the above can be collapsed in one patch.
>
>
> Nicolas
>

Laura

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 2/2] process: change from __backtrace to dump_stack_regs in show_regs

[Nicolas Pitre]

On Wed, 24 Aug 2011, Laura Abbott wrote:

> 
> On Wed, August 24, 2011 11:57 am, Nicolas Pitre wrote:
> > On Wed, 24 Aug 2011, Laura Abbott wrote:
> >
> >> Currently, show_regs calls __backtrace which does
> >> nothing if CONIFG_FRAME_POINTER is not set. Switch to
> >
> > s/CONIFG/CONFIG/
> >
> >> dump_stack_regs to show the backtrace from a given
> >> set of registers.
> >>
> >> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
> >> ---
> >>  arch/arm/kernel/process.c |    2 +-
> >>  1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
> >> index 1a347f4..6f11213 100644
> >> --- a/arch/arm/kernel/process.c
> >> +++ b/arch/arm/kernel/process.c
> >> @@ -319,7 +319,7 @@ void show_regs(struct pt_regs * regs)
> >>  	printk("\n");
> >>  	printk("Pid: %d, comm: %20s\n", task_pid_nr(current), current->comm);
> >>  	__show_regs(regs);
> >> -	__backtrace();
> >> +	dump_stack_regs(regs);
> >
> > This is not equivalent.  A call to __backtrace() will display a call
> > trace leading to this show_regs().  Given that you defined
> > dump_stack_regs(regs) as dump_backtrace(regs, NULL), the trace will
> > instead contain calls leading up to the point where those regs were
> > saved.
> >
> 
> Do other architectures show the call stack to show_regs? From my reading
> of other code (x86, ppc) it seems like if show_regs generates a call stack
> it generates a call stack based off of the saved registers unless I'm
> drastically misunderstanding the code. Generating a call stack based off
> of not saved registers seems inconsistent with the rest of show_regs which
> shows saved state.

Well, that might be true, and I'm not disputing that.  However this 
change in behavior wasn't mentioned in your patch log.

I'd still recommend making an initial patch the way I outlined so a 
kernel using unwind tables would produce the same trace results as a 
kernel with frame pointers always did.  That part is pretty much 
uncontroversial.

Then it might be good to have a second patch changing the actual trace 
content, and discussing the merits of such a change in the patch log.  
This issue should be independent of the backtrace mechanism used.


Nicolas