Re: [PATCH 1/2] linux-user/riscv: vdso: fix call frame info in __vdso_rt_sigreturn

[Richard Henderson <richard.henderson@linaro.org>]

On 1/17/24 10:52, Vineet Gupta wrote:
> Ok it seems the issue is really subtle.
> 
> With 8.2 trunk, the NOP needed before signal trampoline seems to be be
> factored into the unwind info for sigrestorer.
> 
>      0000003c 0000000000000098 00000000 CIE
>        Version:               3
>        Augmentation:          "zRS"
>        Code alignment factor: 1
>        Data alignment factor: -4
>        Return address column: 64
>        Augmentation data:     1b
>        DW_CFA_def_cfa: r2 (sp) ofs 832
>        DW_CFA_offset_extended: r64 at cfa-528
>        DW_CFA_offset: r1 (ra) at cfa-520
>        DW_CFA_offset: r2 (sp) at cfa-512
>      ...
>        DW_CFA_offset: r63 (ft11) at cfa-24
>        DW_CFA_nop
>        DW_CFA_nop
> 
>      000000d8 0000000000000010 000000a0 FDE cie=0000003c
>      pc=000000000000066c..0000000000000678
>                                                                                                                                       
>      ^^^    <--- NOP included
>        DW_CFA_nop
>        DW_CFA_nop
>        DW_CFA_nop
> 
>      0000000000000664 <__vdso_flush_icache>:
>       664:    00000513              li    a0,0
>       668:    00008067              ret
>       66c:    00000013              nop                 <--- this NOP
> 
>      0000000000000670 <__vdso_rt_sigreturn>:
>       670:    08b00893              li    a7,139
>       674:    00000073              ecall
> 
> 
> This is due to the .cfi_startproc bracketing. If we move the nop out of
> the .cfi_{start,end}proc, things start to work as well.
> 
>      diff --git a/linux-user/riscv/vdso.S b/linux-user/riscv/vdso.S
>      index 4b4e34aeea51..8c9f1038cb8c 100644
>      --- a/linux-user/riscv/vdso.S
>      +++ b/linux-user/riscv/vdso.S
>      @@ -92,6 +92,8 @@ endf __vdso_flush_icache
>       
>              .cfi_endproc
>       
>      +       nop
>      +
>       /*
>        * Start the unwind info at least one instruction before the signal
>        * trampoline, because the unwinder will assume we are returning
>      @@ -178,8 +180,6 @@ endf __vdso_flush_icache
>              .cfi_offset     62, B_FR + 30 * sizeof_freg
>              .cfi_offset     63, B_FR + 31 * sizeof_freg     /* f31 */
>       
>      -       nop
>      -
>       __vdso_rt_sigreturn:
>              raw_syscall __NR_rt_sigreturn
>       endf __vdso_rt_sigreturn
> 
> 
> This changes the cfi info slightly as follows:
> 
> 000000d8 0000000000000010 000000a0 FDE cie=0000003c
> pc=0000000000000670..0000000000000678  <-- excludes nop
>    DW_CFA_nop
>    DW_CFA_nop
>    DW_CFA_nop
> 
> 
> 0000000000000664 <__vdso_flush_icache>:
>   664:    00000513              li    a0,0
>   668:    00008067              ret
>   66c:    00000013              nop
> 
> 0000000000000670 <__vdso_rt_sigreturn>:
>   670:    08b00893              li    a7,139
>   674:    00000073              ecall
> 
> I concur this is still not 100% explanation of why things are going off,
> but I have exact same nop quirk for glibc ARC sigrestorer.
> Would an updated patch along those lines be more palatable.

No.

The explanation is right there in the block comment: "Start the unwind info at least one 
instruction before...".  The unwind info is taken from that nop insn.

By moving the nop outside the unwind info, you remove the effect of the unwind info, as 
the nop is now outside of any unwind blocks.  It is the same as removing all of the unwind 
info entirely, which results in the (current) libgcc fallback information being used.


r~