Merge "De-pessimize SigSetConverter usage." into main am: ab9028c8ba am: 75e41cca99 am: e345b3f4a2 am: 72678232a0 am: aa2c50d8ee

Original change: https://android-review.googlesource.com/c/platform/bionic/+/2660920

Change-Id: Ibac8e4c8d183e0a66e1e0ef195ea804251db7853
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

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,

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 <typename SigSetT>
@@ -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) {

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,

libc/bionic/spawn.cpp

@@ -41,7 +41,6 @@
 #include <android/fdsan.h>
 
 #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) {

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) {

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);
+}

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;

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<sigset_t*>(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;
+  };
 };

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);
   });