Message ID | 20210503210005.2891859-2-adhemerval.zanella@linaro.org |
---|---|
State | New |
Headers | show |
Series | [v3,1/6] nptl: Make pthread_kill async-signal-safe | expand |
* 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.
diff --git a/include/pthread.h b/include/pthread.h index 858c869a16..2623fb5e95 100644 --- a/include/pthread.h +++ b/include/pthread.h @@ -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 diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h index 00d2cfe764..3e5aba74ed 100644 --- a/nptl/pthreadP.h +++ b/nptl/pthreadP.h @@ -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); 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); 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 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 --- a/sysdeps/htl/pthreadP.h +++ b/sysdeps/htl/pthreadP.h @@ -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); diff --git a/sysdeps/posix/raise.c b/sysdeps/posix/raise.c index 9fdb4d538b..4806c0cc63 100644 --- a/sysdeps/posix/raise.c +++ b/sysdeps/posix/raise.c @@ -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) diff --git a/sysdeps/unix/sysv/linux/raise.c b/sysdeps/unix/sysv/linux/raise.c deleted file mode 100644 index 9be3b37f53..0000000000 --- a/sysdeps/unix/sysv/linux/raise.c +++ /dev/null @@ -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)