diff mbox series

[v2,3/4] linux: Use pthread_sigmask on sigprocmask

Message ID 20200313194827.4467-3-adhemerval.zanella@linaro.org
State Accepted, archived
Delegated to: Adhemerval Zanella Netto
Headers
Series [v2,1/4] nptl: Move pthread_sigmask implementation to libc |

Commit Message

Adhemerval Zanella March 13, 2020, 7:48 p.m. UTC 
  With pthread_sigmask on libc.so, it allows consolidate both
implementations.

Checked on x86_64-linux-gnu.
---
 nptl/pthreadP.h                       |  1 +
 nptl/pthread_sigmask.c                |  2 ++
 sysdeps/unix/sysv/linux/sigprocmask.c | 20 +++++---------------
 3 files changed, 8 insertions(+), 15 deletions(-)

Comments

Adhemerval Zanella April 17, 2020, 1:25 p.m. UTC | #1 
Ping.

On 13/03/2020 16:48, Adhemerval Zanella wrote:
> With pthread_sigmask on libc.so, it allows consolidate both
> implementations.
> 
> Checked on x86_64-linux-gnu.
> ---
>  nptl/pthreadP.h                       |  1 +
>  nptl/pthread_sigmask.c                |  2 ++
>  sysdeps/unix/sysv/linux/sigprocmask.c | 20 +++++---------------
>  3 files changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
> index edec8d0501..c6d8fc69be 100644
> --- a/nptl/pthreadP.h
> +++ b/nptl/pthreadP.h
> @@ -484,6 +484,7 @@ extern int __pthread_clockjoin_ex (pthread_t, void **, clockid_t,
>    attribute_hidden;
>  extern int __pthread_sigmask (int how, const sigset_t *newmask,
>  			      sigset_t *oldmask);
> +libc_hidden_proto (__pthread_sigmask)
>  
>  
>  #if IS_IN (libpthread)
> diff --git a/nptl/pthread_sigmask.c b/nptl/pthread_sigmask.c
> index 0e326d610c..c6c6e83c08 100644
> --- a/nptl/pthread_sigmask.c
> +++ b/nptl/pthread_sigmask.c
> @@ -46,6 +46,8 @@ __pthread_sigmask (int how, const sigset_t *newmask, sigset_t *oldmask)
>  	  ? INTERNAL_SYSCALL_ERRNO (result)
>  	  : 0);
>  }
> +libc_hidden_def (__pthread_sigmask)
> +
>  versioned_symbol (libc, __pthread_sigmask, pthread_sigmask, GLIBC_2_32);
>  #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_32)
>  strong_alias (__pthread_sigmask, __pthread_sigmask_2);
> diff --git a/sysdeps/unix/sysv/linux/sigprocmask.c b/sysdeps/unix/sysv/linux/sigprocmask.c
> index eb9e4d5e83..6ed0ab1e6a 100644
> --- a/sysdeps/unix/sysv/linux/sigprocmask.c
> +++ b/sysdeps/unix/sysv/linux/sigprocmask.c
> @@ -22,21 +22,11 @@
>  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);
> +  int result = __pthread_sigmask (how, set, oset);
> +  if (result == 0)
> +    return 0;
> +  __set_errno (result);
> +  return result;
>  }
>  libc_hidden_def (__sigprocmask)
>  weak_alias (__sigprocmask, sigprocmask)
>
Florian Weimer April 21, 2020, 12:01 p.m. UTC | #2 
* Adhemerval Zanella via Libc-alpha:

> With pthread_sigmask on libc.so, it allows consolidate both
> implementations.

“in sigprocmask”?

> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
> index edec8d0501..c6d8fc69be 100644
> --- a/nptl/pthreadP.h
> +++ b/nptl/pthreadP.h
> @@ -484,6 +484,7 @@ extern int __pthread_clockjoin_ex (pthread_t, void **, clockid_t,
>    attribute_hidden;
>  extern int __pthread_sigmask (int how, const sigset_t *newmask,
>  			      sigset_t *oldmask);
> +libc_hidden_proto (__pthread_sigmask)
>  
>  
>  #if IS_IN (libpthread)
> diff --git a/nptl/pthread_sigmask.c b/nptl/pthread_sigmask.c
> index 0e326d610c..c6c6e83c08 100644
> --- a/nptl/pthread_sigmask.c
> +++ b/nptl/pthread_sigmask.c
> @@ -46,6 +46,8 @@ __pthread_sigmask (int how, const sigset_t *newmask, sigset_t *oldmask)
>  	  ? INTERNAL_SYSCALL_ERRNO (result)
>  	  : 0);
>  }
> +libc_hidden_def (__pthread_sigmask)
> +
>  versioned_symbol (libc, __pthread_sigmask, pthread_sigmask, GLIBC_2_32);
>  #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_32)
>  strong_alias (__pthread_sigmask, __pthread_sigmask_2);

Ah, perhaps put this into the first commit?

> diff --git a/sysdeps/unix/sysv/linux/sigprocmask.c b/sysdeps/unix/sysv/linux/sigprocmask.c
> index eb9e4d5e83..6ed0ab1e6a 100644
> --- a/sysdeps/unix/sysv/linux/sigprocmask.c
> +++ b/sysdeps/unix/sysv/linux/sigprocmask.c

> +  int result = __pthread_sigmask (how, set, oset);
> +  if (result == 0)
> +    return 0;
> +  __set_errno (result);
> +  return result;

The final statement needs to return -1.  It's curious that this does
not result in a testsuite failure.
Adhemerval Zanella April 21, 2020, 12:30 p.m. UTC | #3 
On 21/04/2020 09:01, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> With pthread_sigmask on libc.so, it allows consolidate both
>> implementations.
> 
> “in sigprocmask”?

Ack.

> 
>> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
>> index edec8d0501..c6d8fc69be 100644
>> --- a/nptl/pthreadP.h
>> +++ b/nptl/pthreadP.h
>> @@ -484,6 +484,7 @@ extern int __pthread_clockjoin_ex (pthread_t, void **, clockid_t,
>>    attribute_hidden;
>>  extern int __pthread_sigmask (int how, const sigset_t *newmask,
>>  			      sigset_t *oldmask);
>> +libc_hidden_proto (__pthread_sigmask)
>>  
>>  
>>  #if IS_IN (libpthread)
>> diff --git a/nptl/pthread_sigmask.c b/nptl/pthread_sigmask.c
>> index 0e326d610c..c6c6e83c08 100644
>> --- a/nptl/pthread_sigmask.c
>> +++ b/nptl/pthread_sigmask.c
>> @@ -46,6 +46,8 @@ __pthread_sigmask (int how, const sigset_t *newmask, sigset_t *oldmask)
>>  	  ? INTERNAL_SYSCALL_ERRNO (result)
>>  	  : 0);
>>  }
>> +libc_hidden_def (__pthread_sigmask)
>> +
>>  versioned_symbol (libc, __pthread_sigmask, pthread_sigmask, GLIBC_2_32);
>>  #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_32)
>>  strong_alias (__pthread_sigmask, __pthread_sigmask_2);
> 
> Ah, perhaps put this into the first commit?

Alright (although it just does require on this set).

> 
>> diff --git a/sysdeps/unix/sysv/linux/sigprocmask.c b/sysdeps/unix/sysv/linux/sigprocmask.c
>> index eb9e4d5e83..6ed0ab1e6a 100644
>> --- a/sysdeps/unix/sysv/linux/sigprocmask.c
>> +++ b/sysdeps/unix/sysv/linux/sigprocmask.c
> 
>> +  int result = __pthread_sigmask (how, set, oset);
>> +  if (result == 0)
>> +    return 0;
>> +  __set_errno (result);
>> +  return result;
> 
> The final statement needs to return -1.  It's curious that this does
> not result in a testsuite failure.
> 

Ack.  I think because sigprocmask only fails for an invalid 'how' 
(EINVAL) or invalid address (EFAULT) for 'set' or 'oldset', which
are not issued on glibc testsuite.
Florian Weimer April 21, 2020, 12:35 p.m. UTC | #4 
* Adhemerval Zanella:

>>> diff --git a/sysdeps/unix/sysv/linux/sigprocmask.c b/sysdeps/unix/sysv/linux/sigprocmask.c
>>> index eb9e4d5e83..6ed0ab1e6a 100644
>>> --- a/sysdeps/unix/sysv/linux/sigprocmask.c
>>> +++ b/sysdeps/unix/sysv/linux/sigprocmask.c
>> 
>>> +  int result = __pthread_sigmask (how, set, oset);
>>> +  if (result == 0)
>>> +    return 0;
>>> +  __set_errno (result);
>>> +  return result;
>> 
>> The final statement needs to return -1.  It's curious that this does
>> not result in a testsuite failure.
>> 
>
> Ack.  I think because sigprocmask only fails for an invalid 'how' 
> (EINVAL) or invalid address (EFAULT) for 'set' or 'oldset', which
> are not issued on glibc testsuite.

Maybe you can add another test to
sysdeps/unix/sysv/linux/test-errno-linux.c, just in case?
Adhemerval Zanella April 21, 2020, 1:01 p.m. UTC | #5 
On 21/04/2020 09:35, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>>>> diff --git a/sysdeps/unix/sysv/linux/sigprocmask.c b/sysdeps/unix/sysv/linux/sigprocmask.c
>>>> index eb9e4d5e83..6ed0ab1e6a 100644
>>>> --- a/sysdeps/unix/sysv/linux/sigprocmask.c
>>>> +++ b/sysdeps/unix/sysv/linux/sigprocmask.c
>>>
>>>> +  int result = __pthread_sigmask (how, set, oset);
>>>> +  if (result == 0)
>>>> +    return 0;
>>>> +  __set_errno (result);
>>>> +  return result;
>>>
>>> The final statement needs to return -1.  It's curious that this does
>>> not result in a testsuite failure.
>>>
>>
>> Ack.  I think because sigprocmask only fails for an invalid 'how' 
>> (EINVAL) or invalid address (EFAULT) for 'set' or 'oldset', which
>> are not issued on glibc testsuite.
> 
> Maybe you can add another test to
> sysdeps/unix/sysv/linux/test-errno-linux.c, just in case?
> 

Ack. I will do it.
diff mbox series

Patch

diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
index edec8d0501..c6d8fc69be 100644
--- a/nptl/pthreadP.h
+++ b/nptl/pthreadP.h
@@ -484,6 +484,7 @@  extern int __pthread_clockjoin_ex (pthread_t, void **, clockid_t,
   attribute_hidden;
 extern int __pthread_sigmask (int how, const sigset_t *newmask,
 			      sigset_t *oldmask);
+libc_hidden_proto (__pthread_sigmask)
 
 
 #if IS_IN (libpthread)
diff --git a/nptl/pthread_sigmask.c b/nptl/pthread_sigmask.c
index 0e326d610c..c6c6e83c08 100644
--- a/nptl/pthread_sigmask.c
+++ b/nptl/pthread_sigmask.c
@@ -46,6 +46,8 @@  __pthread_sigmask (int how, const sigset_t *newmask, sigset_t *oldmask)
 	  ? INTERNAL_SYSCALL_ERRNO (result)
 	  : 0);
 }
+libc_hidden_def (__pthread_sigmask)
+
 versioned_symbol (libc, __pthread_sigmask, pthread_sigmask, GLIBC_2_32);
 #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_32)
 strong_alias (__pthread_sigmask, __pthread_sigmask_2);
diff --git a/sysdeps/unix/sysv/linux/sigprocmask.c b/sysdeps/unix/sysv/linux/sigprocmask.c
index eb9e4d5e83..6ed0ab1e6a 100644
--- a/sysdeps/unix/sysv/linux/sigprocmask.c
+++ b/sysdeps/unix/sysv/linux/sigprocmask.c
@@ -22,21 +22,11 @@ 
 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);
+  int result = __pthread_sigmask (how, set, oset);
+  if (result == 0)
+    return 0;
+  __set_errno (result);
+  return result;
 }
 libc_hidden_def (__sigprocmask)
 weak_alias (__sigprocmask, sigprocmask)