| Message ID | 20200313194827.4467-3-adhemerval.zanella@linaro.org |
|---|---|
| State | Accepted, archived |
| Delegated to: | Adhemerval Zanella Netto |
| Headers | show |
| Series | [v2,1/4] nptl: Move pthread_sigmask implementation to libc | expand |
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) >
* 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.
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.
* 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?
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 --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)