linux-mips
[Top] [All Lists]

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

To: David Daney <ddaney.cavm@gmail.com>
Subject: Re: [PATCH v2 1/2] MIPS: detect sibling call in get_frame_info
From: Tony Wu <tung7970@gmail.com>
Date: Mon, 13 May 2013 00:53:27 +0800
Cc: ralf@linux-mips.org, linux-mips@linux-mips.org
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:x-received:in-reply-to:references:date:message-id :subject:from:to:cc:content-type; bh=JUDx5MxPfg8iFtjBMc4gFfqGgie3kjVWYlMO9gRtCYc=; b=GYvetAEsLoMrpeH9Z0qwhwBdWPKatdKsE1DWG2IAhEUrITk/GUPr2q7Zr024NyDDOa 5lV73yBHvfs09baDCnCVbn7F3G58c7/yuqFCehCByWK/4u/5IJkLXVB5T2/WLPMURL0I 3lAIlY2a8TLRF4aGt1pD0BoO3NkkEyc4K0MQV3UW/rMqJdTDrecqSRm7CUz3gTo1A2b0 7ZHmVAEK01uFl7Jt0WHbk940DiW1ARSJfTeaOAmTbH/yZxKFfqeCR3q1d25DvAEUD8mC 3zeD35a4mfnlwdsI/S41liCxXcZ/cCxTzfiyWQApeghJgw0fDx/Zkbhom/if7GatD9b+ Uz2w==
In-reply-to: <518D1F1D.2080504@gmail.com>
List-archive: <http://www.linux-mips.org/archives/linux-mips/>
List-help: <mailto:ecartis@linux-mips.org?Subject=help>
List-id: linux-mips <linux-mips.eddie.linux-mips.org>
List-owner: <mailto:ralf@linux-mips.org>
List-post: <mailto:linux-mips@linux-mips.org>
List-software: Ecartis version 1.0.0
List-subscribe: <mailto:ecartis@linux-mips.org?subject=subscribe%20linux-mips>
List-unsubscribe: <mailto:ecartis@linux-mips.org?subject=unsubscribe%20linux-mips>
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <20130510110729.GA7499@hades> <518D1F1D.2080504@gmail.com>
Sender: linux-mips-bounce@linux-mips.org

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;



<Prev in Thread] Current Thread [Next in Thread>
  • Re: [PATCH v2 1/2] MIPS: detect sibling call in get_frame_info, Tony Wu <=