Skip to contentSkip to conversation

Smarter stack traces using the frame pointer

RFC In review Adam Litke 3 participants 6 messages Nov 4, 2003 – Nov 10, 2003

lore
Share & tools
lore

Adam Litke

author
#0 lore raw

RFC 1/1 Smarter stack traces using the frame pointer

I was working on this for the mjb tree but I thought I'd throw it out here in case anyone else finds it interesting. This simple change to the stack trace code uses the frame pointer to do the trace instead of assuming any kernel address on the stack is a return address. It makes for much more readable stack traces.

arch/i386/kernel/traps.c +38 -6
diff -purN linux-2.6.0-test9-virgin/arch/i386/kernel/traps.c linux-2.6.0-test9-stack/arch/i386/kernel/traps.c
@@ -93,17 +93,36 @@ asmlinkage void machine_check(void);
 
 static int kstack_depth_to_print = 24;
 
-void show_trace(struct task_struct *task, unsigned long * stack)
+#ifdef CONFIG_FRAME_POINTER
+void show_trace_fp(struct task_struct *task)
+{
+	unsigned long addr, ebp;
+
+	if (task == current) {
+		/* Grab ebp right from our regs */
+		asm ("movl %%ebp, %0" : "=r" (ebp) : );
+	} else {
+		/* ebp is the last reg pushed by switch_to */
+		ebp = *(unsigned long *) task->thread.esp;
+	}
+	
+	while (ebp) {
+		addr = *(unsigned long *) (ebp + 4);
+		if (!kernel_text_address(addr))
+			return;
+		printk(" [<%08lx>] ", addr);
+		print_symbol("%s\n", addr);
+		ebp = *(unsigned long *) ebp;
+	}
+}
+#else
+void show_trace_guess(unsigned long * stack)
 {
 	unsigned long addr;
 
 	if (!stack)
 		stack = (unsigned long*)&stack;
 
-	printk("Call Trace:");
-#ifdef CONFIG_KALLSYMS
-	printk("\n");
-#endif
 	while (!kstack_end(stack)) {
 		addr = *stack++;
 		if (kernel_text_address(addr)) {

@@ -111,6 +130,20 @@ void show_trace(struct task_struct *task
 			print_symbol("%s\n", addr);
 		}
 	}
+}
+#endif
+
+void show_trace(struct task_struct *task, unsigned long * stack)
+{
+	printk("Call Trace:");
+#ifdef CONFIG_KALLSYMS
+	printk("\n");
+#endif
+#ifdef CONFIG_FRAME_POINTER
+	show_trace_fp(task);
+#else
+	show_trace_guess(stack);
+#endif
 	printk("\n");
 } 
--

Adam Litke (agl at us.ibm.com)
IBM Linux Technology Center
(503) 578 - 3283 t/l 775 - 3283

#1 lore raw
Quoted 6 lines · On 04 Nov 2003 14:13:49 -0800 Adam Litke <[email protected]> wrote:

I was working on this for the mjb tree but I thought I'd throw it out here in case anyone else finds it interesting. This simple change to the stack trace code uses the frame pointer to do the trace instead of assuming any kernel address on the stack is a return address. It makes for much more readable stack traces.

I was asked by a colleague to do the same thing and came up with this alternative version which, I think, gets it mostly right - I am not sure about the stack trace from an OOPS.

Don't bother to tell me this is a bit of a hack - I know :-)

Patch relative to 2.6.0-test9.

Signature · 139 lines
Cheers,
Stephen Rothwell                    [email protected]
http://www.canb.auug.org.au/~sfr/

diff -ruN 2.6.0-test9/arch/i386/kernel/process.c 2.6.0-test9-sfr.1/arch/i386/kernel/process.c
--- 2.6.0-test9/arch/i386/kernel/process.c	2003-10-09 10:22:41.000000000 +1000
+++ 2.6.0-test9-sfr.1/arch/i386/kernel/process.c	2003-10-28 17:00:40.000000000 +1100
@@ -243,7 +243,7 @@
 		".previous			\n"
 		: "=r" (cr4): "0" (0));
 	printk("CR0: %08lx CR2: %08lx CR3: %08lx CR4: %08lx\n", cr0, cr2, cr3, cr4);
-	show_trace(NULL, &regs->esp);
+	show_trace(&regs->esp, (unsigned long *)regs->ebp);
 }
 
 /*
diff -ruN 2.6.0-test9/arch/i386/kernel/traps.c 2.6.0-test9-sfr.1/arch/i386/kernel/traps.c
--- 2.6.0-test9/arch/i386/kernel/traps.c	2003-10-20 11:35:54.000000000 +1000
+++ 2.6.0-test9-sfr.1/arch/i386/kernel/traps.c	2003-10-29 15:45:50.000000000 +1100
@@ -91,9 +91,11 @@
 asmlinkage void spurious_interrupt_bug(void);
 asmlinkage void machine_check(void);
 
+#define DECLARE_EBP	register unsigned long *ebp asm("ebp")
+
 static int kstack_depth_to_print = 24;
 
-void show_trace(struct task_struct *task, unsigned long * stack)
+void show_trace(unsigned long *stack, unsigned long *frame)
 {
 	unsigned long addr;
 
@@ -106,10 +108,24 @@
 #endif
 	while (!kstack_end(stack)) {
 		addr = *stack++;
-		if (kernel_text_address(addr)) {
+		if (
+#ifdef CONFIG_FRAME_POINTER
+			/*
+			 * The plus 2 is because the return address
+			 * is immediately above where the frame pointer 
+			 * points and we incremented the stack pointer
+			 * above ...
+			 */
+			(!frame || (stack == (frame + 2))) &&
+#endif
+			kernel_text_address(addr)) {
 			printk(" [<%08lx>] ", addr);
 			print_symbol("%s\n", addr);
 		}
+#ifdef CONFIG_FRAME_POINTER
+		if (frame && (stack == (frame + 2)))
+			frame = (unsigned long *)*frame;
+#endif
 	}
 	printk("\n");
 }
@@ -121,19 +137,45 @@
 	/* User space on another CPU? */
 	if ((esp ^ (unsigned long)tsk->thread_info) & (PAGE_MASK<<1))
 		return;
-	show_trace(tsk, (unsigned long *)esp);
+	/*
+	 * switch_to pushes the frame pointer just before saving
+	 * the stack pointer.
+	 */
+	show_trace((unsigned long *)esp, (unsigned long *)*(unsigned long *)esp);
 }
 
 void show_stack(struct task_struct *task, unsigned long *esp)
 {
 	unsigned long *stack;
 	int i;
+	DECLARE_EBP;
+	unsigned long *frame;
 
 	if (esp == NULL) {
-		if (task)
+		if (task && (task != current)) {
 			esp = (unsigned long*)task->thread.esp;
-		else
+			/*
+			 * switch_to pushes the frame pointer just before
+			 * saving the stack pointer.
+			 */
+			frame = (unsigned long *)*esp;
+		} else {
 			esp = (unsigned long *)&esp;
+			frame = ebp;
+#ifdef CONFIG_FRAME_POINTER
+			/* we want the enclosing frame ... */
+			frame = (unsigned long *)*ebp;
+#endif
+		}
+	} else {
+		/*
+		 * The only place that calls show_stack with task == NULL
+		 * and esp != NULL is show_registers below, where esp
+		 * points at the esp field of a pt_regs structure on the
+		 * stack.  So this gets us the saved ebp value.
+		 */
+		frame = ((struct pt_regs *)((unsigned char *)esp -
+				offsetof(struct pt_regs, esp)))->ebp;
 	}
 
 	stack = esp;
@@ -145,7 +187,7 @@
 		printk("%08lx ", *stack++);
 	}
 	printk("\n");
-	show_trace(task, esp);
+	show_trace(esp, frame);
 }
 
 /*
@@ -154,8 +196,9 @@
 void dump_stack(void)
 {
 	unsigned long stack;
+	DECLARE_EBP;
 
-	show_trace(current, &stack);
+	show_trace(&stack, ebp);
 }
 
 EXPORT_SYMBOL(dump_stack);
diff -ruN 2.6.0-test9/include/asm-i386/processor.h 2.6.0-test9-sfr.1/include/asm-i386/processor.h
--- 2.6.0-test9/include/asm-i386/processor.h	2003-10-20 11:36:07.000000000 +1000
+++ 2.6.0-test9-sfr.1/include/asm-i386/processor.h	2003-10-28 16:57:42.000000000 +1100
@@ -492,7 +492,7 @@
 extern int kernel_thread(int (*fn)(void *), void * arg, unsigned long flags);
 
 extern unsigned long thread_saved_pc(struct task_struct *tsk);
-void show_trace(struct task_struct *task, unsigned long *stack);
+void show_trace(unsigned long *stack, unsigned long *frame);
 
 unsigned long get_wchan(struct task_struct *p);
 #define KSTK_EIP(tsk)	(((unsigned long *)(4096+(unsigned long)(tsk)->thread_info))[1019])
#2 lore raw
Quoted 6 lines
Quoted 5 lines

I was working on this for the mjb tree but I thought I'd throw it out here in case anyone else finds it interesting. This simple change to the stack trace code uses the frame pointer to do the trace instead of assuming any kernel address on the stack is a return address. It makes for much more readable stack traces.

Quoted 7 lines

I was asked by a colleague to do the same thing and came up with this alternative version which, I think, gets it mostly right - I am not sure about the stack trace from an OOPS.

Don't bother to tell me this is a bit of a hack - I know :-)

Patch relative to 2.6.0-test9.

What's the difference between the two patches, apart from the size? Better error handling / functionality somehow?

M.

#3 lore raw
Quoted 3 lines · On Wed, 05 Nov 2003 11:56:34 -0800 "Martin J. Bligh" <[email protected]> wrote:

What's the difference between the two patches, apart from the size? Better error handling / functionality somehow?

I think mine handles more cases, but is much more of a hack ...

Signature · 3 lines
Cheers,
Stephen Rothwell                    [email protected]
http://www.canb.auug.org.au/~sfr/
#4 lore raw
Quoted 4 lines
Quoted 2 lines

What's the difference between the two patches, apart from the size? Better error handling / functionality somehow?

I think mine handles more cases, but is much more of a hack ...

I don't care if it's a hack, as long as it works ;-) Could you elaborate on the other cases it handles?

Thanks,

M.

author
#5 lore raw
In reply to Adam Litke
Quoted 5 lines · Adam Litke

I was working on this for the mjb tree but I thought I'd throw it out here in case anyone else finds it interesting. This simple change to the stack trace code uses the frame pointer to do the trace instead of assuming any kernel address on the stack is a return address. It makes for much more readable stack traces.

Here is an updated patch that fixes a very stupid NULL pointer dereference. Also, it now avoids contradictory stack data when thread == current.

arch/i386/kernel/traps.c +61 -7
diff -purN linux-2.6.0-test9-virgin/arch/i386/kernel/traps.c linux-2.6.0-test9-stack/arch/i386/kernel/traps.c
@@ -91,19 +91,58 @@ asmlinkage void alignment_check(void);
 asmlinkage void spurious_interrupt_bug(void);
 asmlinkage void machine_check(void);
 
-static int kstack_depth_to_print = 24;
+static int kstack_depth_to_print = 128;
 
-void show_trace(struct task_struct *task, unsigned long * stack)
+#ifdef CONFIG_FRAME_POINTER
+void show_stack_frame(unsigned long start, unsigned long end)
+{
+	int i;
+
+	printk("              ");
+	for (i = start; i < end; i += 4) {
+		if ((i - start) && ((i - start)%24 == 0))
+			printk("\n              ");
+		printk("%08lx ", *(unsigned long *) i);
+	}
+	printk("\n");
+}
+		
+void show_trace_fp(struct task_struct *task, unsigned long * stack)
+{
+	unsigned long addr, ebp;
+
+	if (!task)
+		task = current;
+	
+	if (task == current) {
+		/* Grab ebp right from our regs */
+		asm ("movl %%ebp, %0" : "=r" (ebp) : );
+	} else {
+		/* ebp is the last reg pushed by switch_to */
+		ebp = *(unsigned long *) task->thread.esp;
+	}
+
+	show_stack_frame((unsigned long) stack, ebp+4);
+	while (ebp) {
+		addr = *(unsigned long *) (ebp + 4);
+		if (!kernel_text_address(addr))
+			return;
+		printk(" [<%08lx>] ", addr);
+		print_symbol("%s\n", addr);
+
+		/* Show the stack frame starting with args */
+		show_stack_frame(ebp + 8, (*(unsigned long *) ebp) + 4);
+		ebp = *(unsigned long *) ebp;
+	}
+}
+#else
+void show_trace_guess(unsigned long * stack)
 {
 	unsigned long addr;
 
 	if (!stack)
 		stack = (unsigned long*)&stack;
 
-	printk("Call Trace:");
-#ifdef CONFIG_KALLSYMS
-	printk("\n");
-#endif
 	while (!kstack_end(stack)) {
 		addr = *stack++;
 		if (kernel_text_address(addr)) {

@@ -111,6 +150,20 @@ void show_trace(struct task_struct *task
 			print_symbol("%s\n", addr);
 		}
 	}
+}
+#endif
+
+void show_trace(struct task_struct *task, unsigned long * stack)
+{
+	printk("Call Trace:");
+#ifdef CONFIG_KALLSYMS
+	printk("\n");
+#endif
+#ifdef CONFIG_FRAME_POINTER
+	show_trace_fp(task, stack);
+#else
+	show_trace_guess(stack);
+#endif
 	printk("\n");
 }
 

@@ -136,6 +189,7 @@ void show_stack(struct task_struct *task
 			esp = (unsigned long *)&esp;
 	}
 
+#ifndef CONFIG_FRAME_POINTER
 	stack = esp;
 	for(i = 0; i < kstack_depth_to_print; i++) {
 		if (kstack_end(stack))

@@ -145,6 +199,7 @@ void show_stack(struct task_struct *task
 		printk("%08lx ", *stack++);
 	}
 	printk("\n");
+#endif
 	show_trace(task, esp);
 }
--

Adam Litke (agl at us.ibm.com)
IBM Linux Technology Center
(503) 578 - 3283 t/l 775 - 3283