[PATCH] Fix __libc_signal_block_all on sparc64

[PATCH] Fix __libc_signal_block_all on sparc64

[Adhemerval Zanella]

The a2e8aa0d9e shows two regressions on sparc64-linux-gnu:

  nptl/tst-cancel-self-canceltype
  nptl/tst-cancel5

This is not from the patch itself, but rather from an invalid
__NR_rt_sigprocmask issued by the loader:

  rt_sigprocmask(SIG_UNBLOCK, [RTMIN RT_1], NULL, 8) = 0
  rt_sigprocmask(0xffd07c60 /* SIG_??? */, ~[], 0x7feffd07d08, 8) = -1 EINVAL (Invalid argument)

Tracking the culprit it really seems a wrong code generation in the
INTERNAL_SYSCALL due the automatic sigset_t used on
__libc_signal_block_all:

  return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &SIGALL_SET,
                          set, _NSIG / 8);

Where SIGALL_SET is defined as:

  ((__sigset_t) { .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 } })

Building the expanded __libc_signal_block_all on sparc64 with recent
compiler (gcc 8.3.1 and 9.1.1):

  #include <signal>

  int
  _libc_signal_block_all (sigset_t *set)
  {
    INTERNAL_SYSCALL_DECL (err);
    return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &SIGALL_SET,
			     set, _NSIG / 8);
  }

It seems that the first argument (SIG_BLOCK) is not correctly set on
'o0' register:

  __libc_signal_block_all:
	save    %sp, -304, %sp
	add     %fp, 1919, %o0
	mov     128, %o2
	sethi   %hi(.LC0), %o1
	call    memcpy, 0
	 or     %o1, %lo(.LC0), %o1
	add     %fp, 1919, %o1
	mov     %i0, %o2
	mov     8, %o3
	mov     103, %g1
	ta      0x6d;
	bcc,pt  %xcc, 1f
	mov     0, %g1
	sub     %g0, %o0, %o0
	mov     1, %g1
     1:	sra     %o0, 0, %i0
	return  %i7+8
	 nop

Where is I define SIGALL_SET outside INTERNAL_SYSCALL macro, gcc
correctly sets the expected kernel argument in correct register:

        sethi   %hi(.LC0), %o1
        call    memcpy, 0
         or     %o1, %lo(.LC0), %o1
   ->   mov     1, %o0
	add     %fp, 1919, %o1

This patch also changes the return value of __libc_signal_block_all,
__libc_signal_block_app, and __libc_signal_restore_set to 'void'
since the function should not fail if input argument is NULL.  Also,
for Linux the return value is not fully correct on some platforms due
the missing usage of INTERNAL_SYSCALL_ERROR_P / INTERNAL_SYSCALL_ERRNO
macros.

Checked on sparc64-linux-gnu.
---
 sysdeps/generic/internal-signals.h         | 12 ++++++------
 sysdeps/unix/sysv/linux/internal-signals.h | 19 ++++++++++---------
 2 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/sysdeps/generic/internal-signals.h b/sysdeps/generic/internal-signals.h
index a515e3e649..41c24dc4b3 100644
--- a/sysdeps/generic/internal-signals.h
+++ b/sysdeps/generic/internal-signals.h
@@ -34,28 +34,28 @@ __clear_internal_signals (sigset_t *set)
 {
 }
 
-static inline int
+static inline void
 __libc_signal_block_all (sigset_t *set)
 {
   sigset_t allset;
   __sigfillset (&allset);
-  return __sigprocmask (SIG_BLOCK, &allset, set);
+  __sigprocmask (SIG_BLOCK, &allset, set);
 }
 
-static inline int
+static inline void
 __libc_signal_block_app (sigset_t *set)
 {
   sigset_t allset;
   __sigfillset (&allset);
   __clear_internal_signals (&allset);
-  return __sigprocmask (SIG_BLOCK, &allset, set);
+  __sigprocmask (SIG_BLOCK, &allset, set);
 }
 
 /* Restore current process signal mask.  */
-static inline int
+static inline void
 __libc_signal_restore_set (const sigset_t *set)
 {
-  return __sigprocmask (SIG_SETMASK, set, NULL);
+  __sigprocmask (SIG_SETMASK, set, NULL);
 }
 
 
diff --git a/sysdeps/unix/sysv/linux/internal-signals.h b/sysdeps/unix/sysv/linux/internal-signals.h
index 01d8bf0a4c..0a5faf175f 100644
--- a/sysdeps/unix/sysv/linux/internal-signals.h
+++ b/sysdeps/unix/sysv/linux/internal-signals.h
@@ -57,32 +57,33 @@ __clear_internal_signals (sigset_t *set)
   ((__sigset_t) { .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 } })
 
 /* Block all signals, including internal glibc ones.  */
-static inline int
+static inline void
 __libc_signal_block_all (sigset_t *set)
 {
+  sigset_t allset = SIGALL_SET;
   INTERNAL_SYSCALL_DECL (err);
-  return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &SIGALL_SET,
-			   set, _NSIG / 8);
+  INTERNAL_SYSCALL_CALL (rt_sigprocmask, err, SIG_BLOCK, &allset, set,
+			 _NSIG / 8);
 }
 
 /* Block all application signals (excluding internal glibc ones).  */
-static inline int
+static inline void
 __libc_signal_block_app (sigset_t *set)
 {
   sigset_t allset = SIGALL_SET;
   __clear_internal_signals (&allset);
   INTERNAL_SYSCALL_DECL (err);
-  return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &allset, set,
-			   _NSIG / 8);
+  INTERNAL_SYSCALL_CALL (rt_sigprocmask, err, SIG_BLOCK, &allset, set,
+			 _NSIG / 8);
 }
 
 /* Restore current process signal mask.  */
-static inline int
+static inline void
 __libc_signal_restore_set (const sigset_t *set)
 {
   INTERNAL_SYSCALL_DECL (err);
-  return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_SETMASK, set, NULL,
-			   _NSIG / 8);
+  INTERNAL_SYSCALL_CALL (rt_sigprocmask, err, SIG_SETMASK, set, NULL,
+			 _NSIG / 8);
 }
 
 /* Used to communicate with signal handler.  */
-- 
2.17.1

Re: [PATCH] Fix __libc_signal_block_all on sparc64

[Florian Weimer]

* Adhemerval Zanella:

> Where SIGALL_SET is defined as:
>
>   ((__sigset_t) { .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 } })

Shouldn't this refer to a global constant data object?  Then we wouldn't
have to emit many local copies of the same object and then copy it onto
the stack.

(GCC cannot know that the system call will not modify the object.)

Thanks,
Florian

Re: [PATCH] Fix __libc_signal_block_all on sparc64

[Adhemerval Zanella]

On 05/12/2019 11:45, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> Where SIGALL_SET is defined as:
>>
>>   ((__sigset_t) { .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 } })
> 
> Shouldn't this refer to a global constant data object?  Then we wouldn't
> have to emit many local copies of the same object and then copy it onto
> the stack.
> 
> (GCC cannot know that the system call will not modify the object.)

Do we really need to add a sigset_t object on ld (it won't require it
any longer since you sent a reverted patch to the signal block on dlopen),
libc, and libpthread?

In fact I think we can simplify it a bit an just get rid of these
block/unblock signals and just use sigprocmask directly. I haven't done
on posix_spawn because the sigprocmask semantic cleared the internal
signals prior issuing rt_sigprocmask. 

However sigfillset already does it, and this is the canonical way to 
operate on sigset_t. The only way to actually broke this assumption
is if caller initialize sigset with memset or something similar, i.e,
bypassing glibc (and again this is not a valid construction imho).

So I what I am thinking is to consolidate the sigprocmask, remove the
SIGCANCEL/SIGSETXID handling (which is not done on all architectures
btw), and replace __libc_signal_block_all, __libc_signal_block_app,
and __libc_signal_restore_set with straight sigprocmask calls.

Re: [PATCH] Fix __libc_signal_block_all on sparc64

[Florian Weimer]

* Adhemerval Zanella:

> On 05/12/2019 11:45, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>> Where SIGALL_SET is defined as:
>>>
>>>   ((__sigset_t) { .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 } })
>> 
>> Shouldn't this refer to a global constant data object?  Then we wouldn't
>> have to emit many local copies of the same object and then copy it onto
>> the stack.
>> 
>> (GCC cannot know that the system call will not modify the object.)
>
> Do we really need to add a sigset_t object on ld (it won't require it
> any longer since you sent a reverted patch to the signal block on dlopen),
> libc, and libpthread?

It's already there, see the .LC0 label.  GCC should be using memset
instead of memcpy for the initialization, but it currently does not.

> In fact I think we can simplify it a bit an just get rid of these
> block/unblock signals and just use sigprocmask directly. I haven't done
> on posix_spawn because the sigprocmask semantic cleared the internal
> signals prior issuing rt_sigprocmask. 
>
> However sigfillset already does it, and this is the canonical way to 
> operate on sigset_t. The only way to actually broke this assumption
> is if caller initialize sigset with memset or something similar, i.e,
> bypassing glibc (and again this is not a valid construction imho).

Another alternative would be to remove the restriction on blocking
internal signals altogether.  I don't think this would be very
problematic for SIGCANCEL.  For SIGSETXID, I think the code would just
block in the setxid caller due to the missing futex wakeup, and not
continue to run with partially unchanged credentials in case of a
blocked SIGSETXID signal on some thread.

Thanks,
Florian

Re: [PATCH] Fix __libc_signal_block_all on sparc64

[Adhemerval Zanella]

On 05/12/2019 14:05, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 05/12/2019 11:45, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>> Where SIGALL_SET is defined as:
>>>>
>>>>   ((__sigset_t) { .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 } })
>>>
>>> Shouldn't this refer to a global constant data object?  Then we wouldn't
>>> have to emit many local copies of the same object and then copy it onto
>>> the stack.
>>>
>>> (GCC cannot know that the system call will not modify the object.)
>>
>> Do we really need to add a sigset_t object on ld (it won't require it
>> any longer since you sent a reverted patch to the signal block on dlopen),
>> libc, and libpthread?
> 
> It's already there, see the .LC0 label.  GCC should be using memset
> instead of memcpy for the initialization, but it currently does not.

I think we can use something like:

static const sigset_t sigall_set =
  { .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 } };

It would give compiler/linker more information to remove duplicate
definitions.

> 
>> In fact I think we can simplify it a bit an just get rid of these
>> block/unblock signals and just use sigprocmask directly. I haven't done
>> on posix_spawn because the sigprocmask semantic cleared the internal
>> signals prior issuing rt_sigprocmask. 
>>
>> However sigfillset already does it, and this is the canonical way to 
>> operate on sigset_t. The only way to actually broke this assumption
>> is if caller initialize sigset with memset or something similar, i.e,
>> bypassing glibc (and again this is not a valid construction imho).
> 
> Another alternative would be to remove the restriction on blocking
> internal signals altogether.  I don't think this would be very
> problematic for SIGCANCEL.  For SIGSETXID, I think the code would just
> block in the setxid caller due to the missing futex wakeup, and not
> continue to run with partially unchanged credentials in case of a
> blocked SIGSETXID signal on some thread.

I think for sigfillset there is no much gain in allowing all signals,
blocking the internal ones adds some consistency at least.

[PATCH v2 2/3] linux: Remove SIGCANCEL/SIGSETXID handling on sigprocmask

[Adhemerval Zanella]

The sigfillset already does it, and this is the canonical way to operate
on sigset_t.  The only way to actually broke this assumption is if caller
initialize sigset with memset or something similar, i.e, bypassing glibc
(and again this is not a valid construction).

Checked on x86_64-linux-gnu.
---
 sysdeps/unix/sysv/linux/sigprocmask.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/sigprocmask.c b/sysdeps/unix/sysv/linux/sigprocmask.c
index 73b0d0c19a..c6961a8ac4 100644
--- a/sysdeps/unix/sysv/linux/sigprocmask.c
+++ b/sysdeps/unix/sysv/linux/sigprocmask.c
@@ -16,26 +16,12 @@
    <https://www.gnu.org/licenses/>.  */
 
 #include <signal.h>
-#include <nptl/pthreadP.h>              /* SIGCANCEL, SIGSETXID */
+#include <sysdep.h>
 
 /* 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
-      && __glibc_unlikely (__sigismember (set, SIGCANCEL)
-	|| __glibc_unlikely (__sigismember (set, SIGSETXID))))
-    {
-      local_newmask = *set;
-      __sigdelset (&local_newmask, SIGCANCEL);
-      __sigdelset (&local_newmask, SIGSETXID);
-      set = &local_newmask;
-    }
-
   return INLINE_SYSCALL_CALL (rt_sigprocmask, how, set, oset, _NSIG / 8);
 }
 libc_hidden_def (__sigprocmask)
-- 
2.17.1

[PATCH v2 1/3] linux: Consolidate sigprocmask

[Adhemerval Zanella]

All architectures now uses the Linux generic implementation which
uses __NR_rt_sigprocmask.

Checked on x86_64-linux-gnu, sparc64-linux-gnu, ia64-linux-gnu,
s390x-linux-gnu, and alpha-linux-gnu.
---
 sysdeps/unix/sysv/linux/alpha/sigprocmask.c   | 58 -------------------
 sysdeps/unix/sysv/linux/ia64/sigprocmask.c    | 40 -------------
 .../sysv/linux/s390/s390-64/sigprocmask.c     | 38 ------------
 sysdeps/unix/sysv/linux/sigprocmask.c         | 14 +----
 .../sysv/linux/sparc/sparc64/sigprocmask.c    | 34 -----------
 sysdeps/unix/sysv/linux/x86_64/sigprocmask.c  | 39 -------------
 6 files changed, 3 insertions(+), 220 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/sysdeps/unix/sysv/linux/alpha/sigprocmask.c b/sysdeps/unix/sysv/linux/alpha/sigprocmask.c
deleted file mode 100644
index 0e807179bf..0000000000
--- a/sysdeps/unix/sysv/linux/alpha/sigprocmask.c
+++ /dev/null
@@ -1,58 +0,0 @@
-/* Copyright (C) 1993-2019 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
-   <https://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 81c2d3cd8e..0000000000
--- a/sysdeps/unix/sysv/linux/ia64/sigprocmask.c
+++ /dev/null
@@ -1,40 +0,0 @@
-/* Copyright (C) 1997-2019 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
-   <https://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/s390/s390-64/sigprocmask.c b/sysdeps/unix/sysv/linux/s390/s390-64/sigprocmask.c
deleted file mode 100644
index f0eb099748..0000000000
--- a/sysdeps/unix/sysv/linux/s390/s390-64/sigprocmask.c
+++ /dev/null
@@ -1,38 +0,0 @@
-/* Copyright (C) 2001-2019 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/>.  */
-
-/* 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 01521c8107..73b0d0c19a 100644
--- a/sysdeps/unix/sysv/linux/sigprocmask.c
+++ b/sysdeps/unix/sysv/linux/sigprocmask.c
@@ -15,17 +15,9 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
-#include <errno.h>
 #include <signal.h>
-#include <string.h>  /* Needed for string function builtin redirection.  */
-#include <unistd.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)
@@ -35,8 +27,8 @@ __sigprocmask (int how, const sigset_t *set, sigset_t *oset)
   /* 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)))
+      && __glibc_unlikely (__sigismember (set, SIGCANCEL)
+	|| __glibc_unlikely (__sigismember (set, SIGSETXID))))
     {
       local_newmask = *set;
       __sigdelset (&local_newmask, SIGCANCEL);
@@ -44,7 +36,7 @@ __sigprocmask (int how, const sigset_t *set, sigset_t *oset)
       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 5823826ab2..0000000000
--- a/sysdeps/unix/sysv/linux/sparc/sparc64/sigprocmask.c
+++ /dev/null
@@ -1,34 +0,0 @@
-/* Copyright (C) 1997-2019 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/>.  */
-
-#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 c2e721d7b9..0000000000
--- a/sysdeps/unix/sysv/linux/x86_64/sigprocmask.c
+++ /dev/null
@@ -1,39 +0,0 @@
-/* Copyright (C) 1997-2019 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
-   <https://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)
-- 
2.17.1

[PATCH v2 3/3] Fix __libc_signal_block_all on sparc64

[Adhemerval Zanella]

The a2e8aa0d9e shows two regressions on sparc64-linux-gnu:

  nptl/tst-cancel-self-canceltype
  nptl/tst-cancel5

This is not from the patch itself, but rather from an invalid
__NR_rt_sigprocmask issued by the loader:

  rt_sigprocmask(SIG_UNBLOCK, [RTMIN RT_1], NULL, 8) = 0
  rt_sigprocmask(0xffd07c60 /* SIG_??? */, ~[], 0x7feffd07d08, 8) = -1 EINVAL (Invalid argument)

Tracking the culprit it really seems a wrong code generation in the
INTERNAL_SYSCALL due the automatic sigset_t used on
__libc_signal_block_all:

  return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &SIGALL_SET,
                          set, _NSIG / 8);

Where SIGALL_SET is defined as:

  ((__sigset_t) { .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 } })

Building the expanded __libc_signal_block_all on sparc64 with recent
compiler (gcc 8.3.1 and 9.1.1):

  #include <signal>

  int
  _libc_signal_block_all (sigset_t *set)
  {
    INTERNAL_SYSCALL_DECL (err);
    return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &SIGALL_SET,
			     set, _NSIG / 8);
  }

It seems that the first argument (SIG_BLOCK) is not correctly set on
'o0' register:

  __libc_signal_block_all:
	save    %sp, -304, %sp
	add     %fp, 1919, %o0
	mov     128, %o2
	sethi   %hi(.LC0), %o1
	call    memcpy, 0
	 or     %o1, %lo(.LC0), %o1
	add     %fp, 1919, %o1
	mov     %i0, %o2
	mov     8, %o3
	mov     103, %g1
	ta      0x6d;
	bcc,pt  %xcc, 1f
	mov     0, %g1
	sub     %g0, %o0, %o0
	mov     1, %g1
     1:	sra     %o0, 0, %i0
	return  %i7+8
	 nop

Where is I define SIGALL_SET outside INTERNAL_SYSCALL macro, gcc
correctly sets the expected kernel argument in correct register:

        sethi   %hi(.LC0), %o1
        call    memcpy, 0
         or     %o1, %lo(.LC0), %o1
   ->   mov     1, %o0
	add     %fp, 1919, %o1

This patch fixes it by moving both sigset_t that represent all signals
sets and the application set to constant data objects.  This patch also
changes the return value of __libc_signal_block_all,
__libc_signal_block_app, and __libc_signal_restore_set to 'void'
since the function should not fail if input argument is NULL.  Also,
for Linux the return value is not fully correct on some platforms due
the missing usage of INTERNAL_SYSCALL_ERROR_P / INTERNAL_SYSCALL_ERRNO
macros.

Checked on x86_64-linux-gnu, i686-linux-gnu, and sparc64-linux-gnu.
---
 sysdeps/unix/sysv/linux/internal-signals.h | 37 ++++++++++++++--------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/internal-signals.h b/sysdeps/unix/sysv/linux/internal-signals.h
index 01d8bf0a4c..ae2d626ef0 100644
--- a/sysdeps/unix/sysv/linux/internal-signals.h
+++ b/sysdeps/unix/sysv/linux/internal-signals.h
@@ -53,36 +53,47 @@ __clear_internal_signals (sigset_t *set)
   __sigdelset (set, SIGSETXID);
 }
 
-#define SIGALL_SET \
-  ((__sigset_t) { .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 } })
+static const sigset_t sigall_set = {
+   .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 }
+};
+
+static const sigset_t sigapp_set = {
+#if ULONG_MAX == 0xffffffff
+  .__val = { [0]                      = ~0UL & ~(__sigmask (SIGCANCEL)),
+             [1]                      = ~0UL & ~(__sigmask (SIGSETXID)),
+             [2 ... _SIGSET_NWORDS-1] = ~0UL }
+#else
+  .__val = { [0]                      = ~0UL & ~(__sigmask (SIGCANCEL)
+                                                 | __sigmask (SIGSETXID)),
+             [1 ... _SIGSET_NWORDS-1] = ~0UL }
+#endif
+};
 
 /* Block all signals, including internal glibc ones.  */
-static inline int
+static inline void
 __libc_signal_block_all (sigset_t *set)
 {
   INTERNAL_SYSCALL_DECL (err);
-  return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &SIGALL_SET,
-			   set, _NSIG / 8);
+  INTERNAL_SYSCALL_CALL (rt_sigprocmask, err, SIG_BLOCK, &sigall_set, set,
+			 _NSIG / 8);
 }
 
 /* Block all application signals (excluding internal glibc ones).  */
-static inline int
+static inline void
 __libc_signal_block_app (sigset_t *set)
 {
-  sigset_t allset = SIGALL_SET;
-  __clear_internal_signals (&allset);
   INTERNAL_SYSCALL_DECL (err);
-  return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &allset, set,
-			   _NSIG / 8);
+  INTERNAL_SYSCALL_CALL (rt_sigprocmask, err, SIG_BLOCK, &sigapp_set, set,
+			 _NSIG / 8);
 }
 
 /* Restore current process signal mask.  */
-static inline int
+static inline void
 __libc_signal_restore_set (const sigset_t *set)
 {
   INTERNAL_SYSCALL_DECL (err);
-  return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_SETMASK, set, NULL,
-			   _NSIG / 8);
+  INTERNAL_SYSCALL_CALL (rt_sigprocmask, err, SIG_SETMASK, set, NULL,
+			 _NSIG / 8);
 }
 
 /* Used to communicate with signal handler.  */
-- 
2.17.1

Re: [PATCH v2 3/3] Fix __libc_signal_block_all on sparc64

[Adhemerval Zanella]

On 05/12/2019 15:34, Adhemerval Zanella wrote:
> The a2e8aa0d9e shows two regressions on sparc64-linux-gnu:
> 
>   nptl/tst-cancel-self-canceltype
>   nptl/tst-cancel5

It also fixes support/tst-support_capture_subprocess failures I have been 
on sparcv9.

Re: [PATCH] Fix __libc_signal_block_all on sparc64

[Andreas Schwab]

On Dez 05 2019, Adhemerval Zanella wrote:

> Where SIGALL_SET is defined as:
>
>   ((__sigset_t) { .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 } })

Why is that not const?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Fix __libc_signal_block_all on sparc64

[Adhemerval Zanella]

On 09/12/2019 08:00, Andreas Schwab wrote:
> On Dez 05 2019, Adhemerval Zanella wrote:
> 
>> Where SIGALL_SET is defined as:
>>
>>   ((__sigset_t) { .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 } })
> 
> Why is that not const?
> 
> Andreas.
> 

It is on the updated version [1].

[1] https://sourceware.org/ml/libc-alpha/2019-12/msg00190.html

Re: [PATCH] Fix __libc_signal_block_all on sparc64

[Andreas Schwab]

On Dez 09 2019, Adhemerval Zanella wrote:

> On 09/12/2019 08:00, Andreas Schwab wrote:
>> On Dez 05 2019, Adhemerval Zanella wrote:
>> 
>>> Where SIGALL_SET is defined as:
>>>
>>>   ((__sigset_t) { .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 } })
>> 
>> Why is that not const?
>> 
>> Andreas.
>> 
>
> It is on the updated version [1].

Does it help to make it const?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Fix __libc_signal_block_all on sparc64

[Adhemerval Zanella]

On 09/12/2019 08:59, Andreas Schwab wrote:
> On Dez 09 2019, Adhemerval Zanella wrote:
> 
>> On 09/12/2019 08:00, Andreas Schwab wrote:
>>> On Dez 05 2019, Adhemerval Zanella wrote:
>>>
>>>> Where SIGALL_SET is defined as:
>>>>
>>>>   ((__sigset_t) { .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 } })
>>>
>>> Why is that not const?
>>>
>>> Andreas.
>>>
>>
>> It is on the updated version [1].
> 
> Does it help to make it const?
> 

It does, that's the main reason for the updated version (besides
the sigprocmask changes).

Re: [PATCH] Fix __libc_signal_block_all on sparc64

[Andreas Schwab]

On Dez 09 2019, Adhemerval Zanella wrote:

> On 09/12/2019 08:59, Andreas Schwab wrote:
>> On Dez 09 2019, Adhemerval Zanella wrote:
>> 
>>> On 09/12/2019 08:00, Andreas Schwab wrote:
>>>> On Dez 05 2019, Adhemerval Zanella wrote:
>>>>
>>>>> Where SIGALL_SET is defined as:
>>>>>
>>>>>   ((__sigset_t) { .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 } })
>>>>
>>>> Why is that not const?
>>>>
>>>> Andreas.
>>>>
>>>
>>> It is on the updated version [1].
>> 
>> Does it help to make it const?
>> 
>
> It does, that's the main reason for the updated version

No, I mean without the other changes.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Fix __libc_signal_block_all on sparc64

[Adhemerval Zanella]

On 09/12/2019 09:53, Andreas Schwab wrote:
> On Dez 09 2019, Adhemerval Zanella wrote:
> 
>> On 09/12/2019 08:59, Andreas Schwab wrote:
>>> On Dez 09 2019, Adhemerval Zanella wrote:
>>>
>>>> On 09/12/2019 08:00, Andreas Schwab wrote:
>>>>> On Dez 05 2019, Adhemerval Zanella wrote:
>>>>>
>>>>>> Where SIGALL_SET is defined as:
>>>>>>
>>>>>>   ((__sigset_t) { .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 } })
>>>>>
>>>>> Why is that not const?
>>>>>
>>>>> Andreas.
>>>>>
>>>>
>>>> It is on the updated version [1].
>>>
>>> Does it help to make it const?
>>>
>>
>> It does, that's the main reason for the updated version
> 
> No, I mean without the other changes.
> 

The const fixes the issues, the other changes on [1] are essentially 
changing __libc_signal_ to return void.

[1] https://sourceware.org/ml/libc-alpha/2019-12/msg00190.html

Re: [PATCH] Fix __libc_signal_block_all on sparc64

[Andreas Schwab]

On Dez 09 2019, Adhemerval Zanella wrote:

> The const fixes the issues, the other changes on [1] are essentially 
> changing __libc_signal_ to return void.

So the other changes are not necessary?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Fix __libc_signal_block_all on sparc64

[Adhemerval Zanella]

On 09/12/2019 10:23, Andreas Schwab wrote:
> On Dez 09 2019, Adhemerval Zanella wrote:
> 
>> The const fixes the issues, the other changes on [1] are essentially 
>> changing __libc_signal_ to return void.
> 
> So the other changes are not necessary?

Not strictly, the 'Fix __libc_signal_block_all on sparc64' can be applied
independently. 

Re: [PATCH] Fix __libc_signal_block_all on sparc64

[Andreas Schwab]

On Dez 09 2019, Adhemerval Zanella wrote:

> On 09/12/2019 10:23, Andreas Schwab wrote:
>> On Dez 09 2019, Adhemerval Zanella wrote:
>> 
>>> The const fixes the issues, the other changes on [1] are essentially 
>>> changing __libc_signal_ to return void.
>> 
>> So the other changes are not necessary?
>
> Not strictly, the 'Fix __libc_signal_block_all on sparc64' can be applied
> independently. 

But that patch contains the other changes.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Fix __libc_signal_block_all on sparc64

[Adhemerval Zanella]

On 09/12/2019 11:54, Andreas Schwab wrote:
> On Dez 09 2019, Adhemerval Zanella wrote:
> 
>> On 09/12/2019 10:23, Andreas Schwab wrote:
>>> On Dez 09 2019, Adhemerval Zanella wrote:
>>>
>>>> The const fixes the issues, the other changes on [1] are essentially 
>>>> changing __libc_signal_ to return void.
>>>
>>> So the other changes are not necessary?
>>
>> Not strictly, the 'Fix __libc_signal_block_all on sparc64' can be applied
>> independently. 
> 
> But that patch contains the other changes.

Ok, I will send a v3 with the fix only for the referenced issue.