* [PATCH v2 1/2] MIPS: detect sibling call in get_frame_info
@ 2013-05-10 11:07 Tony Wu
2013-05-10 16:23 ` David Daney
0 siblings, 1 reply; 3+ messages in thread
From: Tony Wu @ 2013-05-10 11:07 UTC (permalink / raw)
To: ralf, linux-mips
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.
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;
+ 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;
--
1.7.10.2 (Apple Git-33)
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2 1/2] MIPS: detect sibling call in get_frame_info
2013-05-10 11:07 [PATCH v2 1/2] MIPS: detect sibling call in get_frame_info Tony Wu
@ 2013-05-10 16:23 ` David Daney
2013-05-12 16:53 ` Tony Wu
0 siblings, 1 reply; 3+ messages in thread
From: David Daney @ 2013-05-10 16:23 UTC (permalink / raw)
To: Tony Wu; +Cc: ralf, linux-mips
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;
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2 1/2] MIPS: detect sibling call in get_frame_info
2013-05-10 16:23 ` David Daney
@ 2013-05-12 16:53 ` Tony Wu
0 siblings, 0 replies; 3+ messages in thread
From: Tony Wu @ 2013-05-12 16:53 UTC (permalink / raw)
To: David Daney; +Cc: ralf, linux-mips
[-- Attachment #1: Type: text/plain, Size: 4118 bytes --]
On Sat, May 11, 2013 at 12:23 AM, David Daney <ddaney.cavm@gmail.com> wrote:
> 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?
I was hoping to catch out of range jump only, but I guess we could do
unconditional J since what we really care is sp and ra. And they happen
early in the given function if they ever exist. I have sent the v3 patch
according to your suggestion. It looks better.
>
> + 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;
>>
>>
>
[-- Attachment #2: Type: text/html, Size: 5382 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-06-20 15:55 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-05-10 11:07 [PATCH v2 1/2] MIPS: detect sibling call in get_frame_info Tony Wu
2013-05-10 16:23 ` David Daney
2013-05-12 16:53 ` Tony Wu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox