Re: lib/55719 (Unwind tables for signal trampoline on amd64 are incorrect)

[Nikhil Benesch <nikhil.benesch%gmail.com@localhost>]

The following reply was made to PR lib/55719; it has been noted by GNATS.

From: Nikhil Benesch <nikhil.benesch%gmail.com@localhost>
To: gnats-bugs%netbsd.org@localhost
Cc: Kamil Rytarowski <kamil%netbsd.org@localhost>, netbsd-bugs%netbsd.org@localhost, gnats-admin%netbsd.org@localhost
Subject: Re: lib/55719 (Unwind tables for signal trampoline on amd64 are incorrect)
Date: Sun, 18 Oct 2020 12:01:08 -0400

 Ah, unfortunately I introduced a buglet with the revisions. Apologies
 for not noticing sooner. The problem only presents when backtracing
 through *multiple* frames before a signal handler and when those
 frames don't use a frame pointer. I've included a modified version of
 Kamil's test program that surfaces the bug here:
 
 https://gist.github.com/benesch/2b2b4ffb76019d7a2cda6c6002bb2aa0
 
 With -O0, the backtrace works perfectly on NetBSD-current:
 
 $ ./test
 Backtrace 4 stack frames.
 0x400bb0 <handler+0x30> at ./test
 0x7a64384a7870 <__sigtramp_siginfo_2> at /usr/lib/libc.so.12
 0x400be9 <run+0x4> at ./test
 0x400c0e <main+0x23> at ./test
 
 With -O2 the backtrace gets stuck at the run function:
 
 $ ./test
 Backtrace 3 stack frames.
 0x400ba0 <handler+0x20> at ./test
 0x73959f0a7870 <__sigtramp_siginfo_2> at /usr/lib/libc.so.12
 0x400bc8 <run> at ./test
 
 Looking at the generated CFI for the test program makes the issue
 clear. Without optimizations, the CFA is computed via the base
 pointer. With optimizations, it is computed via the stack pointer. So
 the CFI we added to NetBSD's sigtramp mishandles restoration of RSP.
 The fix is quite straightforward:
 
 ===================================================================
 RCS file: /cvsroot/src/lib/libc/arch/x86_64/sys/__sigtramp2.S,v
 retrieving revision 1.8
 diff -u -r1.8 __sigtramp2.S
 --- arch/x86_64/sys/__sigtramp2.S       12 Oct 2020 17:55:54 -0000      1.8
 +++ arch/x86_64/sys/__sigtramp2.S       18 Oct 2020 15:51:51 -0000
 @@ -58,7 +58,7 @@
         .cfi_offset rsi, UC_GREGS_RSI
         .cfi_offset rdi, UC_GREGS_RDI
         .cfi_offset rbp, UC_GREGS_RBP
 -       /* The unwinder will use the CFA to restore RSP. */
 +       .cfi_offset rsp, UC_GREGS_RSP
         .cfi_offset r8,  UC_GREGS_R8
         .cfi_offset r9,  UC_GREGS_R9
         .cfi_offset r10, UC_GREGS_R10
 
 (My email client has a tendency to misformat patches, so I've included
 another copy of the patch in the GitHub Gist I linked to above.)
 
 Basically, because we changed the CFI for the sigtramp to *not* use
 RSP as the CFA, we need to explicitly restore RSP to the value stored
 in the context. Otherwise the unwinder will implicitly set RSP to the
 CFA, which is not correct in this case. (Of course, without
 optimizations things work out ok, since the CFI doesn't refer to the
 mishandled RSP.)
 
 On Mon, Oct 12, 2020 at 1:58 PM <kamil%netbsd.org@localhost> wrote:
 >
 > Synopsis: Unwind tables for signal trampoline on amd64 are incorrect
 >
 > State-Changed-From-To: open->closed
 > State-Changed-By: kamil%NetBSD.org@localhost
 > State-Changed-When: Mon, 12 Oct 2020 19:58:00 +0200
 > State-Changed-Why:
 > Fix committe.
 >
 >
 >