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

[Franck Bui-Huu <vagabon.xyz@gmail.com>]

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