This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH/v2] Fix PR backtrace/16558: GDB Aarch64 signal frame unwinder issue
- From: Hui Zhu <teawater at gmail dot com>
- To: Marcus Shawcroft <marcus dot shawcroft at gmail dot com>
- Cc: Yao Qi <Yao_Qi at mentor dot com>, gdb-patches ml <gdb-patches at sourceware dot org>, Hui Zhu <hui_zhu at mentor dot com>
- Date: Tue, 13 May 2014 14:17:21 +0800
- Subject: Re: [PATCH/v2] Fix PR backtrace/16558: GDB Aarch64 signal frame unwinder issue
- Authentication-results: sourceware.org; auth=none
- References: <533295A3 dot 90907 at mentor dot com> <534507E2 dot 1000906 at mentor dot com>
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 =
>
>
>