* [PATCH 1/7] Make get_frame_info() more readable.
2006-08-01 9:27 [PATCH 0/7] Improve prologue analysis code Franck Bui-Huu
@ 2006-08-01 9:27 ` Franck Bui-Huu
2006-08-01 15:02 ` Atsushi Nemoto
2006-08-01 9:27 ` [PATCH 2/7] Make get_frame_info() more robust Franck Bui-Huu
` (5 subsequent siblings)
6 siblings, 1 reply; 23+ messages in thread
From: Franck Bui-Huu @ 2006-08-01 9:27 UTC (permalink / raw)
To: anemo; +Cc: ralf, linux-mips, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 1/7] Make get_frame_info() more readable.
2006-08-01 9:27 ` [PATCH 1/7] Make get_frame_info() more readable Franck Bui-Huu
@ 2006-08-01 15:02 ` Atsushi Nemoto
0 siblings, 0 replies; 23+ messages in thread
From: Atsushi Nemoto @ 2006-08-01 15:02 UTC (permalink / raw)
To: vagabon.xyz; +Cc: ralf, linux-mips
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/7] Make get_frame_info() more robust
2006-08-01 9:27 [PATCH 0/7] Improve prologue analysis code Franck Bui-Huu
2006-08-01 9:27 ` [PATCH 1/7] Make get_frame_info() more readable Franck Bui-Huu
@ 2006-08-01 9:27 ` Franck Bui-Huu
2006-08-01 9:27 ` [PATCH 3/7] Make frame_info_init() more readable Franck Bui-Huu
` (4 subsequent siblings)
6 siblings, 0 replies; 23+ messages in thread
From: Franck Bui-Huu @ 2006-08-01 9:27 UTC (permalink / raw)
To: anemo; +Cc: ralf, linux-mips, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 3/7] Make frame_info_init() more readable.
2006-08-01 9:27 [PATCH 0/7] Improve prologue analysis code Franck Bui-Huu
2006-08-01 9:27 ` [PATCH 1/7] Make get_frame_info() more readable Franck Bui-Huu
2006-08-01 9:27 ` [PATCH 2/7] Make get_frame_info() more robust Franck Bui-Huu
@ 2006-08-01 9:27 ` Franck Bui-Huu
2006-08-01 9:27 ` [PATCH 4/7] Remove unused MODULE_RANGE macro Franck Bui-Huu
` (3 subsequent siblings)
6 siblings, 0 replies; 23+ messages in thread
From: Franck Bui-Huu @ 2006-08-01 9:27 UTC (permalink / raw)
To: anemo; +Cc: ralf, linux-mips, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 4/7] Remove unused MODULE_RANGE macro.
2006-08-01 9:27 [PATCH 0/7] Improve prologue analysis code Franck Bui-Huu
` (2 preceding siblings ...)
2006-08-01 9:27 ` [PATCH 3/7] Make frame_info_init() more readable Franck Bui-Huu
@ 2006-08-01 9:27 ` Franck Bui-Huu
2006-08-01 9:27 ` [PATCH 5/7] Miscellaneous cleanup in prologue analysis code Franck Bui-Huu
` (2 subsequent siblings)
6 siblings, 0 replies; 23+ messages in thread
From: Franck Bui-Huu @ 2006-08-01 9:27 UTC (permalink / raw)
To: anemo; +Cc: ralf, linux-mips, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 5/7] Miscellaneous cleanup in prologue analysis code
2006-08-01 9:27 [PATCH 0/7] Improve prologue analysis code Franck Bui-Huu
` (3 preceding siblings ...)
2006-08-01 9:27 ` [PATCH 4/7] Remove unused MODULE_RANGE macro Franck Bui-Huu
@ 2006-08-01 9:27 ` Franck Bui-Huu
2006-08-01 9:27 ` [PATCH 6/7] Fix dump_stack() Franck Bui-Huu
2006-08-01 9:27 ` [PATCH 7/7] Allow unwind_stack() to return ra for leaf function Franck Bui-Huu
6 siblings, 0 replies; 23+ messages in thread
From: Franck Bui-Huu @ 2006-08-01 9:27 UTC (permalink / raw)
To: anemo; +Cc: ralf, linux-mips, 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(®s);
- show_frametrace(current, ®s);
+ show_backtrace(current, ®s);
return;
}
#endif
--
1.4.2.rc2
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 6/7] Fix dump_stack()
2006-08-01 9:27 [PATCH 0/7] Improve prologue analysis code Franck Bui-Huu
` (4 preceding siblings ...)
2006-08-01 9:27 ` [PATCH 5/7] Miscellaneous cleanup in prologue analysis code Franck Bui-Huu
@ 2006-08-01 9:27 ` Franck Bui-Huu
2006-08-01 15:08 ` Atsushi Nemoto
2006-08-01 9:27 ` [PATCH 7/7] Allow unwind_stack() to return ra for leaf function Franck Bui-Huu
6 siblings, 1 reply; 23+ messages in thread
From: Franck Bui-Huu @ 2006-08-01 9:27 UTC (permalink / raw)
To: anemo; +Cc: ralf, linux-mips, 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(®s);
- show_backtrace(current, ®s);
- return;
- }
-#endif
- show_trace(&stack);
+ prepare_frametrace(®s);
+ show_backtrace(current, ®s);
}
EXPORT_SYMBOL(dump_stack);
--
1.4.2.rc2
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 6/7] Fix dump_stack()
2006-08-01 9:27 ` [PATCH 6/7] Fix dump_stack() Franck Bui-Huu
@ 2006-08-01 15:08 ` Atsushi Nemoto
2006-08-01 15:36 ` Franck Bui-Huu
0 siblings, 1 reply; 23+ messages in thread
From: Atsushi Nemoto @ 2006-08-01 15:08 UTC (permalink / raw)
To: vagabon.xyz; +Cc: ralf, linux-mips
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 6/7] Fix dump_stack()
2006-08-01 15:08 ` Atsushi Nemoto
@ 2006-08-01 15:36 ` Franck Bui-Huu
2006-08-01 16:05 ` Atsushi Nemoto
0 siblings, 1 reply; 23+ messages in thread
From: Franck Bui-Huu @ 2006-08-01 15:36 UTC (permalink / raw)
To: Atsushi Nemoto; +Cc: vagabon.xyz, ralf, linux-mips
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 6/7] Fix dump_stack()
2006-08-01 15:36 ` Franck Bui-Huu
@ 2006-08-01 16:05 ` Atsushi Nemoto
2006-08-01 17:43 ` Franck Bui-Huu
0 siblings, 1 reply; 23+ messages in thread
From: Atsushi Nemoto @ 2006-08-01 16:05 UTC (permalink / raw)
To: vagabon.xyz; +Cc: ralf, linux-mips
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 6/7] Fix dump_stack()
2006-08-01 16:05 ` Atsushi Nemoto
@ 2006-08-01 17:43 ` Franck Bui-Huu
2006-08-02 1:54 ` Atsushi Nemoto
0 siblings, 1 reply; 23+ messages in thread
From: Franck Bui-Huu @ 2006-08-01 17:43 UTC (permalink / raw)
To: Atsushi Nemoto; +Cc: ralf, linux-mips
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 7/7] Allow unwind_stack() to return ra for leaf function
2006-08-01 9:27 [PATCH 0/7] Improve prologue analysis code Franck Bui-Huu
` (5 preceding siblings ...)
2006-08-01 9:27 ` [PATCH 6/7] Fix dump_stack() Franck Bui-Huu
@ 2006-08-01 9:27 ` Franck Bui-Huu
2006-08-01 15:48 ` Atsushi Nemoto
6 siblings, 1 reply; 23+ messages in thread
From: Franck Bui-Huu @ 2006-08-01 9:27 UTC (permalink / raw)
To: anemo; +Cc: ralf, linux-mips, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 7/7] Allow unwind_stack() to return ra for leaf function
2006-08-01 9:27 ` [PATCH 7/7] Allow unwind_stack() to return ra for leaf function Franck Bui-Huu
@ 2006-08-01 15:48 ` Atsushi Nemoto
2006-08-01 19:38 ` Franck Bui-Huu
0 siblings, 1 reply; 23+ messages in thread
From: Atsushi Nemoto @ 2006-08-01 15:48 UTC (permalink / raw)
To: vagabon.xyz; +Cc: ralf, linux-mips
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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 7/7] Allow unwind_stack() to return ra for leaf function
2006-08-01 15:48 ` Atsushi Nemoto
@ 2006-08-01 19:38 ` Franck Bui-Huu
2006-08-02 1:51 ` Atsushi Nemoto
0 siblings, 1 reply; 23+ messages in thread
From: Franck Bui-Huu @ 2006-08-01 19:38 UTC (permalink / raw)
To: Atsushi Nemoto; +Cc: ralf, linux-mips
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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 7/7] Allow unwind_stack() to return ra for leaf function
2006-08-01 19:38 ` Franck Bui-Huu
@ 2006-08-02 1:51 ` Atsushi Nemoto
2006-08-02 10:21 ` Franck Bui-Huu
0 siblings, 1 reply; 23+ messages in thread
From: Atsushi Nemoto @ 2006-08-02 1:51 UTC (permalink / raw)
To: vagabon.xyz; +Cc: ralf, linux-mips
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 7/7] Allow unwind_stack() to return ra for leaf function
2006-08-02 1:51 ` Atsushi Nemoto
@ 2006-08-02 10:21 ` Franck Bui-Huu
2006-08-02 11:25 ` Atsushi Nemoto
0 siblings, 1 reply; 23+ messages in thread
From: Franck Bui-Huu @ 2006-08-02 10:21 UTC (permalink / raw)
To: Atsushi Nemoto; +Cc: vagabon.xyz, ralf, linux-mips
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];
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 7/7] Allow unwind_stack() to return ra for leaf function
2006-08-02 10:21 ` Franck Bui-Huu
@ 2006-08-02 11:25 ` Atsushi Nemoto
2006-08-02 13:08 ` Franck Bui-Huu
0 siblings, 1 reply; 23+ messages in thread
From: Atsushi Nemoto @ 2006-08-02 11:25 UTC (permalink / raw)
To: vagabon.xyz; +Cc: ralf, linux-mips
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 7/7] Allow unwind_stack() to return ra for leaf function
2006-08-02 11:25 ` Atsushi Nemoto
@ 2006-08-02 13:08 ` Franck Bui-Huu
2006-08-02 16:00 ` Atsushi Nemoto
0 siblings, 1 reply; 23+ messages in thread
From: Franck Bui-Huu @ 2006-08-02 13:08 UTC (permalink / raw)
To: Atsushi Nemoto; +Cc: vagabon.xyz, ralf, linux-mips
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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 7/7] Allow unwind_stack() to return ra for leaf function
2006-08-02 13:08 ` Franck Bui-Huu
@ 2006-08-02 16:00 ` Atsushi Nemoto
2006-08-02 16:56 ` Franck Bui-Huu
0 siblings, 1 reply; 23+ messages in thread
From: Atsushi Nemoto @ 2006-08-02 16:00 UTC (permalink / raw)
To: vagabon.xyz; +Cc: ralf, linux-mips
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 7/7] Allow unwind_stack() to return ra for leaf function
2006-08-02 16:00 ` Atsushi Nemoto
@ 2006-08-02 16:56 ` Franck Bui-Huu
2006-08-03 4:52 ` Atsushi Nemoto
0 siblings, 1 reply; 23+ messages in thread
From: Franck Bui-Huu @ 2006-08-02 16:56 UTC (permalink / raw)
To: Atsushi Nemoto; +Cc: vagabon.xyz, ralf, linux-mips
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 7/7] Allow unwind_stack() to return ra for leaf function
2006-08-02 16:56 ` Franck Bui-Huu
@ 2006-08-03 4:52 ` Atsushi Nemoto
0 siblings, 0 replies; 23+ messages in thread
From: Atsushi Nemoto @ 2006-08-03 4:52 UTC (permalink / raw)
To: vagabon.xyz; +Cc: ralf, linux-mips
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
^ permalink raw reply [flat|nested] 23+ messages in thread