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

[Marcus Shawcroft <marcus dot shawcroft at gmail dot com>]

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?

>    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.

>     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.

Cheers
/Marcus