[PATCH] dump_stack() based on prologue code analysis (take 2)

[PATCH] dump_stack() based on prologue code analysis (take 2)

[Atsushi Nemoto]

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(&regs);
+		}
+	}
+	show_stacktrace(task, &regs);
 }
 
 /*
@@ -146,6 +212,14 @@ void dump_stack(void)
 {
 	unsigned long stack;
 
+#ifdef CONFIG_KALLSYMS
+	if (!raw_show_trace) {
+		struct pt_regs regs;
+		prepare_frametrace(&regs);
+		show_frametrace(current, &regs);
+		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");
 }

Re: [PATCH] dump_stack() based on prologue code analysis (take 2)

[Franck Bui-Huu]

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(&regs);
-		show_frametrace(current, &regs);
+		show_backtrace(current, &regs);
 		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(&regs);
-		show_frametrace(current, &regs);
+		show_backtrace(current, &regs);
 		return;
 	}
 #endif

Re: [PATCH] dump_stack() based on prologue code analysis (take 2)

[Atsushi Nemoto]

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

Re: [PATCH] dump_stack() based on prologue code analysis (take 2)

[Franck Bui-Huu]

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

Re: [PATCH] dump_stack() based on prologue code analysis (take 2)

[Franck Bui-Huu]

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