Clarify which architectures do/don't need sa_restorer.
In particular: 32-bit x86 doesn't need sa_restorer either.
I still don't fully understand why arm32 and x86-64 do, so I've left the
comments in those .S files alone. I haven't (knowingly) tested
compiler-rt since we switched from libgcc (which is what the comments
refer to), but I have tested libunwindstack since we switched from
libunwind, and that does fail existing bionic tests for unwinds through
signal frames --- I just don't know why, or whether there's a better fix.
Anyway, local testing suggests that the 32-bit x86 code is vestigial, so
let's get rid of it.
Test: treehugger
Change-Id: I3e2616f736d27a8463814356e5adb52fd76a90cc
diff --git a/libc/Android.bp b/libc/Android.bp
index 61ef29d..4a4426d 100644
--- a/libc/Android.bp
+++ b/libc/Android.bp
@@ -954,7 +954,6 @@
"arch-x86/bionic/__bionic_clone.S",
"arch-x86/bionic/_exit_with_stack_teardown.S",
"arch-x86/bionic/libcrt_compat.c",
- "arch-x86/bionic/__restore.S",
"arch-x86/bionic/setjmp.S",
"arch-x86/bionic/syscall.S",
"arch-x86/bionic/vfork.S",
diff --git a/libc/arch-x86/bionic/__restore.S b/libc/arch-x86/bionic/__restore.S
deleted file mode 100644
index 10ab8e5..0000000
--- a/libc/arch-x86/bionic/__restore.S
+++ /dev/null
@@ -1,91 +0,0 @@
-/*
- * Copyright (C) 2014 The Android Open Source Project
- * All rights reserved.
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions
- * are met:
- * * Redistributions of source code must retain the above copyright
- * notice, this list of conditions and the following disclaimer.
- * * Redistributions in binary form must reproduce the above copyright
- * notice, this list of conditions and the following disclaimer in
- * the documentation and/or other materials provided with the
- * distribution.
- *
- * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
- * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
- * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
- * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
- * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
- * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
- * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS
- * OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED
- * AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
- * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
- * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
- * SUCH DAMAGE.
- */
-
-#include <private/bionic_asm.h>
-#include <private/bionic_asm_dwarf_exprs.h>
-
-// Offsets into struct sigcontext.
-#define OFFSET_EDI 16
-#define OFFSET_ESI 20
-#define OFFSET_EBP 24
-#define OFFSET_ESP 28
-#define OFFSET_EBX 32
-#define OFFSET_EDX 36
-#define OFFSET_ECX 40
-#define OFFSET_EAX 44
-#define OFFSET_EIP 56
-
-// Non-standard DWARF constants for the x86 registers.
-#define DW_x86_REG_EAX 0
-#define DW_x86_REG_ECX 1
-#define DW_x86_REG_EDX 2
-#define DW_x86_REG_EBX 3
-#define DW_x86_REG_ESP 4
-#define DW_x86_REG_EBP 5
-#define DW_x86_REG_ESI 6
-#define DW_x86_REG_EDI 7
-#define DW_x86_REG_EIP 8
-
-#define RESTORE_GPR(reg, extra_offset) \
- m_cfi_breg_offset DW_x86_REG_ ## reg, \
- DW_x86_REG_ESP, \
- (OFFSET_ ## reg + (extra_offset));
-
-// Restoring ESP is unnecessary as the unwinder simply uses the CFA value.
-#define RESTORE_GPRS(extra_offset) \
- m_cfi_def_cfa_deref DW_x86_REG_ESP, (OFFSET_ESP + (extra_offset)); \
- RESTORE_GPR(EDI, extra_offset) \
- RESTORE_GPR(ESI, extra_offset) \
- RESTORE_GPR(EBP, extra_offset) \
- RESTORE_GPR(EBX, extra_offset) \
- RESTORE_GPR(EDX, extra_offset) \
- RESTORE_GPR(ECX, extra_offset) \
- RESTORE_GPR(EAX, extra_offset) \
- RESTORE_GPR(EIP, extra_offset) \
-
- .text
-
- .cfi_startproc
- .cfi_signal_frame
- RESTORE_GPRS(4)
- nop // See comment in libc/arch-x86_64/bionic/__restore_rt.S about this nop.
-ENTRY_NO_DWARF_PRIVATE(__restore)
- popl %eax
- RESTORE_GPRS(0)
- movl $__NR_sigreturn, %eax
- int $0x80
-END(__restore) // Not END_NO_DWARF because we _manually_ set up CFI.
-
- .cfi_startproc
- .cfi_signal_frame
- RESTORE_GPRS(160)
- nop // See comment in libc/arch-x86_64/bionic/__restore_rt.S about this nop.
-ENTRY_NO_DWARF_PRIVATE(__restore_rt)
- movl $__NR_rt_sigreturn, %eax
- int $0x80
-END(__restore_rt) // Not END_NO_DWARF because we _manually_ set up CFI.
diff --git a/libc/bionic/sigaction.cpp b/libc/bionic/sigaction.cpp
index 1cdb021..a84886b 100644
--- a/libc/bionic/sigaction.cpp
+++ b/libc/bionic/sigaction.cpp
@@ -39,26 +39,25 @@
extern "C" int __rt_sigaction(int, const struct __kernel_sigaction*, struct __kernel_sigaction*, size_t);
int sigaction(int signal, const struct sigaction* bionic_new_action, struct sigaction* bionic_old_action) {
- __kernel_sigaction kernel_new_action;
+ __kernel_sigaction kernel_new_action = {};
if (bionic_new_action != nullptr) {
kernel_new_action.sa_flags = bionic_new_action->sa_flags;
kernel_new_action.sa_handler = bionic_new_action->sa_handler;
// Don't filter signals here; if the caller asked for everything to be blocked, we should obey.
kernel_new_action.sa_mask = bionic_new_action->sa_mask;
-#if defined(SA_RESTORER)
+#if defined(__x86_64__)
+ // riscv64 doesn't have sa_restorer. For arm64 and 32-bit x86, unwinding
+ // works best if you just let the kernel supply the default restorer
+ // from [vdso]. gdb doesn't care, but libgcc needs the nop that the
+ // kernel includes before the actual code. (We could add that ourselves,
+ // but why bother?)
+ // TODO: why do arm32 and x86-64 need this to unwind through signal handlers?
kernel_new_action.sa_restorer = bionic_new_action->sa_restorer;
-#if defined(__aarch64__)
- // arm64 has sa_restorer, but unwinding works best if you just let the
- // kernel supply the default restorer from [vdso]. gdb doesn't care, but
- // libgcc needs the nop that the kernel includes before the actual code.
- // (We could add that ourselves, but why bother?)
-#else
if (!(kernel_new_action.sa_flags & SA_RESTORER)) {
kernel_new_action.sa_flags |= SA_RESTORER;
kernel_new_action.sa_restorer = &__restore_rt;
}
#endif
-#endif
}
__kernel_sigaction kernel_old_action;
@@ -90,10 +89,11 @@
// by extracting the implementation of sigaction64 to a static function.
static int __sigaction64(int signal, const struct sigaction64* bionic_new,
struct sigaction64* bionic_old) {
- struct sigaction64 kernel_new;
+ struct sigaction64 kernel_new = {};
if (bionic_new) {
kernel_new = *bionic_new;
-#if defined(SA_RESTORER)
+#if defined(__arm__)
+ // (See sa_restorer comment in sigaction() above.)
if (!(kernel_new.sa_flags & SA_RESTORER)) {
kernel_new.sa_flags |= SA_RESTORER;
kernel_new.sa_restorer = (kernel_new.sa_flags & SA_SIGINFO) ? &__restore_rt : &__restore;
@@ -110,9 +110,8 @@
int sigaction(int signal, const struct sigaction* bionic_new, struct sigaction* bionic_old) {
// The 32-bit ABI is broken. struct sigaction includes a too-small sigset_t,
// so we have to translate to struct sigaction64 first.
- struct sigaction64 kernel_new;
+ struct sigaction64 kernel_new = {};
if (bionic_new) {
- kernel_new = {};
kernel_new.sa_flags = bionic_new->sa_flags;
kernel_new.sa_handler = bionic_new->sa_handler;
#if defined(SA_RESTORER)