Re: [PATCH v3 2/3] Consolidate Linux sigprocmask implementation (BZ #22391)

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

Ping.

On 01/01/2018 19:56, Adhemerval Zanella wrote:
> Ping.
> 
> On 12/12/2017 15:03, Adhemerval Zanella wrote:
>> Ping (with Rical suggested changes).
>>
>> On 15/11/2017 15:54, Adhemerval Zanella wrote:
>>> Changes from previous version:
>>>
>>>   - Cleanup __nptl prefix from the generic names.
>>>
>>> ---
>>>
>>> This patch consolidates the sigprocmask Linux syscall implementation on
>>> sysdeps/unix/sysv/linux/sigprocmask.c.  The changes are:
>>>
>>>   1. For ia64, s390-64, sparc64, and x86_64 the default semantic for
>>>      filter out SIGCANCEL and SIGSETXID is used.  Also the Linux pthread
>>>      semantic is documented in the signal chapter.
>>>
>>>   2. A new internal function to check for NPTL internal signals within a
>>>      signal set is added (__has_internal_signal).  It is used to
>>>      simplify the default sigprocmask.c implementation.
>>>
>>> Checked on x86_64-linux-gnu.
>>>
>>> 	[BZ #22391]
>>> 	* manual/signal.texi: Add a note about internal pthread signals
>>> 	on Linux.
>>> 	* sysdeps/unix/sysv/linux/alpha/sigprocmask.c: Remove file.
>>> 	* sysdeps/unix/sysv/linux/ia64/sigprocmask.c: Likewise.
>>> 	* sysdeps/unix/sysv/linux/s390/s390-64/sigprocmask.c: Likewise.
>>> 	* sysdeps/unix/sysv/linux/sparc/sparc64/sigprocmask.c: Likewise.
>>> 	* sysdeps/unix/sysv/linux/x86_64/sigprocmask.c: Likewise.
>>> 	* sysdeps/unix/sysv/linux/internal-signals.h
>>> 	(__has_internal_signal): New function.
>>> 	* sysdeps/unix/sysv/linux/sigprocmask.c (__sigprocmask):
>>> 	Use __has_internal_signal and __clear_internal_signals
>>> 	function.
>>>
>>> Signed-off-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
>>> Signed-off-by: Zack Weinberg <zackw@panix.com>
>>> Reported-by: Yury Norov <ynorov@caviumnetworks.com>
>>> ---
>>>  ChangeLog                                          | 17 +++++++
>>>  manual/signal.texi                                 | 37 ++++++++++++++
>>>  sysdeps/unix/sysv/linux/alpha/sigprocmask.c        | 58 ----------------------
>>>  sysdeps/unix/sysv/linux/ia64/sigprocmask.c         | 40 ---------------
>>>  sysdeps/unix/sysv/linux/internal-signals.h         |  6 +++
>>>  sysdeps/unix/sysv/linux/s390/s390-64/sigprocmask.c | 38 --------------
>>>  sysdeps/unix/sysv/linux/sigprocmask.c              | 23 +++------
>>>  .../unix/sysv/linux/sparc/sparc64/sigprocmask.c    | 34 -------------
>>>  sysdeps/unix/sysv/linux/x86_64/sigprocmask.c       | 39 ---------------
>>>  9 files changed, 66 insertions(+), 226 deletions(-)
>>>  delete mode 100644 sysdeps/unix/sysv/linux/alpha/sigprocmask.c
>>>  delete mode 100644 sysdeps/unix/sysv/linux/ia64/sigprocmask.c
>>>  delete mode 100644 sysdeps/unix/sysv/linux/s390/s390-64/sigprocmask.c
>>>  delete mode 100644 sysdeps/unix/sysv/linux/sparc/sparc64/sigprocmask.c
>>>  delete mode 100644 sysdeps/unix/sysv/linux/x86_64/sigprocmask.c
>>>
>>> diff --git a/manual/signal.texi b/manual/signal.texi
>>> index 9577ff0..4f0ef59 100644
>>> --- a/manual/signal.texi
>>> +++ b/manual/signal.texi
>>> @@ -235,6 +235,7 @@ defined.  Since the signal numbers are allocated consecutively,
>>>  * Job Control Signals::         Signals used to support job control.
>>>  * Operation Error Signals::     Used to report operational system errors.
>>>  * Miscellaneous Signals::       Miscellaneous Signals.
>>> +* Internally-Used Signals::     Signals used internally by the C library.
>>>  * Signal Messages::             Printing a message describing a signal.
>>>  @end menu
>>>  
>>> @@ -794,6 +795,26 @@ in @ref{Signaling Another Process}.
>>>  The default action is to terminate the process.
>>>  @end deftypevr
>>>  
>>> +@deftypevr Macro int SIGRTMIN
>>> +@deftypevrx Macro int SIGRTMAX
>>> +@standards{POSIX.1, signal.h}
>>> +@cindex real-time signals
>>> +The range of signal numbers @code{SIGRTMIN}, @code{SIGRTMIN+1},
>>> +@dots{}, @code{SIGRTMAX} is also set aside for you to use any way you
>>> +want.  In addition, these signals (and no others) are guaranteed to
>>> +support @dfn{real-time} signal semantics, which unfortunately this
>>> +manual does not yet document.
>>> +
>>> +Unlike all of the other signal number macros, @code{SIGRTMIN} and
>>> +@code{SIGRTMAX} are not compile-time constants, because some operating
>>> +systems make the number of real-time signals tunable on a
>>> +per-installation or even per-process basis.  However, POSIX guarantees
>>> +that there will be at least 8 signal numbers in this range.
>>> +
>>> +The default action for all signals in this range is to terminate the
>>> +process.
>>> +@end deftypevr
>>> +
>>>  @deftypevr Macro int SIGWINCH
>>>  @standards{BSD, signal.h}
>>>  Window size change.  This is generated on some systems (including GNU)
>>> @@ -817,6 +838,22 @@ to print some status information about the system and what the process
>>>  is doing.  Otherwise the default is to do nothing.
>>>  @end deftypevr
>>>  
>>> +@node Internally-Used Signals
>>> +@subsection Internally-Used Signals
>>> +@cindex internally used signals
>>> +
>>> +On some operating systems, @theglibc{} needs to use a few signals from
>>> +the ``true'' real-time range internally, to implement thread
>>> +cancellation, cross-thread effective ID synchronization, POSIX timer
>>> +management, etc.  @Theglibc{} adjusts @code{SIGRTMIN} and
>>> +@code{SIGRTMAX} to exclude these signals, and it also takes steps to
>>> +prevent application code from altering their state: @code{sigprocmask}
>>> +cannot block them and @code{sigaction} cannot redefine their handlers,
>>> +for instance.  Therefore, most programs do not need to know or care
>>> +about these signals.  We mainly document their existence for the sake
>>> +of anyone who has ever wondered why there is a gap between the
>>> +highest-numbered ``normal'' signal and @code{SIGRTMIN} on Linux.
>>> +
>>>  @node Signal Messages
>>>  @subsection Signal Messages
>>>  @cindex signal messages
>>> diff --git a/sysdeps/unix/sysv/linux/alpha/sigprocmask.c b/sysdeps/unix/sysv/linux/alpha/sigprocmask.c
>>> deleted file mode 100644
>>> index ebec70c..0000000
>>> --- a/sysdeps/unix/sysv/linux/alpha/sigprocmask.c
>>> +++ /dev/null
>>> @@ -1,58 +0,0 @@
>>> -/* Copyright (C) 1993-2017 Free Software Foundation, Inc.
>>> -   This file is part of the GNU C Library.
>>> -   Contributed by David Mosberger (davidm@azstarnet.com).
>>> -
>>> -   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
>>> -   <http://www.gnu.org/licenses/>.  */
>>> -
>>> -#include <errno.h>
>>> -#include <sysdep.h>
>>> -#include <signal.h>
>>> -
>>> -/* When there is kernel support for more than 64 signals, we'll have to
>>> -   switch to a new system call convention here.  */
>>> -
>>> -int
>>> -__sigprocmask (int how, const sigset_t *set, sigset_t *oset)
>>> -{
>>> -  unsigned long int setval;
>>> -  long result;
>>> -
>>> -  if (set)
>>> -    setval = set->__val[0];
>>> -  else
>>> -    {
>>> -      setval = 0;
>>> -      how = SIG_BLOCK;	/* ensure blocked mask doesn't get changed */
>>> -    }
>>> -
>>> -  result = INLINE_SYSCALL (osf_sigprocmask, 2, how, setval);
>>> -  if (result == -1)
>>> -    /* If there are ever more than 63 signals, we need to recode this
>>> -       in assembler since we wouldn't be able to distinguish a mask of
>>> -       all 1s from -1, but for now, we're doing just fine... */
>>> -    return result;
>>> -
>>> -  if (oset)
>>> -    {
>>> -      oset->__val[0] = result;
>>> -      result = _SIGSET_NWORDS;
>>> -      while (--result > 0)
>>> -	oset->__val[result] = 0;
>>> -    }
>>> -  return 0;
>>> -}
>>> -
>>> -libc_hidden_def (__sigprocmask)
>>> -weak_alias (__sigprocmask, sigprocmask);
>>> diff --git a/sysdeps/unix/sysv/linux/ia64/sigprocmask.c b/sysdeps/unix/sysv/linux/ia64/sigprocmask.c
>>> deleted file mode 100644
>>> index 920c5fd..0000000
>>> --- a/sysdeps/unix/sysv/linux/ia64/sigprocmask.c
>>> +++ /dev/null
>>> @@ -1,40 +0,0 @@
>>> -/* Copyright (C) 1997-2017 Free Software Foundation, Inc.
>>> -   This file is part of the GNU C Library.
>>> -   Linux/IA64 specific sigprocmask
>>> -   Written by Jes Sorensen, <Jes.Sorensen@cern.ch>, April 1999.
>>> -
>>> -   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
>>> -   <http://www.gnu.org/licenses/>.  */
>>> -
>>> -/* Linux/ia64 only has rt signals, thus we do not even want to try falling
>>> -   back to the old style signals as the default Linux handler does. */
>>> -
>>> -#include <errno.h>
>>> -#include <signal.h>
>>> -#include <unistd.h>
>>> -
>>> -#include <sysdep.h>
>>> -#include <sys/syscall.h>
>>> -
>>> -/* Get and/or change the set of blocked signals.  */
>>> -int
>>> -__sigprocmask (int how, const sigset_t *set, sigset_t *oset)
>>> -{
>>> -
>>> -  /* XXX The size argument hopefully will have to be changed to the
>>> -     real size of the user-level sigset_t.  */
>>> -  return INLINE_SYSCALL (rt_sigprocmask, 4, how, set, oset, _NSIG / 8);
>>> -}
>>> -libc_hidden_def (__sigprocmask)
>>> -weak_alias (__sigprocmask, sigprocmask)
>>> diff --git a/sysdeps/unix/sysv/linux/internal-signals.h b/sysdeps/unix/sysv/linux/internal-signals.h
>>> index e96a718..bb8234d 100644
>>> --- a/sysdeps/unix/sysv/linux/internal-signals.h
>>> +++ b/sysdeps/unix/sysv/linux/internal-signals.h
>>> @@ -36,6 +36,12 @@
>>>  #define SIGSETXID       (__SIGRTMIN + 1)
>>>  
>>>  
>>> +static inline bool
>>> +__has_internal_signal (const sigset_t *set)
>>> +{
>>> +  return __sigismember (set, SIGCANCEL) || __sigismember (set, SIGSETXID);
>>> +}
>>> +
>>>  /* Return is sig is used internally.  */
>>>  static inline int
>>>  __is_internal_signal (int sig)
>>> diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/sigprocmask.c b/sysdeps/unix/sysv/linux/s390/s390-64/sigprocmask.c
>>> deleted file mode 100644
>>> index a8010e7..0000000
>>> --- a/sysdeps/unix/sysv/linux/s390/s390-64/sigprocmask.c
>>> +++ /dev/null
>>> @@ -1,38 +0,0 @@
>>> -/* Copyright (C) 2001-2017 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
>>> -   <http://www.gnu.org/licenses/>.  */
>>> -
>>> -/* 64 bit Linux for S/390 only has rt signals, thus we do not even want to try
>>> -   falling back to the old style signals as the default Linux handler does. */
>>> -
>>> -#include <errno.h>
>>> -#include <signal.h>
>>> -#include <unistd.h>
>>> -
>>> -#include <sysdep.h>
>>> -#include <sys/syscall.h>
>>> -
>>> -/* Get and/or change the set of blocked signals.  */
>>> -int
>>> -__sigprocmask (int how, const sigset_t *set, sigset_t *oset)
>>> -{
>>> -
>>> -  /* XXX The size argument hopefully will have to be changed to the
>>> -     real size of the user-level sigset_t.  */
>>> -  return INLINE_SYSCALL (rt_sigprocmask, 4, how, set, oset, _NSIG / 8);
>>> -}
>>> -libc_hidden_def (__sigprocmask)
>>> -weak_alias (__sigprocmask, sigprocmask)
>>> diff --git a/sysdeps/unix/sysv/linux/sigprocmask.c b/sysdeps/unix/sysv/linux/sigprocmask.c
>>> index e776563..e574b5d 100644
>>> --- a/sysdeps/unix/sysv/linux/sigprocmask.c
>>> +++ b/sysdeps/unix/sysv/linux/sigprocmask.c
>>> @@ -1,4 +1,5 @@
>>> -/* Copyright (C) 1997-2017 Free Software Foundation, Inc.
>>> +/* Get and/or change the set of blocked signals.  Linux version.
>>> +   Copyright (C) 1997-2017 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
>>> @@ -17,34 +18,22 @@
>>>  
>>>  #include <errno.h>
>>>  #include <signal.h>
>>> -#include <string.h>  /* Needed for string function builtin redirection.  */
>>> -#include <unistd.h>
>>> +#include <internal-signals.h>
>>>  
>>> -#include <sysdep.h>
>>> -#include <sys/syscall.h>
>>>  
>>> -#include <nptl/pthreadP.h>              /* SIGCANCEL, SIGSETXID */
>>> -
>>> -
>>> -/* Get and/or change the set of blocked signals.  */
>>>  int
>>>  __sigprocmask (int how, const sigset_t *set, sigset_t *oset)
>>>  {
>>>    sigset_t local_newmask;
>>>  
>>> -  /* The only thing we have to make sure here is that SIGCANCEL and
>>> -     SIGSETXID are not blocked.  */
>>> -  if (set != NULL
>>> -      && (__builtin_expect (__sigismember (set, SIGCANCEL), 0)
>>> -	  || __builtin_expect (__sigismember (set, SIGSETXID), 0)))
>>> +  if (set != NULL && __glibc_unlikely (__has_internal_signal (set)))
>>>      {
>>>        local_newmask = *set;
>>> -      __sigdelset (&local_newmask, SIGCANCEL);
>>> -      __sigdelset (&local_newmask, SIGSETXID);
>>> +      __clear_internal_signals (&local_newmask);
>>>        set = &local_newmask;
>>>      }
>>>  
>>> -  return INLINE_SYSCALL (rt_sigprocmask, 4, how, set, oset, _NSIG / 8);
>>> +  return INLINE_SYSCALL_CALL (rt_sigprocmask, how, set, oset, _NSIG / 8);
>>>  }
>>>  libc_hidden_def (__sigprocmask)
>>>  weak_alias (__sigprocmask, sigprocmask)
>>> diff --git a/sysdeps/unix/sysv/linux/sparc/sparc64/sigprocmask.c b/sysdeps/unix/sysv/linux/sparc/sparc64/sigprocmask.c
>>> deleted file mode 100644
>>> index ef7d7fe..0000000
>>> --- a/sysdeps/unix/sysv/linux/sparc/sparc64/sigprocmask.c
>>> +++ /dev/null
>>> @@ -1,34 +0,0 @@
>>> -/* Copyright (C) 1997-2017 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
>>> -   <http://www.gnu.org/licenses/>.  */
>>> -
>>> -#include <errno.h>
>>> -#include <signal.h>
>>> -#include <unistd.h>
>>> -
>>> -#include <sysdep.h>
>>> -#include <sys/syscall.h>
>>> -
>>> -/* Get and/or change the set of blocked signals.  */
>>> -int
>>> -__sigprocmask (int how, const sigset_t *set, sigset_t *oset)
>>> -{
>>> -  /* XXX The size argument hopefully will have to be changed to the
>>> -     real size of the user-level sigset_t.  */
>>> -  return INLINE_SYSCALL (rt_sigprocmask, 4, how, set, oset, _NSIG / 8);
>>> -}
>>> -libc_hidden_def (__sigprocmask)
>>> -weak_alias (__sigprocmask, sigprocmask)
>>> diff --git a/sysdeps/unix/sysv/linux/x86_64/sigprocmask.c b/sysdeps/unix/sysv/linux/x86_64/sigprocmask.c
>>> deleted file mode 100644
>>> index 1610ddf..0000000
>>> --- a/sysdeps/unix/sysv/linux/x86_64/sigprocmask.c
>>> +++ /dev/null
>>> @@ -1,39 +0,0 @@
>>> -/* Copyright (C) 1997-2017 Free Software Foundation, Inc.
>>> -   This file is part of the GNU C Library.
>>> -   Written by Jes Sorensen, <Jes.Sorensen@cern.ch>, April 1999.
>>> -
>>> -   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
>>> -   <http://www.gnu.org/licenses/>.  */
>>> -
>>> -/* Linux/x86_64 only has rt signals, thus we do not even want to try falling
>>> -   back to the old style signals as the default Linux handler does. */
>>> -
>>> -#include <errno.h>
>>> -#include <signal.h>
>>> -#include <unistd.h>
>>> -
>>> -#include <sysdep.h>
>>> -#include <sys/syscall.h>
>>> -
>>> -/* Get and/or change the set of blocked signals.  */
>>> -int
>>> -__sigprocmask (int how, const sigset_t *set, sigset_t *oset)
>>> -{
>>> -
>>> -  /* XXX The size argument hopefully will have to be changed to the
>>> -     real size of the user-level sigset_t.  */
>>> -  return INLINE_SYSCALL (rt_sigprocmask, 4, how, set, oset, _NSIG / 8);
>>> -}
>>> -libc_hidden_def (__sigprocmask)
>>> -weak_alias (__sigprocmask, sigprocmask)
>>>