[v3,2/6] nptl: Implement raise in terms of pthread_kill
Commit Message
Changes from v2:
* Rebase against master.
* Do not added the versioned symbol for rtld.
Changes from v1:
* Handle vfork usage for raise and obtain the TID using a syscall
instead of reading from TCB.
---
Now that pthread_kill is provided by libc.so and async-signal-safe,
it is possible to implement the generic POSIX implementation as
pthread_kill(pthread_self(), sig).
For Linux implementation, pthread_kill read the targetting TID from
the TCB. For raise, this it not possible because it would make raise
fail when issue after vfork (where creates the resulting process
has a different TID from the parent, but its TCB is not updated as
for pthread_create). For this case, gettid is called directly.
Checked on x86_64-linux-gnu.
---
include/pthread.h | 5 ++++
nptl/pthreadP.h | 4 +--
nptl/pthread_kill.c | 42 +++++++++++++++-----------
nptl/pthread_self.c | 4 ++-
sysdeps/htl/pthreadP.h | 2 --
sysdeps/posix/raise.c | 11 +++++--
sysdeps/unix/sysv/linux/raise.c | 52 ---------------------------------
7 files changed, 44 insertions(+), 76 deletions(-)
delete mode 100644 sysdeps/unix/sysv/linux/raise.c
Comments
* Adhemerval Zanella via Libc-alpha:
> Now that pthread_kill is provided by libc.so and async-signal-safe,
> it is possible to implement the generic POSIX implementation as
> pthread_kill(pthread_self(), sig).
This part no longer matches the patch, I think.
> For Linux implementation, pthread_kill read the targetting TID from
> the TCB. For raise, this it not possible because it would make raise
> fail when issue after vfork (where creates the resulting process
> has a different TID from the parent, but its TCB is not updated as
> for pthread_create). For this case, gettid is called directly.
>
> Checked on x86_64-linux-gnu.
> ---
> include/pthread.h | 5 ++++
> nptl/pthreadP.h | 4 +--
> nptl/pthread_kill.c | 42 +++++++++++++++-----------
> nptl/pthread_self.c | 4 ++-
> sysdeps/htl/pthreadP.h | 2 --
> sysdeps/posix/raise.c | 11 +++++--
> sysdeps/unix/sysv/linux/raise.c | 52 ---------------------------------
The new raise doesn't use pthread_kill but direct system calls, so the
libpthread changes are unnecessary, and the implementation should remain
as sysdeps/unix/sysv/linux/raise.c.
Thanks,
Florian
On 11/05/2021 11:29, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
>
>> Now that pthread_kill is provided by libc.so and async-signal-safe,
>> it is possible to implement the generic POSIX implementation as
>> pthread_kill(pthread_self(), sig).
>
> This part no longer matches the patch, I think.
We should be since b76658451c81, no?
>
>> For Linux implementation, pthread_kill read the targetting TID from
>> the TCB. For raise, this it not possible because it would make raise
>> fail when issue after vfork (where creates the resulting process
>> has a different TID from the parent, but its TCB is not updated as
>> for pthread_create). For this case, gettid is called directly.
>>
>> Checked on x86_64-linux-gnu.
>> ---
>> include/pthread.h | 5 ++++
>> nptl/pthreadP.h | 4 +--
>> nptl/pthread_kill.c | 42 +++++++++++++++-----------
>> nptl/pthread_self.c | 4 ++-
>> sysdeps/htl/pthreadP.h | 2 --
>> sysdeps/posix/raise.c | 11 +++++--
>> sysdeps/unix/sysv/linux/raise.c | 52 ---------------------------------
>
> The new raise doesn't use pthread_kill but direct system calls, so the
> libpthread changes are unnecessary, and the implementation should remain
> as sysdeps/unix/sysv/linux/raise.c.
>
But the new sysdeps/posix/raise.c does uses, this patch change it to:
int
raise (int sig)
{
int ret = __pthread_kill (__pthread_self (), sig);
if (ret != 0)
{
__set_errno (ret);
ret = -1;
}
return ret;
}
* Adhemerval Zanella:
> On 11/05/2021 11:29, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>>
>>> Now that pthread_kill is provided by libc.so and async-signal-safe,
>>> it is possible to implement the generic POSIX implementation as
>>> pthread_kill(pthread_self(), sig).
>>
>> This part no longer matches the patch, I think.
>
> We should be since b76658451c81, no?
>
>>
>>> For Linux implementation, pthread_kill read the targetting TID from
>>> the TCB. For raise, this it not possible because it would make raise
>>> fail when issue after vfork (where creates the resulting process
>>> has a different TID from the parent, but its TCB is not updated as
>>> for pthread_create). For this case, gettid is called directly.
>>>
>>> Checked on x86_64-linux-gnu.
>>> ---
>>> include/pthread.h | 5 ++++
>>> nptl/pthreadP.h | 4 +--
>>> nptl/pthread_kill.c | 42 +++++++++++++++-----------
>>> nptl/pthread_self.c | 4 ++-
>>> sysdeps/htl/pthreadP.h | 2 --
>>> sysdeps/posix/raise.c | 11 +++++--
>>> sysdeps/unix/sysv/linux/raise.c | 52 ---------------------------------
>>
>> The new raise doesn't use pthread_kill but direct system calls, so the
>> libpthread changes are unnecessary, and the implementation should remain
>> as sysdeps/unix/sysv/linux/raise.c.
>>
>
> But the new sysdeps/posix/raise.c does uses, this patch change it to:
>
> int
> raise (int sig)
> {
> int ret = __pthread_kill (__pthread_self (), sig);
> if (ret != 0)
> {
> __set_errno (ret);
> ret = -1;
> }
> return ret;
> }
Hmm, I may have been confused. Let me re-review.
Florian
* Adhemerval Zanella via Libc-alpha:
> Now that pthread_kill is provided by libc.so and async-signal-safe,
> it is possible to implement the generic POSIX implementation as
> pthread_kill(pthread_self(), sig).
Second try: I think the comment about async-signal-safety is
unnecessary.
Furthermore, pthread_kill is not available in Hurd's libc.so, so I'm not
quite sure how this will build there.
> For Linux implementation, pthread_kill read the targetting TID from
> the TCB. For raise, this it not possible because it would make raise
> fail when issue after vfork (where creates the resulting process
> has a different TID from the parent, but its TCB is not updated as
> for pthread_create). For this case, gettid is called directly.
I think this should just say that pthread_kill is made ready for use
from vfork. The mechanical details of doing that should be captured in
a comment.
> diff --git a/nptl/pthread_kill.c b/nptl/pthread_kill.c
> index 71c362adce..d79531c10c 100644
> --- a/nptl/pthread_kill.c
> +++ b/nptl/pthread_kill.c
> @@ -31,32 +31,40 @@ __pthread_kill (pthread_t threadid, int signo)
> sigset_t set;
> __libc_signal_block_all (&set);
>
> - int val;
> -
> - /* Force load of pd->tid into local variable or register. Otherwise
> - if a thread exits between ESRCH test and tgkill, we might return
> - EINVAL, because pd->tid would be cleared by the kernel. */
> + pid_t tid;
> struct pthread *pd = (struct pthread *) threadid;
> - pid_t tid = atomic_forced_read (pd->tid);
> - if (__glibc_unlikely (tid <= 0))
> - /* Not a valid thread handle. */
> - val = ESRCH;
> +
> + if (pd == THREAD_SELF)
> + tid = INLINE_SYSCALL_CALL (gettid);
This should have a comment referencing vfork.
> else
> - {
> - /* We have a special syscall to do the work. */
> - pid_t pid = __getpid ();
> + /* Force load of pd->tid into local variable or register. Otherwise
> + if a thread exits between ESRCH test and tgkill, we might return
> + EINVAL, because pd->tid would be cleared by the kernel. */
> + tid = atomic_forced_read (pd->tid);
> +
> + int val;
> + if (__glibc_likely (tid > 0))
> + {
> + pid_t pid = __getpid ();
>
> - val = INTERNAL_SYSCALL_CALL (tgkill, pid, tid, signo);
> - val = (INTERNAL_SYSCALL_ERROR_P (val)
> - ? INTERNAL_SYSCALL_ERRNO (val) : 0);
> - }
> + val = INTERNAL_SYSCALL_CALL (tgkill, pid, tid, signo);
> + val = (INTERNAL_SYSCALL_ERROR_P (val)
> + ? INTERNAL_SYSCALL_ERRNO (val) : 0);
> + }
> + else
> + val = ESRCH;
>
> __libc_signal_restore_set (&set);
>
> return val;
> }
> +/* Some architectures (for instance arm) might pull raise through libgcc, so
> + avoid the symbol version if it ends up being used on ld.so. */
> +#if !IS_IN(rtld)
> +libc_hidden_def (__pthread_kill)
> versioned_symbol (libc, __pthread_kill, pthread_kill, GLIBC_2_34);
>
> -#if OTHER_SHLIB_COMPAT (libpthread, GLIBC_2_0, GLIBC_2_34)
> +# if OTHER_SHLIB_COMPAT (libpthread, GLIBC_2_0, GLIBC_2_34)
> compat_symbol (libc, __pthread_kill, pthread_kill, GLIBC_2_0);
> +# endif
> #endif
Rest of the pthread_kill change looks okay.
> diff --git a/nptl/pthread_self.c b/nptl/pthread_self.c
> index f877a2e6bd..196d93fb8e 100644
> --- a/nptl/pthread_self.c
> +++ b/nptl/pthread_self.c
> @@ -20,7 +20,9 @@
> #include <tls.h>
>
> pthread_t
> -pthread_self (void)
> +__pthread_self (void)
> {
> return (pthread_t) THREAD_SELF;
> }
> +libc_hidden_def (__pthread_self)
> +weak_alias (__pthread_self, pthread_self)
> diff --git a/sysdeps/htl/pthreadP.h b/sysdeps/htl/pthreadP.h
> index 8e2cf2ce65..3b357b7bdc 100644
THREAD_SELF is now available on Hurd, too. So any reference to
__pthread_self should use THREAD_SELF directly, I think.
Thanks,
Florian
On 11/05/2021 12:50, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
>
>> Now that pthread_kill is provided by libc.so and async-signal-safe,
>> it is possible to implement the generic POSIX implementation as
>> pthread_kill(pthread_self(), sig).
>
> Second try: I think the comment about async-signal-safety is
> unnecessary.
Fair enough.
>
> Furthermore, pthread_kill is not available in Hurd's libc.so, so I'm not
> quite sure how this will build there.
Hurd uses sysdeps/hurd/htl/pt-kill.c instead.
>
>> For Linux implementation, pthread_kill read the targetting TID from
>> the TCB. For raise, this it not possible because it would make raise
>> fail when issue after vfork (where creates the resulting process
>> has a different TID from the parent, but its TCB is not updated as
>> for pthread_create). For this case, gettid is called directly.
>
> I think this should just say that pthread_kill is made ready for use
> from vfork. The mechanical details of doing that should be captured in
> a comment.
What about:
Now that pthread_kill is provided by libc.so it is possible to implement
the generic POSIX implementation as pthread_kill(pthread_self(), sig).
For Linux implementation, pthread_kill read the targeting TID from
the TCB. For raise, this it not possible because it would make raise
fail when issue after vfork (where creates the resulting process
has a different TID from the parent, but its TCB is not updated as
for pthread_create). To make raise use pthread_kill, it is make
usable from vfork by getting the target thread id through gettid
syscall.
>
>> diff --git a/nptl/pthread_kill.c b/nptl/pthread_kill.c
>> index 71c362adce..d79531c10c 100644
>> --- a/nptl/pthread_kill.c
>> +++ b/nptl/pthread_kill.c
>> @@ -31,32 +31,40 @@ __pthread_kill (pthread_t threadid, int signo)
>> sigset_t set;
>> __libc_signal_block_all (&set);
>>
>> - int val;
>> -
>> - /* Force load of pd->tid into local variable or register. Otherwise
>> - if a thread exits between ESRCH test and tgkill, we might return
>> - EINVAL, because pd->tid would be cleared by the kernel. */
>> + pid_t tid;
>> struct pthread *pd = (struct pthread *) threadid;
>> - pid_t tid = atomic_forced_read (pd->tid);
>> - if (__glibc_unlikely (tid <= 0))
>> - /* Not a valid thread handle. */
>> - val = ESRCH;
>> +
>> + if (pd == THREAD_SELF)
>> + tid = INLINE_SYSCALL_CALL (gettid);
>
> This should have a comment referencing vfork.
Ack.
>
>> else
>> - {
>> - /* We have a special syscall to do the work. */
>> - pid_t pid = __getpid ();
>> + /* Force load of pd->tid into local variable or register. Otherwise
>> + if a thread exits between ESRCH test and tgkill, we might return
>> + EINVAL, because pd->tid would be cleared by the kernel. */
>> + tid = atomic_forced_read (pd->tid);
>> +
>> + int val;
>> + if (__glibc_likely (tid > 0))
>> + {
>> + pid_t pid = __getpid ();
>>
>> - val = INTERNAL_SYSCALL_CALL (tgkill, pid, tid, signo);
>> - val = (INTERNAL_SYSCALL_ERROR_P (val)
>> - ? INTERNAL_SYSCALL_ERRNO (val) : 0);
>> - }
>> + val = INTERNAL_SYSCALL_CALL (tgkill, pid, tid, signo);
>> + val = (INTERNAL_SYSCALL_ERROR_P (val)
>> + ? INTERNAL_SYSCALL_ERRNO (val) : 0);
>> + }
>> + else
>> + val = ESRCH;
>>
>> __libc_signal_restore_set (&set);
>>
>> return val;
>> }
>> +/* Some architectures (for instance arm) might pull raise through libgcc, so
>> + avoid the symbol version if it ends up being used on ld.so. */
>> +#if !IS_IN(rtld)
>> +libc_hidden_def (__pthread_kill)
>> versioned_symbol (libc, __pthread_kill, pthread_kill, GLIBC_2_34);
>>
>> -#if OTHER_SHLIB_COMPAT (libpthread, GLIBC_2_0, GLIBC_2_34)
>> +# if OTHER_SHLIB_COMPAT (libpthread, GLIBC_2_0, GLIBC_2_34)
>> compat_symbol (libc, __pthread_kill, pthread_kill, GLIBC_2_0);
>> +# endif
>> #endif
>
> Rest of the pthread_kill change looks okay.
>
>> diff --git a/nptl/pthread_self.c b/nptl/pthread_self.c
>> index f877a2e6bd..196d93fb8e 100644
>> --- a/nptl/pthread_self.c
>> +++ b/nptl/pthread_self.c
>> @@ -20,7 +20,9 @@
>> #include <tls.h>
>>
>> pthread_t
>> -pthread_self (void)
>> +__pthread_self (void)
>> {
>> return (pthread_t) THREAD_SELF;
>> }
>> +libc_hidden_def (__pthread_self)
>> +weak_alias (__pthread_self, pthread_self)
>> diff --git a/sysdeps/htl/pthreadP.h b/sysdeps/htl/pthreadP.h
>> index 8e2cf2ce65..3b357b7bdc 100644
>
> THREAD_SELF is now available on Hurd, too. So any reference to
> __pthread_self should use THREAD_SELF directly, I think.
In that case maybe move the consolidation to signal/raise.c instead
of sysdeps/posix/raise.c.
@@ -13,4 +13,9 @@ extern int __pthread_barrier_wait (pthread_barrier_t *__barrier)
/* This function is called to initialize the pthread library. */
extern void __pthread_initialize (void) __attribute__ ((weak));
+
+extern int __pthread_kill (pthread_t threadid, int signo);
+
+extern pthread_t __pthread_self (void);
+
#endif
@@ -559,11 +559,11 @@ extern int __pthread_once (pthread_once_t *once_control,
libc_hidden_proto (__pthread_once)
extern int __pthread_atfork (void (*prepare) (void), void (*parent) (void),
void (*child) (void));
-extern pthread_t __pthread_self (void);
+libc_hidden_proto (__pthread_self)
extern int __pthread_equal (pthread_t thread1, pthread_t thread2);
extern int __pthread_detach (pthread_t th);
extern int __pthread_cancel (pthread_t th);
-extern int __pthread_kill (pthread_t threadid, int signo);
+libc_hidden_proto (__pthread_kill)
extern void __pthread_exit (void *value) __attribute__ ((__noreturn__));
libc_hidden_proto (__pthread_exit)
extern int __pthread_join (pthread_t threadid, void **thread_return);
@@ -31,32 +31,40 @@ __pthread_kill (pthread_t threadid, int signo)
sigset_t set;
__libc_signal_block_all (&set);
- int val;
-
- /* Force load of pd->tid into local variable or register. Otherwise
- if a thread exits between ESRCH test and tgkill, we might return
- EINVAL, because pd->tid would be cleared by the kernel. */
+ pid_t tid;
struct pthread *pd = (struct pthread *) threadid;
- pid_t tid = atomic_forced_read (pd->tid);
- if (__glibc_unlikely (tid <= 0))
- /* Not a valid thread handle. */
- val = ESRCH;
+
+ if (pd == THREAD_SELF)
+ tid = INLINE_SYSCALL_CALL (gettid);
else
- {
- /* We have a special syscall to do the work. */
- pid_t pid = __getpid ();
+ /* Force load of pd->tid into local variable or register. Otherwise
+ if a thread exits between ESRCH test and tgkill, we might return
+ EINVAL, because pd->tid would be cleared by the kernel. */
+ tid = atomic_forced_read (pd->tid);
+
+ int val;
+ if (__glibc_likely (tid > 0))
+ {
+ pid_t pid = __getpid ();
- val = INTERNAL_SYSCALL_CALL (tgkill, pid, tid, signo);
- val = (INTERNAL_SYSCALL_ERROR_P (val)
- ? INTERNAL_SYSCALL_ERRNO (val) : 0);
- }
+ val = INTERNAL_SYSCALL_CALL (tgkill, pid, tid, signo);
+ val = (INTERNAL_SYSCALL_ERROR_P (val)
+ ? INTERNAL_SYSCALL_ERRNO (val) : 0);
+ }
+ else
+ val = ESRCH;
__libc_signal_restore_set (&set);
return val;
}
+/* Some architectures (for instance arm) might pull raise through libgcc, so
+ avoid the symbol version if it ends up being used on ld.so. */
+#if !IS_IN(rtld)
+libc_hidden_def (__pthread_kill)
versioned_symbol (libc, __pthread_kill, pthread_kill, GLIBC_2_34);
-#if OTHER_SHLIB_COMPAT (libpthread, GLIBC_2_0, GLIBC_2_34)
+# if OTHER_SHLIB_COMPAT (libpthread, GLIBC_2_0, GLIBC_2_34)
compat_symbol (libc, __pthread_kill, pthread_kill, GLIBC_2_0);
+# endif
#endif
@@ -20,7 +20,9 @@
#include <tls.h>
pthread_t
-pthread_self (void)
+__pthread_self (void)
{
return (pthread_t) THREAD_SELF;
}
+libc_hidden_def (__pthread_self)
+weak_alias (__pthread_self, pthread_self)
@@ -31,8 +31,6 @@ extern void __pthread_init_static_tls (struct link_map *) attribute_hidden;
/* These represent the interface used by glibc itself. */
-extern pthread_t __pthread_self (void);
-extern int __pthread_kill (pthread_t threadid, int signo);
extern struct __pthread **__pthread_threads;
extern int __pthread_mutex_init (pthread_mutex_t *__mutex, const pthread_mutexattr_t *__attr);
@@ -16,13 +16,20 @@
<https://www.gnu.org/licenses/>. */
#include <signal.h>
-#include <unistd.h>
+#include <errno.h>
+#include <pthread.h>
/* Raise the signal SIG. */
int
raise (int sig)
{
- return __kill (__getpid (), sig);
+ int ret = __pthread_kill (__pthread_self (), sig);
+ if (ret != 0)
+ {
+ __set_errno (ret);
+ ret = -1;
+ }
+ return ret;
}
libc_hidden_def (raise)
weak_alias (raise, gsignal)
deleted file mode 100644
@@ -1,52 +0,0 @@
-/* Copyright (C) 2002-2021 Free Software Foundation, Inc.
- This file is part of the GNU C Library.
- Contributed by Ulrich Drepper <drepper@redhat.com>, 2002.
-
- 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 <signal.h>
-#include <sysdep.h>
-#include <errno.h>
-#include <sys/types.h>
-#include <unistd.h>
-#include <internal-signals.h>
-
-int
-raise (int sig)
-{
- /* rt_sigprocmask may fail if:
-
- 1. sigsetsize != sizeof (sigset_t) (EINVAL)
- 2. a failure in copy from/to user space (EFAULT)
- 3. an invalid 'how' operation (EINVAL)
-
- The first case is already handle in glibc syscall call by using the arch
- defined _NSIG. Second case is handled by using a stack allocated mask.
- The last one should be handled by the block/unblock functions. */
-
- sigset_t set;
- __libc_signal_block_app (&set);
-
- pid_t pid = INTERNAL_SYSCALL_CALL (getpid);
- pid_t tid = INTERNAL_SYSCALL_CALL (gettid);
-
- int ret = INLINE_SYSCALL_CALL (tgkill, pid, tid, sig);
-
- __libc_signal_restore_set (&set);
-
- return ret;
-}
-libc_hidden_def (raise)
-weak_alias (raise, gsignal)