| Message ID | 20240115231552.3217789-1-vineetg@rivosinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] linux-user/riscv: vdso: fix call frame info in __vdso_rt_sigreturn | expand |
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~
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
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~
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~
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
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(-)