From: Carlos O'Donell <carlos@redhat.com>
To: Adhemerval Zanella <adhemerval.zanella@linaro.org>,
libc-alpha@sourceware.org
Subject: Re: [PATCH v5 04/13] linux: Fix __NSIG_WORDS and add __NSIG_BYTES
Date: Thu, 2 Jul 2020 15:08:36 -0400 [thread overview]
Message-ID: <2b6d4024-28ee-a56f-1ecb-f36cf510c781@redhat.com> (raw)
In-Reply-To: <20200619134352.297146-4-adhemerval.zanella@linaro.org>
On 6/19/20 9:43 AM, Adhemerval Zanella wrote:
> The __NSIG_WORDS value is based on minimum number of words to hold
> the maximum number of signal supported by the architecture. Maximum
s/signal/signals/g
> number of signals non multiple of word is rounded up.
^^^ This sentence needs rewriting for clarity in the commit message.
> This patch also adds __NSIG_BYTES, which is the number of bytes
> required to represent the support number of signals. It is used on
s/support/supported/g, s/on/in/g
> syscall which takes a sigset_t.
s/sycall/syscalls/g
Conceptually I like changes like this which move from hard-coded values
like foo/8 to more concrete semantics e.g. use a value that represents
what you want to express (and not the result of some other computation).
> Checked on x86_64-linux-gnu and i686-linux-gnu.
OK for master with cleaned up commit message.
No regressions on x86_64 and i686.
Tested-by: Carlos O'Donell <carlos@redhat.com>
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> ---
> include/signal.h | 2 ++
> nptl/nptl-init.c | 2 +-
> nptl/pthread_sigmask.c | 2 +-
> sysdeps/unix/sysv/linux/aio_misc.h | 9 ++++++---
> sysdeps/unix/sysv/linux/epoll_pwait.c | 2 +-
> sysdeps/unix/sysv/linux/internal-signals.h | 10 +++++-----
> sysdeps/unix/sysv/linux/ppoll.c | 7 ++++---
> sysdeps/unix/sysv/linux/pselect.c | 2 +-
> sysdeps/unix/sysv/linux/sigaction.c | 3 ++-
> sysdeps/unix/sysv/linux/signalfd.c | 2 +-
> sysdeps/unix/sysv/linux/sigpending.c | 2 +-
> sysdeps/unix/sysv/linux/sigsetops.h | 18 +++++++++++++-----
> sysdeps/unix/sysv/linux/sigsuspend.c | 2 +-
> sysdeps/unix/sysv/linux/sigtimedwait.c | 3 ++-
> sysdeps/unix/sysv/linux/x86/setjmpP.h | 5 ++---
> 15 files changed, 43 insertions(+), 28 deletions(-)
>
> diff --git a/include/signal.h b/include/signal.h
> index aa68f45886..3d6315b741 100644
> --- a/include/signal.h
> +++ b/include/signal.h
> @@ -2,6 +2,8 @@
> # include <signal/signal.h>
>
> # ifndef _ISOMAC
> +# include <sigsetops.h>
> +
OK.
> libc_hidden_proto (sigemptyset)
> libc_hidden_proto (sigfillset)
> libc_hidden_proto (sigaddset)
> diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
> index d4cf20e3d1..95c60a524a 100644
> --- a/nptl/nptl-init.c
> +++ b/nptl/nptl-init.c
> @@ -281,7 +281,7 @@ __pthread_initialize_minimal_internal (void)
> __sigaddset (&sa.sa_mask, SIGCANCEL);
> __sigaddset (&sa.sa_mask, SIGSETXID);
> INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_UNBLOCK, &sa.sa_mask,
> - NULL, _NSIG / 8);
> + NULL, __NSIG_BYTES);
OK.
>
> /* Get the size of the static and alignment requirements for the TLS
> block. */
> diff --git a/nptl/pthread_sigmask.c b/nptl/pthread_sigmask.c
> index d266d296c5..7b65ae1f27 100644
> --- a/nptl/pthread_sigmask.c
> +++ b/nptl/pthread_sigmask.c
> @@ -39,7 +39,7 @@ __pthread_sigmask (int how, const sigset_t *newmask, sigset_t *oldmask)
>
> /* We know that realtime signals are available if NPTL is used. */
> int result = INTERNAL_SYSCALL_CALL (rt_sigprocmask, how, newmask,
> - oldmask, _NSIG / 8);
> + oldmask, __NSIG_BYTES);
OK.
>
> return (INTERNAL_SYSCALL_ERROR_P (result)
> ? INTERNAL_SYSCALL_ERRNO (result)
> diff --git a/sysdeps/unix/sysv/linux/aio_misc.h b/sysdeps/unix/sysv/linux/aio_misc.h
> index 30c3cd778e..e31ca8edbe 100644
> --- a/sysdeps/unix/sysv/linux/aio_misc.h
> +++ b/sysdeps/unix/sysv/linux/aio_misc.h
> @@ -31,7 +31,8 @@ __aio_start_notify_thread (void)
> {
> sigset_t ss;
> sigemptyset (&ss);
> - INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_SETMASK, &ss, NULL, _NSIG / 8);
> + INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_SETMASK, &ss, NULL,
> + __NSIG_BYTES);
> }
>
> extern inline int
> @@ -52,12 +53,14 @@ __aio_create_helper_thread (pthread_t *threadp, void *(*tf) (void *),
> sigset_t ss;
> sigset_t oss;
> sigfillset (&ss);
> - INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_SETMASK, &ss, &oss, _NSIG / 8);
> + INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_SETMASK, &ss, &oss,
> + __NSIG_BYTES);
>
> int ret = pthread_create (threadp, &attr, tf, arg);
>
> /* Restore the signal mask. */
> - INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_SETMASK, &oss, NULL, _NSIG / 8);
> + INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_SETMASK, &oss, NULL,
> + __NSIG_BYTES);
>
> (void) pthread_attr_destroy (&attr);
> return ret;
> diff --git a/sysdeps/unix/sysv/linux/epoll_pwait.c b/sysdeps/unix/sysv/linux/epoll_pwait.c
> index 66f04482c7..af6d0fd713 100644
> --- a/sysdeps/unix/sysv/linux/epoll_pwait.c
> +++ b/sysdeps/unix/sysv/linux/epoll_pwait.c
> @@ -38,6 +38,6 @@ int epoll_pwait (int epfd, struct epoll_event *events,
> const sigset_t *set)
> {
> return SYSCALL_CANCEL (epoll_pwait, epfd, events, maxevents,
> - timeout, set, _NSIG / 8);
> + timeout, set, __NSIG_BYTES);
> }
> libc_hidden_def (epoll_pwait)
> diff --git a/sysdeps/unix/sysv/linux/internal-signals.h b/sysdeps/unix/sysv/linux/internal-signals.h
> index 3fbd4807d1..35f2de04c5 100644
> --- a/sysdeps/unix/sysv/linux/internal-signals.h
> +++ b/sysdeps/unix/sysv/linux/internal-signals.h
> @@ -68,7 +68,7 @@ static inline void
> __libc_signal_block_all (sigset_t *set)
> {
> INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_BLOCK, &sigall_set, set,
> - _NSIG / 8);
> + __NSIG_BYTES);
> }
>
> /* Block all application signals (excluding internal glibc ones). */
> @@ -78,7 +78,7 @@ __libc_signal_block_app (sigset_t *set)
> sigset_t allset = sigall_set;
> __clear_internal_signals (&allset);
> INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_BLOCK, &allset, set,
> - _NSIG / 8);
> + __NSIG_BYTES);
> }
>
> /* Block only SIGTIMER and return the previous set on SET. */
> @@ -86,7 +86,7 @@ static inline void
> __libc_signal_block_sigtimer (sigset_t *set)
> {
> INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_BLOCK, &sigtimer_set, set,
> - _NSIG / 8);
> + __NSIG_BYTES);
> }
>
> /* Unblock only SIGTIMER and return the previous set on SET. */
> @@ -94,7 +94,7 @@ static inline void
> __libc_signal_unblock_sigtimer (sigset_t *set)
> {
> INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_UNBLOCK, &sigtimer_set, set,
> - _NSIG / 8);
> + __NSIG_BYTES);
> }
>
> /* Restore current process signal mask. */
> @@ -102,7 +102,7 @@ static inline void
> __libc_signal_restore_set (const sigset_t *set)
> {
> INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_SETMASK, set, NULL,
> - _NSIG / 8);
> + __NSIG_BYTES);
> }
>
> /* Used to communicate with signal handler. */
> diff --git a/sysdeps/unix/sysv/linux/ppoll.c b/sysdeps/unix/sysv/linux/ppoll.c
> index 4ffb23710e..0f15636cce 100644
> --- a/sysdeps/unix/sysv/linux/ppoll.c
> +++ b/sysdeps/unix/sysv/linux/ppoll.c
> @@ -41,11 +41,12 @@ __ppoll64 (struct pollfd *fds, nfds_t nfds, const struct __timespec64 *timeout,
> # ifndef __NR_ppoll_time64
> # define __NR_ppoll_time64 __NR_ppoll
> # endif
> - return SYSCALL_CANCEL (ppoll_time64, fds, nfds, timeout, sigmask, _NSIG / 8);
> + return SYSCALL_CANCEL (ppoll_time64, fds, nfds, timeout, sigmask,
> + __NSIG_BYTES);
> #else
> # ifdef __NR_ppoll_time64
> int ret = SYSCALL_CANCEL (ppoll_time64, fds, nfds, timeout, sigmask,
> - _NSIG / 8);
> + __NSIG_BYTES);
> if (ret >= 0 || errno != ENOSYS)
> return ret;
> # endif
> @@ -62,7 +63,7 @@ __ppoll64 (struct pollfd *fds, nfds_t nfds, const struct __timespec64 *timeout,
> }
>
> return SYSCALL_CANCEL (ppoll, fds, nfds, timeout ? &ts32 : NULL, sigmask,
> - _NSIG / 8);
> + __NSIG_BYTES);
> #endif
> }
>
> diff --git a/sysdeps/unix/sysv/linux/pselect.c b/sysdeps/unix/sysv/linux/pselect.c
> index d7c6ff8fdb..304db03338 100644
> --- a/sysdeps/unix/sysv/linux/pselect.c
> +++ b/sysdeps/unix/sysv/linux/pselect.c
> @@ -43,7 +43,7 @@ __pselect (int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
> } data;
>
> data.ss = (__syscall_ulong_t) (uintptr_t) sigmask;
> - data.ss_len = _NSIG / 8;
> + data.ss_len = __NSIG_BYTES;
>
> return SYSCALL_CANCEL (pselect6, nfds, readfds, writefds, exceptfds,
> timeout, &data);
> diff --git a/sysdeps/unix/sysv/linux/sigaction.c b/sysdeps/unix/sysv/linux/sigaction.c
> index 4e6d11a6ae..f27349d552 100644
> --- a/sysdeps/unix/sysv/linux/sigaction.c
> +++ b/sysdeps/unix/sysv/linux/sigaction.c
> @@ -57,7 +57,8 @@ __libc_sigaction (int sig, const struct sigaction *act, struct sigaction *oact)
> real size of the user-level sigset_t. */
> result = INLINE_SYSCALL_CALL (rt_sigaction, sig,
> act ? &kact : NULL,
> - oact ? &koact : NULL, STUB (act, _NSIG / 8));
> + oact ? &koact : NULL, STUB (act,
> + __NSIG_BYTES));
>
> if (oact && result >= 0)
> {
> diff --git a/sysdeps/unix/sysv/linux/signalfd.c b/sysdeps/unix/sysv/linux/signalfd.c
> index 64d7bccba9..71d91fdde5 100644
> --- a/sysdeps/unix/sysv/linux/signalfd.c
> +++ b/sysdeps/unix/sysv/linux/signalfd.c
> @@ -24,5 +24,5 @@
> int
> signalfd (int fd, const sigset_t *mask, int flags)
> {
> - return INLINE_SYSCALL (signalfd4, 4, fd, mask, _NSIG / 8, flags);
> + return INLINE_SYSCALL (signalfd4, 4, fd, mask, __NSIG_BYTES, flags);
> }
> diff --git a/sysdeps/unix/sysv/linux/sigpending.c b/sysdeps/unix/sysv/linux/sigpending.c
> index 458a3cf99e..8898fe8b34 100644
> --- a/sysdeps/unix/sysv/linux/sigpending.c
> +++ b/sysdeps/unix/sysv/linux/sigpending.c
> @@ -24,5 +24,5 @@
> int
> sigpending (sigset_t *set)
> {
> - return INLINE_SYSCALL (rt_sigpending, 2, set, _NSIG / 8);
> + return INLINE_SYSCALL_CALL (rt_sigpending, set, __NSIG_BYTES);
> }
> diff --git a/sysdeps/unix/sysv/linux/sigsetops.h b/sysdeps/unix/sysv/linux/sigsetops.h
> index db8f378cf0..3f29ead009 100644
OK. All above is just search and replace refactor.
> --- a/sysdeps/unix/sysv/linux/sigsetops.h
> +++ b/sysdeps/unix/sysv/linux/sigsetops.h
> @@ -20,23 +20,31 @@
> #define _SIGSETOPS_H 1
>
> #include <signal.h>
> +#include <limits.h>
> +#include <libc-pointer-arith.h>
OK.
>
> /* Return a mask that includes the bit for SIG only. */
> -# define __sigmask(sig) \
> - (((unsigned long int) 1) << (((sig) - 1) % (8 * sizeof (unsigned long int))))
> +#define __sigmask(sig) \
> + (1UL << (((sig) - 1) % ULONG_WIDTH))
OK.
>
> /* Return the word index for SIG. */
> static inline unsigned long int
> __sigword (int sig)
> {
> - return (sig - 1) / (8 * sizeof (unsigned long int));
> + return (sig - 1) / ULONG_WIDTH;
OK.
> }
>
> /* Linux sig* functions only handle up to __NSIG_WORDS words instead of
> full _SIGSET_NWORDS sigset size. The signal numbers are 1-based, and
> bit 0 of a signal mask is for signal 1. */
> -
> -# define __NSIG_WORDS (_NSIG / (8 * sizeof (unsigned long int )))
> +#define __NSIG_WORDS (ALIGN_UP ((_NSIG - 1), ULONG_WIDTH) / ULONG_WIDTH)
> +_Static_assert (__NSIG_WORDS <= _SIGSET_NWORDS,
> + "__NSIG_WORDS > _SIGSET_WORDS");
OK.
> +
> +/* This macro is used on syscall that takes a sigset_t to specify the expected
> + size in bytes. As for glibc, kernel sigset is implemented as an array of
> + unsigned long. */
> +#define __NSIG_BYTES (__NSIG_WORDS * (ULONG_WIDTH / UCHAR_WIDTH))
OK.
>
> static inline void
> __sigemptyset (sigset_t *set)
> diff --git a/sysdeps/unix/sysv/linux/sigsuspend.c b/sysdeps/unix/sysv/linux/sigsuspend.c
> index dd5df5af25..b4bf2ec4bc 100644
> --- a/sysdeps/unix/sysv/linux/sigsuspend.c
> +++ b/sysdeps/unix/sysv/linux/sigsuspend.c
> @@ -23,7 +23,7 @@
> int
> __sigsuspend (const sigset_t *set)
> {
> - return SYSCALL_CANCEL (rt_sigsuspend, set, _NSIG / 8);
> + return SYSCALL_CANCEL (rt_sigsuspend, set, __NSIG_BYTES);
> }
> libc_hidden_def (__sigsuspend)
> weak_alias (__sigsuspend, sigsuspend)
> diff --git a/sysdeps/unix/sysv/linux/sigtimedwait.c b/sysdeps/unix/sysv/linux/sigtimedwait.c
> index 6b3d8f705f..f2ef3aad45 100644
> --- a/sysdeps/unix/sysv/linux/sigtimedwait.c
> +++ b/sysdeps/unix/sysv/linux/sigtimedwait.c
> @@ -26,7 +26,8 @@ __sigtimedwait (const sigset_t *set, siginfo_t *info,
> {
> /* XXX The size argument hopefully will have to be changed to the
> real size of the user-level sigset_t. */
> - int result = SYSCALL_CANCEL (rt_sigtimedwait, set, info, timeout, _NSIG / 8);
> + int result = SYSCALL_CANCEL (rt_sigtimedwait, set, info, timeout,
> + __NSIG_BYTES);
>
> /* The kernel generates a SI_TKILL code in si_code in case tkill is
> used. tkill is transparently used in raise(). Since having
> diff --git a/sysdeps/unix/sysv/linux/x86/setjmpP.h b/sysdeps/unix/sysv/linux/x86/setjmpP.h
> index 1783b8eb78..a5de31bfd6 100644
> --- a/sysdeps/unix/sysv/linux/x86/setjmpP.h
> +++ b/sysdeps/unix/sysv/linux/x86/setjmpP.h
> @@ -21,6 +21,7 @@
>
> #include <bits/types/__sigset_t.h>
> #include <libc-pointer-arith.h>
> +#include <sigsetops.h>
>
> /* <setjmp/setjmp.h> has
>
> @@ -113,11 +114,9 @@ typedef union
>
> #include <signal.h>
>
> -#define _SIGPROCMASK_NSIG_WORDS (_NSIG / (8 * sizeof (unsigned long int)))
> -
> typedef struct
> {
> - unsigned long int __val[_SIGPROCMASK_NSIG_WORDS];
> + unsigned long int __val[__NSIG_WORDS];
> } __sigprocmask_sigset_t;
>
> extern jmp_buf ___buf;
>
OK.
--
Cheers,
Carlos.
next prev parent reply other threads:[~2020-07-02 19:08 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-19 13:43 [PATCH v5 01/13] signal: Add signum-{generic,arch}.h Adhemerval Zanella
2020-06-19 13:43 ` [PATCH v5 02/13] signal: Move sys_siglist to a compat symbol Adhemerval Zanella
2020-07-02 18:32 ` Carlos O'Donell
2020-07-03 17:58 ` Adhemerval Zanella
2020-06-19 13:43 ` [PATCH v5 03/13] signal: Move sys_errlist " Adhemerval Zanella
2020-07-02 18:48 ` Carlos O'Donell
2020-07-03 19:17 ` Adhemerval Zanella
2020-06-19 13:43 ` [PATCH v5 04/13] linux: Fix __NSIG_WORDS and add __NSIG_BYTES Adhemerval Zanella
2020-07-02 19:08 ` Carlos O'Donell [this message]
2020-07-03 19:23 ` Adhemerval Zanella
2020-06-19 13:43 ` [PATCH v5 05/13] string: Remove old TLS usage on strsignal Adhemerval Zanella
2020-07-02 19:32 ` Carlos O'Donell
2020-07-03 19:26 ` Adhemerval Zanella
2020-06-19 13:43 ` [PATCH v5 06/13] string: Implement strerror in terms of strerror_l Adhemerval Zanella
2020-07-02 19:44 ` Carlos O'Donell
2020-07-03 19:53 ` Adhemerval Zanella
2020-06-19 13:43 ` [PATCH v5 07/13] string: Use tls-internal on strerror_l Adhemerval Zanella
2020-07-02 19:48 ` Carlos O'Donell
2020-06-19 13:43 ` [PATCH v5 08/13] string: Simplify strerror_r Adhemerval Zanella
2020-07-02 19:51 ` Carlos O'Donell
2020-06-19 13:43 ` [PATCH v5 09/13] string: Add strsignal test Adhemerval Zanella
2020-07-02 19:55 ` Carlos O'Donell
2020-07-02 20:01 ` Carlos O'Donell
2020-07-03 20:06 ` Adhemerval Zanella
2020-06-19 13:43 ` [PATCH v5 10/13] string: Add strerror, strerror_r, and strerror_l test Adhemerval Zanella
2020-07-02 20:02 ` Carlos O'Donell
2020-06-19 13:43 ` [PATCH v5 11/13] string: Add strerror_l on test-strerror-errno Adhemerval Zanella
2020-07-02 20:05 ` Carlos O'Donell
2020-06-19 13:43 ` [PATCH v5 12/13] string: Add sigabbrev_np and sigdescr_np Adhemerval Zanella
2020-07-02 21:13 ` Carlos O'Donell
2020-07-03 20:23 ` Adhemerval Zanella
[not found] ` <VI1PR08MB532546AD22FA628741788C5BFF670@VI1PR08MB5325.eurprd08.prod.outlook.com>
2020-07-08 11:18 ` Tamar Christina
2020-07-08 11:36 ` Florian Weimer
2020-07-08 11:48 ` Tamar Christina
2020-07-08 11:52 ` Florian Weimer
2020-07-08 12:16 ` Tamar Christina
2020-07-08 12:23 ` Adhemerval Zanella
2020-06-19 13:43 ` [PATCH v5 13/13] string: Add strerrorname and strerrordesc Adhemerval Zanella
2020-07-02 21:43 ` Carlos O'Donell
2020-07-03 21:01 ` Adhemerval Zanella
2020-07-03 21:19 ` Carlos O'Donell
2020-07-03 22:34 ` Adhemerval Zanella
2020-07-06 12:43 ` Carlos O'Donell
2020-07-06 13:57 ` [PATCH v6] string: Add strerrorname_np and strerrordesc_np Adhemerval Zanella
2020-07-06 14:04 ` Andreas Schwab
2020-07-06 14:06 ` Adhemerval Zanella
2020-07-06 15:07 ` Carlos O'Donell
2020-07-02 18:26 ` [PATCH v5 01/13] signal: Add signum-{generic,arch}.h Carlos O'Donell
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2b6d4024-28ee-a56f-1ecb-f36cf510c781@redhat.com \
--to=carlos@redhat.com \
--cc=adhemerval.zanella@linaro.org \
--cc=libc-alpha@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).