* [PATCH] dump_stack() based on prologue code analysis (take 2) @ 2006-07-29 14:27 Atsushi Nemoto 2006-07-31 8:59 ` Franck Bui-Huu 0 siblings, 1 reply; 5+ messages in thread From: Atsushi Nemoto @ 2006-07-29 14:27 UTC (permalink / raw) To: linux-mips; +Cc: ralf, vagabon.xyz Take 2. Reflecting some advices from Franck Bui-Huu. Subject: [PATCH] dump_stack() based on prologue code analysis Instead of dump all possible address in the stack, unwind the stack frame based on prologue code analysis, as like as get_wchan() does. While the code analysis might fail for some reason, there is a new kernel option "raw_show_trace" to disable this feature. Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp> diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c index 7ab67f7..8709a46 100644 --- a/arch/mips/kernel/process.c +++ b/arch/mips/kernel/process.c @@ -281,7 +281,7 @@ static struct mips_frame_info { } *schedule_frame, mfinfo[64]; static int mfinfo_num; -static int __init get_frame_info(struct mips_frame_info *info) +static int get_frame_info(struct mips_frame_info *info) { int i; void *func = info->func; @@ -329,14 +329,12 @@ #endif ip->i_format.simmediate / sizeof(long); } } - if (info->pc_offset == -1 || info->frame_size == 0) { - if (func == schedule) - printk("Can't analyze prologue code at %p\n", func); - info->pc_offset = -1; - info->frame_size = 0; - } - - return 0; + if (info->frame_size && info->pc_offset >= 0) /* nested */ + return 0; + if (info->pc_offset < 0) /* leaf */ + return 1; + /* prologue seems boggus... */ + return -1; } static int __init frame_info_init(void) @@ -367,8 +365,15 @@ #else mfinfo[0].func = schedule; schedule_frame = &mfinfo[0]; #endif - for (i = 0; i < ARRAY_SIZE(mfinfo) && mfinfo[i].func; i++) - get_frame_info(&mfinfo[i]); + for (i = 0; i < ARRAY_SIZE(mfinfo) && mfinfo[i].func; i++) { + struct mips_frame_info *info = &mfinfo[i]; + if (get_frame_info(info)) { + /* leaf or unknown */ + if (info->func == schedule) + printk("Can't analyze prologue code at %p\n", + info->func); + } + } mfinfo_num = i; return 0; @@ -427,6 +432,8 @@ #ifdef CONFIG_KALLSYMS if (i < 0) break; + if (mfinfo[i].pc_offset < 0) + break; pc = ((unsigned long *)frame)[mfinfo[i].pc_offset]; if (!mfinfo[i].frame_size) break; @@ -437,3 +444,40 @@ #endif return pc; } +#ifdef CONFIG_KALLSYMS +/* used by show_frametrace() */ +unsigned long unwind_stack(struct task_struct *task, + unsigned long **sp, unsigned long pc) +{ + unsigned long stack_page; + struct mips_frame_info info; + char *modname; + char namebuf[KSYM_NAME_LEN + 1]; + unsigned long size, ofs; + + stack_page = (unsigned long)task_stack_page(task); + if (!stack_page) + return 0; + + if (!kallsyms_lookup(pc, &size, &ofs, &modname, namebuf)) + return 0; + if (ofs == 0) + return 0; + + info.func = (void *)(pc - ofs); + info.func_size = ofs; /* analyze from start to ofs */ + if (get_frame_info(&info)) { + /* leaf or unknown */ + *sp += info.frame_size / sizeof(long); + return 0; + } + if ((unsigned long)*sp < stack_page || + (unsigned long)*sp + info.frame_size / sizeof(long) > + stack_page + THREAD_SIZE - 32) + return 0; + + pc = (*sp)[info.pc_offset]; + *sp += info.frame_size / sizeof(long); + return pc; +} +#endif diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c index c6f7046..7aa9dfc 100644 --- a/arch/mips/kernel/traps.c +++ b/arch/mips/kernel/traps.c @@ -98,24 +98,53 @@ #endif printk("\n"); } +#ifdef CONFIG_KALLSYMS +static int raw_show_trace; +static int __init set_raw_show_trace(char *str) +{ + raw_show_trace = 1; + return 1; +} +__setup("raw_show_trace", set_raw_show_trace); + +extern unsigned long unwind_stack(struct task_struct *task, + unsigned long **sp, unsigned long pc); +static void show_frametrace(struct task_struct *task, struct pt_regs *regs) +{ + const int field = 2 * sizeof(unsigned long); + unsigned long *stack = (long *)regs->regs[29]; + unsigned long pc = regs->cp0_epc; + int top = 1; + + if (raw_show_trace || !__kernel_text_address(pc)) { + show_trace(stack); + return; + } + printk("Call Trace:\n"); + while (__kernel_text_address(pc)) { + printk(" [<%0*lx>] ", field, pc); + print_symbol("%s\n", pc); + pc = unwind_stack(task, &stack, pc); + if (top && pc == 0) + pc = regs->regs[31]; /* leaf? */ + top = 0; + } + printk("\n"); +} +#else +#define show_frametrace(task, r) show_trace((long *)(r)->regs[29]); +#endif + /* * This routine abuses get_user()/put_user() to reference pointers * with at least a bit of error checking ... */ -void show_stack(struct task_struct *task, unsigned long *sp) +static void show_stacktrace(struct task_struct *task, struct pt_regs *regs) { const int field = 2 * sizeof(unsigned long); long stackdata; int i; - unsigned long *stack; - - if (!sp) { - if (task && task != current) - sp = (unsigned long *) task->thread.reg29; - else - sp = (unsigned long *) &sp; - } - stack = sp; + unsigned long *sp = (unsigned long *)regs->regs[29]; printk("Stack :"); i = 0; @@ -136,7 +165,44 @@ void show_stack(struct task_struct *task i++; } printk("\n"); - show_trace(stack); + show_frametrace(task, regs); +} + +static noinline void prepare_frametrace(struct pt_regs *regs) +{ + __asm__ __volatile__( + "1: la $2, 1b\n\t" +#ifdef CONFIG_64BIT + "sd $2, %0\n\t" + "sd $29, %1\n\t" + "sd $31, %2\n\t" +#else + "sw $2, %0\n\t" + "sw $29, %1\n\t" + "sw $31, %2\n\t" +#endif + : "=m" (regs->cp0_epc), + "=m" (regs->regs[29]), "=m" (regs->regs[31]) + : : "memory"); +} + +void show_stack(struct task_struct *task, unsigned long *sp) +{ + struct pt_regs regs; + if (sp) { + regs.regs[29] = (unsigned long)sp; + regs.regs[31] = 0; + regs.cp0_epc = 0; + } else { + if (task && task != current) { + regs.regs[29] = task->thread.reg29; + regs.regs[31] = 0; + regs.cp0_epc = task->thread.reg31; + } else { + prepare_frametrace(®s); + } + } + show_stacktrace(task, ®s); } /* @@ -146,6 +212,14 @@ void dump_stack(void) { unsigned long stack; +#ifdef CONFIG_KALLSYMS + if (!raw_show_trace) { + struct pt_regs regs; + prepare_frametrace(®s); + show_frametrace(current, ®s); + return; + } +#endif show_trace(&stack); } @@ -265,7 +339,7 @@ void show_registers(struct pt_regs *regs print_modules(); printk("Process %s (pid: %d, threadinfo=%p, task=%p)\n", current->comm, current->pid, current_thread_info(), current); - show_stack(current, (long *) regs->regs[29]); + show_stacktrace(current, regs); show_code((unsigned int *) regs->cp0_epc); printk("\n"); } ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] dump_stack() based on prologue code analysis (take 2) 2006-07-29 14:27 [PATCH] dump_stack() based on prologue code analysis (take 2) Atsushi Nemoto @ 2006-07-31 8:59 ` Franck Bui-Huu 2006-07-31 14:56 ` Atsushi Nemoto 0 siblings, 1 reply; 5+ messages in thread From: Franck Bui-Huu @ 2006-07-31 8:59 UTC (permalink / raw) To: Atsushi Nemoto; +Cc: linux-mips, ralf, vagabon.xyz Atsushi Nemoto wrote: > > Subject: [PATCH] dump_stack() based on prologue code analysis > > Instead of dump all possible address in the stack, unwind the stack > frame based on prologue code analysis, as like as get_wchan() does. > While the code analysis might fail for some reason, there is a new > kernel option "raw_show_trace" to disable this feature. > my comments included with this patch...(you can find the plain patch at the end of this email) diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c index 8709a46..3bb4d47 100644 --- a/arch/mips/kernel/process.c +++ b/arch/mips/kernel/process.c @@ -365,15 +365,15 @@ #else mfinfo[0].func = schedule; schedule_frame = &mfinfo[0]; #endif - for (i = 0; i < ARRAY_SIZE(mfinfo) && mfinfo[i].func; i++) { - struct mips_frame_info *info = &mfinfo[i]; - if (get_frame_info(info)) { - /* leaf or unknown */ - if (info->func == schedule) - printk("Can't analyze prologue code at %p\n", - info->func); - } - } + for (i = 0; i < ARRAY_SIZE(mfinfo) && mfinfo[i].func; i++) + get_frame_info(mfinfo + i); + + /* + * Without schedule() frame info, result given by + * thread_saved_pc() and get_wchan() are not reliable. + */ + if (schedule_frame->pc_offset < 0) + printk("Can't analyze schedule() prologue at %p\n", schedule); >>>>>>>>>>>>> I just put the test out of the loop and add a comment. <<<<<<<<<<<<< mfinfo_num = i; return 0; @@ -446,14 +446,15 @@ #endif #ifdef CONFIG_KALLSYMS /* used by show_frametrace() */ -unsigned long unwind_stack(struct task_struct *task, - unsigned long **sp, unsigned long pc) +unsigned long unwind_stack(struct task_struct *task, unsigned long **sp, + unsigned long pc, struct pt_regs *regs) { unsigned long stack_page; struct mips_frame_info info; char *modname; char namebuf[KSYM_NAME_LEN + 1]; unsigned long size, ofs; + int rv; stack_page = (unsigned long)task_stack_page(task); if (!stack_page) @@ -466,18 +467,21 @@ unsigned long unwind_stack(struct task_s info.func = (void *)(pc - ofs); info.func_size = ofs; /* analyze from start to ofs */ - if (get_frame_info(&info)) { - /* leaf or unknown */ - *sp += info.frame_size / sizeof(long); + rv = get_frame_info(&info); + if (rv < 0) return 0; - } + if ((unsigned long)*sp < stack_page || (unsigned long)*sp + info.frame_size / sizeof(long) > stack_page + THREAD_SIZE - 32) return 0; - pc = (*sp)[info.pc_offset]; + if (rv) /* leaf */ + pc = regs->regs[31]; + else /* nested */ + pc = (*sp)[info.pc_offset]; + *sp += info.frame_size / sizeof(long); - return pc; + return __kernel_text_address(pc) ? pc : 0; >>>>>>>>>>>>> I pass regs to unwind_stack(), that simplify the caller because it needn't to deal with leaf or nested case. Simply test for pc is 0. <<<<<<<<<<<<< } #endif diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c index 7aa9dfc..bbd1cf9 100644 --- a/arch/mips/kernel/traps.c +++ b/arch/mips/kernel/traps.c @@ -73,13 +73,8 @@ void (*board_nmi_handler_setup)(void); void (*board_ejtag_handler_setup)(void); void (*board_bind_eic_interrupt)(int irq, int regset); -/* - * These constant is for searching for possible module text segments. - * MODULE_RANGE is a guess of how much space is likely to be vmalloced. - */ -#define MODULE_RANGE (8*1024*1024) >>>>>>>>>>>>> seems to be unused now... <<<<<<<<<<<<< -static void show_trace(unsigned long *stack) +static void show_trace(unsigned long *sp) { const int field = 2 * sizeof(unsigned long); unsigned long addr; @@ -88,8 +83,8 @@ static void show_trace(unsigned long *st #ifdef CONFIG_KALLSYMS printk("\n"); #endif - while (!kstack_end(stack)) { - addr = *stack++; + while (!kstack_end(sp)) { + addr = *sp++; >>>>>>>>>>>>> now show_trace calls sp sp. (nothing is too late) <<<<<<<<<<<<< if (__kernel_text_address(addr)) { printk(" [<%0*lx>] ", field, addr); print_symbol("%s\n", addr); @@ -107,32 +102,27 @@ static int __init set_raw_show_trace(cha } __setup("raw_show_trace", set_raw_show_trace); -extern unsigned long unwind_stack(struct task_struct *task, - unsigned long **sp, unsigned long pc); -static void show_frametrace(struct task_struct *task, struct pt_regs *regs) +extern unsigned long unwind_stack(struct task_struct *task, unsigned long **sp, + unsigned long pc, struct pt_regs *regs); + +static void show_backtrace(struct task_struct *task, struct pt_regs *regs) >>>>>>>>>>>>> Just renamed show_stacktrace() into show_backtrace(). I think we usually use the latter no ? <<<<<<<<<<<<< { - const int field = 2 * sizeof(unsigned long); - unsigned long *stack = (long *)regs->regs[29]; + unsigned long *sp = (long *)regs->regs[29]; unsigned long pc = regs->cp0_epc; - int top = 1; if (raw_show_trace || !__kernel_text_address(pc)) { - show_trace(stack); + show_trace(sp); return; } printk("Call Trace:\n"); - while (__kernel_text_address(pc)) { - printk(" [<%0*lx>] ", field, pc); + do { + printk(" [<%0*lx>] ", 2*sizeof(unsigned long), pc); print_symbol("%s\n", pc); - pc = unwind_stack(task, &stack, pc); - if (top && pc == 0) - pc = regs->regs[31]; /* leaf? */ - top = 0; - } + } while ((pc = unwind_stack(task, &sp, pc, regs))); >>>>>>>>>>>>> Now don't deal with leaf case since unwind_stack() does it for us. <<<<<<<<<<<<< printk("\n"); } #else -#define show_frametrace(task, r) show_trace((long *)(r)->regs[29]); +#define show_backtrace(task, r) show_trace((long *)(r)->regs[29]); #endif /* @@ -165,7 +155,7 @@ static void show_stacktrace(struct task_ i++; } printk("\n"); - show_frametrace(task, regs); + show_backtrace(task, regs); } static noinline void prepare_frametrace(struct pt_regs *regs) @@ -216,7 +206,7 @@ #ifdef CONFIG_KALLSYMS if (!raw_show_trace) { struct pt_regs regs; prepare_frametrace(®s); - show_frametrace(current, ®s); + show_backtrace(current, ®s); return; } #endif Here is the plain patch. -- >8 -- diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c index 8709a46..3bb4d47 100644 --- a/arch/mips/kernel/process.c +++ b/arch/mips/kernel/process.c @@ -365,15 +365,15 @@ #else mfinfo[0].func = schedule; schedule_frame = &mfinfo[0]; #endif - for (i = 0; i < ARRAY_SIZE(mfinfo) && mfinfo[i].func; i++) { - struct mips_frame_info *info = &mfinfo[i]; - if (get_frame_info(info)) { - /* leaf or unknown */ - if (info->func == schedule) - printk("Can't analyze prologue code at %p\n", - info->func); - } - } + for (i = 0; i < ARRAY_SIZE(mfinfo) && mfinfo[i].func; i++) + get_frame_info(mfinfo + i); + + /* + * Without schedule() frame info, result given by + * thread_saved_pc() and get_wchan() are not reliable. + */ + if (schedule_frame->pc_offset < 0) + printk("Can't analyze schedule() prologue at %p\n", schedule); mfinfo_num = i; return 0; @@ -446,14 +446,15 @@ #endif #ifdef CONFIG_KALLSYMS /* used by show_frametrace() */ -unsigned long unwind_stack(struct task_struct *task, - unsigned long **sp, unsigned long pc) +unsigned long unwind_stack(struct task_struct *task, unsigned long **sp, + unsigned long pc, struct pt_regs *regs) { unsigned long stack_page; struct mips_frame_info info; char *modname; char namebuf[KSYM_NAME_LEN + 1]; unsigned long size, ofs; + int rv; stack_page = (unsigned long)task_stack_page(task); if (!stack_page) @@ -466,18 +467,21 @@ unsigned long unwind_stack(struct task_s info.func = (void *)(pc - ofs); info.func_size = ofs; /* analyze from start to ofs */ - if (get_frame_info(&info)) { - /* leaf or unknown */ - *sp += info.frame_size / sizeof(long); + rv = get_frame_info(&info); + if (rv < 0) return 0; - } + if ((unsigned long)*sp < stack_page || (unsigned long)*sp + info.frame_size / sizeof(long) > stack_page + THREAD_SIZE - 32) return 0; - pc = (*sp)[info.pc_offset]; + if (rv) /* leaf */ + pc = regs->regs[31]; + else /* nested */ + pc = (*sp)[info.pc_offset]; + *sp += info.frame_size / sizeof(long); - return pc; + return __kernel_text_address(pc) ? pc : 0; } #endif diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c index 7aa9dfc..bbd1cf9 100644 --- a/arch/mips/kernel/traps.c +++ b/arch/mips/kernel/traps.c @@ -73,13 +73,8 @@ void (*board_nmi_handler_setup)(void); void (*board_ejtag_handler_setup)(void); void (*board_bind_eic_interrupt)(int irq, int regset); -/* - * These constant is for searching for possible module text segments. - * MODULE_RANGE is a guess of how much space is likely to be vmalloced. - */ -#define MODULE_RANGE (8*1024*1024) -static void show_trace(unsigned long *stack) +static void show_trace(unsigned long *sp) { const int field = 2 * sizeof(unsigned long); unsigned long addr; @@ -88,8 +83,8 @@ static void show_trace(unsigned long *st #ifdef CONFIG_KALLSYMS printk("\n"); #endif - while (!kstack_end(stack)) { - addr = *stack++; + while (!kstack_end(sp)) { + addr = *sp++; if (__kernel_text_address(addr)) { printk(" [<%0*lx>] ", field, addr); print_symbol("%s\n", addr); @@ -107,32 +102,27 @@ static int __init set_raw_show_trace(cha } __setup("raw_show_trace", set_raw_show_trace); -extern unsigned long unwind_stack(struct task_struct *task, - unsigned long **sp, unsigned long pc); -static void show_frametrace(struct task_struct *task, struct pt_regs *regs) +extern unsigned long unwind_stack(struct task_struct *task, unsigned long **sp, + unsigned long pc, struct pt_regs *regs); + +static void show_backtrace(struct task_struct *task, struct pt_regs *regs) { - const int field = 2 * sizeof(unsigned long); - unsigned long *stack = (long *)regs->regs[29]; + unsigned long *sp = (long *)regs->regs[29]; unsigned long pc = regs->cp0_epc; - int top = 1; if (raw_show_trace || !__kernel_text_address(pc)) { - show_trace(stack); + show_trace(sp); return; } printk("Call Trace:\n"); - while (__kernel_text_address(pc)) { - printk(" [<%0*lx>] ", field, pc); + do { + printk(" [<%0*lx>] ", 2*sizeof(unsigned long), pc); print_symbol("%s\n", pc); - pc = unwind_stack(task, &stack, pc); - if (top && pc == 0) - pc = regs->regs[31]; /* leaf? */ - top = 0; - } + } while ((pc = unwind_stack(task, &sp, pc, regs))); printk("\n"); } #else -#define show_frametrace(task, r) show_trace((long *)(r)->regs[29]); +#define show_backtrace(task, r) show_trace((long *)(r)->regs[29]); #endif /* @@ -165,7 +155,7 @@ static void show_stacktrace(struct task_ i++; } printk("\n"); - show_frametrace(task, regs); + show_backtrace(task, regs); } static noinline void prepare_frametrace(struct pt_regs *regs) @@ -216,7 +206,7 @@ #ifdef CONFIG_KALLSYMS if (!raw_show_trace) { struct pt_regs regs; prepare_frametrace(®s); - show_frametrace(current, ®s); + show_backtrace(current, ®s); return; } #endif ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] dump_stack() based on prologue code analysis (take 2) 2006-07-31 8:59 ` Franck Bui-Huu @ 2006-07-31 14:56 ` Atsushi Nemoto 2006-07-31 16:30 ` Franck Bui-Huu 2006-08-01 8:37 ` Franck Bui-Huu 0 siblings, 2 replies; 5+ messages in thread From: Atsushi Nemoto @ 2006-07-31 14:56 UTC (permalink / raw) To: vagabon.xyz; +Cc: linux-mips, ralf On Mon, 31 Jul 2006 10:59:03 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote: > my comments included with this patch...(you can find the plain patch > at the end of this email) > > diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c > index 8709a46..3bb4d47 100644 > --- a/arch/mips/kernel/process.c > +++ b/arch/mips/kernel/process.c > @@ -365,15 +365,15 @@ #else > mfinfo[0].func = schedule; > schedule_frame = &mfinfo[0]; > #endif > - for (i = 0; i < ARRAY_SIZE(mfinfo) && mfinfo[i].func; i++) { > - struct mips_frame_info *info = &mfinfo[i]; > - if (get_frame_info(info)) { > - /* leaf or unknown */ > - if (info->func == schedule) > - printk("Can't analyze prologue code at %p\n", > - info->func); > - } > - } > + for (i = 0; i < ARRAY_SIZE(mfinfo) && mfinfo[i].func; i++) > + get_frame_info(mfinfo + i); > + > + /* > + * Without schedule() frame info, result given by > + * thread_saved_pc() and get_wchan() are not reliable. > + */ > + if (schedule_frame->pc_offset < 0) > + printk("Can't analyze schedule() prologue at %p\n", schedule); > > >>>>>>>>>>>>> > I just put the test out of the loop and add a comment. > <<<<<<<<<<<<< Looks good. > @@ -466,18 +467,21 @@ unsigned long unwind_stack(struct task_s > > info.func = (void *)(pc - ofs); > info.func_size = ofs; /* analyze from start to ofs */ > - if (get_frame_info(&info)) { > - /* leaf or unknown */ > - *sp += info.frame_size / sizeof(long); > + rv = get_frame_info(&info); > + if (rv < 0) > return 0; > - } > + > if ((unsigned long)*sp < stack_page || > (unsigned long)*sp + info.frame_size / sizeof(long) > > stack_page + THREAD_SIZE - 32) > return 0; > > - pc = (*sp)[info.pc_offset]; > + if (rv) /* leaf */ > + pc = regs->regs[31]; > + else /* nested */ > + pc = (*sp)[info.pc_offset]; > + > *sp += info.frame_size / sizeof(long); > - return pc; > + return __kernel_text_address(pc) ? pc : 0; > > >>>>>>>>>>>>> > I pass regs to unwind_stack(), that simplify the caller because > it needn't to deal with leaf or nested case. Simply test for pc > is 0. > <<<<<<<<<<<<< It seems a bit fragile. The regs->regs[31] can be used for top of stack, but we should consider that get_frame_info() might return wrong result (again, get_frame_info() is not perfect). If get_frame_info() returned 0 on middle level of the stack, taking regs->regs[31] leads wrong trace. Maybe you can use NULL value as regs for non-toplevel. > diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c > index 7aa9dfc..bbd1cf9 100644 > --- a/arch/mips/kernel/traps.c > +++ b/arch/mips/kernel/traps.c > @@ -73,13 +73,8 @@ void (*board_nmi_handler_setup)(void); > void (*board_ejtag_handler_setup)(void); > void (*board_bind_eic_interrupt)(int irq, int regset); > > -/* > - * These constant is for searching for possible module text segments. > - * MODULE_RANGE is a guess of how much space is likely to be vmalloced. > - */ > -#define MODULE_RANGE (8*1024*1024) > > >>>>>>>>>>>>> > seems to be unused now... > <<<<<<<<<<<<< This is irrelevant. It would be better to make another patch. > -static void show_trace(unsigned long *stack) > +static void show_trace(unsigned long *sp) > { > const int field = 2 * sizeof(unsigned long); > unsigned long addr; > @@ -88,8 +83,8 @@ static void show_trace(unsigned long *st > #ifdef CONFIG_KALLSYMS > printk("\n"); > #endif > - while (!kstack_end(stack)) { > - addr = *stack++; > + while (!kstack_end(sp)) { > + addr = *sp++; > > >>>>>>>>>>>>> > now show_trace calls sp sp. (nothing is too late) > <<<<<<<<<<<<< I feel "stack" is better than "sp" in this case, but do not object to this change. > @@ -107,32 +102,27 @@ static int __init set_raw_show_trace(cha > } > __setup("raw_show_trace", set_raw_show_trace); > > -extern unsigned long unwind_stack(struct task_struct *task, > - unsigned long **sp, unsigned long pc); > -static void show_frametrace(struct task_struct *task, struct pt_regs *regs) > +extern unsigned long unwind_stack(struct task_struct *task, unsigned long **sp, > + unsigned long pc, struct pt_regs *regs); > + > +static void show_backtrace(struct task_struct *task, struct pt_regs *regs) > > >>>>>>>>>>>>> > Just renamed show_stacktrace() into show_backtrace(). I think we > usually use the latter no ? > <<<<<<<<<<<<< No objection. > { > - const int field = 2 * sizeof(unsigned long); > - unsigned long *stack = (long *)regs->regs[29]; > + unsigned long *sp = (long *)regs->regs[29]; > unsigned long pc = regs->cp0_epc; > - int top = 1; > > if (raw_show_trace || !__kernel_text_address(pc)) { > - show_trace(stack); > + show_trace(sp); > return; > } > printk("Call Trace:\n"); > - while (__kernel_text_address(pc)) { > - printk(" [<%0*lx>] ", field, pc); > + do { > + printk(" [<%0*lx>] ", 2*sizeof(unsigned long), pc); > print_symbol("%s\n", pc); > - pc = unwind_stack(task, &stack, pc); > - if (top && pc == 0) > - pc = regs->regs[31]; /* leaf? */ > - top = 0; > - } > + } while ((pc = unwind_stack(task, &sp, pc, regs))); > > >>>>>>>>>>>>> > Now don't deal with leaf case since unwind_stack() does it for us. > <<<<<<<<<<<<< As I wrote above, passing "regs" for all level seems not appropriate. --- Atsushi Nemoto ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] dump_stack() based on prologue code analysis (take 2) 2006-07-31 14:56 ` Atsushi Nemoto @ 2006-07-31 16:30 ` Franck Bui-Huu 2006-08-01 8:37 ` Franck Bui-Huu 1 sibling, 0 replies; 5+ messages in thread From: Franck Bui-Huu @ 2006-07-31 16:30 UTC (permalink / raw) To: Atsushi Nemoto; +Cc: vagabon.xyz, linux-mips, ralf Atsushi Nemoto wrote: > On Mon, 31 Jul 2006 10:59:03 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote: >> my comments included with this patch...(you can find the plain patch >> at the end of this email) >> This time your patch a _really_ been commited. So there won't be a take 3. I'll start a new thread including my comments I sent tomorrow. Thanks. Franck ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] dump_stack() based on prologue code analysis (take 2) 2006-07-31 14:56 ` Atsushi Nemoto 2006-07-31 16:30 ` Franck Bui-Huu @ 2006-08-01 8:37 ` Franck Bui-Huu 1 sibling, 0 replies; 5+ messages in thread From: Franck Bui-Huu @ 2006-08-01 8:37 UTC (permalink / raw) To: Atsushi Nemoto; +Cc: vagabon.xyz, linux-mips, ralf Atsushi Nemoto wrote: > On Mon, 31 Jul 2006 10:59:03 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote: >> >> I pass regs to unwind_stack(), that simplify the caller because >> it needn't to deal with leaf or nested case. Simply test for pc >> is 0. > > It seems a bit fragile. The regs->regs[31] can be used for top of > stack, but we should consider that get_frame_info() might return wrong > result (again, get_frame_info() is not perfect). If get_frame_info() > returned 0 on middle level of the stack, taking regs->regs[31] leads > wrong trace. Maybe you can use NULL value as regs for non-toplevel. > Yes get_frame_info() is not perfect in sense where it can't analyses _all_ possible frames. But it should be able to detect these case at least. Franck ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2006-08-01 8:38 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2006-07-29 14:27 [PATCH] dump_stack() based on prologue code analysis (take 2) Atsushi Nemoto 2006-07-31 8:59 ` Franck Bui-Huu 2006-07-31 14:56 ` Atsushi Nemoto 2006-07-31 16:30 ` Franck Bui-Huu 2006-08-01 8:37 ` Franck Bui-Huu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox