Re: [PATCH v2 1/2] MIPS: detect sibling call in get_frame_info

[David Daney <ddaney.cavm@gmail.com>]

On 05/10/2013 04:07 AM, Tony Wu wrote:
> Given a function, get_frame_info() analyzes its instructions
> to figure out frame size and return address. get_frame_info()
> works as follows:
>
> 1. analyze up to 128 instructions if the function size is unknown
> 2. search for 'addiu/daddiu sp,sp,-immed' for frame size
> 3. search for 'sw ra,offset(sp)' for return address
> 4. end search when it sees jr/jal/jalr
>
> This leads to an issue when the given function is a sibling
> call, example given as follows.
>
> 801ca110 <schedule>:
> 801ca110:       8f820000        lw      v0,0(gp)
> 801ca114:       8c420000        lw      v0,0(v0)
> 801ca118:       080726f0        j       801c9bc0 <__schedule>
> 801ca11c:       00000000        nop
>
> 801ca120 <io_schedule>:
> 801ca120:       27bdffe8        addiu   sp,sp,-24
> 801ca124:       3c028022        lui     v0,0x8022
> 801ca128:       afbf0014        sw      ra,20(sp)
>
> In this case, get_frame_info() cannot properly detect schedule's
> frame info, and eventually returns io_schedule's info instead.
>
> This patch adds sibling call check by detecting out of range jump.

I think this is more complex than it needs to be.  Also you already 
handle the case of a sib call via a function pointer ....


>
> Signed-off-by: Tony Wu <tung7970@gmail.com>
> ---
>   arch/mips/kernel/process.c |   22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
>
> diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
> index cfc742d..a794eb5 100644
> --- a/arch/mips/kernel/process.c
> +++ b/arch/mips/kernel/process.c
> @@ -223,6 +223,9 @@ struct mips_frame_info {
>   	int		pc_offset;
>   };
>
> +#define J_TARGET(pc,target)	\
> +		(((unsigned long)(pc) & 0xf0000000) | ((target) << 2))
> +
>   static inline int is_ra_save_ins(union mips_instruction *ip)
>   {
>   	/* sw / sd $ra, offset($sp) */
> @@ -250,11 +253,25 @@ static inline int is_sp_move_ins(union mips_instruction *ip)
>   	return 0;
>   }
>
> +static inline int is_sibling_j_ins(union mips_instruction *ip,
> +				   unsigned long func_begin, unsigned long func_end)
> +{
> +	if (ip->j_format.opcode == j_op) {
> +		unsigned long addr;
> +
> +		addr = J_TARGET(ip, ip->j_format.target);
> +		if (addr < func_begin || addr > func_end)
> +			return 1;
> +	}
> +	return 0;
> +}
> +
>   static int get_frame_info(struct mips_frame_info *info)
>   {
>   	union mips_instruction *ip = info->func;
>   	unsigned max_insns = info->func_size / sizeof(union mips_instruction);
>   	unsigned i;
> +	unsigned long func_begin, func_end;
>
>   	info->pc_offset = -1;
>   	info->frame_size = 0;
> @@ -266,10 +283,15 @@ static int get_frame_info(struct mips_frame_info *info)
>   		max_insns = 128U;	/* unknown function size */
>   	max_insns = min(128U, max_insns);
>
> +	func_begin = (unsigned long) info->func;
> +	func_end = func_begin + max_insns * sizeof(union mips_instruction);
> +
>   	for (i = 0; i < max_insns; i++, ip++) {
>
>   		if (is_jal_jalr_jr_ins(ip))
>   			break;

... here.  So why not just add an unconditional J to the list detected, 
and get rid of all the rest of the patch?


> +		if (is_sibling_j_ins(ip, func_begin, func_end))
> +			break;
>   		if (!info->frame_size) {
>   			if (is_sp_move_ins(ip))
>   				info->frame_size = - ip->i_format.simmediate;
>