diff mbox

[PATCH/v2] Fix PR backtrace/16558: GDB Aarch64 signal frame unwinder issue

Message ID 534507E2.1000906@mentor.com
State Committed
Headers show

Commit Message

Hui Zhu April 9, 2014, 8:42 a.m. UTC
Ping.

Thanks,
Hui


-------- Original Message --------
Subject: Re: [PATCH/v2] Fix PR backtrace/16558: GDB Aarch64 signal frame unwinder issue
Date: Wed, 26 Mar 2014 16:53:55 +0800
From: Hui Zhu <hui_zhu@mentor.com>
To: Marcus Shawcroft <marcus.shawcroft@gmail.com>
CC: gdb-patches ml <gdb-patches@sourceware.org>, Yao Qi <Yao_Qi@mentor.com>

On 03/20/14 00:01, Marcus Shawcroft wrote:
> On 5 March 2014 06:33, Hui Zhu <hui_zhu@mentor.com> wrote:
>
>>    /* Signal frame handling.
>>   -      +----------+  ^
>> -      | saved lr |  |
>> -   +->| saved fp |--+
>> -   |  |          |
>> -   |  |          |
>> -   |  +----------+
>> -   |  | saved lr |
>> -   +--| saved fp |
>> -   ^  |          |
>> -   |  |          |
>> -   |  +----------+
>> -   ^  |          |
>> -   |  | signal   |
>> -   |  |          |
>> -   |  | saved lr |-->interrupted_function_pc
>> -   +--| saved fp |
>> -   |  +----------+
>> -   |  | saved lr |--> default_restorer (movz x8, NR_sys_rt_sigreturn; svc
>> 0)
>> -   +--| saved fp |<- FP
>> -      |          |
>> -      |          |<- SP
>> -      +----------+
>> -
>
> Better no diagram than a broken diagram, but wouldn't it be better to
> fix the diagram rather than just remove the whole comment?
>

Add the pic back according to Yao's pic:
       +------------+  ^
       | saved lr   |  |
    +->| saved fp   |--+
    |  |            |
    |  |            |
    |  +------------+
    |  | saved lr   |
    +--| saved fp   |
    ^  |            |
    |  |            |
    |  +------------+
    ^  |            |
    |  | signal     |
    |  |            |        SIGTRAMP_FRAME (struct rt_sigframe)
    |  | saved regs |
    +--| saved sp   |--> interrupted_sp
    |  | saved pc   |--> interrupted_pc
    |  |            |
    |  +------------+
    |  | saved lr   |--> default_restorer (movz x8, NR_sys_rt_sigreturn; svc 0)
    +--| saved fp   |<- FP
       |            |         NORMAL_FRAME
       |            |<- SP
       +------------+

I removed "saved lr |-->interrupted_function_pc".  Because the lr didn't save the
address of of interrupted_function.  It saved the caller address of interrupted_function.
This is not a special behavior of ABI.  So I think It does not need to be added here.

>>     On signal delivery, the kernel will create a signal handler stack
>> -  frame and setup the return address in LR to point at restorer stub.
>> +  frame in arch/arm64/kernel/signal.c:setup_rt_frame.
>
> I don;t think documenting the name of a function in a different source
> tree here in a comment is a good idea, should the kernel guys decide
> to refactor that code in the future the comment will be left bit
> rotten.  It would be better to say what we are expecting to find on
> the stack and in the registers rather than who we are expecting to
> setup the stack and registers.
>
>>     The signal stack frame is defined by:
>>      struct rt_sigframe
>> @@ -123,8 +100,8 @@
>>     d28015a8        movz    x8, #0xad
>>     d4000001        svc     #0x0
>>   -  We detect signal frames by snooping the return code for the restorer
>> -  instruction sequence.
>> +  This is a system call sys_rt_sigreturn.  The kernel will detect signal
>> +  frame from sp and call arch/arm64/kernel/signal.c:restore_sigframe.
>
> Likewise.

I added them back.

>
>>      The handler then needs to recover the saved register set from
>>     ucontext.uc_mcontext.  */
>> @@ -146,7 +123,6 @@ aarch64_linux_sigframe_init (const struc
>>
>>   {
>>     struct gdbarch *gdbarch = get_frame_arch (this_frame);
>>     CORE_ADDR sp = get_frame_register_unsigned (this_frame,
>> AARCH64_SP_REGNUM);
>> -  CORE_ADDR fp = get_frame_register_unsigned (this_frame,
>> AARCH64_FP_REGNUM);
>>     CORE_ADDR sigcontext_addr =
>>       sp
>>       + AARCH64_RT_SIGFRAME_UCONTEXT_OFFSET
>> @@ -160,12 +136,14 @@ aarch64_linux_sigframe_init (const struc
>>
>>   cd                               sigcontext_addr +
>> AARCH64_SIGCONTEXT_XO_OFFSET
>>                                 + i * AARCH64_SIGCONTEXT_REG_SIZE);
>>       }
>> +  trad_frame_set_reg_addr (this_cache, AARCH64_SP_REGNUM,
>> +                          sigcontext_addr + AARCH64_SIGCONTEXT_XO_OFFSET
>> +                            + 31 * AARCH64_SIGCONTEXT_REG_SIZE);
>> +  trad_frame_set_reg_addr (this_cache, AARCH64_PC_REGNUM,
>> +                          sigcontext_addr + AARCH64_SIGCONTEXT_XO_OFFSET
>> +                            + 32 * AARCH64_SIGCONTEXT_REG_SIZE);
>>   -  trad_frame_set_reg_addr (this_cache, AARCH64_FP_REGNUM, fp);
>> -  trad_frame_set_reg_addr (this_cache, AARCH64_LR_REGNUM, fp + 8);
>> -  trad_frame_set_reg_addr (this_cache, AARCH64_PC_REGNUM, fp + 8);
>> -
>> -  trad_frame_set_id (this_cache, frame_id_build (fp, func));
>> +  trad_frame_set_id (this_cache, frame_id_build (sp, func));
>
> The comments above aside, I think the actual functional change in this
> patch looks reasonable.   However I cannot approve patches in GDB.
>

Thanks for your help.

Best,
Hui

2014-03-26  Hui Zhu  <hui@codesourcery.com>
	    Yao Qi  <yao@codesourcery.com>

	PR backtrace/16558
	* aarch64-linux-tdep.c (aarch64_linux_sigframe_init): Update comments
	and change address of sp and pc.

Comments

Hui Zhu May 13, 2014, 6:17 a.m. UTC | #1
Ping.

Thanks,
Hui

On Wed, Apr 9, 2014 at 4:42 PM, Hui Zhu <hui_zhu@mentor.com> wrote:
> Ping.
>
> Thanks,
> Hui
>
>
>
> -------- Original Message --------
> Subject: Re: [PATCH/v2] Fix PR backtrace/16558: GDB Aarch64 signal frame
> unwinder issue
> Date: Wed, 26 Mar 2014 16:53:55 +0800
> From: Hui Zhu <hui_zhu@mentor.com>
> To: Marcus Shawcroft <marcus.shawcroft@gmail.com>
> CC: gdb-patches ml <gdb-patches@sourceware.org>, Yao Qi <Yao_Qi@mentor.com>
>
> On 03/20/14 00:01, Marcus Shawcroft wrote:
>>
>> On 5 March 2014 06:33, Hui Zhu <hui_zhu@mentor.com> wrote:
>>
>>>    /* Signal frame handling.
>>>   -      +----------+  ^
>>> -      | saved lr |  |
>>> -   +->| saved fp |--+
>>> -   |  |          |
>>> -   |  |          |
>>> -   |  +----------+
>>> -   |  | saved lr |
>>> -   +--| saved fp |
>>> -   ^  |          |
>>> -   |  |          |
>>> -   |  +----------+
>>> -   ^  |          |
>>> -   |  | signal   |
>>> -   |  |          |
>>> -   |  | saved lr |-->interrupted_function_pc
>>> -   +--| saved fp |
>>> -   |  +----------+
>>> -   |  | saved lr |--> default_restorer (movz x8, NR_sys_rt_sigreturn;
>>> svc
>>> 0)
>>> -   +--| saved fp |<- FP
>>> -      |          |
>>> -      |          |<- SP
>>> -      +----------+
>>> -
>>
>>
>> Better no diagram than a broken diagram, but wouldn't it be better to
>> fix the diagram rather than just remove the whole comment?
>>
>
> Add the pic back according to Yao's pic:
>       +------------+  ^
>       | saved lr   |  |
>    +->| saved fp   |--+
>    |  |            |
>    |  |            |
>    |  +------------+
>    |  | saved lr   |
>    +--| saved fp   |
>    ^  |            |
>    |  |            |
>    |  +------------+
>    ^  |            |
>    |  | signal     |
>    |  |            |        SIGTRAMP_FRAME (struct rt_sigframe)
>    |  | saved regs |
>    +--| saved sp   |--> interrupted_sp
>    |  | saved pc   |--> interrupted_pc
>    |  |            |
>    |  +------------+
>    |  | saved lr   |--> default_restorer (movz x8, NR_sys_rt_sigreturn; svc
> 0)
>    +--| saved fp   |<- FP
>       |            |         NORMAL_FRAME
>       |            |<- SP
>       +------------+
>
> I removed "saved lr |-->interrupted_function_pc".  Because the lr didn't
> save the
> address of of interrupted_function.  It saved the caller address of
> interrupted_function.
> This is not a special behavior of ABI.  So I think It does not need to be
> added here.
>
>>>     On signal delivery, the kernel will create a signal handler stack
>>> -  frame and setup the return address in LR to point at restorer stub.
>>> +  frame in arch/arm64/kernel/signal.c:setup_rt_frame.
>>
>>
>> I don;t think documenting the name of a function in a different source
>> tree here in a comment is a good idea, should the kernel guys decide
>> to refactor that code in the future the comment will be left bit
>> rotten.  It would be better to say what we are expecting to find on
>> the stack and in the registers rather than who we are expecting to
>> setup the stack and registers.
>>
>>>     The signal stack frame is defined by:
>>>      struct rt_sigframe
>>> @@ -123,8 +100,8 @@
>>>     d28015a8        movz    x8, #0xad
>>>     d4000001        svc     #0x0
>>>   -  We detect signal frames by snooping the return code for the restorer
>>> -  instruction sequence.
>>> +  This is a system call sys_rt_sigreturn.  The kernel will detect signal
>>> +  frame from sp and call arch/arm64/kernel/signal.c:restore_sigframe.
>>
>>
>> Likewise.
>
>
> I added them back.
>
>>
>>>      The handler then needs to recover the saved register set from
>>>     ucontext.uc_mcontext.  */
>>> @@ -146,7 +123,6 @@ aarch64_linux_sigframe_init (const struc
>>>
>>>   {
>>>     struct gdbarch *gdbarch = get_frame_arch (this_frame);
>>>     CORE_ADDR sp = get_frame_register_unsigned (this_frame,
>>> AARCH64_SP_REGNUM);
>>> -  CORE_ADDR fp = get_frame_register_unsigned (this_frame,
>>> AARCH64_FP_REGNUM);
>>>     CORE_ADDR sigcontext_addr =
>>>       sp
>>>       + AARCH64_RT_SIGFRAME_UCONTEXT_OFFSET
>>> @@ -160,12 +136,14 @@ aarch64_linux_sigframe_init (const struc
>>>
>>>   cd                               sigcontext_addr +
>>> AARCH64_SIGCONTEXT_XO_OFFSET
>>>                                 + i * AARCH64_SIGCONTEXT_REG_SIZE);
>>>       }
>>> +  trad_frame_set_reg_addr (this_cache, AARCH64_SP_REGNUM,
>>> +                          sigcontext_addr + AARCH64_SIGCONTEXT_XO_OFFSET
>>> +                            + 31 * AARCH64_SIGCONTEXT_REG_SIZE);
>>> +  trad_frame_set_reg_addr (this_cache, AARCH64_PC_REGNUM,
>>> +                          sigcontext_addr + AARCH64_SIGCONTEXT_XO_OFFSET
>>> +                            + 32 * AARCH64_SIGCONTEXT_REG_SIZE);
>>>   -  trad_frame_set_reg_addr (this_cache, AARCH64_FP_REGNUM, fp);
>>> -  trad_frame_set_reg_addr (this_cache, AARCH64_LR_REGNUM, fp + 8);
>>> -  trad_frame_set_reg_addr (this_cache, AARCH64_PC_REGNUM, fp + 8);
>>> -
>>> -  trad_frame_set_id (this_cache, frame_id_build (fp, func));
>>> +  trad_frame_set_id (this_cache, frame_id_build (sp, func));
>>
>>
>> The comments above aside, I think the actual functional change in this
>> patch looks reasonable.   However I cannot approve patches in GDB.
>>
>
> Thanks for your help.
>
> Best,
> Hui
>
> 2014-03-26  Hui Zhu  <hui@codesourcery.com>
>             Yao Qi  <yao@codesourcery.com>
>
>         PR backtrace/16558
>         * aarch64-linux-tdep.c (aarch64_linux_sigframe_init): Update
> comments
>         and change address of sp and pc.
>
> --- a/gdb/aarch64-linux-tdep.c
> +++ b/gdb/aarch64-linux-tdep.c
> @@ -53,28 +53,30 @@
>   /* Signal frame handling.
>  -      +----------+  ^
> -      | saved lr |  |
> -   +->| saved fp |--+
> -   |  |          |
> -   |  |          |
> -   |  +----------+
> -   |  | saved lr |
> -   +--| saved fp |
> -   ^  |          |
> -   |  |          |
> -   |  +----------+
> -   ^  |          |
> -   |  | signal   |
> -   |  |          |
> -   |  | saved lr |-->interrupted_function_pc
> -   +--| saved fp |
> -   |  +----------+
> -   |  | saved lr |--> default_restorer (movz x8, NR_sys_rt_sigreturn; svc
> 0)
> -   +--| saved fp |<- FP
> -      |          |
> -      |          |<- SP
> -      +----------+
> +      +------------+  ^
> +      | saved lr   |  |
> +   +->| saved fp   |--+
> +   |  |            |
> +   |  |            |
> +   |  +------------+
> +   |  | saved lr   |
> +   +--| saved fp   |
> +   ^  |            |
> +   |  |            |
> +   |  +------------+
> +   ^  |            |
> +   |  | signal     |
> +   |  |            |        SIGTRAMP_FRAME (struct rt_sigframe)
> +   |  | saved regs |
> +   +--| saved sp   |--> interrupted_sp
> +   |  | saved pc   |--> interrupted_pc
> +   |  |            |
> +   |  +------------+
> +   |  | saved lr   |--> default_restorer (movz x8, NR_sys_rt_sigreturn; svc
> 0)
> +   +--| saved fp   |<- FP
> +      |            |         NORMAL_FRAME
> +      |            |<- SP
> +      +------------+
>     On signal delivery, the kernel will create a signal handler stack
>    frame and setup the return address in LR to point at restorer stub.
> @@ -123,6 +125,8 @@
>    d28015a8        movz    x8, #0xad
>    d4000001        svc     #0x0
>  +  This is a system call sys_rt_sigreturn.
> +
>    We detect signal frames by snooping the return code for the restorer
>    instruction sequence.
>  @@ -146,7 +150,6 @@ aarch64_linux_sigframe_init (const struc
>  {
>    struct gdbarch *gdbarch = get_frame_arch (this_frame);
>    CORE_ADDR sp = get_frame_register_unsigned (this_frame,
> AARCH64_SP_REGNUM);
> -  CORE_ADDR fp = get_frame_register_unsigned (this_frame,
> AARCH64_FP_REGNUM);
>    CORE_ADDR sigcontext_addr =
>      sp
>      + AARCH64_RT_SIGFRAME_UCONTEXT_OFFSET
> @@ -160,12 +163,14 @@ aarch64_linux_sigframe_init (const struc
>                                sigcontext_addr +
> AARCH64_SIGCONTEXT_XO_OFFSET
>                                + i * AARCH64_SIGCONTEXT_REG_SIZE);
>      }
> +  trad_frame_set_reg_addr (this_cache, AARCH64_SP_REGNUM,
> +                          sigcontext_addr + AARCH64_SIGCONTEXT_XO_OFFSET
> +                            + 31 * AARCH64_SIGCONTEXT_REG_SIZE);
> +  trad_frame_set_reg_addr (this_cache, AARCH64_PC_REGNUM,
> +                          sigcontext_addr + AARCH64_SIGCONTEXT_XO_OFFSET
> +                            + 32 * AARCH64_SIGCONTEXT_REG_SIZE);
>  -  trad_frame_set_reg_addr (this_cache, AARCH64_FP_REGNUM, fp);
> -  trad_frame_set_reg_addr (this_cache, AARCH64_LR_REGNUM, fp + 8);
> -  trad_frame_set_reg_addr (this_cache, AARCH64_PC_REGNUM, fp + 8);
> -
> -  trad_frame_set_id (this_cache, frame_id_build (fp, func));
> +  trad_frame_set_id (this_cache, frame_id_build (sp, func));
>  }
>   static const struct tramp_frame aarch64_linux_rt_sigframe =
>
>
>
diff mbox

Patch

--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -53,28 +53,30 @@ 
  
  /* Signal frame handling.
  
-      +----------+  ^
-      | saved lr |  |
-   +->| saved fp |--+
-   |  |          |
-   |  |          |
-   |  +----------+
-   |  | saved lr |
-   +--| saved fp |
-   ^  |          |
-   |  |          |
-   |  +----------+
-   ^  |          |
-   |  | signal   |
-   |  |          |
-   |  | saved lr |-->interrupted_function_pc
-   +--| saved fp |
-   |  +----------+
-   |  | saved lr |--> default_restorer (movz x8, NR_sys_rt_sigreturn; svc 0)
-   +--| saved fp |<- FP
-      |          |
-      |          |<- SP
-      +----------+
+      +------------+  ^
+      | saved lr   |  |
+   +->| saved fp   |--+
+   |  |            |
+   |  |            |
+   |  +------------+
+   |  | saved lr   |
+   +--| saved fp   |
+   ^  |            |
+   |  |            |
+   |  +------------+
+   ^  |            |
+   |  | signal     |
+   |  |            |        SIGTRAMP_FRAME (struct rt_sigframe)
+   |  | saved regs |
+   +--| saved sp   |--> interrupted_sp
+   |  | saved pc   |--> interrupted_pc
+   |  |            |
+   |  +------------+
+   |  | saved lr   |--> default_restorer (movz x8, NR_sys_rt_sigreturn; svc 0)
+   +--| saved fp   |<- FP
+      |            |         NORMAL_FRAME
+      |            |<- SP
+      +------------+
  
    On signal delivery, the kernel will create a signal handler stack
    frame and setup the return address in LR to point at restorer stub.
@@ -123,6 +125,8 @@ 
    d28015a8        movz    x8, #0xad
    d4000001        svc     #0x0
  
+  This is a system call sys_rt_sigreturn.
+
    We detect signal frames by snooping the return code for the restorer
    instruction sequence.
  
@@ -146,7 +150,6 @@  aarch64_linux_sigframe_init (const struc
  {
    struct gdbarch *gdbarch = get_frame_arch (this_frame);
    CORE_ADDR sp = get_frame_register_unsigned (this_frame, AARCH64_SP_REGNUM);
-  CORE_ADDR fp = get_frame_register_unsigned (this_frame, AARCH64_FP_REGNUM);
    CORE_ADDR sigcontext_addr =
      sp
      + AARCH64_RT_SIGFRAME_UCONTEXT_OFFSET
@@ -160,12 +163,14 @@  aarch64_linux_sigframe_init (const struc
  			       sigcontext_addr + AARCH64_SIGCONTEXT_XO_OFFSET
  			       + i * AARCH64_SIGCONTEXT_REG_SIZE);
      }
+  trad_frame_set_reg_addr (this_cache, AARCH64_SP_REGNUM,
+			   sigcontext_addr + AARCH64_SIGCONTEXT_XO_OFFSET
+			     + 31 * AARCH64_SIGCONTEXT_REG_SIZE);
+  trad_frame_set_reg_addr (this_cache, AARCH64_PC_REGNUM,
+			   sigcontext_addr + AARCH64_SIGCONTEXT_XO_OFFSET
+			     + 32 * AARCH64_SIGCONTEXT_REG_SIZE);
  
-  trad_frame_set_reg_addr (this_cache, AARCH64_FP_REGNUM, fp);
-  trad_frame_set_reg_addr (this_cache, AARCH64_LR_REGNUM, fp + 8);
-  trad_frame_set_reg_addr (this_cache, AARCH64_PC_REGNUM, fp + 8);
-
-  trad_frame_set_id (this_cache, frame_id_build (fp, func));
+  trad_frame_set_id (this_cache, frame_id_build (sp, func));
  }
  
  static const struct tramp_frame aarch64_linux_rt_sigframe =