| Message ID | 1314206507-6487-3-git-send-email-lauraa@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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);
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(-)