Improve prologue analysis code

PATCH In review Franck Bui-Huu 2 participants 23 messages Aug 1, 2006 – Aug 3, 2006 MIPS

lore

Share & tools

lore

Franck Bui-Huu

author

2006-08-01 09:29 UTC #0 lore raw

cover Improve prologue analysis code

This patch serie clean up or improves this code. I splitted out this into 7 patches to make the review easier.

Franck

arch/mips/kernel/process.c | 111 +++++++++++++++++++++++---------------------

 arch/mips/kernel/traps.c   |   55 ++++++++--------------
 2 files changed, 76 insertions(+), 90 deletions(-)

Franck Bui-Huu

author

2006-08-01 09:28 UTC #1 lore raw

PATCH 1/7 Make get_frame_info() more readable.

Signed-off-by Atsushi Nemoto <[email protected]>

Signed-off-by Franck Bui-Huu <[email protected]>

---
 arch/mips/kernel/process.c |   63 ++++++++++++++++++++++----------------------
 1 files changed, 32 insertions(+), 31 deletions(-)

arch/mips/kernel/process.c +32 ~~-31~~

index 8709a46..8d0e4fa 100644
@@ -281,48 +281,49 @@ static struct mips_frame_info {
 } *schedule_frame, mfinfo[64];
 static int mfinfo_num;
 
+static inline int is_ra_save_ins(union mips_instruction *pc)
+{
+	/* sw / sd $ra, offset($sp) */
+	return (pc->i_format.opcode == sw_op || pc->i_format.opcode == sd_op) &&
+		pc->i_format.rs == 29 &&
+		pc->i_format.rt == 31;
+}
+
+static inline int is_jal_jalr_jr_ins(union mips_instruction *pc)
+{
+	return pc->j_format.opcode == jal_op ||
+		(pc->r_format.opcode == spec_op &&
+			(pc->r_format.func == jalr_op || pc->r_format.func == jr_op));
+}
+
+static inline int is_sp_move_ins(union mips_instruction *pc)
+{
+	/* addiu/daddiu sp,sp,-imm */
+	return (pc->i_format.opcode == addiu_op || pc->i_format.opcode == daddiu_op) && \
+		pc->i_format.rs == 29 && \
+		pc->i_format.rt == 29;
+}
+
 static int get_frame_info(struct mips_frame_info *info)
 {
-	int i;
-	void *func = info->func;
-	union mips_instruction *ip = (union mips_instruction *)func;
+	union mips_instruction *ip = info->func;
+	int i, max_insns =
+		min(128UL, info->func_size / sizeof(union mips_instruction));
+
 	info->pc_offset = -1;
 	info->frame_size = 0;
-	for (i = 0; i < 128; i++, ip++) {
-		/* if jal, jalr, jr, stop. */
-		if (ip->j_format.opcode == jal_op ||
-		    (ip->r_format.opcode == spec_op &&
-		     (ip->r_format.func == jalr_op ||
-		      ip->r_format.func == jr_op)))
-			break;
 
-		if (info->func_size && i >= info->func_size / 4)
+	for (i = 0; i < max_insns; i++, ip++) {
+
+		if (is_jal_jalr_jr_ins(ip))
 			break;
-		if (
-#ifdef CONFIG_32BIT
-		    ip->i_format.opcode == addiu_op &&
-#endif
-#ifdef CONFIG_64BIT
-		    ip->i_format.opcode == daddiu_op &&
-#endif
-		    ip->i_format.rs == 29 &&
-		    ip->i_format.rt == 29) {
-			/* addiu/daddiu sp,sp,-imm */
+		if (is_sp_move_ins(ip)) {
 			if (info->frame_size)
 				continue;
 			info->frame_size = - ip->i_format.simmediate;
 		}
 
-		if (
-#ifdef CONFIG_32BIT
-		    ip->i_format.opcode == sw_op &&
-#endif
-#ifdef CONFIG_64BIT
-		    ip->i_format.opcode == sd_op &&
-#endif
-		    ip->i_format.rs == 29 &&
-		    ip->i_format.rt == 31) {
-			/* sw / sd $ra, offset($sp) */
+		if (is_ra_save_ins(ip)) {
 			if (info->pc_offset != -1)
 				continue;
 			info->pc_offset =

Signature · 1 line

1.4.2.rc2

Atsushi Nemoto

2006-08-01 15:01 UTC #2 lore raw

Re: [PATCH 1/7] Make get_frame_info() more readable.

arch/mips/kernel/process.c static struct mips_frame_info { · line 290

Quoted 7 lines · Franck Bui-Huu

arch/mips/kernel/process.c +7 -0

+static inline int is_ra_save_ins(union mips_instruction *pc)
+{
+	/* sw / sd $ra, offset($sp) */
+	return (pc->i_format.opcode == sw_op || pc->i_format.opcode == sd_op) &&
+		pc->i_format.rs == 29 &&
+		pc->i_format.rt == 31;
+}

Separating these function would be good, but let's play with 80 columns rule.

---
Atsushi Nemoto

Franck Bui-Huu

author

2006-08-01 09:30 UTC #3 lore raw

PATCH 2/7 Make get_frame_info() more robust

In reply to Franck Bui-Huu

Now get_frame_info() wants to detect move sp instruction first. It assumes that the save ra in the stack instruction can't happen before allocating frame size space into the stack.

Signed-off-by Franck Bui-Huu <[email protected]>

---
 arch/mips/kernel/process.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

arch/mips/kernel/process.c +6 -8

index 8d0e4fa..333f0bb 100644
@@ -317,17 +317,15 @@ static int get_frame_info(struct mips_fr
 
 		if (is_jal_jalr_jr_ins(ip))
 			break;
-		if (is_sp_move_ins(ip)) {
-			if (info->frame_size)
-				continue;
-			info->frame_size = - ip->i_format.simmediate;
+		if (!info->frame_size) {
+			if (is_sp_move_ins(ip))
+				info->frame_size = - ip->i_format.simmediate;
+			continue;
 		}
-
-		if (is_ra_save_ins(ip)) {
-			if (info->pc_offset != -1)
-				continue;
+		if (info->pc_offset == -1 && is_ra_save_ins(ip)) {
 			info->pc_offset =
 				ip->i_format.simmediate / sizeof(long);
+			break;
 		}
 	}
 	if (info->frame_size && info->pc_offset >= 0) /* nested */

Signature · 1 line

1.4.2.rc2

Franck Bui-Huu

author

2006-08-01 09:30 UTC #4 lore raw

PATCH 3/7 Make frame_info_init() more readable.

In reply to Franck Bui-Huu

Signed-off-by Franck Bui-Huu <[email protected]>

---
 arch/mips/kernel/process.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

arch/mips/kernel/process.c +9 -9

index 333f0bb..539b23b 100644
@@ -364,15 +364,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;

Signature · 1 line

1.4.2.rc2

Franck Bui-Huu

author

2006-08-01 09:31 UTC #5 lore raw

PATCH 4/7 Remove unused MODULE_RANGE macro.

In reply to Franck Bui-Huu

Signed-off-by Franck Bui-Huu <[email protected]>

---
 arch/mips/kernel/traps.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

arch/mips/kernel/traps.c +0 -5

index 7aa9dfc..4a11a3d 100644
@@ -73,11 +73,6 @@ 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)
 {

Signature · 1 line

1.4.2.rc2

Franck Bui-Huu

author

2006-08-01 09:32 UTC #6 lore raw

PATCH 5/7 Miscellaneous cleanup in prologue analysis code

In reply to Franck Bui-Huu

We usually use backtrace term for dumping a call tree during debug. Therefore this patch renames show_frametrace() into show_backtrace().

Signed-off-by Franck Bui-Huu <[email protected]>

---
 arch/mips/kernel/traps.c |   26 ++++++++++++--------------
 1 files changed, 12 insertions(+), 14 deletions(-)

arch/mips/kernel/traps.c +12 ~~-14~~

index 4a11a3d..15fa445 100644
@@ -74,19 +74,18 @@ void (*board_ejtag_handler_setup)(void);
 void (*board_bind_eic_interrupt)(int irq, int regset);
 
 
-static void show_trace(unsigned long *stack)
+static void show_trace(unsigned long *sp)
 {
-	const int field = 2 * sizeof(unsigned long);
 	unsigned long addr;
 
 	printk("Call Trace:");
 #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);
+			printk(" [<%0*lx>] ", 2 * sizeof(unsigned long), addr);
 			print_symbol("%s\n", addr);
 		}
 	}

@@ -104,22 +103,21 @@ __setup("raw_show_trace", set_raw_show_t
 
 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)
+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);
+		printk(" [<%0*lx>] ", 2 * sizeof(unsigned long), pc);
 		print_symbol("%s\n", pc);
-		pc = unwind_stack(task, &stack, pc);
+		pc = unwind_stack(task, &sp, pc);
 		if (top && pc == 0)
 			pc = regs->regs[31];	/* leaf? */
 		top = 0;

@@ -127,7 +125,7 @@ static void show_frametrace(struct task_
 	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
 
 /*

@@ -160,7 +158,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)

@@ -211,7 +209,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

Signature · 1 line

1.4.2.rc2

Franck Bui-Huu

author

2006-08-01 09:33 UTC #7 lore raw

PATCH 6/7 Fix dump_stack()

In reply to Franck Bui-Huu

When CONFIG_KALLSYMS is not set stack local is not initialized. Therefore show_trace() won't display anything useful. This patch uses prepare_frametrace() to setup the stack pointer before calling show_trace().

Signed-off-by Franck Bui-Huu <[email protected]>

---
 arch/mips/kernel/traps.c |   13 +++----------
 1 files changed, 3 insertions(+), 10 deletions(-)

arch/mips/kernel/traps.c +3 ~~-10~~

index 15fa445..07191a6 100644
@@ -203,17 +203,10 @@ void show_stack(struct task_struct *task
  */
 void dump_stack(void)
 {
-	unsigned long stack;
+	struct pt_regs regs;
 
-#ifdef CONFIG_KALLSYMS
-	if (!raw_show_trace) {
-		struct pt_regs regs;
-		prepare_frametrace(&regs);
-		show_backtrace(current, &regs);
-		return;
-	}
-#endif
-	show_trace(&stack);
+	prepare_frametrace(&regs);
+	show_backtrace(current, &regs);
 }
 
 EXPORT_SYMBOL(dump_stack);

Signature · 1 line

1.4.2.rc2

Atsushi Nemoto

2006-08-01 15:07 UTC #8 lore raw

Re: [PATCH 6/7] Fix dump_stack()

Quoted 4 lines · Franck Bui-Huu

When CONFIG_KALLSYMS is not set stack local is not initialized. Therefore show_trace() won't display anything useful. This patch uses prepare_frametrace() to setup the stack pointer before calling show_trace().

It's not a bug. The original show_trace() needs an address on stack and dump_stack() surely give it by taking an address of local variable.

Eliminating the #ifdef itself looks good, but if you cleared contents of the "regs" before prepare_frametrace, you will get less false entries in the output.

---
Atsushi Nemoto

Franck Bui-Huu

author

2006-08-01 15:38 UTC #9 lore raw

Re: [PATCH 6/7] Fix dump_stack()

Quoted 10 lines · Atsushi Nemoto

Quoted 4 lines · Franck Bui-Huu
When CONFIG_KALLSYMS is not set stack local is not initialized. Therefore show_trace() won't display anything useful. This patch uses prepare_frametrace() to setup the stack pointer before calling show_trace().
It's not a bug. The original show_trace() needs an address on stack and dump_stack() surely give it by taking an address of local variable.

sorry, was drunk when writing the commit message...

Quoted 4 lines

Eliminating the #ifdef itself looks good, but if you cleared contents of the "regs" before prepare_frametrace, you will get less false entries in the output.

well I don't see why...show_trace() is going to only use regs[29] which is setup by prepare_frametrace()...

One other thing, why did you mark prepare_frametrace() as noinline ? I would mark it as always_inline to get one less false entry in the output.

Franck

Atsushi Nemoto

2006-08-01 16:04 UTC #10 lore raw

Re: [PATCH 6/7] Fix dump_stack()

Quoted 6 lines · Franck Bui-Huu

Quoted 3 lines
Eliminating the #ifdef itself looks good, but if you cleared contents of the "regs" before prepare_frametrace, you will get less false entries in the output.
well I don't see why...show_trace() is going to only use regs[29] which is setup by prepare_frametrace()...

If CONFIG_KALLSYMS was not set, show_trace() print all possible entries starting from the sp. The sp value stored in "regs" by prepare_frametrace() will be little smaller one than the address of the "regs" itself. So if some values like function addresses were in the "regs", show_trace() will report them.

Quoted 3 lines

One other thing, why did you mark prepare_frametrace() as noinline ? I would mark it as always_inline to get one less false entry in the output.

Well, I tried some prepare_frametrace() implementations before the final one. I wanted to make sure that prepare_frametrace() does really what desired, and noinline make it a bit easy.

But now I think current prepare_frametrace() is quite safe for inlining. So always_inline would be OK.

---
Atsushi Nemoto

Franck Bui-Huu

author

2006-08-01 17:43 UTC #11 lore raw

Re: [PATCH 6/7] Fix dump_stack()

2006/8/1, Atsushi Nemoto <[email protected]>:

Quoted 5 lines · Franck Bui-Huu

Quoted 4 lines
Quoted 3 lines
Eliminating the #ifdef itself looks good, but if you cleared contents of the "regs" before prepare_frametrace, you will get less false entries in the output.

Quoted 9 lines

Quoted 2 lines
well I don't see why...show_trace() is going to only use regs[29] which is setup by prepare_frametrace()...
If CONFIG_KALLSYMS was not set, show_trace() print all possible entries starting from the sp. The sp value stored in "regs" by prepare_frametrace() will be little smaller one than the address of the "regs" itself. So if some values like function addresses were in the "regs", show_trace() will report them.

OK, I'll do that.

BTW what about rename show_trace() into show_raw_backtrace() ?

Signature · 1 line

              Franck

Atsushi Nemoto

2006-08-02 01:54 UTC #12 lore raw

Re: [PATCH 6/7] Fix dump_stack()

Quoted 1 line · Franck Bui-Huu

BTW what about rename show_trace() into show_raw_backtrace() ?

Looks good for me.
---
Atsushi Nemoto

Franck Bui-Huu

author

2006-08-01 09:34 UTC #13 lore raw

PATCH 7/7 Allow unwind_stack() to return ra for leaf function

In reply to Franck Bui-Huu

Since get_frame_info() is more robust, unwind_stack() can returns ra value for leaf functions.

Signed-off-by Franck Bui-Huu <[email protected]>

---
 arch/mips/kernel/process.c |   20 ++++++++++++--------
 arch/mips/kernel/traps.c   |   15 ++++++---------
 2 files changed, 18 insertions(+), 17 deletions(-)

arch/mips/kernel/process.c +12 -8

index 539b23b..6377b17 100644
@@ -445,14 +445,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 leaf;
 
 	stack_page = (unsigned long)task_stack_page(task);
 	if (!stack_page)

@@ -465,18 +466,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);
+	leaf = get_frame_info(&info);
+	if (leaf < 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 (leaf)
+		pc = regs->regs[31];
+	else
+		pc = (*sp)[info.pc_offset];
+
 	*sp += info.frame_size / sizeof(long);
-	return pc;
+	return __kernel_text_address(pc) ? pc : 0;
 }
 #endif

arch/mips/kernel/traps.c +6 -9

index 07191a6..78aed61 100644
@@ -101,8 +101,9 @@ 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);
+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)
 {
 	unsigned long *sp = (long *)regs->regs[29];

@@ -114,14 +115,10 @@ static void show_backtrace(struct task_s
 		return;
 	}
 	printk("Call Trace:\n");
-	while (__kernel_text_address(pc)) {
+	do {
 		printk(" [<%0*lx>] ", 2 * sizeof(unsigned long), pc);
-		print_symbol("%s\n", pc);
-		pc = unwind_stack(task, &sp, pc);
-		if (top && pc == 0)
-			pc = regs->regs[31];	/* leaf? */
-		top = 0;
-	}
+ 		print_symbol("%s\n", pc);
+	} while ((pc = unwind_stack(task, &sp, pc, regs)));
 	printk("\n");
 }
 #else

Signature · 1 line

1.4.2.rc2

Atsushi Nemoto

2006-08-01 15:47 UTC #14 lore raw

Re: [PATCH 7/7] Allow unwind_stack() to return ra for leaf function

Quoted 2 lines · Franck Bui-Huu

Since get_frame_info() is more robust, unwind_stack() can returns ra value for leaf functions.

I think it is still fragile. The get_frame_info() might misdetect nested function as leaf. For example, I can craft this code:

int nestfunc(int arg)

{
	if (arg)
		return 0;
	func();
	return 1;
}

.set noreorder
nestfunc:

	beqz	a0, 1f
	 nop
	jr	ra
	 move	v0, zero
1:
	addiu	sp, sp, -24
	sw	ra, 16(sp)
	jal	func
	 nop
	lw	ra, 16(sp)
	li	v0, 1
	jr	ra
	 addiu	sp, sp, 24

(Though it seems a bit artificial, who believe gcc never do it same?)

The get_frame_info() will think this is a leaf. With your patch, unwind_stack() might fall into endless loop at worst (if the "func" was leaf and an exception happened in the "func").

I think you should ensure unwind_stack() never use regs->regs[31] elsewhere than top of the stack.

---
Atsushi Nemoto

Franck Bui-Huu

author

2006-08-01 19:38 UTC #15 lore raw

Re: [PATCH 7/7] Allow unwind_stack() to return ra for leaf function

2006/8/1, Atsushi Nemoto <[email protected]>:

Quoted 7 lines · Franck Bui-Huu

Quoted 2 lines
Since get_frame_info() is more robust, unwind_stack() can returns ra value for leaf functions.
I think it is still fragile. The get_frame_info() might misdetect nested function as leaf. For example, I can craft this code:

Considering (wrongly) a nested function as a leaf one is not a big issue. "ra" reg should _always_ store a valid address (nested or not). The only (small) impact would be to skip an entry when showing the backtrace.

Signature · 1 line

              Franck

Atsushi Nemoto

2006-08-02 01:51 UTC #16 lore raw

Re: [PATCH 7/7] Allow unwind_stack() to return ra for leaf function

Quoted 4 lines · Franck Bui-Huu

Considering (wrongly) a nested function as a leaf one is not a big issue. "ra" reg should _always_ store a valid address (nested or not). The only (small) impact would be to skip an entry when showing the backtrace.

The unwind_stack() uses regs->regs[31] for a leaf, and regs->regs[31] always holds RA value of _top_ of the stack, not at that level.

---
Atsushi Nemoto

Franck Bui-Huu

author

2006-08-02 10:22 UTC #17 lore raw

Re: [PATCH 7/7] Allow unwind_stack() to return ra for leaf function

Quoted 9 lines · Atsushi Nemoto

Quoted 4 lines · Franck Bui-Huu
Considering (wrongly) a nested function as a leaf one is not a big issue. "ra" reg should _always_ store a valid address (nested or not). The only (small) impact would be to skip an entry when showing the backtrace.
The unwind_stack() uses regs->regs[31] for a leaf, and regs->regs[31] always holds RA value of _top_ of the stack, not at that level.

does something like this on top of this patch make you feel better ?

-- >8 --

arch/mips/kernel/process.c +7 -1

index 4ceddfa..8a9db45 100644
@@ -480,7 +480,13 @@ unsigned long unwind_stack(struct task_s
 		return 0;
 
 	if (leaf)
-		pc = regs->regs[31];
+		/*
+		 * For some extreme cases, get_frame_info() can
+		 * consider wrongly a nested function as a leaf
+		 * one. In that cases avoid to return always the
+		 * same value.
+		 */
+		pc = pc != regs->regs[31] ? regs->regs[31] : 0;
 	else
 		pc = (*sp)[info.pc_offset];

Atsushi Nemoto

2006-08-02 11:25 UTC #18 lore raw

Re: [PATCH 7/7] Allow unwind_stack() to return ra for leaf function

Quoted 10 lines · Franck Bui-Huu

does something like this on top of this patch make you feel better ?
-- >8 --
arch/mips/kernel/process.c +0 -0
index 4ceddfa..8a9db45 100644
@@ -480,7 +480,13 @@ unsigned long unwind_stack(struct task_s
 		return 0;

arch/mips/kernel/process.c unsigned long unwind_stack(struct task_s · line 489

Quoted 10 lines

arch/mips/kernel/process.c +7 -1

 
 	if (leaf)
-		pc = regs->regs[31];
+		/*
+		 * For some extreme cases, get_frame_info() can
+		 * consider wrongly a nested function as a leaf
+		 * one. In that cases avoid to return always the
+		 * same value.
+		 */
+		pc = pc != regs->regs[31] ? regs->regs[31] : 0;

Yes, it should be safe. But still I'm not sure unwind_stack() should take "regs" as its argument...

---
Atsushi Nemoto

Franck Bui-Huu

author

2006-08-02 13:09 UTC #19 lore raw

Re: [PATCH 7/7] Allow unwind_stack() to return ra for leaf function

Quoted 12 lines · Atsushi Nemoto

Quoted 11 lines · Franck Bui-Huu
does something like this on top of this patch make you feel better ?
-- >8 --
arch/mips/kernel/process.c +0 -0
index 4ceddfa..8a9db45 100644
@@ -480,7 +480,13 @@ unsigned long unwind_stack(struct task_s
 		return 0;
 

Quoted 13 lines

arch/mips/kernel/process.c unsigned long unwind_stack(struct task_s · line 489
Quoted 9 lines
arch/mips/kernel/process.c +7 -1
 	if (leaf)
-		pc = regs->regs[31];
+		/*
+		 * For some extreme cases, get_frame_info() can
+		 * consider wrongly a nested function as a leaf
+		 * one. In that cases avoid to return always the
+		 * same value.
+		 */
+		pc = pc != regs->regs[31] ? regs->regs[31] : 0;
Yes, it should be safe. But still I'm not sure unwind_stack() should take "regs" as its argument...

does this updated patch make you really happy ? If so I'll resend the whole updated patchset.

-- >8 --
Subject: Improve unwind_stack()

This patch allows unwind_stack() to return ra for leaf function. But it tries to detects cases where get_frame_info() wrongly consider nested function as a leaf one.

It also pass 'unsinged long *sp' instead of 'unsigned long **sp' as second parameter. The code looks cleaner.

Signed-off-by Franck Bui-Huu <[email protected]>

---
 arch/mips/kernel/process.c |   35 ++++++++++++++++++++++-------------
 arch/mips/kernel/traps.c   |   24 ++++++++++++------------
 2 files changed, 34 insertions(+), 25 deletions(-)

arch/mips/kernel/process.c +22 ~~-13~~

index 309bfa4..951bf9c 100644
@@ -448,15 +448,16 @@ #endif
 }
 
 #ifdef CONFIG_KALLSYMS
-/* used by show_frametrace() */
-unsigned long unwind_stack(struct task_struct *task,
-			   unsigned long **sp, unsigned long pc)
+/* used by show_backtrace() */
+unsigned long unwind_stack(struct task_struct *task, unsigned long *sp,
+			   unsigned long pc, unsigned long ra)
 {
 	unsigned long stack_page;
 	struct mips_frame_info info;
 	char *modname;
 	char namebuf[KSYM_NAME_LEN + 1];
 	unsigned long size, ofs;
+	int leaf;
 
 	stack_page = (unsigned long)task_stack_page(task);
 	if (!stack_page)

@@ -469,18 +470,26 @@ 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);
+	leaf = get_frame_info(&info);
+	if (leaf < 0)
 		return 0;
-	}
-	if ((unsigned long)*sp < stack_page ||
-	    (unsigned long)*sp + info.frame_size / sizeof(long) >
-	    stack_page + THREAD_SIZE - 32)
+
+	if (*sp < stack_page ||
+	    *sp + info.frame_size > stack_page + THREAD_SIZE - 32)
 		return 0;
 
-	pc = (*sp)[info.pc_offset];
-	*sp += info.frame_size / sizeof(long);
-	return pc;
+	if (leaf)
+		/*
+		 * For some extreme cases, get_frame_info() can
+		 * consider wrongly a nested function as a leaf
+		 * one. In that cases avoid to return always the
+		 * same value.
+		 */
+		pc = pc != ra ? ra : 0;
+	else
+		pc = ((unsigned long *)(*sp))[info.pc_offset];
+
+	*sp += info.frame_size;
+	return __kernel_text_address(pc) ? pc : 0;
 }
 #endif

arch/mips/kernel/traps.c +12 ~~-12~~

index 303f008..ab77034 100644
@@ -74,8 +74,9 @@ void (*board_ejtag_handler_setup)(void);
 void (*board_bind_eic_interrupt)(int irq, int regset);
 
 
-static void show_raw_backtrace(unsigned long *sp)
+static void show_raw_backtrace(unsigned long reg29)
 {
+	unsigned long *sp = (unsigned long *)reg29;
 	unsigned long addr;
 
 	printk("Call Trace:");

@@ -99,30 +100,29 @@ 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);
+extern unsigned long unwind_stack(struct task_struct *task, unsigned long *sp,
+				  unsigned long pc, unsigned long ra);
+
 static void show_backtrace(struct task_struct *task, struct pt_regs *regs)
 {
-	unsigned long *sp = (long *)regs->regs[29];
+	unsigned long sp = regs->regs[29];
+	unsigned long ra = regs->regs[31];
 	unsigned long pc = regs->cp0_epc;
-	int top = 1;
 
 	if (raw_show_trace || !__kernel_text_address(pc)) {
 		show_raw_backtrace(sp);
 		return;
 	}
 	printk("Call Trace:\n");
-	while (__kernel_text_address(pc)) {
+	do {
 		print_ip_sym(pc);
-		pc = unwind_stack(task, &sp, pc);
-		if (top && pc == 0)
-			pc = regs->regs[31];	/* leaf? */
-		top = 0;
-	}
+		pc = unwind_stack(task, &sp, pc, ra);
+		ra = 0;
+	} while (pc);
 	printk("\n");
 }
 #else
-#define show_backtrace(task, r) show_raw_backtrace((long *)(r)->regs[29]);
+#define show_backtrace(task, r) show_raw_backtrace((r)->regs[29]);
 #endif
 
 /*

Signature · 1 line

1.4.2.rc2

Atsushi Nemoto

2006-08-02 16:18 UTC #20 lore raw

Re: [PATCH 7/7] Allow unwind_stack() to return ra for leaf function

Quoted 2 lines · Franck Bui-Huu

does this updated patch make you really happy ? If so I'll resend the whole updated patchset.

Yes, looks good for me.

Just one comment: no need to do "pc = pc != ra ? ra : 0" anymore. Just "pc = ra" is enough, isn't it?

---
Atsushi Nemoto

Franck Bui-Huu

author

2006-08-02 16:57 UTC #21 lore raw

Re: [PATCH 7/7] Allow unwind_stack() to return ra for leaf function

Quoted 9 lines · Atsushi Nemoto

Quoted 2 lines · Franck Bui-Huu
does this updated patch make you really happy ? If so I'll resend the whole updated patchset.
Yes, looks good for me.
Just one comment: no need to do "pc = pc != ra ? ra : 0" anymore. Just "pc = ra" is enough, isn't it?

Well I let it just for cases where the caller does not reset ra after the first call. It should be safter. But I can remove it if you want.

Franck

Atsushi Nemoto

2006-08-03 04:52 UTC #22 lore raw

Re: [PATCH 7/7] Allow unwind_stack() to return ra for leaf function

Quoted 6 lines · Franck Bui-Huu

Quoted 2 lines
Just one comment: no need to do "pc = pc != ra ? ra : 0" anymore. Just "pc = ra" is enough, isn't it?
Well I let it just for cases where the caller does not reset ra after the first call. It should be safter. But I can remove it if you want.

OK, I see. No problem.

---
Atsushi Nemoto