[PATCH 0/7] Improve prologue analysis code

[PATCH 0/7] Improve prologue analysis code

[Franck Bui-Huu]

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(-)

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

[Franck Bui-Huu]

Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
Signed-off-by: Franck Bui-Huu <vagabon.xyz@gmail.com>
---
 arch/mips/kernel/process.c |   63 ++++++++++++++++++++++----------------------
 1 files changed, 32 insertions(+), 31 deletions(-)

diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index 8709a46..8d0e4fa 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -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 =
-- 
1.4.2.rc2

[PATCH 2/7] Make get_frame_info() more robust

[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 <vagabon.xyz@gmail.com>
---
 arch/mips/kernel/process.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index 8d0e4fa..333f0bb 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -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 */
-- 
1.4.2.rc2

[PATCH 3/7] Make frame_info_init() more readable.

[Franck Bui-Huu]

Signed-off-by: Franck Bui-Huu <vagabon.xyz@gmail.com>
---
 arch/mips/kernel/process.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index 333f0bb..539b23b 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -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;
-- 
1.4.2.rc2

[PATCH 4/7] Remove unused MODULE_RANGE macro.

[Franck Bui-Huu]

Signed-off-by: Franck Bui-Huu <vagabon.xyz@gmail.com>
---
 arch/mips/kernel/traps.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
index 7aa9dfc..4a11a3d 100644
--- a/arch/mips/kernel/traps.c
+++ b/arch/mips/kernel/traps.c
@@ -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)
 {
-- 
1.4.2.rc2

[PATCH 5/7] Miscellaneous cleanup in prologue analysis code

[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 <vagabon.xyz@gmail.com>
---
 arch/mips/kernel/traps.c |   26 ++++++++++++--------------
 1 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
index 4a11a3d..15fa445 100644
--- a/arch/mips/kernel/traps.c
+++ b/arch/mips/kernel/traps.c
@@ -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
-- 
1.4.2.rc2

[PATCH 6/7] Fix dump_stack()

[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 <vagabon.xyz@gmail.com>
---
 arch/mips/kernel/traps.c |   13 +++----------
 1 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
index 15fa445..07191a6 100644
--- a/arch/mips/kernel/traps.c
+++ b/arch/mips/kernel/traps.c
@@ -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);
-- 
1.4.2.rc2

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

[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 <vagabon.xyz@gmail.com>
---
 arch/mips/kernel/process.c |   20 ++++++++++++--------
 arch/mips/kernel/traps.c   |   15 ++++++---------
 2 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index 539b23b..6377b17 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -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
diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
index 07191a6..78aed61 100644
--- a/arch/mips/kernel/traps.c
+++ b/arch/mips/kernel/traps.c
@@ -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
-- 
1.4.2.rc2

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

[Atsushi Nemoto]

On Tue,  1 Aug 2006 11:27:11 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> +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

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

[Atsushi Nemoto]

On Tue,  1 Aug 2006 11:27:16 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> 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

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

[Franck Bui-Huu]

Atsushi Nemoto wrote:
> On Tue,  1 Aug 2006 11:27:16 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
>> 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...

> 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

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

[Atsushi Nemoto]

On Tue,  1 Aug 2006 11:27:17 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> 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

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

[Atsushi Nemoto]

On Tue, 01 Aug 2006 17:36:38 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> > 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.

> 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

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

[Franck Bui-Huu]

2006/8/1, Atsushi Nemoto <anemo@mba.ocn.ne.jp>:
> On Tue, 01 Aug 2006 17:36:38 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> > > 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.
>

OK, I'll do that.

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

-- 
               Franck

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

[Franck Bui-Huu]

2006/8/1, Atsushi Nemoto <anemo@mba.ocn.ne.jp>:
> On Tue,  1 Aug 2006 11:27:17 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> > 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.

-- 
               Franck

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

[Atsushi Nemoto]

On Tue, 1 Aug 2006 21:38:18 +0200, "Franck Bui-Huu" <vagabon.xyz@gmail.com> wrote:
> 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

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

[Atsushi Nemoto]

On Tue, 1 Aug 2006 19:43:39 +0200, "Franck Bui-Huu" <vagabon.xyz@gmail.com> wrote:
> BTW what about rename show_trace() into show_raw_backtrace() ?

Looks good for me.
---
Atsushi Nemoto

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

[Franck Bui-Huu]

Atsushi Nemoto wrote:
> On Tue, 1 Aug 2006 21:38:18 +0200, "Franck Bui-Huu" <vagabon.xyz@gmail.com> wrote:
>> 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 --

diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index 4ceddfa..8a9db45 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -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];
 

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

[Atsushi Nemoto]

On Wed, 02 Aug 2006 12:21:11 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> does something like this on top of this patch make you feel better ?
> 
> -- >8 --
> 
> diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
> index 4ceddfa..8a9db45 100644
> --- a/arch/mips/kernel/process.c
> +++ b/arch/mips/kernel/process.c
> @@ -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;

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

---
Atsushi Nemoto

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

[Franck Bui-Huu]

Atsushi Nemoto wrote:
> On Wed, 02 Aug 2006 12:21:11 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
>> does something like this on top of this patch make you feel better ?
>>
>> -- >8 --
>>
>> diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
>> index 4ceddfa..8a9db45 100644
>> --- a/arch/mips/kernel/process.c
>> +++ b/arch/mips/kernel/process.c
>> @@ -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;
> 
> 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 <vagabon.xyz@gmail.com>
---
 arch/mips/kernel/process.c |   35 ++++++++++++++++++++++-------------
 arch/mips/kernel/traps.c   |   24 ++++++++++++------------
 2 files changed, 34 insertions(+), 25 deletions(-)

diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index 309bfa4..951bf9c 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -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
diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
index 303f008..ab77034 100644
--- a/arch/mips/kernel/traps.c
+++ b/arch/mips/kernel/traps.c
@@ -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
 
 /*
-- 
1.4.2.rc2

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

[Atsushi Nemoto]

On Wed, 02 Aug 2006 15:08:00 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> 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

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

[Franck Bui-Huu]

Atsushi Nemoto wrote:
> On Wed, 02 Aug 2006 15:08:00 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
>> 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

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

[Atsushi Nemoto]

On Wed, 02 Aug 2006 18:56:34 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> > 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

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

[Franck Bui-Huu]

Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
Signed-off-by: Franck Bui-Huu <vagabon.xyz@gmail.com>
---
 arch/mips/kernel/process.c |   67 ++++++++++++++++++++++++--------------------
 1 files changed, 36 insertions(+), 31 deletions(-)

diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index 8709a46..93d5432 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -281,48 +281,53 @@ static struct mips_frame_info {
 } *schedule_frame, mfinfo[64];
 static int mfinfo_num;
 
+static inline int is_ra_save_ins(union mips_instruction *ip)
+{
+	/* sw / sd $ra, offset($sp) */
+	return (ip->i_format.opcode == sw_op || ip->i_format.opcode == sd_op) &&
+		ip->i_format.rs == 29 &&
+		ip->i_format.rt == 31;
+}
+
+static inline int is_jal_jalr_jr_ins(union mips_instruction *ip)
+{
+	if (ip->j_format.opcode == jal_op)
+		return 1;
+	if (ip->r_format.opcode != spec_op)
+		return 0;
+	return ip->r_format.func == jalr_op || ip->r_format.func == jr_op;
+}
+
+static inline int is_sp_move_ins(union mips_instruction *ip)
+{
+	/* addiu/daddiu sp,sp,-imm */
+	if (ip->i_format.rs != 29 || ip->i_format.rt != 29)
+		return 0;
+	if (ip->i_format.opcode == addiu_op || ip->i_format.opcode == daddiu_op)
+		return 1;
+	return 0;
+}
+
 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 =
-- 
1.4.2.rc2