[PATCH/v2] Fix PR backtrace/16558: GDB Aarch64 signal frame unwinder issue
Commit Message
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" == Hui Zhu <hui_zhu@mentor.com> writes:
Hui> 2014-03-26 Hui Zhu <hui@codesourcery.com>
Hui> Yao Qi <yao@codesourcery.com>
Hui> PR backtrace/16558
Hui> * aarch64-linux-tdep.c (aarch64_linux_sigframe_init): Update comments
Hui> and change address of sp and pc.
I read through the thread and looked at the code a bit.
This patch is ok. Thanks.
Tom
On Mon, May 19, 2014 at 10:14 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Hui" == Hui Zhu <hui_zhu@mentor.com> writes:
>
> Hui> 2014-03-26 Hui Zhu <hui@codesourcery.com>
> Hui> Yao Qi <yao@codesourcery.com>
>
> Hui> PR backtrace/16558
> Hui> * aarch64-linux-tdep.c (aarch64_linux_sigframe_init): Update comments
> Hui> and change address of sp and pc.
>
> I read through the thread and looked at the code a bit.
> This patch is ok. Thanks.
>
Thanks.
Committed https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=f2205de0080d999c9b67872c9db471c31b53e378
Best,
Hui
> Tom
@@ -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 =