[2/2] Refactor internal-signals.h
Checks
Context |
Check |
Description |
dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
dj/TryBot-32bit |
success
|
Build for i686
|
Commit Message
The main drive is to optimize the internal usage and required size
when sigset_t in embedded in other data structures. On Linux, the
current supported signal set requires up to 8 bytes (16 on mips),
wat lower than the user defined sigset_t (128 bytes).
A new internal type internal_sigset_t is added, along with the
functions to operate on it similar to the ones for sigset_t. The
internal-signals.h is also refactores to remove unused functions
Besides small stack usage on some functions (posix_spawn, abort)
it lower the struct pthread by about 120 bytes (112 on mips).
Checked on x86_64-linux-gnu.
---
nptl/descr.h | 3 +-
nptl/pthread_attr_setsigmask.c | 2 +-
nptl/pthread_create.c | 16 +--
nptl/pthread_kill.c | 10 +-
nptl/pthread_sigmask.c | 2 +-
rt/tst-timer-sigmask.c | 2 +-
signal/sigaction.c | 2 +-
signal/sigaddset.c | 2 +-
signal/sigdelset.c | 2 +-
signal/sigfillset.c | 2 +-
stdlib/abort.c | 10 +-
sysdeps/generic/internal-signals.h | 31 +-----
sysdeps/posix/signal.c | 2 +-
sysdeps/unix/sysv/linux/ia64/unwind_longjmp.c | 2 +-
sysdeps/unix/sysv/linux/internal-signals.h | 53 +++++----
sysdeps/unix/sysv/linux/internal-sigset.h | 105 ++++++++++++++++++
sysdeps/unix/sysv/linux/spawni.c | 14 ++-
sysdeps/unix/sysv/linux/timer_routines.c | 2 +-
18 files changed, 178 insertions(+), 84 deletions(-)
create mode 100644 sysdeps/unix/sysv/linux/internal-sigset.h
Comments
Hi Adhemerval,
Overall, the patch looks mostly okay to me. I have some comments,
questions, and minor typos noted inline:
> The main drive is to optimize the internal usage and required size
> when sigset_t in embedded in other data structures. On Linux, the
Typo: s/in/is/
> current supported signal set requires up to 8 bytes (16 on mips),
> wat lower than the user defined sigset_t (128 bytes).
Typo: was
> A new internal type internal_sigset_t is added, along with the
> functions to operate on it similar to the ones for sigset_t. The
> internal-signals.h is also refactores to remove unused functions
Minor typo: refactored
> Besides small stack usage on some functions (posix_spawn, abort)
> it lower the struct pthread by about 120 bytes (112 on mips).
>
> Checked on x86_64-linux-gnu.
OK. I rebuilt the Fedora Rawhide glibc package with this patch a few
days ago and the testsuite run was clean.
> ---
> nptl/descr.h | 3 +-
> nptl/pthread_attr_setsigmask.c | 2 +-
> nptl/pthread_create.c | 16 +--
> nptl/pthread_kill.c | 10 +-
> nptl/pthread_sigmask.c | 2 +-
> rt/tst-timer-sigmask.c | 2 +-
> signal/sigaction.c | 2 +-
> signal/sigaddset.c | 2 +-
> signal/sigdelset.c | 2 +-
> signal/sigfillset.c | 2 +-
> stdlib/abort.c | 10 +-
> sysdeps/generic/internal-signals.h | 31 +-----
> sysdeps/posix/signal.c | 2 +-
> sysdeps/unix/sysv/linux/ia64/unwind_longjmp.c | 2 +-
> sysdeps/unix/sysv/linux/internal-signals.h | 53 +++++----
> sysdeps/unix/sysv/linux/internal-sigset.h | 105 ++++++++++++++++++
> sysdeps/unix/sysv/linux/spawni.c | 14 ++-
> sysdeps/unix/sysv/linux/timer_routines.c | 2 +-
> 18 files changed, 178 insertions(+), 84 deletions(-)
> create mode 100644 sysdeps/unix/sysv/linux/internal-sigset.h
>
> diff --git a/nptl/descr.h b/nptl/descr.h
> index b5852632e3..5cacb286f3 100644
> --- a/nptl/descr.h
> +++ b/nptl/descr.h
> @@ -35,6 +35,7 @@
> #include <kernel-features.h>
> #include <tls-internal-struct.h>
> #include <sys/rseq.h>
> +#include <internal-sigset.h>
OK. New header defining internal_sigset_t and functions that operate on it.
>
> #ifndef TCB_ALIGNMENT
> # define TCB_ALIGNMENT 32
> @@ -387,7 +388,7 @@ struct pthread
> /* Signal mask for the new thread. Used during thread startup to
> restore the signal mask. (Threads are launched with all signals
> masked.) */
> - sigset_t sigmask;
> + internal_sigset_t sigmask;
OK. Use newer, smaller struct.
>
> /* Indicates whether is a C11 thread created by thrd_creat. */
> bool c11;
> diff --git a/sysdeps/generic/internal-signals.h b/sysdeps/generic/internal-signals.h
> index 6121c117ab..0c8f67f1a2 100644
> --- a/sysdeps/generic/internal-signals.h
> +++ b/sysdeps/generic/internal-signals.h
OK. Changes to the generic header.
> @@ -29,39 +29,20 @@
> #define RESERVED_SIGRT 0
>
> static inline bool
> -__is_internal_signal (int sig)
> +is_internal_signal (int sig)
OK. Renamed function that isn't visible externally.
> {
> return false;
> }
>
> static inline void
> -__clear_internal_signals (sigset_t *set)
> +clear_internal_signals (sigset_t *set)
OK. Same.
> {
> }
>
> -static inline void
> -__libc_signal_block_all (sigset_t *set)
> -{
> - sigset_t allset;
> - __sigfillset (&allset);
> - __sigprocmask (SIG_BLOCK, &allset, set);
> -}
OK. Remove unused function.
> -
> -static inline void
> -__libc_signal_block_app (sigset_t *set)
> -{
> - sigset_t allset;
> - __sigfillset (&allset);
> - __clear_internal_signals (&allset);
> - __sigprocmask (SIG_BLOCK, &allset, set);
> -}
OK. Remove unused function.
> -
> -/* Restore current process signal mask. */
> -static inline void
> -__libc_signal_restore_set (const sigset_t *set)
> -{
> - __sigprocmask (SIG_SETMASK, set, NULL);
> -}
I noticed that nptl/pthread_{kill,create}.c use
internal_signal_restore_set which isn't defined in the generic header
any more. I also noticed that internal_sigfillset and
internal_sigdelset are in use but not defined by the generic header
any more. Am I missing something?
> +typedef sigset_t internal_sigset_t;
OK. In the generic version, internal_sigset_t is just an alias for
sigset_t. We continue to use the old type.
>
> +#define internal_sigemptyset(__s) sigemptyset (__s)
> +#define internal_sigaddset(__s, __i) sigaddset (__s, __i)
> +#define internal_sigprocmask(__h, __s, __o) sigprocmask (__h, __s, __o)
OK. References to these three have been replaced by references to
internal_ versions in abort.c so we need those definitions.
>
> #endif /* __INTERNAL_SIGNALS_H */
> diff --git a/sysdeps/unix/sysv/linux/internal-signals.h b/sysdeps/unix/sysv/linux/internal-signals.h
> index f9efb6a159..96cfa0a962 100644
> --- a/sysdeps/unix/sysv/linux/internal-signals.h
> +++ b/sysdeps/unix/sysv/linux/internal-signals.h
OK. Changes to the linux specific header.
> @@ -19,10 +19,11 @@
> #ifndef __INTERNAL_SIGNALS_H
> # define __INTERNAL_SIGNALS_H
>
> +#include <internal-sigset.h>
> +#include <limits.h>
> #include <signal.h>
> #include <sigsetops.h>
> #include <stdbool.h>
> -#include <limits.h>
> #include <stddef.h>
> #include <sysdep.h>
>
> @@ -47,49 +48,60 @@
>
> /* Return is sig is used internally. */
> static inline bool
> -__is_internal_signal (int sig)
> +is_internal_signal (int sig)
> {
> return (sig == SIGCANCEL) || (sig == SIGSETXID);
> }
OK. Dropped __ prefix in a function not visible externally.
>
> /* Remove internal glibc signal from the mask. */
> static inline void
> -__clear_internal_signals (sigset_t *set)
> +clear_internal_signals (sigset_t *set)
> {
> __sigdelset (set, SIGCANCEL);
> __sigdelset (set, SIGSETXID);
> }
OK. Function renamed. I was momentarily confused by the "internal_" in
the name, with this patch being about internal_sigset_t - but I see
that the "internal" here refers to signals internally used by glibc.
Obviously, it continues to use the old type because that's how it is
still used.
>
> -static const sigset_t sigall_set = {
> - .__val = {[0 ... _SIGSET_NWORDS-1 ] = -1 }
> +static const internal_sigset_t sigall_set = {
> + .__val = {[0 ... __NSIG_WORDS-1 ] = -1 }
> };
OK. sigall_set is now an internal_sigset and thus only NSIG_WORDS words long.
>
> -static const sigset_t sigtimer_set = {
> - .__val = { [0] = __sigmask (SIGTIMER),
> - [1 ... _SIGSET_NWORDS-1] = 0 }
> -};
OK. This definition was moved further down.
> +/* Obtain and changed blocked signals, including internal glibc ones. */
Typo. Looks like it should be "Obtain and change".
> +static inline int
> +internal_sigprocmask (int how, const internal_sigset_t *set,
> + internal_sigset_t *oldset)
> +{
> + return INTERNAL_SYSCALL_CALL (rt_sigprocmask, how, set, oldset,
> + __NSIG_BYTES);
> +}
OK. New internal function. Yes, internal_sigset_t is __NSIG_BYTES wide.
>
> /* Block all signals, including internal glibc ones. */
> static inline void
> -__libc_signal_block_all (sigset_t *set)
> +internal_signal_block_all (internal_sigset_t *set)
> {
> INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_BLOCK, &sigall_set, set,
> __NSIG_BYTES);
> }
While we are changing things here, calling the argument 'oset' instead
of 'set' might be clearer as to its intent?
>
> -/* Block all application signals (excluding internal glibc ones). */
> +/* Restore current process signal mask. */
> static inline void
> -__libc_signal_block_app (sigset_t *set)
> +internal_signal_restore_set (const internal_sigset_t *set)
> {
> - sigset_t allset = sigall_set;
> - __clear_internal_signals (&allset);
> - INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_BLOCK, &allset, set,
> + INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_SETMASK, set, NULL,
> __NSIG_BYTES);
> }
OK. signal_block_app is removed because it wasn't being used.
OK. internal_signal_restore_set replaces __libc_signal_restore_set;
uses the new type.
>
> +
> +/* It is used on timer_create code directly on sigwaitinfo call, so it can not
> + use the internal_sigset_t definitions. */
> +static const sigset_t sigtimer_set = {
> + .__val = { [0] = __sigmask (SIGTIMER),
> + [1 ... _SIGSET_NWORDS-1] = 0
> + }
> +};
OK. This definition was moved down.
> +
> /* Block only SIGTIMER and return the previous set on SET. */
> static inline void
> -__libc_signal_block_sigtimer (sigset_t *set)
> +signal_block_sigtimer (sigset_t *set)
Internal function renamed. But looks like it isn't used at all?
> {
> INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_BLOCK, &sigtimer_set, set,
> __NSIG_BYTES);
> @@ -97,17 +109,10 @@ __libc_signal_block_sigtimer (sigset_t *set)
>
> /* Unblock only SIGTIMER and return the previous set on SET. */
> static inline void
> -__libc_signal_unblock_sigtimer (sigset_t *set)
> +signal_unblock_sigtimer (sigset_t *set)
Internal function renamed. Looks like this is only ever used once with
a NULL argument and could be switched to internal_sigset_t for
consistency?
> {
> INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_UNBLOCK, &sigtimer_set, set,
> __NSIG_BYTES);
> }
>
> -/* Restore current process signal mask. */
> -static inline void
> -__libc_signal_restore_set (const sigset_t *set)
> -{
> - INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_SETMASK, set, NULL,
> - __NSIG_BYTES);
> -}
OK. This got replaced by internal_signal_restore_set.
> #endif
> diff --git a/sysdeps/unix/sysv/linux/internal-sigset.h b/sysdeps/unix/sysv/linux/internal-sigset.h
> new file mode 100644
> index 0000000000..cdd7260b1c
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/internal-sigset.h
> @@ -0,0 +1,105 @@
> +/* Internal sigset_t definition.
> + Copyright (C) 2022 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <https://www.gnu.org/licenses/>. */
> +
> +#ifndef _INTERNAL_SIGSET_H
> +#define _INTERNAL_SIGSET_H
> +
> +#include <sigsetops.h>
> +
> +typedef struct
> +{
> + unsigned long int __val[__NSIG_WORDS];
> +} internal_sigset_t;
OK. New internal structure that is only NSIG_WORDS words long.
> +
> +static inline void
> +internal_sigset_from_sigset (internal_sigset_t *iset, const sigset_t *set)
> +{
> + int cnt = __NSIG_WORDS;
> + while (--cnt >= 0)
> + iset->__val[cnt] = set->__val[cnt];
> +}
OK. Converts sigset to internal_sigset. We only care about the first
NSIG_WORDS words.
> +
> +static inline void
> +internal_sigemptyset (internal_sigset_t *set)
> +{
> + int cnt = __NSIG_WORDS;
> + while (--cnt >= 0)
> + set->__val[cnt] = 0;
> +}
OK.
> +
> +static inline void
> +internal_sigfillset (internal_sigset_t *set)
> +{
> + int cnt = __NSIG_WORDS;
> + while (--cnt >= 0)
> + set->__val[cnt] = ~0UL;
> +}
OK.
> +
> +static inline int
> +internal_sigisemptyset (const internal_sigset_t *set)
> +{
> + int cnt = __NSIG_WORDS;
> + int ret = set->__val[--cnt];
> + while (ret == 0 && --cnt >= 0)
> + ret = set->__val[cnt];
> + return ret == 0;
> +}
OK. Check each word (starting from the last) for zero. Stop checking
as soon as we see a non-zero.
I wonder if it's easier to read a `for' loop here instead.
> +
> +static inline void
> +internal_sigandset (internal_sigset_t *dest, const internal_sigset_t *left,
> + const internal_sigset_t *right)
> +{
> + int cnt = __NSIG_WORDS;
> + while (--cnt >= 0)
> + dest->__val[cnt] = left->__val[cnt] & right->__val[cnt];
> +}
OK. Bitwise-and each word of left and right, and store it in dest.
> +
> +static inline void
> +internal_sigorset (internal_sigset_t *dest, const internal_sigset_t *left,
> + const internal_sigset_t *right)
> +{
> + int cnt = __NSIG_WORDS;
> + while (--cnt >= 0)
> + dest->__val[cnt] = left->__val[cnt] | right->__val[cnt];
> +}
OK. Same for bitwise-or.
> +
> +static inline int
> +internal_sigismember (const internal_sigset_t *set, int sig)
> +{
> + unsigned long int mask = __sigmask (sig);
> + unsigned long int word = __sigword (sig);
> + return set->__val[word] & mask ? 1 : 0;
> +}
OK.
sigmask returns the word containing sig from a mask where only sig is set.
sigword returns the index of the word containing sig.
Doing a bitwise-and of val[word] and mask will tell if the signal is set in val.
> +
> +static inline void
> +internal_sigaddset (internal_sigset_t *set, int sig)
> +{
> + unsigned long int mask = __sigmask (sig);
> + unsigned long int word = __sigword (sig);
> + set->__val[word] |= mask;
> +}
OK.
> +
> +static inline void
> +internal_sigdelset (internal_sigset_t *set, int sig)
> +{
> + unsigned long int mask = __sigmask (sig);
> + unsigned long int word = __sigword (sig);
> + set->__val[word] &= ~mask;
> +}
OK.
> +
> +#endif
Might be useful to include a comment referring to the corresponding #if here.
Moving on to changes that depend on the above code.
> diff --git a/nptl/pthread_attr_setsigmask.c b/nptl/pthread_attr_setsigmask.c
> index a6e650f4cc..908a0c13ef 100644
> --- a/nptl/pthread_attr_setsigmask.c
> +++ b/nptl/pthread_attr_setsigmask.c
> @@ -28,7 +28,7 @@ pthread_attr_setsigmask_np (pthread_attr_t *attr, const sigset_t *sigmask)
>
> /* Filter out internal signals. */
> struct pthread_attr *iattr = (struct pthread_attr *) attr;
> - __clear_internal_signals (&iattr->extension->sigmask);
> + clear_internal_signals (&iattr->extension->sigmask);
OK. Function was renamed.
>
> return 0;
> }
> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
> index e7a099acb7..360e78eb13 100644
> --- a/nptl/pthread_create.c
> +++ b/nptl/pthread_create.c
> @@ -423,7 +423,7 @@ start_thread (void *arg)
> /* Store the new cleanup handler info. */
> THREAD_SETMEM (pd, cleanup_jmp_buf, &unwind_buf);
>
> - __libc_signal_restore_set (&pd->sigmask);
> + internal_signal_restore_set (&pd->sigmask);
OK. pd->sigmask is now internal_sigset_t.
>
> LIBC_PROBE (pthread_start, 3, (pthread_t) pd, pd->start_routine, pd->arg);
>
> @@ -501,8 +501,8 @@ start_thread (void *arg)
> signal to be delivered. (SIGSETXID cannot run application code,
> nor does it use pthread_kill.) Reuse the pd->sigmask space for
> computing the signal mask, to save stack space. */
> - __sigfillset (&pd->sigmask);
> - __sigdelset (&pd->sigmask, SIGSETXID);
> + internal_sigfillset (&pd->sigmask);
> + internal_sigdelset (&pd->sigmask, SIGSETXID);
OK. Switch to functions that operate on internal_sigset_t since
pd->sigmask is now of this type.
> INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_BLOCK, &pd->sigmask, NULL,
> __NSIG_BYTES);
>
> @@ -766,14 +766,14 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
> /* Block all signals, so that the new thread starts out with
> signals disabled. This avoids race conditions in the thread
> startup. */
> - sigset_t original_sigmask;
> - __libc_signal_block_all (&original_sigmask);
> + internal_sigset_t original_sigmask;
> + internal_signal_block_all (&original_sigmask);
OK. Use the new type.
>
> if (iattr->extension != NULL && iattr->extension->sigmask_set)
> /* Use the signal mask in the attribute. The internal signals
> have already been filtered by the public
> pthread_attr_setsigmask_np interface. */
> - pd->sigmask = iattr->extension->sigmask;
> + internal_sigset_from_sigset (&pd->sigmask, &iattr->extension->sigmask);
OK. Replace the assignment with conversion to internal type since
pd->sigmask is now an internal_sigset_t.
> else
> {
> /* Conceptually, the new thread needs to inherit the signal mask
> @@ -783,7 +783,7 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
> pd->sigmask = original_sigmask;
> /* Reset the cancellation signal mask in case this thread is
> running cancellation. */
> - __sigdelset (&pd->sigmask, SIGCANCEL);
> + internal_sigdelset (&pd->sigmask, SIGCANCEL);
OK.
> }
>
> /* Start the thread. */
> @@ -830,7 +830,7 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
>
> /* Return to the previous signal mask, after creating the new
> thread. */
> - __libc_signal_restore_set (&original_sigmask);
> + internal_signal_restore_set (&original_sigmask);
OK. original_sigmask is now an internal_sigset_t.
>
> if (__glibc_unlikely (retval != 0))
> {
> diff --git a/nptl/pthread_kill.c b/nptl/pthread_kill.c
> index 8015e10b70..178d38d3bd 100644
> --- a/nptl/pthread_kill.c
> +++ b/nptl/pthread_kill.c
> @@ -45,8 +45,8 @@ __pthread_kill_implementation (pthread_t threadid, int signo, int no_tid)
> }
>
> /* Block all signals, as required by pd->exit_lock. */
> - sigset_t old_mask;
> - __libc_signal_block_all (&old_mask);
> + internal_sigset_t old_mask;
> + internal_signal_block_all (&old_mask);
OK. Use new type.
> __libc_lock_lock (pd->exit_lock);
>
> int ret;
> @@ -64,7 +64,7 @@ __pthread_kill_implementation (pthread_t threadid, int signo, int no_tid)
> }
>
> __libc_lock_unlock (pd->exit_lock);
> - __libc_signal_restore_set (&old_mask);
> + internal_signal_restore_set (&old_mask);
OK. Use new function for new type.
>
> return ret;
> }
> @@ -83,7 +83,7 @@ __pthread_kill (pthread_t threadid, int signo)
> {
> /* Disallow sending the signal we use for cancellation, timers,
> for the setxid implementation. */
> - if (__is_internal_signal (signo))
> + if (is_internal_signal (signo))
OK. Function was renamed.
> return EINVAL;
>
> return __pthread_kill_internal (threadid, signo);
> @@ -102,7 +102,7 @@ int
> attribute_compat_text_section
> __pthread_kill_esrch (pthread_t threadid, int signo)
> {
> - if (__is_internal_signal (signo))
> + if (is_internal_signal (signo))
OK.
> return EINVAL;
>
> return __pthread_kill_implementation (threadid, signo, ESRCH);
> diff --git a/nptl/pthread_sigmask.c b/nptl/pthread_sigmask.c
> index 20f811cc6b..977740b97e 100644
> --- a/nptl/pthread_sigmask.c
> +++ b/nptl/pthread_sigmask.c
> @@ -32,7 +32,7 @@ __pthread_sigmask (int how, const sigset_t *newmask, sigset_t *oldmask)
> || __glibc_unlikely (__sigismember (newmask, SIGSETXID))))
> {
> local_newmask = *newmask;
> - __clear_internal_signals (&local_newmask);
> + clear_internal_signals (&local_newmask);
OK. Function was renamed.
> newmask = &local_newmask;
> }
>
> diff --git a/rt/tst-timer-sigmask.c b/rt/tst-timer-sigmask.c
> index 11acb670ab..4d088a691d 100644
> --- a/rt/tst-timer-sigmask.c
> +++ b/rt/tst-timer-sigmask.c
> @@ -43,7 +43,7 @@ thread_handler (union sigval sv)
> if (sigismember (&ss, sig))
> {
> TEST_VERIFY (sig != SIGKILL && sig != SIGSTOP);
> - TEST_VERIFY (!__is_internal_signal (sig));
> + TEST_VERIFY (!is_internal_signal (sig));
OK. Function was renamed.
> }
> if (test_verbose && sigismember (&ss, sig))
> printf ("%d, ", sig);
> diff --git a/signal/sigaction.c b/signal/sigaction.c
> index 8e94b9a10b..d56b3196ca 100644
> --- a/signal/sigaction.c
> +++ b/signal/sigaction.c
> @@ -24,7 +24,7 @@
> int
> __sigaction (int sig, const struct sigaction *act, struct sigaction *oact)
> {
> - if (sig <= 0 || sig >= NSIG || __is_internal_signal (sig))
> + if (sig <= 0 || sig >= NSIG || is_internal_signal (sig))
OK. Function was renamed.
> {
> __set_errno (EINVAL);
> return -1;
> diff --git a/signal/sigaddset.c b/signal/sigaddset.c
> index c6ef0a953a..f0fed84dd7 100644
> --- a/signal/sigaddset.c
> +++ b/signal/sigaddset.c
> @@ -25,7 +25,7 @@ int
> sigaddset (sigset_t *set, int signo)
> {
> if (set == NULL || signo <= 0 || signo >= NSIG
> - || __is_internal_signal (signo))
> + || is_internal_signal (signo))
OK. Function was renamed.
> {
> __set_errno (EINVAL);
> return -1;
> diff --git a/signal/sigdelset.c b/signal/sigdelset.c
> index 81bb9a908a..2c973c9b84 100644
> --- a/signal/sigdelset.c
> +++ b/signal/sigdelset.c
> @@ -25,7 +25,7 @@ int
> sigdelset (sigset_t *set, int signo)
> {
> if (set == NULL || signo <= 0 || signo >= NSIG
> - || __is_internal_signal (signo))
> + || is_internal_signal (signo))
OK. Function was renamed.
> {
> __set_errno (EINVAL);
> return -1;
> diff --git a/signal/sigfillset.c b/signal/sigfillset.c
> index b52ef06aa0..1d11ae4fec 100644
> --- a/signal/sigfillset.c
> +++ b/signal/sigfillset.c
> @@ -31,7 +31,7 @@ sigfillset (sigset_t *set)
> }
>
> __sigfillset (set);
> - __clear_internal_signals (set);
> + clear_internal_signals (set);
OK. Function was renamed.
> return 0;
> }
> libc_hidden_def (sigfillset)
> diff --git a/stdlib/abort.c b/stdlib/abort.c
> index bcf72f9356..9c1065af0e 100644
> --- a/stdlib/abort.c
> +++ b/stdlib/abort.c
> @@ -21,7 +21,7 @@
> #include <stdlib.h>
> #include <string.h>
> #include <unistd.h>
> -#include <sigsetops.h>
> +#include <internal-signals.h>
OK. We will be operating on internal_sigset_t.
>
> /* Try to get a machine dependent instruction which will make the
> program crash. This is used in case everything else fails. */
> @@ -48,7 +48,6 @@ void
> abort (void)
> {
> struct sigaction act;
> - sigset_t sigs;
OK. Declared later.
>
> /* First acquire the lock. */
> __libc_lock_lock_recursive (lock);
> @@ -59,9 +58,10 @@ abort (void)
> if (stage == 0)
> {
> ++stage;
> - __sigemptyset (&sigs);
> - __sigaddset (&sigs, SIGABRT);
> - __sigprocmask (SIG_UNBLOCK, &sigs, 0);
> + internal_sigset_t sigs;
> + internal_sigemptyset (&sigs);
> + internal_sigaddset (&sigs, SIGABRT);
> + internal_sigprocmask (SIG_UNBLOCK, &sigs, NULL);
> }
OK. Use new type and functions.
>
> /* Send signal which possibly calls a user handler. */
> diff --git a/sysdeps/posix/signal.c b/sysdeps/posix/signal.c
> index 8f17b4278d..88c3757126 100644
> --- a/sysdeps/posix/signal.c
> +++ b/sysdeps/posix/signal.c
> @@ -32,7 +32,7 @@ __bsd_signal (int sig, __sighandler_t handler)
>
> /* Check signal extents to protect __sigismember. */
> if (handler == SIG_ERR || sig < 1 || sig >= NSIG
> - || __is_internal_signal (sig))
> + || is_internal_signal (sig))
OK.
> {
> __set_errno (EINVAL);
> return SIG_ERR;
> diff --git a/sysdeps/unix/sysv/linux/ia64/unwind_longjmp.c b/sysdeps/unix/sysv/linux/ia64/unwind_longjmp.c
> index a59ad78036..a53ae79965 100644
> --- a/sysdeps/unix/sysv/linux/ia64/unwind_longjmp.c
> +++ b/sysdeps/unix/sysv/linux/ia64/unwind_longjmp.c
> @@ -34,7 +34,7 @@ __libc_unwind_longjmp (sigjmp_buf env, int val)
>
> if (env[0].__mask_was_saved)
> /* Restore the saved signal mask. */
> - __libc_signal_restore_set (&env[0].__saved_mask);
> + __sigprocmask (SIG_SETMASK, &env[0].__saved_mask, NULL);
OK. internal_signal_restore_set operates on internal_sigset_t, but
here we have a sigset_t. __saved_mask is a sigset_t, so we cannot use
it here. Instead, we directly call sigprocmask like we did in
__libc_signal_restore_set.
>
> /* Call the machine-dependent function to restore machine state. */
> __sigstack_longjmp (env[0].__jmpbuf, val ?: 1);
> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
> index d703485e3f..75f5c06e32 100644
> --- a/sysdeps/unix/sysv/linux/spawni.c
> +++ b/sysdeps/unix/sysv/linux/spawni.c
> @@ -57,7 +57,7 @@
>
> struct posix_spawn_args
> {
> - sigset_t oldmask;
> + internal_sigset_t oldmask;
OK.
> const char *file;
> int (*exec) (const char *, char *const *, char *const *);
> const posix_spawn_file_actions_t *fa;
> @@ -124,7 +124,7 @@ __spawni_child (void *arguments)
> }
> else if (__sigismember (&hset, sig))
> {
> - if (__is_internal_signal (sig))
> + if (is_internal_signal (sig))
OK.
> sa.sa_handler = SIG_IGN;
> else
> {
> @@ -284,8 +284,10 @@ __spawni_child (void *arguments)
>
> /* Set the initial signal mask of the child if POSIX_SPAWN_SETSIGMASK
> is set, otherwise restore the previous one. */
> - __sigprocmask (SIG_SETMASK, (attr->__flags & POSIX_SPAWN_SETSIGMASK)
> - ? &attr->__ss : &args->oldmask, 0);
> + if (attr->__flags & POSIX_SPAWN_SETSIGMASK)
> + __sigprocmask (SIG_SETMASK, &attr->__ss, NULL);
> + else
> + internal_sigprocmask (SIG_SETMASK, &args->oldmask, NULL);
OK. One is a sigset_t and the other is an internal_sigset_t, so we
write this differently now. It's also more readable than the "?:"
version.
>
> args->exec (args->file, args->argv, args->envp);
>
> @@ -368,7 +370,7 @@ __spawnix (pid_t * pid, const char *file,
> args.envp = envp;
> args.xflags = xflags;
>
> - __libc_signal_block_all (&args.oldmask);
> + internal_signal_block_all (&args.oldmask);
OK.
>
> /* The clone flags used will create a new child that will run in the same
> memory space (CLONE_VM) and the execution of calling thread will be
> @@ -416,7 +418,7 @@ __spawnix (pid_t * pid, const char *file,
> if ((ec == 0) && (pid != NULL))
> *pid = new_pid;
>
> - __libc_signal_restore_set (&args.oldmask);
> + internal_signal_restore_set (&args.oldmask);
OK.
>
> __pthread_setcancelstate (state, NULL);
>
> diff --git a/sysdeps/unix/sysv/linux/timer_routines.c b/sysdeps/unix/sysv/linux/timer_routines.c
> index 8f0a2f635d..fc3deb6823 100644
> --- a/sysdeps/unix/sysv/linux/timer_routines.c
> +++ b/sysdeps/unix/sysv/linux/timer_routines.c
> @@ -41,7 +41,7 @@ struct thread_start_data
> static void *
> timer_sigev_thread (void *arg)
> {
> - __libc_signal_unblock_sigtimer (NULL);
> + signal_unblock_sigtimer (NULL);
OK.
>
> struct thread_start_data *td = (struct thread_start_data *) arg;
> void (*thrfunc) (sigval_t) = td->thrfunc;
> --
> 2.34.1
>
> On 28 Jun 2022, at 14:16, Arjun Shankar <arjun@redhat.com> wrote:
>
> Hi Adhemerval,
>
> Overall, the patch looks mostly okay to me. I have some comments,
> questions, and minor typos noted inline:
>
>> The main drive is to optimize the internal usage and required size
>> when sigset_t in embedded in other data structures. On Linux, the
>
> Typo: s/in/is/
Ack.
>
>> current supported signal set requires up to 8 bytes (16 on mips),
>> wat lower than the user defined sigset_t (128 bytes).
>
> Typo: was
Ack.
>
>> A new internal type internal_sigset_t is added, along with the
>> functions to operate on it similar to the ones for sigset_t. The
>> internal-signals.h is also refactores to remove unused functions
>
> Minor typo: refactored
Ack.
>
>> Besides small stack usage on some functions (posix_spawn, abort)
>> it lower the struct pthread by about 120 bytes (112 on mips).
>>
>> Checked on x86_64-linux-gnu.
>
> OK. I rebuilt the Fedora Rawhide glibc package with this patch a few
> days ago and the testsuite run was clean.
Thanks for the review, I will send an updated version.
>
>> ---
>> nptl/descr.h | 3 +-
>> nptl/pthread_attr_setsigmask.c | 2 +-
>> nptl/pthread_create.c | 16 +--
>> nptl/pthread_kill.c | 10 +-
>> nptl/pthread_sigmask.c | 2 +-
>> rt/tst-timer-sigmask.c | 2 +-
>> signal/sigaction.c | 2 +-
>> signal/sigaddset.c | 2 +-
>> signal/sigdelset.c | 2 +-
>> signal/sigfillset.c | 2 +-
>> stdlib/abort.c | 10 +-
>> sysdeps/generic/internal-signals.h | 31 +-----
>> sysdeps/posix/signal.c | 2 +-
>> sysdeps/unix/sysv/linux/ia64/unwind_longjmp.c | 2 +-
>> sysdeps/unix/sysv/linux/internal-signals.h | 53 +++++----
>> sysdeps/unix/sysv/linux/internal-sigset.h | 105 ++++++++++++++++++
>> sysdeps/unix/sysv/linux/spawni.c | 14 ++-
>> sysdeps/unix/sysv/linux/timer_routines.c | 2 +-
>> 18 files changed, 178 insertions(+), 84 deletions(-)
>> create mode 100644 sysdeps/unix/sysv/linux/internal-sigset.h
>>
>> diff --git a/nptl/descr.h b/nptl/descr.h
>> index b5852632e3..5cacb286f3 100644
>> --- a/nptl/descr.h
>> +++ b/nptl/descr.h
>> @@ -35,6 +35,7 @@
>> #include <kernel-features.h>
>> #include <tls-internal-struct.h>
>> #include <sys/rseq.h>
>> +#include <internal-sigset.h>
>
> OK. New header defining internal_sigset_t and functions that operate on it.
>
>>
>> #ifndef TCB_ALIGNMENT
>> # define TCB_ALIGNMENT 32
>> @@ -387,7 +388,7 @@ struct pthread
>> /* Signal mask for the new thread. Used during thread startup to
>> restore the signal mask. (Threads are launched with all signals
>> masked.) */
>> - sigset_t sigmask;
>> + internal_sigset_t sigmask;
>
> OK. Use newer, smaller struct.
>
>>
>> /* Indicates whether is a C11 thread created by thrd_creat. */
>> bool c11;
>
>> diff --git a/sysdeps/generic/internal-signals.h b/sysdeps/generic/internal-signals.h
>> index 6121c117ab..0c8f67f1a2 100644
>> --- a/sysdeps/generic/internal-signals.h
>> +++ b/sysdeps/generic/internal-signals.h
>
> OK. Changes to the generic header.
>
>> @@ -29,39 +29,20 @@
>> #define RESERVED_SIGRT 0
>>
>> static inline bool
>> -__is_internal_signal (int sig)
>> +is_internal_signal (int sig)
>
> OK. Renamed function that isn't visible externally.
>
>> {
>> return false;
>> }
>>
>> static inline void
>> -__clear_internal_signals (sigset_t *set)
>> +clear_internal_signals (sigset_t *set)
>
> OK. Same.
>
>> {
>> }
>>
>> -static inline void
>> -__libc_signal_block_all (sigset_t *set)
>> -{
>> - sigset_t allset;
>> - __sigfillset (&allset);
>> - __sigprocmask (SIG_BLOCK, &allset, set);
>> -}
>
> OK. Remove unused function.
>
>> -
>> -static inline void
>> -__libc_signal_block_app (sigset_t *set)
>> -{
>> - sigset_t allset;
>> - __sigfillset (&allset);
>> - __clear_internal_signals (&allset);
>> - __sigprocmask (SIG_BLOCK, &allset, set);
>> -}
>
> OK. Remove unused function.
>
>> -
>> -/* Restore current process signal mask. */
>> -static inline void
>> -__libc_signal_restore_set (const sigset_t *set)
>> -{
>> - __sigprocmask (SIG_SETMASK, set, NULL);
>> -}
>
> I noticed that nptl/pthread_{kill,create}.c use
> internal_signal_restore_set which isn't defined in the generic header
> any more. I also noticed that internal_sigfillset and
> internal_sigdelset are in use but not defined by the generic header
> any more. Am I missing something?
The nptl folder is in fact a Linux specific implementation of pthread, so
for a non-Linux possible port it won’t be used as default (as Hurd does
with the htl). All the new internal signal usage are restricted to nptl
or Linux implementations:
$ git grep internal_signal_restore_set
nptl/pthread_create.c: internal_signal_restore_set (&pd->sigmask);
nptl/pthread_create.c: internal_signal_restore_set (&original_sigmask);
nptl/pthread_kill.c: internal_signal_restore_set (&old_mask);
sysdeps/unix/sysv/linux/internal-signals.h:internal_signal_restore_set (const internal_sigset_t *set)
sysdeps/unix/sysv/linux/spawni.c: internal_signal_restore_set (&args.oldmask);
$ git grep internal_sigfillset
nptl/pthread_create.c: internal_sigfillset (&pd->sigmask);
sysdeps/unix/sysv/linux/internal-sigset.h:internal_sigfillset (internal_sigset_t *set)
$ git grep internal_sigdelset
nptl/pthread_create.c: internal_sigdelset (&pd->sigmask, SIGSETXID);
nptl/pthread_create.c: internal_sigdelset (&pd->sigmask, SIGCANCEL);
sysdeps/unix/sysv/linux/internal-sigset.h:internal_sigdelset (internal_sigset_t *set, int sig)
>
>> +typedef sigset_t internal_sigset_t;
>
> OK. In the generic version, internal_sigset_t is just an alias for
> sigset_t. We continue to use the old type.
>
>>
>> +#define internal_sigemptyset(__s) sigemptyset (__s)
>> +#define internal_sigaddset(__s, __i) sigaddset (__s, __i)
>> +#define internal_sigprocmask(__h, __s, __o) sigprocmask (__h, __s, __o)
>
> OK. References to these three have been replaced by references to
> internal_ versions in abort.c so we need those definitions.
>
>>
>> #endif /* __INTERNAL_SIGNALS_H */
>
>> diff --git a/sysdeps/unix/sysv/linux/internal-signals.h b/sysdeps/unix/sysv/linux/internal-signals.h
>> index f9efb6a159..96cfa0a962 100644
>> --- a/sysdeps/unix/sysv/linux/internal-signals.h
>> +++ b/sysdeps/unix/sysv/linux/internal-signals.h
>
> OK. Changes to the linux specific header.
>
>> @@ -19,10 +19,11 @@
>> #ifndef __INTERNAL_SIGNALS_H
>> # define __INTERNAL_SIGNALS_H
>>
>> +#include <internal-sigset.h>
>> +#include <limits.h>
>> #include <signal.h>
>> #include <sigsetops.h>
>> #include <stdbool.h>
>> -#include <limits.h>
>> #include <stddef.h>
>> #include <sysdep.h>
>>
>> @@ -47,49 +48,60 @@
>>
>> /* Return is sig is used internally. */
>> static inline bool
>> -__is_internal_signal (int sig)
>> +is_internal_signal (int sig)
>> {
>> return (sig == SIGCANCEL) || (sig == SIGSETXID);
>> }
>
> OK. Dropped __ prefix in a function not visible externally.
>
>>
>> /* Remove internal glibc signal from the mask. */
>> static inline void
>> -__clear_internal_signals (sigset_t *set)
>> +clear_internal_signals (sigset_t *set)
>> {
>> __sigdelset (set, SIGCANCEL);
>> __sigdelset (set, SIGSETXID);
>> }
>
> OK. Function renamed. I was momentarily confused by the "internal_" in
> the name, with this patch being about internal_sigset_t - but I see
> that the "internal" here refers to signals internally used by glibc.
> Obviously, it continues to use the old type because that's how it is
> still used.
Yeah, I used the internal_ prefix for the function that operate on the
new internal_sigset_t.
>
>>
>> -static const sigset_t sigall_set = {
>> - .__val = {[0 ... _SIGSET_NWORDS-1 ] = -1 }
>> +static const internal_sigset_t sigall_set = {
>> + .__val = {[0 ... __NSIG_WORDS-1 ] = -1 }
>> };
>
> OK. sigall_set is now an internal_sigset and thus only NSIG_WORDS words long.
>
>>
>> -static const sigset_t sigtimer_set = {
>> - .__val = { [0] = __sigmask (SIGTIMER),
>> - [1 ... _SIGSET_NWORDS-1] = 0 }
>> -};
>
> OK. This definition was moved further down.
>
>> +/* Obtain and changed blocked signals, including internal glibc ones. */
>
> Typo. Looks like it should be "Obtain and change".
Ack.
>
>> +static inline int
>> +internal_sigprocmask (int how, const internal_sigset_t *set,
>> + internal_sigset_t *oldset)
>> +{
>> + return INTERNAL_SYSCALL_CALL (rt_sigprocmask, how, set, oldset,
>> + __NSIG_BYTES);
>> +}
>
> OK. New internal function. Yes, internal_sigset_t is __NSIG_BYTES wide.
>
>>
>> /* Block all signals, including internal glibc ones. */
>> static inline void
>> -__libc_signal_block_all (sigset_t *set)
>> +internal_signal_block_all (internal_sigset_t *set)
>> {
>> INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_BLOCK, &sigall_set, set,
>> __NSIG_BYTES);
>> }
>
> While we are changing things here, calling the argument 'oset' instead
> of 'set' might be clearer as to its intent?
It does make sense.
>
>>
>> -/* Block all application signals (excluding internal glibc ones). */
>> +/* Restore current process signal mask. */
>> static inline void
>> -__libc_signal_block_app (sigset_t *set)
>> +internal_signal_restore_set (const internal_sigset_t *set)
>> {
>> - sigset_t allset = sigall_set;
>> - __clear_internal_signals (&allset);
>> - INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_BLOCK, &allset, set,
>> + INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_SETMASK, set, NULL,
>> __NSIG_BYTES);
>> }
>
> OK. signal_block_app is removed because it wasn't being used.
>
> OK. internal_signal_restore_set replaces __libc_signal_restore_set;
> uses the new type.
>
>>
>> +
>> +/* It is used on timer_create code directly on sigwaitinfo call, so it can not
>> + use the internal_sigset_t definitions. */
>> +static const sigset_t sigtimer_set = {
>> + .__val = { [0] = __sigmask (SIGTIMER),
>> + [1 ... _SIGSET_NWORDS-1] = 0
>> + }
>> +};
>
> OK. This definition was moved down.
>
>> +
>> /* Block only SIGTIMER and return the previous set on SET. */
>> static inline void
>> -__libc_signal_block_sigtimer (sigset_t *set)
>> +signal_block_sigtimer (sigset_t *set)
>
> Internal function renamed. But looks like it isn't used at all?
Indeed, I will remove it.
>
>> {
>> INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_BLOCK, &sigtimer_set, set,
>> __NSIG_BYTES);
>> @@ -97,17 +109,10 @@ __libc_signal_block_sigtimer (sigset_t *set)
>>
>> /* Unblock only SIGTIMER and return the previous set on SET. */
>> static inline void
>> -__libc_signal_unblock_sigtimer (sigset_t *set)
>> +signal_unblock_sigtimer (sigset_t *set)
>
> Internal function renamed. Looks like this is only ever used once with
> a NULL argument and could be switched to internal_sigset_t for
> consistency?
Right, I think it would be better to just remove the argument (we can add
it later if it is really required).
>
>> {
>> INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_UNBLOCK, &sigtimer_set, set,
>> __NSIG_BYTES);
>> }
>>
>> -/* Restore current process signal mask. */
>> -static inline void
>> -__libc_signal_restore_set (const sigset_t *set)
>> -{
>> - INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_SETMASK, set, NULL,
>> - __NSIG_BYTES);
>> -}
>
> OK. This got replaced by internal_signal_restore_set.
>
>> #endif
>> diff --git a/sysdeps/unix/sysv/linux/internal-sigset.h b/sysdeps/unix/sysv/linux/internal-sigset.h
>> new file mode 100644
>> index 0000000000..cdd7260b1c
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/internal-sigset.h
>> @@ -0,0 +1,105 @@
>> +/* Internal sigset_t definition.
>> + Copyright (C) 2022 Free Software Foundation, Inc.
>> + This file is part of the GNU C Library.
>> +
>> + The GNU C Library is free software; you can redistribute it and/or
>> + modify it under the terms of the GNU Lesser General Public
>> + License as published by the Free Software Foundation; either
>> + version 2.1 of the License, or (at your option) any later version.
>> +
>> + The GNU C Library is distributed in the hope that it will be useful,
>> + but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + Lesser General Public License for more details.
>> +
>> + You should have received a copy of the GNU Lesser General Public
>> + License along with the GNU C Library; if not, see
>> + <https://www.gnu.org/licenses/>. */
>> +
>> +#ifndef _INTERNAL_SIGSET_H
>> +#define _INTERNAL_SIGSET_H
>> +
>> +#include <sigsetops.h>
>> +
>> +typedef struct
>> +{
>> + unsigned long int __val[__NSIG_WORDS];
>> +} internal_sigset_t;
>
> OK. New internal structure that is only NSIG_WORDS words long.
>
>> +
>> +static inline void
>> +internal_sigset_from_sigset (internal_sigset_t *iset, const sigset_t *set)
>> +{
>> + int cnt = __NSIG_WORDS;
>> + while (--cnt >= 0)
>> + iset->__val[cnt] = set->__val[cnt];
>> +}
>
> OK. Converts sigset to internal_sigset. We only care about the first
> NSIG_WORDS words.
>
>> +
>> +static inline void
>> +internal_sigemptyset (internal_sigset_t *set)
>> +{
>> + int cnt = __NSIG_WORDS;
>> + while (--cnt >= 0)
>> + set->__val[cnt] = 0;
>> +}
>
> OK.
>
>> +
>> +static inline void
>> +internal_sigfillset (internal_sigset_t *set)
>> +{
>> + int cnt = __NSIG_WORDS;
>> + while (--cnt >= 0)
>> + set->__val[cnt] = ~0UL;
>> +}
>
> OK.
>
>> +
>> +static inline int
>> +internal_sigisemptyset (const internal_sigset_t *set)
>> +{
>> + int cnt = __NSIG_WORDS;
>> + int ret = set->__val[--cnt];
>> + while (ret == 0 && --cnt >= 0)
>> + ret = set->__val[cnt];
>> + return ret == 0;
>> +}
>
> OK. Check each word (starting from the last) for zero. Stop checking
> as soon as we see a non-zero.
> I wonder if it's easier to read a `for' loop here instead.
>
>> +
>> +static inline void
>> +internal_sigandset (internal_sigset_t *dest, const internal_sigset_t *left,
>> + const internal_sigset_t *right)
>> +{
>> + int cnt = __NSIG_WORDS;
>> + while (--cnt >= 0)
>> + dest->__val[cnt] = left->__val[cnt] & right->__val[cnt];
>> +}
>
> OK. Bitwise-and each word of left and right, and store it in dest.
>
>> +
>> +static inline void
>> +internal_sigorset (internal_sigset_t *dest, const internal_sigset_t *left,
>> + const internal_sigset_t *right)
>> +{
>> + int cnt = __NSIG_WORDS;
>> + while (--cnt >= 0)
>> + dest->__val[cnt] = left->__val[cnt] | right->__val[cnt];
>> +}
>
> OK. Same for bitwise-or.
>
>> +
>> +static inline int
>> +internal_sigismember (const internal_sigset_t *set, int sig)
>> +{
>> + unsigned long int mask = __sigmask (sig);
>> + unsigned long int word = __sigword (sig);
>> + return set->__val[word] & mask ? 1 : 0;
>> +}
>
> OK.
> sigmask returns the word containing sig from a mask where only sig is set.
> sigword returns the index of the word containing sig.
> Doing a bitwise-and of val[word] and mask will tell if the signal is set in val.
>
>> +
>> +static inline void
>> +internal_sigaddset (internal_sigset_t *set, int sig)
>> +{
>> + unsigned long int mask = __sigmask (sig);
>> + unsigned long int word = __sigword (sig);
>> + set->__val[word] |= mask;
>> +}
>
> OK.
>
>> +
>> +static inline void
>> +internal_sigdelset (internal_sigset_t *set, int sig)
>> +{
>> + unsigned long int mask = __sigmask (sig);
>> + unsigned long int word = __sigword (sig);
>> + set->__val[word] &= ~mask;
>> +}
>
> OK.
>
>> +
>> +#endif
>
> Might be useful to include a comment referring to the corresponding #if here.
Ack.
>
> Moving on to changes that depend on the above code.
>
>> diff --git a/nptl/pthread_attr_setsigmask.c b/nptl/pthread_attr_setsigmask.c
>> index a6e650f4cc..908a0c13ef 100644
>> --- a/nptl/pthread_attr_setsigmask.c
>> +++ b/nptl/pthread_attr_setsigmask.c
>> @@ -28,7 +28,7 @@ pthread_attr_setsigmask_np (pthread_attr_t *attr, const sigset_t *sigmask)
>>
>> /* Filter out internal signals. */
>> struct pthread_attr *iattr = (struct pthread_attr *) attr;
>> - __clear_internal_signals (&iattr->extension->sigmask);
>> + clear_internal_signals (&iattr->extension->sigmask);
>
> OK. Function was renamed.
>
>>
>> return 0;
>> }
>> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
>> index e7a099acb7..360e78eb13 100644
>> --- a/nptl/pthread_create.c
>> +++ b/nptl/pthread_create.c
>> @@ -423,7 +423,7 @@ start_thread (void *arg)
>> /* Store the new cleanup handler info. */
>> THREAD_SETMEM (pd, cleanup_jmp_buf, &unwind_buf);
>>
>> - __libc_signal_restore_set (&pd->sigmask);
>> + internal_signal_restore_set (&pd->sigmask);
>
> OK. pd->sigmask is now internal_sigset_t.
>
>>
>> LIBC_PROBE (pthread_start, 3, (pthread_t) pd, pd->start_routine, pd->arg);
>>
>> @@ -501,8 +501,8 @@ start_thread (void *arg)
>> signal to be delivered. (SIGSETXID cannot run application code,
>> nor does it use pthread_kill.) Reuse the pd->sigmask space for
>> computing the signal mask, to save stack space. */
>> - __sigfillset (&pd->sigmask);
>> - __sigdelset (&pd->sigmask, SIGSETXID);
>> + internal_sigfillset (&pd->sigmask);
>> + internal_sigdelset (&pd->sigmask, SIGSETXID);
>
> OK. Switch to functions that operate on internal_sigset_t since
> pd->sigmask is now of this type.
>
>> INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_BLOCK, &pd->sigmask, NULL,
>> __NSIG_BYTES);
>>
>> @@ -766,14 +766,14 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
>> /* Block all signals, so that the new thread starts out with
>> signals disabled. This avoids race conditions in the thread
>> startup. */
>> - sigset_t original_sigmask;
>> - __libc_signal_block_all (&original_sigmask);
>> + internal_sigset_t original_sigmask;
>> + internal_signal_block_all (&original_sigmask);
>
> OK. Use the new type.
>
>>
>> if (iattr->extension != NULL && iattr->extension->sigmask_set)
>> /* Use the signal mask in the attribute. The internal signals
>> have already been filtered by the public
>> pthread_attr_setsigmask_np interface. */
>> - pd->sigmask = iattr->extension->sigmask;
>> + internal_sigset_from_sigset (&pd->sigmask, &iattr->extension->sigmask);
>
> OK. Replace the assignment with conversion to internal type since
> pd->sigmask is now an internal_sigset_t.
>
>> else
>> {
>> /* Conceptually, the new thread needs to inherit the signal mask
>> @@ -783,7 +783,7 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
>> pd->sigmask = original_sigmask;
>> /* Reset the cancellation signal mask in case this thread is
>> running cancellation. */
>> - __sigdelset (&pd->sigmask, SIGCANCEL);
>> + internal_sigdelset (&pd->sigmask, SIGCANCEL);
>
> OK.
>
>> }
>>
>> /* Start the thread. */
>> @@ -830,7 +830,7 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
>>
>> /* Return to the previous signal mask, after creating the new
>> thread. */
>> - __libc_signal_restore_set (&original_sigmask);
>> + internal_signal_restore_set (&original_sigmask);
>
> OK. original_sigmask is now an internal_sigset_t.
>
>>
>> if (__glibc_unlikely (retval != 0))
>> {
>> diff --git a/nptl/pthread_kill.c b/nptl/pthread_kill.c
>> index 8015e10b70..178d38d3bd 100644
>> --- a/nptl/pthread_kill.c
>> +++ b/nptl/pthread_kill.c
>> @@ -45,8 +45,8 @@ __pthread_kill_implementation (pthread_t threadid, int signo, int no_tid)
>> }
>>
>> /* Block all signals, as required by pd->exit_lock. */
>> - sigset_t old_mask;
>> - __libc_signal_block_all (&old_mask);
>> + internal_sigset_t old_mask;
>> + internal_signal_block_all (&old_mask);
>
> OK. Use new type.
>
>> __libc_lock_lock (pd->exit_lock);
>>
>> int ret;
>> @@ -64,7 +64,7 @@ __pthread_kill_implementation (pthread_t threadid, int signo, int no_tid)
>> }
>>
>> __libc_lock_unlock (pd->exit_lock);
>> - __libc_signal_restore_set (&old_mask);
>> + internal_signal_restore_set (&old_mask);
>
> OK. Use new function for new type.
>
>>
>> return ret;
>> }
>> @@ -83,7 +83,7 @@ __pthread_kill (pthread_t threadid, int signo)
>> {
>> /* Disallow sending the signal we use for cancellation, timers,
>> for the setxid implementation. */
>> - if (__is_internal_signal (signo))
>> + if (is_internal_signal (signo))
>
> OK. Function was renamed.
>
>> return EINVAL;
>>
>> return __pthread_kill_internal (threadid, signo);
>> @@ -102,7 +102,7 @@ int
>> attribute_compat_text_section
>> __pthread_kill_esrch (pthread_t threadid, int signo)
>> {
>> - if (__is_internal_signal (signo))
>> + if (is_internal_signal (signo))
>
> OK.
>
>> return EINVAL;
>>
>> return __pthread_kill_implementation (threadid, signo, ESRCH);
>> diff --git a/nptl/pthread_sigmask.c b/nptl/pthread_sigmask.c
>> index 20f811cc6b..977740b97e 100644
>> --- a/nptl/pthread_sigmask.c
>> +++ b/nptl/pthread_sigmask.c
>> @@ -32,7 +32,7 @@ __pthread_sigmask (int how, const sigset_t *newmask, sigset_t *oldmask)
>> || __glibc_unlikely (__sigismember (newmask, SIGSETXID))))
>> {
>> local_newmask = *newmask;
>> - __clear_internal_signals (&local_newmask);
>> + clear_internal_signals (&local_newmask);
>
> OK. Function was renamed.
>
>> newmask = &local_newmask;
>> }
>>
>> diff --git a/rt/tst-timer-sigmask.c b/rt/tst-timer-sigmask.c
>> index 11acb670ab..4d088a691d 100644
>> --- a/rt/tst-timer-sigmask.c
>> +++ b/rt/tst-timer-sigmask.c
>> @@ -43,7 +43,7 @@ thread_handler (union sigval sv)
>> if (sigismember (&ss, sig))
>> {
>> TEST_VERIFY (sig != SIGKILL && sig != SIGSTOP);
>> - TEST_VERIFY (!__is_internal_signal (sig));
>> + TEST_VERIFY (!is_internal_signal (sig));
>
> OK. Function was renamed.
>
>> }
>> if (test_verbose && sigismember (&ss, sig))
>> printf ("%d, ", sig);
>> diff --git a/signal/sigaction.c b/signal/sigaction.c
>> index 8e94b9a10b..d56b3196ca 100644
>> --- a/signal/sigaction.c
>> +++ b/signal/sigaction.c
>> @@ -24,7 +24,7 @@
>> int
>> __sigaction (int sig, const struct sigaction *act, struct sigaction *oact)
>> {
>> - if (sig <= 0 || sig >= NSIG || __is_internal_signal (sig))
>> + if (sig <= 0 || sig >= NSIG || is_internal_signal (sig))
>
> OK. Function was renamed.
>
>> {
>> __set_errno (EINVAL);
>> return -1;
>> diff --git a/signal/sigaddset.c b/signal/sigaddset.c
>> index c6ef0a953a..f0fed84dd7 100644
>> --- a/signal/sigaddset.c
>> +++ b/signal/sigaddset.c
>> @@ -25,7 +25,7 @@ int
>> sigaddset (sigset_t *set, int signo)
>> {
>> if (set == NULL || signo <= 0 || signo >= NSIG
>> - || __is_internal_signal (signo))
>> + || is_internal_signal (signo))
>
> OK. Function was renamed.
>
>> {
>> __set_errno (EINVAL);
>> return -1;
>> diff --git a/signal/sigdelset.c b/signal/sigdelset.c
>> index 81bb9a908a..2c973c9b84 100644
>> --- a/signal/sigdelset.c
>> +++ b/signal/sigdelset.c
>> @@ -25,7 +25,7 @@ int
>> sigdelset (sigset_t *set, int signo)
>> {
>> if (set == NULL || signo <= 0 || signo >= NSIG
>> - || __is_internal_signal (signo))
>> + || is_internal_signal (signo))
>
> OK. Function was renamed.
>
>> {
>> __set_errno (EINVAL);
>> return -1;
>> diff --git a/signal/sigfillset.c b/signal/sigfillset.c
>> index b52ef06aa0..1d11ae4fec 100644
>> --- a/signal/sigfillset.c
>> +++ b/signal/sigfillset.c
>> @@ -31,7 +31,7 @@ sigfillset (sigset_t *set)
>> }
>>
>> __sigfillset (set);
>> - __clear_internal_signals (set);
>> + clear_internal_signals (set);
>
> OK. Function was renamed.
>
>> return 0;
>> }
>> libc_hidden_def (sigfillset)
>> diff --git a/stdlib/abort.c b/stdlib/abort.c
>> index bcf72f9356..9c1065af0e 100644
>> --- a/stdlib/abort.c
>> +++ b/stdlib/abort.c
>> @@ -21,7 +21,7 @@
>> #include <stdlib.h>
>> #include <string.h>
>> #include <unistd.h>
>> -#include <sigsetops.h>
>> +#include <internal-signals.h>
>
> OK. We will be operating on internal_sigset_t.
>
>>
>> /* Try to get a machine dependent instruction which will make the
>> program crash. This is used in case everything else fails. */
>> @@ -48,7 +48,6 @@ void
>> abort (void)
>> {
>> struct sigaction act;
>> - sigset_t sigs;
>
> OK. Declared later.
>
>>
>> /* First acquire the lock. */
>> __libc_lock_lock_recursive (lock);
>> @@ -59,9 +58,10 @@ abort (void)
>> if (stage == 0)
>> {
>> ++stage;
>> - __sigemptyset (&sigs);
>> - __sigaddset (&sigs, SIGABRT);
>> - __sigprocmask (SIG_UNBLOCK, &sigs, 0);
>> + internal_sigset_t sigs;
>> + internal_sigemptyset (&sigs);
>> + internal_sigaddset (&sigs, SIGABRT);
>> + internal_sigprocmask (SIG_UNBLOCK, &sigs, NULL);
>> }
>
> OK. Use new type and functions.
>
>>
>> /* Send signal which possibly calls a user handler. */
>
>> diff --git a/sysdeps/posix/signal.c b/sysdeps/posix/signal.c
>> index 8f17b4278d..88c3757126 100644
>> --- a/sysdeps/posix/signal.c
>> +++ b/sysdeps/posix/signal.c
>> @@ -32,7 +32,7 @@ __bsd_signal (int sig, __sighandler_t handler)
>>
>> /* Check signal extents to protect __sigismember. */
>> if (handler == SIG_ERR || sig < 1 || sig >= NSIG
>> - || __is_internal_signal (sig))
>> + || is_internal_signal (sig))
>
> OK.
>
>> {
>> __set_errno (EINVAL);
>> return SIG_ERR;
>> diff --git a/sysdeps/unix/sysv/linux/ia64/unwind_longjmp.c b/sysdeps/unix/sysv/linux/ia64/unwind_longjmp.c
>> index a59ad78036..a53ae79965 100644
>> --- a/sysdeps/unix/sysv/linux/ia64/unwind_longjmp.c
>> +++ b/sysdeps/unix/sysv/linux/ia64/unwind_longjmp.c
>> @@ -34,7 +34,7 @@ __libc_unwind_longjmp (sigjmp_buf env, int val)
>>
>> if (env[0].__mask_was_saved)
>> /* Restore the saved signal mask. */
>> - __libc_signal_restore_set (&env[0].__saved_mask);
>> + __sigprocmask (SIG_SETMASK, &env[0].__saved_mask, NULL);
>
> OK. internal_signal_restore_set operates on internal_sigset_t, but
> here we have a sigset_t. __saved_mask is a sigset_t, so we cannot use
> it here. Instead, we directly call sigprocmask like we did in
> __libc_signal_restore_set.
>
>>
>> /* Call the machine-dependent function to restore machine state. */
>> __sigstack_longjmp (env[0].__jmpbuf, val ?: 1);
>
>> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
>> index d703485e3f..75f5c06e32 100644
>> --- a/sysdeps/unix/sysv/linux/spawni.c
>> +++ b/sysdeps/unix/sysv/linux/spawni.c
>> @@ -57,7 +57,7 @@
>>
>> struct posix_spawn_args
>> {
>> - sigset_t oldmask;
>> + internal_sigset_t oldmask;
>
> OK.
>
>> const char *file;
>> int (*exec) (const char *, char *const *, char *const *);
>> const posix_spawn_file_actions_t *fa;
>> @@ -124,7 +124,7 @@ __spawni_child (void *arguments)
>> }
>> else if (__sigismember (&hset, sig))
>> {
>> - if (__is_internal_signal (sig))
>> + if (is_internal_signal (sig))
>
> OK.
>
>> sa.sa_handler = SIG_IGN;
>> else
>> {
>> @@ -284,8 +284,10 @@ __spawni_child (void *arguments)
>>
>> /* Set the initial signal mask of the child if POSIX_SPAWN_SETSIGMASK
>> is set, otherwise restore the previous one. */
>> - __sigprocmask (SIG_SETMASK, (attr->__flags & POSIX_SPAWN_SETSIGMASK)
>> - ? &attr->__ss : &args->oldmask, 0);
>> + if (attr->__flags & POSIX_SPAWN_SETSIGMASK)
>> + __sigprocmask (SIG_SETMASK, &attr->__ss, NULL);
>> + else
>> + internal_sigprocmask (SIG_SETMASK, &args->oldmask, NULL);
>
> OK. One is a sigset_t and the other is an internal_sigset_t, so we
> write this differently now. It's also more readable than the "?:"
> version.
>
>>
>> args->exec (args->file, args->argv, args->envp);
>>
>> @@ -368,7 +370,7 @@ __spawnix (pid_t * pid, const char *file,
>> args.envp = envp;
>> args.xflags = xflags;
>>
>> - __libc_signal_block_all (&args.oldmask);
>> + internal_signal_block_all (&args.oldmask);
>
> OK.
>
>>
>> /* The clone flags used will create a new child that will run in the same
>> memory space (CLONE_VM) and the execution of calling thread will be
>> @@ -416,7 +418,7 @@ __spawnix (pid_t * pid, const char *file,
>> if ((ec == 0) && (pid != NULL))
>> *pid = new_pid;
>>
>> - __libc_signal_restore_set (&args.oldmask);
>> + internal_signal_restore_set (&args.oldmask);
>
> OK.
>
>>
>> __pthread_setcancelstate (state, NULL);
>>
>> diff --git a/sysdeps/unix/sysv/linux/timer_routines.c b/sysdeps/unix/sysv/linux/timer_routines.c
>> index 8f0a2f635d..fc3deb6823 100644
>> --- a/sysdeps/unix/sysv/linux/timer_routines.c
>> +++ b/sysdeps/unix/sysv/linux/timer_routines.c
>> @@ -41,7 +41,7 @@ struct thread_start_data
>> static void *
>> timer_sigev_thread (void *arg)
>> {
>> - __libc_signal_unblock_sigtimer (NULL);
>> + signal_unblock_sigtimer (NULL);
>
> OK.
>
>>
>> struct thread_start_data *td = (struct thread_start_data *) arg;
>> void (*thrfunc) (sigval_t) = td->thrfunc;
>> --
>> 2.34.1
@@ -35,6 +35,7 @@
#include <kernel-features.h>
#include <tls-internal-struct.h>
#include <sys/rseq.h>
+#include <internal-sigset.h>
#ifndef TCB_ALIGNMENT
# define TCB_ALIGNMENT 32
@@ -387,7 +388,7 @@ struct pthread
/* Signal mask for the new thread. Used during thread startup to
restore the signal mask. (Threads are launched with all signals
masked.) */
- sigset_t sigmask;
+ internal_sigset_t sigmask;
/* Indicates whether is a C11 thread created by thrd_creat. */
bool c11;
@@ -28,7 +28,7 @@ pthread_attr_setsigmask_np (pthread_attr_t *attr, const sigset_t *sigmask)
/* Filter out internal signals. */
struct pthread_attr *iattr = (struct pthread_attr *) attr;
- __clear_internal_signals (&iattr->extension->sigmask);
+ clear_internal_signals (&iattr->extension->sigmask);
return 0;
}
@@ -423,7 +423,7 @@ start_thread (void *arg)
/* Store the new cleanup handler info. */
THREAD_SETMEM (pd, cleanup_jmp_buf, &unwind_buf);
- __libc_signal_restore_set (&pd->sigmask);
+ internal_signal_restore_set (&pd->sigmask);
LIBC_PROBE (pthread_start, 3, (pthread_t) pd, pd->start_routine, pd->arg);
@@ -501,8 +501,8 @@ start_thread (void *arg)
signal to be delivered. (SIGSETXID cannot run application code,
nor does it use pthread_kill.) Reuse the pd->sigmask space for
computing the signal mask, to save stack space. */
- __sigfillset (&pd->sigmask);
- __sigdelset (&pd->sigmask, SIGSETXID);
+ internal_sigfillset (&pd->sigmask);
+ internal_sigdelset (&pd->sigmask, SIGSETXID);
INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_BLOCK, &pd->sigmask, NULL,
__NSIG_BYTES);
@@ -766,14 +766,14 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
/* Block all signals, so that the new thread starts out with
signals disabled. This avoids race conditions in the thread
startup. */
- sigset_t original_sigmask;
- __libc_signal_block_all (&original_sigmask);
+ internal_sigset_t original_sigmask;
+ internal_signal_block_all (&original_sigmask);
if (iattr->extension != NULL && iattr->extension->sigmask_set)
/* Use the signal mask in the attribute. The internal signals
have already been filtered by the public
pthread_attr_setsigmask_np interface. */
- pd->sigmask = iattr->extension->sigmask;
+ internal_sigset_from_sigset (&pd->sigmask, &iattr->extension->sigmask);
else
{
/* Conceptually, the new thread needs to inherit the signal mask
@@ -783,7 +783,7 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
pd->sigmask = original_sigmask;
/* Reset the cancellation signal mask in case this thread is
running cancellation. */
- __sigdelset (&pd->sigmask, SIGCANCEL);
+ internal_sigdelset (&pd->sigmask, SIGCANCEL);
}
/* Start the thread. */
@@ -830,7 +830,7 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
/* Return to the previous signal mask, after creating the new
thread. */
- __libc_signal_restore_set (&original_sigmask);
+ internal_signal_restore_set (&original_sigmask);
if (__glibc_unlikely (retval != 0))
{
@@ -45,8 +45,8 @@ __pthread_kill_implementation (pthread_t threadid, int signo, int no_tid)
}
/* Block all signals, as required by pd->exit_lock. */
- sigset_t old_mask;
- __libc_signal_block_all (&old_mask);
+ internal_sigset_t old_mask;
+ internal_signal_block_all (&old_mask);
__libc_lock_lock (pd->exit_lock);
int ret;
@@ -64,7 +64,7 @@ __pthread_kill_implementation (pthread_t threadid, int signo, int no_tid)
}
__libc_lock_unlock (pd->exit_lock);
- __libc_signal_restore_set (&old_mask);
+ internal_signal_restore_set (&old_mask);
return ret;
}
@@ -83,7 +83,7 @@ __pthread_kill (pthread_t threadid, int signo)
{
/* Disallow sending the signal we use for cancellation, timers,
for the setxid implementation. */
- if (__is_internal_signal (signo))
+ if (is_internal_signal (signo))
return EINVAL;
return __pthread_kill_internal (threadid, signo);
@@ -102,7 +102,7 @@ int
attribute_compat_text_section
__pthread_kill_esrch (pthread_t threadid, int signo)
{
- if (__is_internal_signal (signo))
+ if (is_internal_signal (signo))
return EINVAL;
return __pthread_kill_implementation (threadid, signo, ESRCH);
@@ -32,7 +32,7 @@ __pthread_sigmask (int how, const sigset_t *newmask, sigset_t *oldmask)
|| __glibc_unlikely (__sigismember (newmask, SIGSETXID))))
{
local_newmask = *newmask;
- __clear_internal_signals (&local_newmask);
+ clear_internal_signals (&local_newmask);
newmask = &local_newmask;
}
@@ -43,7 +43,7 @@ thread_handler (union sigval sv)
if (sigismember (&ss, sig))
{
TEST_VERIFY (sig != SIGKILL && sig != SIGSTOP);
- TEST_VERIFY (!__is_internal_signal (sig));
+ TEST_VERIFY (!is_internal_signal (sig));
}
if (test_verbose && sigismember (&ss, sig))
printf ("%d, ", sig);
@@ -24,7 +24,7 @@
int
__sigaction (int sig, const struct sigaction *act, struct sigaction *oact)
{
- if (sig <= 0 || sig >= NSIG || __is_internal_signal (sig))
+ if (sig <= 0 || sig >= NSIG || is_internal_signal (sig))
{
__set_errno (EINVAL);
return -1;
@@ -25,7 +25,7 @@ int
sigaddset (sigset_t *set, int signo)
{
if (set == NULL || signo <= 0 || signo >= NSIG
- || __is_internal_signal (signo))
+ || is_internal_signal (signo))
{
__set_errno (EINVAL);
return -1;
@@ -25,7 +25,7 @@ int
sigdelset (sigset_t *set, int signo)
{
if (set == NULL || signo <= 0 || signo >= NSIG
- || __is_internal_signal (signo))
+ || is_internal_signal (signo))
{
__set_errno (EINVAL);
return -1;
@@ -31,7 +31,7 @@ sigfillset (sigset_t *set)
}
__sigfillset (set);
- __clear_internal_signals (set);
+ clear_internal_signals (set);
return 0;
}
libc_hidden_def (sigfillset)
@@ -21,7 +21,7 @@
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
-#include <sigsetops.h>
+#include <internal-signals.h>
/* Try to get a machine dependent instruction which will make the
program crash. This is used in case everything else fails. */
@@ -48,7 +48,6 @@ void
abort (void)
{
struct sigaction act;
- sigset_t sigs;
/* First acquire the lock. */
__libc_lock_lock_recursive (lock);
@@ -59,9 +58,10 @@ abort (void)
if (stage == 0)
{
++stage;
- __sigemptyset (&sigs);
- __sigaddset (&sigs, SIGABRT);
- __sigprocmask (SIG_UNBLOCK, &sigs, 0);
+ internal_sigset_t sigs;
+ internal_sigemptyset (&sigs);
+ internal_sigaddset (&sigs, SIGABRT);
+ internal_sigprocmask (SIG_UNBLOCK, &sigs, NULL);
}
/* Send signal which possibly calls a user handler. */
@@ -29,39 +29,20 @@
#define RESERVED_SIGRT 0
static inline bool
-__is_internal_signal (int sig)
+is_internal_signal (int sig)
{
return false;
}
static inline void
-__clear_internal_signals (sigset_t *set)
+clear_internal_signals (sigset_t *set)
{
}
-static inline void
-__libc_signal_block_all (sigset_t *set)
-{
- sigset_t allset;
- __sigfillset (&allset);
- __sigprocmask (SIG_BLOCK, &allset, set);
-}
-
-static inline void
-__libc_signal_block_app (sigset_t *set)
-{
- sigset_t allset;
- __sigfillset (&allset);
- __clear_internal_signals (&allset);
- __sigprocmask (SIG_BLOCK, &allset, set);
-}
-
-/* Restore current process signal mask. */
-static inline void
-__libc_signal_restore_set (const sigset_t *set)
-{
- __sigprocmask (SIG_SETMASK, set, NULL);
-}
+typedef sigset_t internal_sigset_t;
+#define internal_sigemptyset(__s) sigemptyset (__s)
+#define internal_sigaddset(__s, __i) sigaddset (__s, __i)
+#define internal_sigprocmask(__h, __s, __o) sigprocmask (__h, __s, __o)
#endif /* __INTERNAL_SIGNALS_H */
@@ -32,7 +32,7 @@ __bsd_signal (int sig, __sighandler_t handler)
/* Check signal extents to protect __sigismember. */
if (handler == SIG_ERR || sig < 1 || sig >= NSIG
- || __is_internal_signal (sig))
+ || is_internal_signal (sig))
{
__set_errno (EINVAL);
return SIG_ERR;
@@ -34,7 +34,7 @@ __libc_unwind_longjmp (sigjmp_buf env, int val)
if (env[0].__mask_was_saved)
/* Restore the saved signal mask. */
- __libc_signal_restore_set (&env[0].__saved_mask);
+ __sigprocmask (SIG_SETMASK, &env[0].__saved_mask, NULL);
/* Call the machine-dependent function to restore machine state. */
__sigstack_longjmp (env[0].__jmpbuf, val ?: 1);
@@ -19,10 +19,11 @@
#ifndef __INTERNAL_SIGNALS_H
# define __INTERNAL_SIGNALS_H
+#include <internal-sigset.h>
+#include <limits.h>
#include <signal.h>
#include <sigsetops.h>
#include <stdbool.h>
-#include <limits.h>
#include <stddef.h>
#include <sysdep.h>
@@ -47,49 +48,60 @@
/* Return is sig is used internally. */
static inline bool
-__is_internal_signal (int sig)
+is_internal_signal (int sig)
{
return (sig == SIGCANCEL) || (sig == SIGSETXID);
}
/* Remove internal glibc signal from the mask. */
static inline void
-__clear_internal_signals (sigset_t *set)
+clear_internal_signals (sigset_t *set)
{
__sigdelset (set, SIGCANCEL);
__sigdelset (set, SIGSETXID);
}
-static const sigset_t sigall_set = {
- .__val = {[0 ... _SIGSET_NWORDS-1 ] = -1 }
+static const internal_sigset_t sigall_set = {
+ .__val = {[0 ... __NSIG_WORDS-1 ] = -1 }
};
-static const sigset_t sigtimer_set = {
- .__val = { [0] = __sigmask (SIGTIMER),
- [1 ... _SIGSET_NWORDS-1] = 0 }
-};
+/* Obtain and changed blocked signals, including internal glibc ones. */
+static inline int
+internal_sigprocmask (int how, const internal_sigset_t *set,
+ internal_sigset_t *oldset)
+{
+ return INTERNAL_SYSCALL_CALL (rt_sigprocmask, how, set, oldset,
+ __NSIG_BYTES);
+}
/* Block all signals, including internal glibc ones. */
static inline void
-__libc_signal_block_all (sigset_t *set)
+internal_signal_block_all (internal_sigset_t *set)
{
INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_BLOCK, &sigall_set, set,
__NSIG_BYTES);
}
-/* Block all application signals (excluding internal glibc ones). */
+/* Restore current process signal mask. */
static inline void
-__libc_signal_block_app (sigset_t *set)
+internal_signal_restore_set (const internal_sigset_t *set)
{
- sigset_t allset = sigall_set;
- __clear_internal_signals (&allset);
- INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_BLOCK, &allset, set,
+ INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_SETMASK, set, NULL,
__NSIG_BYTES);
}
+
+/* It is used on timer_create code directly on sigwaitinfo call, so it can not
+ use the internal_sigset_t definitions. */
+static const sigset_t sigtimer_set = {
+ .__val = { [0] = __sigmask (SIGTIMER),
+ [1 ... _SIGSET_NWORDS-1] = 0
+ }
+};
+
/* Block only SIGTIMER and return the previous set on SET. */
static inline void
-__libc_signal_block_sigtimer (sigset_t *set)
+signal_block_sigtimer (sigset_t *set)
{
INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_BLOCK, &sigtimer_set, set,
__NSIG_BYTES);
@@ -97,17 +109,10 @@ __libc_signal_block_sigtimer (sigset_t *set)
/* Unblock only SIGTIMER and return the previous set on SET. */
static inline void
-__libc_signal_unblock_sigtimer (sigset_t *set)
+signal_unblock_sigtimer (sigset_t *set)
{
INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_UNBLOCK, &sigtimer_set, set,
__NSIG_BYTES);
}
-/* Restore current process signal mask. */
-static inline void
-__libc_signal_restore_set (const sigset_t *set)
-{
- INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_SETMASK, set, NULL,
- __NSIG_BYTES);
-}
#endif
new file mode 100644
@@ -0,0 +1,105 @@
+/* Internal sigset_t definition.
+ Copyright (C) 2022 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+#ifndef _INTERNAL_SIGSET_H
+#define _INTERNAL_SIGSET_H
+
+#include <sigsetops.h>
+
+typedef struct
+{
+ unsigned long int __val[__NSIG_WORDS];
+} internal_sigset_t;
+
+static inline void
+internal_sigset_from_sigset (internal_sigset_t *iset, const sigset_t *set)
+{
+ int cnt = __NSIG_WORDS;
+ while (--cnt >= 0)
+ iset->__val[cnt] = set->__val[cnt];
+}
+
+static inline void
+internal_sigemptyset (internal_sigset_t *set)
+{
+ int cnt = __NSIG_WORDS;
+ while (--cnt >= 0)
+ set->__val[cnt] = 0;
+}
+
+static inline void
+internal_sigfillset (internal_sigset_t *set)
+{
+ int cnt = __NSIG_WORDS;
+ while (--cnt >= 0)
+ set->__val[cnt] = ~0UL;
+}
+
+static inline int
+internal_sigisemptyset (const internal_sigset_t *set)
+{
+ int cnt = __NSIG_WORDS;
+ int ret = set->__val[--cnt];
+ while (ret == 0 && --cnt >= 0)
+ ret = set->__val[cnt];
+ return ret == 0;
+}
+
+static inline void
+internal_sigandset (internal_sigset_t *dest, const internal_sigset_t *left,
+ const internal_sigset_t *right)
+{
+ int cnt = __NSIG_WORDS;
+ while (--cnt >= 0)
+ dest->__val[cnt] = left->__val[cnt] & right->__val[cnt];
+}
+
+static inline void
+internal_sigorset (internal_sigset_t *dest, const internal_sigset_t *left,
+ const internal_sigset_t *right)
+{
+ int cnt = __NSIG_WORDS;
+ while (--cnt >= 0)
+ dest->__val[cnt] = left->__val[cnt] | right->__val[cnt];
+}
+
+static inline int
+internal_sigismember (const internal_sigset_t *set, int sig)
+{
+ unsigned long int mask = __sigmask (sig);
+ unsigned long int word = __sigword (sig);
+ return set->__val[word] & mask ? 1 : 0;
+}
+
+static inline void
+internal_sigaddset (internal_sigset_t *set, int sig)
+{
+ unsigned long int mask = __sigmask (sig);
+ unsigned long int word = __sigword (sig);
+ set->__val[word] |= mask;
+}
+
+static inline void
+internal_sigdelset (internal_sigset_t *set, int sig)
+{
+ unsigned long int mask = __sigmask (sig);
+ unsigned long int word = __sigword (sig);
+ set->__val[word] &= ~mask;
+}
+
+#endif
@@ -57,7 +57,7 @@
struct posix_spawn_args
{
- sigset_t oldmask;
+ internal_sigset_t oldmask;
const char *file;
int (*exec) (const char *, char *const *, char *const *);
const posix_spawn_file_actions_t *fa;
@@ -124,7 +124,7 @@ __spawni_child (void *arguments)
}
else if (__sigismember (&hset, sig))
{
- if (__is_internal_signal (sig))
+ if (is_internal_signal (sig))
sa.sa_handler = SIG_IGN;
else
{
@@ -284,8 +284,10 @@ __spawni_child (void *arguments)
/* Set the initial signal mask of the child if POSIX_SPAWN_SETSIGMASK
is set, otherwise restore the previous one. */
- __sigprocmask (SIG_SETMASK, (attr->__flags & POSIX_SPAWN_SETSIGMASK)
- ? &attr->__ss : &args->oldmask, 0);
+ if (attr->__flags & POSIX_SPAWN_SETSIGMASK)
+ __sigprocmask (SIG_SETMASK, &attr->__ss, NULL);
+ else
+ internal_sigprocmask (SIG_SETMASK, &args->oldmask, NULL);
args->exec (args->file, args->argv, args->envp);
@@ -368,7 +370,7 @@ __spawnix (pid_t * pid, const char *file,
args.envp = envp;
args.xflags = xflags;
- __libc_signal_block_all (&args.oldmask);
+ internal_signal_block_all (&args.oldmask);
/* The clone flags used will create a new child that will run in the same
memory space (CLONE_VM) and the execution of calling thread will be
@@ -416,7 +418,7 @@ __spawnix (pid_t * pid, const char *file,
if ((ec == 0) && (pid != NULL))
*pid = new_pid;
- __libc_signal_restore_set (&args.oldmask);
+ internal_signal_restore_set (&args.oldmask);
__pthread_setcancelstate (state, NULL);
@@ -41,7 +41,7 @@ struct thread_start_data
static void *
timer_sigev_thread (void *arg)
{
- __libc_signal_unblock_sigtimer (NULL);
+ signal_unblock_sigtimer (NULL);
struct thread_start_data *td = (struct thread_start_data *) arg;
void (*thrfunc) (sigval_t) = td->thrfunc;