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

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

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