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

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

[Vineet Gupta]

When testing gcc testsuite against QEMU v8.2 we found some additional
failures vs. v8.1.2.

| FAIL: gcc.dg/cleanup-10.c execution test
| FAIL: gcc.dg/cleanup-11.c execution test
| FAIL: gcc.dg/cleanup-8.c execution test
| FAIL: gcc.dg/cleanup-9.c execution test

All of these tests involve unwinding off signal stack and v8.2 did
introduce a vdso with sigreturn trampoline and associated unwinding
info. It seems that info is not correct and making it similar to
to one in the linux kernel fixes the above failures.

Fixes: 468c1bb5cac9 ("linux-user/riscv: Add vdso")
Reported-by: Edwin Lu <ewlu@rivosinc.com>
Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
---
 linux-user/riscv/vdso.S | 87 ++---------------------------------------
 1 file changed, 4 insertions(+), 83 deletions(-)

diff --git a/linux-user/riscv/vdso.S b/linux-user/riscv/vdso.S
index a86d8fc488e0..20119010c11b 100644
--- a/linux-user/riscv/vdso.S
+++ b/linux-user/riscv/vdso.S
@@ -97,91 +97,12 @@ endf __vdso_flush_icache
  * trampoline, because the unwinder will assume we are returning
  * after a call site.
  */
-
-	.cfi_startproc simple
-	.cfi_signal_frame
-
-#define sizeof_reg	(__riscv_xlen / 4)
-#define sizeof_freg	8
-#define B_GR	(offsetof_uc_mcontext - sizeof_rt_sigframe)
-#define B_FR	(offsetof_uc_mcontext - sizeof_rt_sigframe + offsetof_freg0)
-
-	.cfi_def_cfa	2, sizeof_rt_sigframe
-
-	/* Return address */
-	.cfi_return_column 64
-	.cfi_offset	64, B_GR + 0			/* pc */
-
-	/* Integer registers */
-	.cfi_offset	1, B_GR + 1 * sizeof_reg	/* r1 (ra) */
-	.cfi_offset	2, B_GR + 2 * sizeof_reg	/* r2 (sp) */
-	.cfi_offset	3, B_GR + 3 * sizeof_reg
-	.cfi_offset	4, B_GR + 4 * sizeof_reg
-	.cfi_offset	5, B_GR + 5 * sizeof_reg
-	.cfi_offset	6, B_GR + 6 * sizeof_reg
-	.cfi_offset	7, B_GR + 7 * sizeof_reg
-	.cfi_offset	8, B_GR + 8 * sizeof_reg
-	.cfi_offset	9, B_GR + 9 * sizeof_reg
-	.cfi_offset	10, B_GR + 10 * sizeof_reg
-	.cfi_offset	11, B_GR + 11 * sizeof_reg
-	.cfi_offset	12, B_GR + 12 * sizeof_reg
-	.cfi_offset	13, B_GR + 13 * sizeof_reg
-	.cfi_offset	14, B_GR + 14 * sizeof_reg
-	.cfi_offset	15, B_GR + 15 * sizeof_reg
-	.cfi_offset	16, B_GR + 16 * sizeof_reg
-	.cfi_offset	17, B_GR + 17 * sizeof_reg
-	.cfi_offset	18, B_GR + 18 * sizeof_reg
-	.cfi_offset	19, B_GR + 19 * sizeof_reg
-	.cfi_offset	20, B_GR + 20 * sizeof_reg
-	.cfi_offset	21, B_GR + 21 * sizeof_reg
-	.cfi_offset	22, B_GR + 22 * sizeof_reg
-	.cfi_offset	23, B_GR + 23 * sizeof_reg
-	.cfi_offset	24, B_GR + 24 * sizeof_reg
-	.cfi_offset	25, B_GR + 25 * sizeof_reg
-	.cfi_offset	26, B_GR + 26 * sizeof_reg
-	.cfi_offset	27, B_GR + 27 * sizeof_reg
-	.cfi_offset	28, B_GR + 28 * sizeof_reg
-	.cfi_offset	29, B_GR + 29 * sizeof_reg
-	.cfi_offset	30, B_GR + 30 * sizeof_reg
-	.cfi_offset	31, B_GR + 31 * sizeof_reg	/* r31 */
-
-	.cfi_offset	32, B_FR + 0			/* f0 */
-	.cfi_offset	33, B_FR + 1 * sizeof_freg	/* f1 */
-	.cfi_offset	34, B_FR + 2 * sizeof_freg
-	.cfi_offset	35, B_FR + 3 * sizeof_freg
-	.cfi_offset	36, B_FR + 4 * sizeof_freg
-	.cfi_offset	37, B_FR + 5 * sizeof_freg
-	.cfi_offset	38, B_FR + 6 * sizeof_freg
-	.cfi_offset	39, B_FR + 7 * sizeof_freg
-	.cfi_offset	40, B_FR + 8 * sizeof_freg
-	.cfi_offset	41, B_FR + 9 * sizeof_freg
-	.cfi_offset	42, B_FR + 10 * sizeof_freg
-	.cfi_offset	43, B_FR + 11 * sizeof_freg
-	.cfi_offset	44, B_FR + 12 * sizeof_freg
-	.cfi_offset	45, B_FR + 13 * sizeof_freg
-	.cfi_offset	46, B_FR + 14 * sizeof_freg
-	.cfi_offset	47, B_FR + 15 * sizeof_freg
-	.cfi_offset	48, B_FR + 16 * sizeof_freg
-	.cfi_offset	49, B_FR + 17 * sizeof_freg
-	.cfi_offset	50, B_FR + 18 * sizeof_freg
-	.cfi_offset	51, B_FR + 19 * sizeof_freg
-	.cfi_offset	52, B_FR + 20 * sizeof_freg
-	.cfi_offset	53, B_FR + 21 * sizeof_freg
-	.cfi_offset	54, B_FR + 22 * sizeof_freg
-	.cfi_offset	55, B_FR + 23 * sizeof_freg
-	.cfi_offset	56, B_FR + 24 * sizeof_freg
-	.cfi_offset	57, B_FR + 25 * sizeof_freg
-	.cfi_offset	58, B_FR + 26 * sizeof_freg
-	.cfi_offset	59, B_FR + 27 * sizeof_freg
-	.cfi_offset	60, B_FR + 28 * sizeof_freg
-	.cfi_offset	61, B_FR + 29 * sizeof_freg
-	.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
+	.cfi_startproc
+	.cfi_signal_frame
+       raw_syscall __NR_rt_sigreturn
+       .cfi_endproc
 endf __vdso_rt_sigreturn
 
-	.cfi_endproc
-- 
2.34.1

[PATCH 2/2] linux-user/riscv: rebuild vdso binaries after prev fix

[Vineet Gupta]

Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
---
Splitting this from prev patch in case maintainers want to regenerate
the vdso at their end. Or if they choose to, this can be squashed with
prev change too.
---

Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
---
 linux-user/riscv/vdso-32.so | Bin 2900 -> 2836 bytes
 linux-user/riscv/vdso-64.so | Bin 3856 -> 3792 bytes
 2 files changed, 0 insertions(+), 0 deletions(-)

diff --git a/linux-user/riscv/vdso-32.so b/linux-user/riscv/vdso-32.so
index 1ad1e5cbbbb8b1fe36b0fe4bcb6c06fab8219ecd..ee158b8374d14973492a4d05f705bf12c6cf42b2 100755
GIT binary patch
delta 563
zcmca2HbrcL0%OiZMRVRQEDUhqGci@34J5FIWnx#Ah{>aRivX+7%f)pfCby{;th~y{
zaAdLs;~pUuppponE+GaU1}z}n!7}+7qnyzipfCqiSQJRt0Qo#nz7LRIfFy4R<WFJQ
zY{rzwDB1&LTLAGBAPxXx5k>}vipe{f<y}F#Ko&6s@F}G5F)#=KnFc@%(xC&yAOI2r
z0p_ZpU<Sq-Mn-8~CXko}5KjSOj>!vIX7G3eSw)$}$z=?Tj+0$kO*!MMa&t>_3=0e<
zH!_M&p1`_+QDw3tn?8^%WRtG<1Q`Hy1_&@ReEc5-qz?d{4+0>54v?O~4CVp}E{1v_
z>A(cxgXE{eq=2*_kiP&(BYS@fkdGXSCxHAZh~TnkxCRtJ4$oIWeg{GU!#^M&IeZzC
zla0z;OWoY{@)=YrCVR3PB7C^ni@l3+^CgaDjEotR1G$7J_i*vBf^1irJeNzI6%^q<
llO4H*C*R=`U}Tv5mP?)O02>2?Fymw?ZY8G6?33-d#Q-6rVafmi

delta 697
zcmbOtc13K00^^#Aisrl)tPF7AJTX<C4J2T}I<c!t#JNs;F>9{TMW>6xcV8$Z&ddsZ
z{cy4b;~t?EEDQ`0KwUx%JPcYunum4rGe$Wh1E5Y0sIVx=P#~WN%J%`%DoFBnK)wX)
zW;3QdMo|Gqpp8I&29OQ_(o=x+pUFF!<?Am1c_51z0{9eC_!t-jfJ_4*2I<fNi37nD
z2*to$6%@?CSmVGb&BL^TmDM4oF{vq`Ij$w9HL5M5J**?7GpH+|+pou`*Q?K?-)(}+
zM5jp(lkKM1OtqS3G2Lv2$xNeJ2DA0%=*-ocr!ik`fy+WCuSE`v?UvXqwOVGe+-!x(
zN~2W<tM%6Ctkqhlv0iP1%0{J43Y+D&$ZVC`Cb3;?hsaK$T>`uL_VDcG+Q+e<4eZ!C
zK*vo0VxGyeEHikVfUKg-;^Z<0MvKV@SxhH4GKx(WVco#EV)9W|{d!QaLI5b#5P*f@
z<9~i2R|4p35MTuIC4jU8Fg!s3#5V%cGnm0#Ai>4p3M3tvAbgN~G)xLe3j+CRKpHtd
zsu&oe8Ia?(52!!_DFQ)0L5|=JKt2zW{1G4@Il>u|lLLw?eG;?v@)=YrHgm9dF>ap1
zv4oNF&169?;mJN+Jggw=R!olNQfCFFIp@iY+`^ORa0xKhOkN8VvtegoxCs_}!zI8Z
K$}#yLmlyyeGL7y4

diff --git a/linux-user/riscv/vdso-64.so b/linux-user/riscv/vdso-64.so
index 83992bebe6d0182f24edfffc531015fd2f4e1cfb..f2e250fc6ca1bfb79bd7f350ae914fa7b366a1f5 100755
GIT binary patch
delta 694
zcmbOrcR_Z72IGW@n&FHeCSPP1p1gpGh0$m7Mjr8r8x*)curh$b2iA$Vj)^$8=3hz_
z5MXOLa6sbt;}gq7+qw5{-olu}D76Nvjt8QgK?_I=G4L=PVV!KntmpLtD$N0v7KKXh
zK-1;}6~BX~-VQ2$fpzmH<|;<99w6Tah-UzC01%%5;tGbzPOM6f9zZ6{6#_uE4iGy)
zX&}V_5(5F|s-R#7#u`ROX<jCfm;?}m^m0trW%J?j2C|AWi<8S37#%0KvYB$mSLNoG
z<`@<jOrFT3xcLqn4<q9mU~q{}{>N?vW(iLg;Sllk1nOgC`1pSZBandvf`Dw0I4nG2
zQaNA-7lQ)S!!Yss$qPBe>!$*_f<Rp`Q5d}d$N+^ddN6H)ilfH{D0o2XFQ5hYHK;gx
z^t^(In**Z{$N~abfc^urK?b468$)ukXIh?dkcD18gG$BZPBueCfNjp>T*L@WRcxCD
zxEC-nrcB<*tIq{<;TqP-uXyb^VZq`v*^tki(P45XpFP(D4zSe>!i<wQ@tJXc=7eyk
I0h6XY01KLL+5i9m

delta 798
zcmca0J3(%O2IGZ^n&C_pY?CK4i7`4)F60rPxIuxdf{g(TD%d98IwmsLTQ+8Y$tU)*
zHJmxWKcC)j(5ue5c?)9>qm&6$9S=k~gBFk$V&Gx$VVi8ltmjn#mF9p-i$bLx(6sqL
z#Z%DK+d;)6*fwusu3{7uVPs&i0g5RA=>Q<@1Ed)kCp)n!)n5RzV6G4VvUPy?1e69+
z3^PCk5HME-1v4<#I50}{FdblHb!cI1Y+!7vX|8A~X)S2WY0v0L=}hQ~>5k|L=?&=f
z>Gzo6GSOj@&18!yCQ}Wj=}gy{p)ylpmdtF4IU;if=JCwuSm3hIX_151V!I_aORbh!
zEH_(WveIak!D_uVI%~DoX{=Y<pt4bElfq`XEizlBwn=Ok+aa=3XqUilzCAp9x%P4F
z2YDIf-~~YJ0>nI%m$LfsI00EjnZ?Ov42%|&|FW6_qep3T3>yz4lL_18EOs|RSjYp_
zLBQmR?BbsMKsF=8$NxJ(0fPi2fNYR>12lkPQbu3~7lQ&TOawx^PFCa)=Zpq&1cAC3
zCNuI1*Ms5}J%Xx$DnQ~e*T7<+4=Nsk7SVH{;^+xu1614xP5lw5I55FT1DS{@W=Kws
z2rzdwa@NadP^s9g!MTWW^98O2OpI?P7xL<J!Q2B3tW~^roUr8VJozK9IpcxJntb-0
a37lX98Eyj8y!_+|d=i|NTo93IAQ1pY&yJD+

-- 
2.34.1

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

[Richard Henderson]

On 1/16/24 10:15, Vineet Gupta wrote:
> When testing gcc testsuite against QEMU v8.2 we found some additional
> failures vs. v8.1.2.
> 
> | FAIL: gcc.dg/cleanup-10.c execution test
> | FAIL: gcc.dg/cleanup-11.c execution test
> | FAIL: gcc.dg/cleanup-8.c execution test
> | FAIL: gcc.dg/cleanup-9.c execution test
> 
> All of these tests involve unwinding off signal stack and v8.2 did
> introduce a vdso with sigreturn trampoline and associated unwinding
> info. It seems that info is not correct and making it similar to
> to one in the linux kernel fixes the above failures.

So.. you didn't actually determine what might be off in the unwind info?

> +	.cfi_startproc
> +	.cfi_signal_frame
> +       raw_syscall __NR_rt_sigreturn
> +       .cfi_endproc

No, this is wrong.  It indicates that the unwind info is present and trivial.


r~

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

[Vineet Gupta]

On 1/15/24 15:18, Richard Henderson wrote:
> On 1/16/24 10:15, Vineet Gupta wrote:
>> When testing gcc testsuite against QEMU v8.2 we found some additional
>> failures vs. v8.1.2.
>>
>> | FAIL: gcc.dg/cleanup-10.c execution test
>> | FAIL: gcc.dg/cleanup-11.c execution test
>> | FAIL: gcc.dg/cleanup-8.c execution test
>> | FAIL: gcc.dg/cleanup-9.c execution test
>>
>> All of these tests involve unwinding off signal stack and v8.2 did
>> introduce a vdso with sigreturn trampoline and associated unwinding
>> info. It seems that info is not correct and making it similar to
>> to one in the linux kernel fixes the above failures.
> So.. you didn't actually determine what might be off in the unwind info?

Not yet. I just tried what kernel had and that worked.

>
>> +	.cfi_startproc
>> +	.cfi_signal_frame
>> +       raw_syscall __NR_rt_sigreturn
>> +       .cfi_endproc
> No, this is wrong.  It indicates that the unwind info is present and trivial.

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.

Thx,
-Vineet

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

[Richard Henderson]

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~

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

[Richard Henderson]

On 1/16/24 10:18, Richard Henderson wrote:
> On 1/16/24 10:15, Vineet Gupta wrote:
>> When testing gcc testsuite against QEMU v8.2 we found some additional
>> failures vs. v8.1.2.
>>
>> | FAIL: gcc.dg/cleanup-10.c execution test
>> | FAIL: gcc.dg/cleanup-11.c execution test
>> | FAIL: gcc.dg/cleanup-8.c execution test
>> | FAIL: gcc.dg/cleanup-9.c execution test
>>
>> All of these tests involve unwinding off signal stack and v8.2 did
>> introduce a vdso with sigreturn trampoline and associated unwinding
>> info. It seems that info is not correct and making it similar to
>> to one in the linux kernel fixes the above failures.
> 
> So.. you didn't actually determine what might be off in the unwind info?

I have just run the gcc testsuite with my sizeof_reg fix installed, and these tests passed.


r~