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

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

Hi Atsushi ;)

Atsushi Nemoto wrote:
> Instead of dump all possible address in the stack, unwind the stack
> frame based on prologue code analysis, as like as get_chan() does.
> While the code analysis might fail for some reason, there is a new
> kernel option "raw_show_trace" to disable this feature.
> 
> Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
> 
> diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
> index 7ab67f7..8f1a5fe 100644
> --- a/arch/mips/kernel/process.c
> +++ b/arch/mips/kernel/process.c
> @@ -281,7 +281,7 @@ static struct mips_frame_info {
>  } *schedule_frame, mfinfo[64];
>  static int mfinfo_num;
>  
> -static int __init get_frame_info(struct mips_frame_info *info)
> +static int get_frame_info(struct mips_frame_info *info)
>  {
>  	int i;
>  	void *func = info->func;
> @@ -329,12 +329,6 @@ #endif
>  				ip->i_format.simmediate / sizeof(long);
>  		}
>  	}
> -	if (info->pc_offset == -1 || info->frame_size == 0) {
> -		if (func == schedule)
> -			printk("Can't analyze prologue code at %p\n", func);
> -		info->pc_offset = -1;
> -		info->frame_size = 0;
> -	}
>  
>  	return 0;
>  }
> @@ -367,8 +361,17 @@ #else
>  	mfinfo[0].func = schedule;
>  	schedule_frame = &mfinfo[0];
>  #endif
> -	for (i = 0; i < ARRAY_SIZE(mfinfo) && mfinfo[i].func; i++)
> -		get_frame_info(&mfinfo[i]);
> +	for (i = 0; i < ARRAY_SIZE(mfinfo) && mfinfo[i].func; i++) {
> +		struct mips_frame_info *info = &mfinfo[i];
> +		get_frame_info(info);
> +		if (info->pc_offset < 0 || info->frame_size == 0) {
> +			if (info->func == schedule)

This can't happen since "schedule" is not a leaf function. Something I'm
missing here but I would have said:

			if (func != schedule)

instead, no ?

> +				printk("Can't analyze prologue code at %p\n",
> +				       info->func);
> +			info->pc_offset = -1;
> +			info->frame_size = 0;
> +		}
> +	}
>  
>  	mfinfo_num = i;
>  	return 0;
> @@ -437,3 +440,41 @@ #endif
>  	return pc;
>  }
>  
> +#ifdef CONFIG_KALLSYMS
> +/* used by show_frametrace() */
> +unsigned long unwind_stack(struct task_struct *task,
> +			   unsigned long **sp, unsigned long pc)
> +{
> +	unsigned long stack_page;
> +	struct mips_frame_info info;
> +	char *modname;
> +	char namebuf[KSYM_NAME_LEN + 1];
> +	unsigned long size, ofs;
> +
> +	stack_page = (unsigned long)task_stack_page(task);
> +	if (!stack_page)
> +		return 0;
> +
> +	if (!kallsyms_lookup(pc, &size, &ofs, &modname, namebuf))
> +		return 0;
> +	if (ofs == 0)
> +		return 0;
> +
> +	info.func = (void *)(pc - ofs);
> +	info.func_size = ofs;	/* analyze from start to ofs */
> +	get_frame_info(&info);
> +	if (info.pc_offset < 0 || !info.frame_size) {
> +		/* leaf? */

for leaf case, can't we simply do this test:

	if (info.pc_offset < 0) {

IOW, can a leaf function move sp ? I would say yes...

BTW why not let this logic inside get_frame_info() ? Hence this function
could return:

	if (info.frame_size && info.pc_offset > 0) /* nested */
		return 0;
	if (info.pc_offset < 0) /* leaf */
		return 1;
	/* prologue seems boggus... */
	printk("Can't analyze prologue code at %p\n", info->func);
	return -1;

> +		*sp += info.frame_size / sizeof(long);
> +		return 0;

why not returning:
		return regs->regs[31];

and removes the leaf detection logic in show_frametrace() ?

> +	}
> +	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];
> +	*sp += info.frame_size / sizeof(long);
> +	return pc;

why not directly doing:

	return (*sp)[info.pc_offset];

and remove:

	pc = (*sp)[info.pc_offset];

> +}
> +#endif
> diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
> index c6f7046..bf36fcc 100644
> --- a/arch/mips/kernel/traps.c
> +++ b/arch/mips/kernel/traps.c
> @@ -98,24 +98,53 @@ #endif
>  	printk("\n");
>  }
>  
> +#ifdef CONFIG_KALLSYMS
> +static int raw_show_trace;
> +static int __init set_raw_show_trace(char *str)
> +{
> +	raw_show_trace = 1;
> +	return 1;
> +}
> +__setup("raw_show_trace", set_raw_show_trace);
> +
> +extern unsigned long unwind_stack(struct task_struct *task,
> +				  unsigned long **sp, unsigned long pc);
> +static void show_frametrace(struct task_struct *task, struct pt_regs *regs)
> +{
> +	const int field = 2 * sizeof(unsigned long);
> +	unsigned long *stack = (long *)regs->regs[29];

why not calling that "sp" ?

> +	unsigned long pc = regs->cp0_epc;
> +	int top = 1;
> +
> +	if (raw_show_trace || !__kernel_text_address(pc)) {
> +		show_trace(stack);
> +		return;
> +	}
> +	printk("Call Trace:\n");
> +	while (__kernel_text_address(pc)) {
> +		printk(" [<%0*lx>] ", field, pc);
> +		print_symbol("%s\n", pc);
> +		pc = unwind_stack(task, &stack, pc);
> +		if (top && pc == 0)
> +			pc = regs->regs[31];	/* leaf? */
> +		top = 0;
> +	}
> +	printk("\n");
> +}
> +#else
> +#define show_frametrace(task, r) show_trace((long *)(r)->regs[29]);
> +#endif
> +
>  /*
>   * This routine abuses get_user()/put_user() to reference pointers
>   * with at least a bit of error checking ...
>   */
> -void show_stack(struct task_struct *task, unsigned long *sp)
> +static void show_stacktrace(struct task_struct *task, struct pt_regs *regs)
>  {
>  	const int field = 2 * sizeof(unsigned long);
>  	long stackdata;
>  	int i;
> -	unsigned long *stack;
> -
> -	if (!sp) {
> -		if (task && task != current)
> -			sp = (unsigned long *) task->thread.reg29;
> -		else
> -			sp = (unsigned long *) &sp;
> -	}
> -	stack = sp;
> +	unsigned long *sp = (unsigned long *)regs->regs[29];
>  
>  	printk("Stack :");
>  	i = 0;
> @@ -136,7 +165,44 @@ void show_stack(struct task_struct *task
>  		i++;
>  	}
>  	printk("\n");
> -	show_trace(stack);
> +	show_frametrace(task, regs);
> +}
> +
> +static noinline void dump_stack_top(struct pt_regs *regs)

This sounds weird, you're actually dumping v0, ra, and sp, no ?
If so "dump_stack_top" seems not be appropriate, does it ?

> +{
> +	__asm__ __volatile__(
> +		"1: la $2, 1b\n\t"
> +#ifdef CONFIG_64BIT
> +		"sd $2, %0\n\t"
> +		"sd $29, %1\n\t"
> +		"sd $31, %2\n\t"
> +#else
> +		"sw $2, %0\n\t"
> +		"sw $29, %1\n\t"
> +		"sw $31, %2\n\t"
> +#endif
> +		: "=m" (regs->cp0_epc),
> +		"=m" (regs->regs[29]), "=m" (regs->regs[31])
> +		: : "memory");
> +}
> +
> +void show_stack(struct task_struct *task, unsigned long *sp)
> +{
> +	struct pt_regs regs;
> +	if (sp) {
> +		regs.regs[29] = (unsigned long)sp;
> +		regs.regs[31] = 0;
> +		regs.cp0_epc = 0;
> +	} else {
> +		if (task && task != current) {
> +			regs.regs[29] = task->thread.reg29;
> +			regs.regs[31] = 0;
> +			regs.cp0_epc = task->thread.reg31;
> +		} else {
> +			dump_stack_top(&regs);
> +		}
> +	}
> +	show_stacktrace(task, &regs);
>  }
>  
>  /*
> @@ -146,6 +212,14 @@ void dump_stack(void)
>  {
>  	unsigned long stack;
>  
> +#ifdef CONFIG_KALLSYMS
> +	if (!raw_show_trace) {
> +		struct pt_regs regs;
> +		dump_stack_top(&regs);
> +		show_frametrace(current, &regs);
> +		return;
> +	}
> +#endif
>  	show_trace(&stack);
>  }
>  
> @@ -265,7 +339,7 @@ void show_registers(struct pt_regs *regs
>  	print_modules();
>  	printk("Process %s (pid: %d, threadinfo=%p, task=%p)\n",
>  	        current->comm, current->pid, current_thread_info(), current);
> -	show_stack(current, (long *) regs->regs[29]);
> +	show_stacktrace(current, regs);
>  	show_code((unsigned int *) regs->cp0_epc);
>  	printk("\n");
>  }
> 
>