From 215baed16f00d7954a7500f338f96cca4f63824f Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Mon, 17 Jul 2023 17:15:01 -0700 Subject: [PATCH] De-pessimize SigSetConverter usage. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While looking at the disassembly for the epoll stuff I noticed that this expands to quite a lot of code that the compiler can't optimize out for LP64 (because it doesn't know that the "copy the argument into a local and then use the local" bit isn't important). There are two obvious options here. Something like this: ``` int signalfd64(int fd, const sigset64_t* mask, int flags) {   return __signalfd4(fd, mask, sizeof(*mask), flags); } int signalfd(int fd, const sigset_t* mask, int flags) { #if defined(__LP64__)   return signalfd64(fd, mask, flags); #else   SigSetConverter set = {.sigset = *mask};   return signalfd64(fd, &set.sigset64, flags); #endif } ``` Or something like this: ``` int signalfd64(int fd, const sigset64_t* mask, int flags) {   return __signalfd4(fd, mask, sizeof(*mask), flags); } #if defined(__LP64__) __strong_alias(signalfd, signalfd64); #else int signalfd(int fd, const sigset_t* mask, int flags) {   SigSetConverter set = {};   set.sigset = *mask;   return signalfd64(fd, &set.sigset64, flags); } #endif ``` The former is slightly more verbose, but seems a bit more obvious, so I initially went with that. (The former is more verbose in the generated code too, given that the latter expands to _no_ code, just another symbol pointing to the same code address.) Having done that, I realized that slight changes to the interface would let clang optimize away most/all of the overhead for LP64 with the only preprocessor hackery being in SigSetConverter itself. I also pulled out the legacy bsd `int` conversions since they're only used in two (secret!) functions, so it's clearer to just have a separate union for them. While doing so, I suppressed those functions for riscv64, since there's no reason to keep carrying that mistake forward. posix_spawn() is another simple case that doesn't actually benefit from SigSetConverter, so I've given that its own anonymous union too. Test: treehugger Change-Id: Iaf67486da40d40fc53ec69717c3492ab7ab81ad6 --- libc/bionic/poll.cpp | 20 +++---------- libc/bionic/signal.cpp | 53 ++++++++++++++++------------------ libc/bionic/sigprocmask.cpp | 23 ++++----------- libc/bionic/spawn.cpp | 7 +++-- libc/bionic/sys_epoll.cpp | 10 ++----- libc/bionic/sys_signalfd.cpp | 12 ++++---- libc/libc.map.txt | 4 +-- libc/private/SigSetConverter.h | 45 ++++++++++++++++++++++++++--- tests/signal_test.cpp | 7 +++-- 9 files changed, 94 insertions(+), 87 deletions(-) diff --git a/libc/bionic/poll.cpp b/libc/bionic/poll.cpp index 329031530..7d80b4c7b 100644 --- a/libc/bionic/poll.cpp +++ b/libc/bionic/poll.cpp @@ -50,14 +50,8 @@ int poll(pollfd* fds, nfds_t fd_count, int ms) { int ppoll(pollfd* fds, nfds_t fd_count, const timespec* ts, const sigset_t* ss) { // The underlying `__ppoll` system call only takes `sigset64_t`. - SigSetConverter set; - sigset64_t* ss_ptr = nullptr; - if (ss != nullptr) { - set = {}; - set.sigset = *ss; - ss_ptr = &set.sigset64; - } - return ppoll64(fds, fd_count, ts, ss_ptr); + SigSetConverter set{ss}; + return ppoll64(fds, fd_count, ts, set.ptr); } int ppoll64(pollfd* fds, nfds_t fd_count, const timespec* ts, const sigset64_t* ss) { @@ -99,14 +93,8 @@ int select(int fd_count, fd_set* read_fds, fd_set* write_fds, fd_set* error_fds, int pselect(int fd_count, fd_set* read_fds, fd_set* write_fds, fd_set* error_fds, const timespec* ts, const sigset_t* ss) { // The underlying `__pselect6` system call only takes `sigset64_t`. - SigSetConverter set; - sigset64_t* ss_ptr = nullptr; - if (ss != nullptr) { - set = {}; - set.sigset = *ss; - ss_ptr = &set.sigset64; - } - return pselect64(fd_count, read_fds, write_fds, error_fds, ts, ss_ptr); + SigSetConverter set{ss}; + return pselect64(fd_count, read_fds, write_fds, error_fds, ts, set.ptr); } int pselect64(int fd_count, fd_set* read_fds, fd_set* write_fds, fd_set* error_fds, diff --git a/libc/bionic/signal.cpp b/libc/bionic/signal.cpp index b581b5a2e..2cf9940a6 100644 --- a/libc/bionic/signal.cpp +++ b/libc/bionic/signal.cpp @@ -76,13 +76,23 @@ int sigaddset64(sigset64_t* set, int sig) { return SigAddSet(set, sig); } -// This isn't in our header files, but is exposed on all architectures. +union BsdSigSet { + int mask; + sigset64_t set; +}; + +// This isn't in our header files, but is exposed on all architectures except riscv64. extern "C" int sigblock(int mask) { - SigSetConverter in, out; - sigemptyset(&in.sigset); - in.bsd = mask; - if (sigprocmask(SIG_BLOCK, &in.sigset, &out.sigset) == -1) return -1; - return out.bsd; + BsdSigSet in{.mask = mask}, out; + if (sigprocmask64(SIG_BLOCK, &in.set, &out.set) == -1) return -1; + return out.mask; +} + +// This isn't in our header files, but is exposed on all architectures except riscv64. +extern "C" int sigsetmask(int mask) { + BsdSigSet in{.mask = mask}, out; + if (sigprocmask64(SIG_SETMASK, &in.set, &out.set) == -1) return -1; + return out.mask; } template @@ -198,10 +208,9 @@ int sigpause(int sig) { } int sigpending(sigset_t* bionic_set) { - SigSetConverter set = {}; - set.sigset = *bionic_set; - if (__rt_sigpending(&set.sigset64, sizeof(set.sigset64)) == -1) return -1; - *bionic_set = set.sigset; + SigSetConverter set{bionic_set}; + if (__rt_sigpending(set.ptr, sizeof(sigset64_t)) == -1) return -1; + set.copy_out(); return 0; } @@ -245,19 +254,9 @@ sighandler_t sigset(int sig, sighandler_t disp) { return sigismember64(&old_mask, sig) ? SIG_HOLD : old_sa.sa_handler; } -// This isn't in our header files, but is exposed on all architectures. -extern "C" int sigsetmask(int mask) { - SigSetConverter in, out; - sigemptyset(&in.sigset); - in.bsd = mask; - if (sigprocmask(SIG_SETMASK, &in.sigset, &out.sigset) == -1) return -1; - return out.bsd; -} - int sigsuspend(const sigset_t* bionic_set) { - SigSetConverter set = {}; - set.sigset = *bionic_set; - return sigsuspend64(&set.sigset64); + SigSetConverter set{bionic_set}; + return sigsuspend64(set.ptr); } int sigsuspend64(const sigset64_t* set) { @@ -271,9 +270,8 @@ int sigsuspend64(const sigset64_t* set) { } int sigtimedwait(const sigset_t* bionic_set, siginfo_t* info, const timespec* timeout) { - SigSetConverter set = {}; - set.sigset = *bionic_set; - return sigtimedwait64(&set.sigset64, info, timeout); + SigSetConverter set{bionic_set}; + return sigtimedwait64(set.ptr, info, timeout); } int sigtimedwait64(const sigset64_t* set, siginfo_t* info, const timespec* timeout) { @@ -287,9 +285,8 @@ int sigtimedwait64(const sigset64_t* set, siginfo_t* info, const timespec* timeo } int sigwait(const sigset_t* bionic_set, int* sig) { - SigSetConverter set = {}; - set.sigset = *bionic_set; - return sigwait64(&set.sigset64, sig); + SigSetConverter set{bionic_set}; + return sigwait64(set.ptr, sig); } int sigwait64(const sigset64_t* set, int* sig) { diff --git a/libc/bionic/sigprocmask.cpp b/libc/bionic/sigprocmask.cpp index 8781c9b4d..6d436a681 100644 --- a/libc/bionic/sigprocmask.cpp +++ b/libc/bionic/sigprocmask.cpp @@ -44,24 +44,13 @@ extern "C" int __rt_sigprocmask(int, const sigset64_t*, sigset64_t*, size_t); int sigprocmask(int how, const sigset_t* bionic_new_set, sigset_t* bionic_old_set) __attribute__((__noinline__)) { - SigSetConverter new_set; - sigset64_t* new_set_ptr = nullptr; - if (bionic_new_set != nullptr) { - sigemptyset64(&new_set.sigset64); - new_set.sigset = *bionic_new_set; - new_set_ptr = &new_set.sigset64; + SigSetConverter new_set{bionic_new_set}; + SigSetConverter old_set{bionic_old_set}; + int rc = sigprocmask64(how, new_set.ptr, old_set.ptr); + if (rc == 0 && bionic_old_set != nullptr) { + old_set.copy_out(); } - - SigSetConverter old_set; - if (sigprocmask64(how, new_set_ptr, &old_set.sigset64) == -1) { - return -1; - } - - if (bionic_old_set != nullptr) { - *bionic_old_set = old_set.sigset; - } - - return 0; + return rc; } int sigprocmask64(int how, diff --git a/libc/bionic/spawn.cpp b/libc/bionic/spawn.cpp index 5d76f77ca..38f99ad6f 100644 --- a/libc/bionic/spawn.cpp +++ b/libc/bionic/spawn.cpp @@ -41,7 +41,6 @@ #include #include "private/ScopedSignalBlocker.h" -#include "private/SigSetConverter.h" static int set_cloexec(int i) { int v = fcntl(i, F_GETFD); @@ -131,8 +130,10 @@ struct __posix_spawnattr { pid_t pgroup; sched_param schedparam; int schedpolicy; - SigSetConverter sigmask; - SigSetConverter sigdefault; + union { + sigset_t sigset; + sigset64_t sigset64; + } sigmask, sigdefault; }; static void ApplyAttrs(short flags, const posix_spawnattr_t* attr) { diff --git a/libc/bionic/sys_epoll.cpp b/libc/bionic/sys_epoll.cpp index 22d0a98d7..cffa17380 100644 --- a/libc/bionic/sys_epoll.cpp +++ b/libc/bionic/sys_epoll.cpp @@ -48,14 +48,8 @@ int epoll_create1(int flags) { } int epoll_pwait(int fd, epoll_event* events, int max_events, int timeout, const sigset_t* ss) { - SigSetConverter set; - sigset64_t* ss_ptr = nullptr; - if (ss != nullptr) { - set = {}; - set.sigset = *ss; - ss_ptr = &set.sigset64; - } - return epoll_pwait64(fd, events, max_events, timeout, ss_ptr); + SigSetConverter set{ss}; + return epoll_pwait64(fd, events, max_events, timeout, set.ptr); } int epoll_pwait64(int fd, epoll_event* events, int max_events, int timeout, const sigset64_t* ss) { diff --git a/libc/bionic/sys_signalfd.cpp b/libc/bionic/sys_signalfd.cpp index 53d1f25c9..1e62cf4d9 100644 --- a/libc/bionic/sys_signalfd.cpp +++ b/libc/bionic/sys_signalfd.cpp @@ -32,12 +32,12 @@ extern "C" int __signalfd4(int, const sigset64_t*, size_t, int); -int signalfd(int fd, const sigset_t* mask, int flags) { - SigSetConverter set = {}; - set.sigset = *mask; - return signalfd64(fd, &set.sigset64, flags); -} - int signalfd64(int fd, const sigset64_t* mask, int flags) { return __signalfd4(fd, mask, sizeof(*mask), flags); } + +int signalfd(int fd, const sigset_t* mask, int flags) { + // The underlying `__signalfd4` system call only takes `sigset64_t`. + SigSetConverter set{mask}; + return signalfd64(fd, set.ptr, flags); +} diff --git a/libc/libc.map.txt b/libc/libc.map.txt index 41d18a609..86c828800 100644 --- a/libc/libc.map.txt +++ b/libc/libc.map.txt @@ -955,7 +955,7 @@ LIBC { sigaction; sigaddset; # introduced=21 sigaltstack; - sigblock; + sigblock; # arm x86 arm64 x86_64 sigdelset; # introduced=21 sigemptyset; # introduced=21 sigfillset; # introduced=21 @@ -968,7 +968,7 @@ LIBC { sigprocmask; sigqueue; # introduced=23 sigsetjmp; # introduced-arm=9 introduced-arm64=21 introduced-x86=12 introduced-x86_64=21 - sigsetmask; + sigsetmask; # arm x86 arm64 x86_64 sigsuspend; sigtimedwait; # introduced=23 sigwait; diff --git a/libc/private/SigSetConverter.h b/libc/private/SigSetConverter.h index 7d0b21573..9e9df73ba 100644 --- a/libc/private/SigSetConverter.h +++ b/libc/private/SigSetConverter.h @@ -28,8 +28,45 @@ #pragma once -union SigSetConverter { - int bsd; - sigset_t sigset; - sigset64_t sigset64; +// Android's 32-bit ABI shipped with a sigset_t too small to include any +// of the realtime signals, so we have both sigset_t and sigset64_t. Many +// new system calls only accept a sigset64_t, so this helps paper over +// the difference at zero cost to LP64 in most cases after the optimizer +// removes the unnecessary temporary `ptr`. +struct SigSetConverter { + public: + SigSetConverter(const sigset_t* s) : SigSetConverter(const_cast(s)) {} + + SigSetConverter(sigset_t* s) { +#if defined(__LP64__) + // sigset_t == sigset64_t on LP64. + ptr = s; +#else + sigset64 = {}; + if (s != nullptr) { + original_ptr = s; + sigset = *s; + ptr = &sigset64; + } else { + ptr = nullptr; + } +#endif + } + + void copy_out() { +#if defined(__LP64__) + // We used the original pointer directly, so no copy needed. +#else + *original_ptr = sigset; +#endif + } + + sigset64_t* ptr; + + private: + [[maybe_unused]] sigset_t* original_ptr; + union { + sigset_t sigset; + sigset64_t sigset64; + }; }; diff --git a/tests/signal_test.cpp b/tests/signal_test.cpp index fa648d230..2e6908c85 100644 --- a/tests/signal_test.cpp +++ b/tests/signal_test.cpp @@ -530,14 +530,15 @@ TEST(signal, sighold_filter) { #endif } -#if defined(__BIONIC__) +#if defined(__BIONIC__) && !defined(__riscv) // Not exposed via headers, but the symbols are available if you declare them yourself. extern "C" int sigblock(int); extern "C" int sigsetmask(int); +#define HAVE_SIGBLOCK_SIGSETMASK #endif TEST(signal, sigblock_filter) { -#if defined(__BIONIC__) +#if defined(HAVE_SIGBLOCK_SIGSETMASK) TestSignalMaskFunction([]() { sigblock(~0U); }); @@ -545,7 +546,7 @@ TEST(signal, sigblock_filter) { } TEST(signal, sigsetmask_filter) { -#if defined(__BIONIC__) +#if defined(HAVE_SIGBLOCK_SIGSETMASK) TestSignalMaskFunction([]() { sigsetmask(~0U); });