This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] nptl: SIGCANCEL, SIGTIMER, SIGSETXID are always defined
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: libc-alpha at sourceware dot org
- Date: Fri, 18 Oct 2019 09:30:59 -0300
- Subject: Re: [PATCH] nptl: SIGCANCEL, SIGTIMER, SIGSETXID are always defined
- References: <87zhhyv4nj.fsf@oldenburg2.str.redhat.com>
On 18/10/2019 05:28, Florian Weimer wrote:
> All nptl targets have these signal definitions nowadays. This
> changes also replaces the nptl-generic version of pthread_sigmask
> with the Linux version.
>
> Tested on x86_64-linux-gnu and i686-linux-gnu. Built with
> build-many-glibcs.py.
Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
>
> -----
> nptl/allocatestack.c | 3 --
> nptl/nptl-init.c | 16 ---------
> nptl/pthread_cancel.c | 7 ----
> nptl/pthread_create.c | 2 --
> nptl/pthread_setcanceltype.c | 5 ---
> nptl/pthread_sigmask.c | 40 ++++++++++++++++-------
> nptl/tst-cancel25.c | 2 --
> nptl/tst-signal7.c | 4 ---
> sysdeps/nptl/allocrtsig.c | 29 +++--------------
> sysdeps/unix/sysv/linux/pthread_sigmask.c | 54 -------------------------------
> 10 files changed, 34 insertions(+), 128 deletions(-)
>
> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
> index 64a9ae6066..086efb75ec 100644
> --- a/nptl/allocatestack.c
> +++ b/nptl/allocatestack.c
> @@ -963,7 +963,6 @@ __reclaim_stacks (void)
> }
>
>
> -#ifdef SIGSETXID
> static void
> setxid_mark_thread (struct xid_command *cmdp, struct pthread *t)
> {
> @@ -1174,8 +1173,6 @@ __nptl_setxid (struct xid_command *cmdp)
> lll_unlock (stack_cache_lock, LLL_PRIVATE);
> return result;
> }
> -#endif /* SIGSETXID. */
> -
>
> static inline void __attribute__((always_inline))
> init_one_static_tls (struct pthread *curp, struct link_map *map)
Ok.
> diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
> index ea91b9e138..acc0f3672b 100644
> --- a/nptl/nptl-init.c
> +++ b/nptl/nptl-init.c
> @@ -114,9 +114,7 @@ static const struct pthread_functions pthread_functions =
> .ptr_nthreads = &__nptl_nthreads,
> .ptr___pthread_unwind = &__pthread_unwind,
> .ptr__nptl_deallocate_tsd = __nptl_deallocate_tsd,
> -# ifdef SIGSETXID
> .ptr__nptl_setxid = __nptl_setxid,
> -# endif
> .ptr_set_robust = __nptl_set_robust
> };
> # define ptr_pthread_functions &pthread_functions
Ok.
> @@ -139,7 +137,6 @@ __nptl_set_robust (struct pthread *self)
> }
>
>
> -#ifdef SIGCANCEL
> /* For asynchronous cancellation we use a signal. This is the handler. */
> static void
> sigcancel_handler (int sig, siginfo_t *si, void *ctx)
> @@ -185,10 +182,8 @@ sigcancel_handler (int sig, siginfo_t *si, void *ctx)
> oldval = curval;
> }
> }
> -#endif
>
>
> -#ifdef SIGSETXID
> struct xid_command *__xidcmd attribute_hidden;
>
> /* We use the SIGSETXID signal in the setuid, setgid, etc. implementations to
> @@ -234,7 +229,6 @@ sighandler_setxid (int sig, siginfo_t *si, void *ctx)
> if (atomic_decrement_val (&__xidcmd->cntr) == 0)
> futex_wake ((unsigned int *) &__xidcmd->cntr, 1, FUTEX_PRIVATE);
> }
> -#endif
>
>
> /* When using __thread for this, we do it in libc so as not
Ok.
> @@ -285,41 +279,31 @@ __pthread_initialize_minimal_internal (void)
> had to set __nptl_initial_report_events. Propagate its setting. */
> THREAD_SETMEM (pd, report_events, __nptl_initial_report_events);
>
> -#if defined SIGCANCEL || defined SIGSETXID
> struct sigaction sa;
> __sigemptyset (&sa.sa_mask);
>
> -# ifdef SIGCANCEL
> /* Install the cancellation signal handler. If for some reason we
> cannot install the handler we do not abort. Maybe we should, but
> it is only asynchronous cancellation which is affected. */
> sa.sa_sigaction = sigcancel_handler;
> sa.sa_flags = SA_SIGINFO;
> (void) __libc_sigaction (SIGCANCEL, &sa, NULL);
> -# endif
>
> -# ifdef SIGSETXID
> /* Install the handle to change the threads' uid/gid. */
> sa.sa_sigaction = sighandler_setxid;
> sa.sa_flags = SA_SIGINFO | SA_RESTART;
> (void) __libc_sigaction (SIGSETXID, &sa, NULL);
> -# endif
>
> /* The parent process might have left the signals blocked. Just in
> case, unblock it. We reuse the signal mask in the sigaction
> structure. It is already cleared. */
> -# ifdef SIGCANCEL
> __sigaddset (&sa.sa_mask, SIGCANCEL);
> -# endif
> -# ifdef SIGSETXID
> __sigaddset (&sa.sa_mask, SIGSETXID);
> -# endif
> {
> INTERNAL_SYSCALL_DECL (err);
> (void) INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_UNBLOCK, &sa.sa_mask,
> NULL, _NSIG / 8);
> }
> -#endif
>
> /* Get the size of the static and alignment requirements for the TLS
> block. */
Ok.
> diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
> index 64ac12e60a..f6be8c1dd6 100644
> --- a/nptl/pthread_cancel.c
> +++ b/nptl/pthread_cancel.c
> @@ -63,7 +63,6 @@ __pthread_cancel (pthread_t th)
> oldval))
> goto again;
>
> -#ifdef SIGCANCEL
> /* The cancellation handler will take care of marking the
> thread as canceled. */
> pid_t pid = __getpid ();
> @@ -73,12 +72,6 @@ __pthread_cancel (pthread_t th)
> SIGCANCEL);
> if (INTERNAL_SYSCALL_ERROR_P (val, err))
> result = INTERNAL_SYSCALL_ERRNO (val, err);
> -#else
> - /* It should be impossible to get here at all, since
> - pthread_setcanceltype should never have allowed
> - PTHREAD_CANCEL_ASYNCHRONOUS to be set. */
> - abort ();
> -#endif
>
> break;
> }
Ok.
> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
> index 130937c3c4..5682c9c2c0 100644
> --- a/nptl/pthread_create.c
> +++ b/nptl/pthread_create.c
> @@ -402,7 +402,6 @@ START_THREAD_DEFN
> }
> #endif
>
> -#ifdef SIGCANCEL
> /* If the parent was running cancellation handlers while creating
> the thread the new thread inherited the signal mask. Reset the
> cancellation signal mask. */
> @@ -415,7 +414,6 @@ START_THREAD_DEFN
> (void) INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_UNBLOCK, &mask,
> NULL, _NSIG / 8);
> }
> -#endif
>
> /* This is where the try/finally block should be created. For
> compilers without that support we do use setjmp. */
OK.
> diff --git a/nptl/pthread_setcanceltype.c b/nptl/pthread_setcanceltype.c
> index d771c31e46..99bc13e993 100644
> --- a/nptl/pthread_setcanceltype.c
> +++ b/nptl/pthread_setcanceltype.c
> @@ -27,11 +27,6 @@ __pthread_setcanceltype (int type, int *oldtype)
> if (type < PTHREAD_CANCEL_DEFERRED || type > PTHREAD_CANCEL_ASYNCHRONOUS)
> return EINVAL;
>
> -#ifndef SIGCANCEL
> - if (type == PTHREAD_CANCEL_ASYNCHRONOUS)
> - return ENOTSUP;
> -#endif
> -
> volatile struct pthread *self = THREAD_SELF;
>
> int oldval = THREAD_GETMEM (self, cancelhandling);
OK.
> diff --git a/nptl/pthread_sigmask.c b/nptl/pthread_sigmask.c
> index c8df527995..4aa774d01d 100644
> --- a/nptl/pthread_sigmask.c
> +++ b/nptl/pthread_sigmask.c
> @@ -1,6 +1,6 @@
> -/* Examine and change blocked signals for a thread. Generic POSIX version.
> - Copyright (C) 2014-2019 Free Software Foundation, Inc.
> +/* Copyright (C) 2002-2019 Free Software Foundation, Inc.
> This file is part of the GNU C Library.
> + Contributed by Ulrich Drepper <drepper@redhat.com>, 2002.
>
> The GNU C Library is free software; you can redistribute it and/or
> modify it under the terms of the GNU Lesser General Public
I would say to keep the only comment, maybe adjusting the following line.
> @@ -19,18 +19,36 @@
> #include <errno.h>
> #include <signal.h>
> #include <pthreadP.h>
> +#include <sysdep.h>
>
> -#if defined SIGCANCEL || defined SIGTIMER || defined SIGSETXID
> -# error "This implementation assumes no internal-only signal numbers."
> -#endif
>
> int
> pthread_sigmask (int how, const sigset_t *newmask, sigset_t *oldmask)
> {
> - /* Here we assume that sigprocmask actually does everything right.
> - The only difference is the return value protocol. */
> - int result = sigprocmask (how, newmask, oldmask);
> - if (result < 0)
> - result = errno;
> - return result;
> + sigset_t local_newmask;
> +
> + /* The only thing we have to make sure here is that SIGCANCEL and
> + SIGSETXID is not blocked. */
> + if (newmask != NULL
> + && (__builtin_expect (__sigismember (newmask, SIGCANCEL), 0)
> + || __builtin_expect (__sigismember (newmask, SIGSETXID), 0)))
> + {
Since you are touching it replace both with __glibc_likely.
> + local_newmask = *newmask;
> + __sigdelset (&local_newmask, SIGCANCEL);
> + __sigdelset (&local_newmask, SIGSETXID);
> + newmask = &local_newmask;
> + }
> +
> +#ifdef INTERNAL_SYSCALL
> + /* We know that realtime signals are available if NPTL is used. */
> + INTERNAL_SYSCALL_DECL (err);
> + int result = INTERNAL_SYSCALL (rt_sigprocmask, err, 4, how, newmask,
> + oldmask, _NSIG / 8);
> +
> + return (INTERNAL_SYSCALL_ERROR_P (result, err)
> + ? INTERNAL_SYSCALL_ERRNO (result, err)
> + : 0);
> +#else
> + return sigprocmask (how, newmask, oldmask) == -1 ? errno : 0;
> +#endif
> }
Also, I think there is no need to keep checking for INTERNAL_SYSCALL.
Just use INTERNAL_SYSCALL_CALL.
> diff --git a/nptl/tst-cancel25.c b/nptl/tst-cancel25.c
> index 24ddd3c01c..c6e1c23c02 100644
> --- a/nptl/tst-cancel25.c
> +++ b/nptl/tst-cancel25.c
> @@ -12,7 +12,6 @@ static pthread_t th2;
> static void *
> tf2 (void *arg)
> {
> -#ifdef SIGCANCEL
> sigset_t mask;
> if (pthread_sigmask (SIG_SETMASK, NULL, &mask) != 0)
> {
> @@ -24,7 +23,6 @@ tf2 (void *arg)
> puts ("SIGCANCEL blocked in new thread");
> exit (1);
> }
> -#endif
>
> /* Sync with the main thread so that we do not test anything else. */
> int e = pthread_barrier_wait (&b);
Ok.
> diff --git a/nptl/tst-signal7.c b/nptl/tst-signal7.c
> index de67ab493c..f42ff6887a 100644
> --- a/nptl/tst-signal7.c
> +++ b/nptl/tst-signal7.c
> @@ -27,7 +27,6 @@ do_test (void)
> {
> int result = 0;
>
> -#ifdef SIGCANCEL
> errno = 0;
> if (sigaction (SIGCANCEL, NULL, NULL) == 0)
> {
> @@ -39,9 +38,7 @@ do_test (void)
> puts ("sigaction(SIGCANCEL) did not set errno to EINVAL");
> result = 1;
> }
> -#endif
>
> -#ifdef SIGSETXID
> errno = 0;
> if (sigaction (SIGSETXID, NULL, NULL) == 0)
> {
> @@ -53,7 +50,6 @@ do_test (void)
> puts ("sigaction(SIGSETXID) did not set errno to EINVAL");
> result = 1;
> }
> -#endif
>
> return result;
> }
Ok.
> diff --git a/sysdeps/nptl/allocrtsig.c b/sysdeps/nptl/allocrtsig.c
> index e9ea038655..3f62bf40e7 100644
> --- a/sysdeps/nptl/allocrtsig.c
> +++ b/sysdeps/nptl/allocrtsig.c
> @@ -19,32 +19,13 @@
> #include <signal.h>
> #include <nptl/pthreadP.h>
>
> -/* Up to three special signals might be used privately by libpthread.
> - Figure out how many unique ones are actually used. */
> -
> -#ifdef SIGCANCEL
> -# define SIGCANCEL_CONSUMES 1
> -#else
> -# define SIGCANCEL_CONSUMES 0
> -#endif
> -
> -#if defined SIGTIMER && (!defined SIGCANCEL || SIGTIMER != SIGCANCEL)
> -# define SIGTIMER_CONSUMES 1
> -#else
> -# define SIGTIMER_CONSUMES 0
> -#endif
> -
> -#if (defined SIGSETXID \
> - && (!defined SIGCANCEL || SIGSETXID != SIGCANCEL) \
> - && (!defined SIGTIMER || SIGSETXID != SIGTIMER))
> -# define SIGSETXID_CONSUMES 1
> -#else
> -# define SIGSETXID_CONSUMES 0
> +#if SIGTIMER != SIGCANCEL
> +# error "SIGTIMER and SIGCANCEL must be the same"
> #endif
Not sure if it is really required them to be the same.
>
> /* This tells the generic code (included below) how many signal
> - numbers need to be reserved for libpthread's private uses. */
> -#define RESERVED_SIGRT \
> - (SIGCANCEL_CONSUMES + SIGTIMER_CONSUMES + SIGSETXID_CONSUMES)
> + numbers need to be reserved for libpthread's private uses
> + (SIGCANCEL and SIGSETXID). */
> +#define RESERVED_SIGRT 2
>
> #include <signal/allocrtsig.c>
Ok.
> diff --git a/sysdeps/unix/sysv/linux/pthread_sigmask.c b/sysdeps/unix/sysv/linux/pthread_sigmask.c
> deleted file mode 100644
> index 4aa774d01d..0000000000
> --- a/sysdeps/unix/sysv/linux/pthread_sigmask.c
> +++ /dev/null
> @@ -1,54 +0,0 @@
> -/* Copyright (C) 2002-2019 Free Software Foundation, Inc.
> - This file is part of the GNU C Library.
> - Contributed by Ulrich Drepper <drepper@redhat.com>, 2002.
> -
> - 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/>. */
> -
> -#include <errno.h>
> -#include <signal.h>
> -#include <pthreadP.h>
> -#include <sysdep.h>
> -
> -
> -int
> -pthread_sigmask (int how, const sigset_t *newmask, sigset_t *oldmask)
> -{
> - sigset_t local_newmask;
> -
> - /* The only thing we have to make sure here is that SIGCANCEL and
> - SIGSETXID is not blocked. */
> - if (newmask != NULL
> - && (__builtin_expect (__sigismember (newmask, SIGCANCEL), 0)
> - || __builtin_expect (__sigismember (newmask, SIGSETXID), 0)))
> - {
> - local_newmask = *newmask;
> - __sigdelset (&local_newmask, SIGCANCEL);
> - __sigdelset (&local_newmask, SIGSETXID);
> - newmask = &local_newmask;
> - }
> -
> -#ifdef INTERNAL_SYSCALL
> - /* We know that realtime signals are available if NPTL is used. */
> - INTERNAL_SYSCALL_DECL (err);
> - int result = INTERNAL_SYSCALL (rt_sigprocmask, err, 4, how, newmask,
> - oldmask, _NSIG / 8);
> -
> - return (INTERNAL_SYSCALL_ERROR_P (result, err)
> - ? INTERNAL_SYSCALL_ERRNO (result, err)
> - : 0);
> -#else
> - return sigprocmask (how, newmask, oldmask) == -1 ? errno : 0;
> -#endif
> -}
>
Ok.