[PATCH] [RFC] y2038: nptl: Convert pthread_cond_{clock|timed}wait to support 64 bit time
Adhemerval Zanella
adhemerval.zanella@linaro.org
Tue Aug 25 14:43:59 GMT 2020
On 24/08/2020 13:40, Lukasz Majewski wrote:
> The pthread_cond_clockwait and pthread_cond_timedwait have been converted
> to support 64 bit time.
>
> This change introduces new futex_abstimed_wait_cancelable64 function in
> ./sysdeps/nptl/futex-helpers.c, which uses futex_time64 where possible
> and tries to replace low-level preprocessor macros from
> lowlevellock-futex.h
> The pthread_cond_{clock|timed}wait only accepts absolute time. Moreover,
> there is no need to check for NULL passed as *abstime pointer as
> __pthread_cond_wait_common() always passes non-NULL struct __timespec64
> pointer to futex_abstimed_wait_cancellable64().
>
> For systems with __TIMESIZE != 64 && __WORDSIZE == 32:
> - Conversions between 64 bit time to 32 bit are necessary
> - Redirection to __pthread_cond_{clock|timed}wait64 will provide support
> for 64 bit time
>
> The futex_abstimed_wait_cancelable64 function has been put into a separate
> file on the purpose - to avoid issues apparent on m68k architecture related
> to small number of available registers (there is not enough registers to
> put all necessary arguments in them when inlining).
> In fact - the futex_helper.c is reused, but extra flag "-fno-inline" is set
> when it is build for this architecture. Such approach fixes the build issue.
>
>
> Build tests:
> ./src/scripts/build-many-glibcs.py glibcs
>
> Run-time tests:
> - Run specific tests on ARM/x86 32bit systems (qemu):
> https://github.com/lmajewski/meta-y2038 and run tests:
> https://github.com/lmajewski/y2038-tests/commits/master
>
> Above tests were performed with Y2038 redirection applied as well as without
> to test the proper usage of both __pthread_cond_{clock|timed}wait64 and
> __pthread_cond_{clock|timed}wait.
Some comments below. As a general comment, I think you are using 8 whitespace
instead of tab.
> ---
> nptl/pthreadP.h | 11 +++
> nptl/pthread_cond_wait.c | 46 ++++++++--
> sysdeps/nptl/Makefile | 2 +-
> sysdeps/nptl/futex-helpers.c | 89 ++++++++++++++++++++
> sysdeps/nptl/futex-internal.h | 11 +++
> sysdeps/unix/sysv/linux/m68k/Makefile | 5 ++
> sysdeps/unix/sysv/linux/m68k/futex-helpers.c | 19 +++++
> 7 files changed, 173 insertions(+), 10 deletions(-)
> create mode 100644 sysdeps/nptl/futex-helpers.c
> create mode 100644 sysdeps/unix/sysv/linux/m68k/futex-helpers.c
>
> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
> index 99713c8447..e288c7e778 100644
> --- a/nptl/pthreadP.h
> +++ b/nptl/pthreadP.h
> @@ -462,6 +462,8 @@ extern int __pthread_cond_wait (pthread_cond_t *cond, pthread_mutex_t *mutex);
> #if __TIMESIZE == 64
> # define __pthread_clockjoin_np64 __pthread_clockjoin_np
> # define __pthread_timedjoin_np64 __pthread_timedjoin_np
> +# define __pthread_cond_timedwait64 __pthread_cond_timedwait
> +# define __pthread_cond_clockwait64 __pthread_cond_clockwait
> #else
> extern int __pthread_clockjoin_np64 (pthread_t threadid, void **thread_return,
> clockid_t clockid,
Ok.
> @@ -470,6 +472,15 @@ libc_hidden_proto (__pthread_clockjoin_np64)
> extern int __pthread_timedjoin_np64 (pthread_t threadid, void **thread_return,
> const struct __timespec64 *abstime);
> libc_hidden_proto (__pthread_timedjoin_np64)
> +extern int __pthread_cond_timedwait64 (pthread_cond_t *cond,
> + pthread_mutex_t *mutex,
> + const struct __timespec64 *abstime);
> +libc_hidden_proto (__pthread_cond_timedwait64)
> +extern int __pthread_cond_clockwait64 (pthread_cond_t *cond,
> + pthread_mutex_t *mutex,
> + clockid_t clockid,
> + const struct __timespec64 *abstime);
> +libc_hidden_proto (__pthread_cond_clockwait64)
> #endif
>
> extern int __pthread_cond_timedwait (pthread_cond_t *cond,
They are symbol that will be implementated only for libpthread, so this should
be libpthread_hidden_proto instead.
> diff --git a/nptl/pthread_cond_wait.c b/nptl/pthread_cond_wait.c
> index 85ddbc1011..560b30129b 100644
> --- a/nptl/pthread_cond_wait.c
> +++ b/nptl/pthread_cond_wait.c
> @@ -31,6 +31,7 @@
> #include <stap-probe.h>
> #include <time.h>
>
> +
Gratuitous new line.
> #include "pthread_cond_common.c"
>
>
> @@ -378,8 +379,7 @@ __condvar_cleanup_waiting (void *arg)
> */
> static __always_inline int
> __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex,
> - clockid_t clockid,
> - const struct timespec *abstime)
> + clockid_t clockid, const struct __timespec64 *abstime)
> {
> const int maxspin = 0;
> int err;
> @@ -517,7 +517,7 @@ __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex,
> err = ETIMEDOUT;
> else
> {
> - err = futex_abstimed_wait_cancelable
> + err = futex_abstimed_wait_cancelable64
> (cond->__data.__g_signals + g, 0, clockid, abstime,
> private);
> }
Ok.
> @@ -640,8 +640,8 @@ __pthread_cond_wait (pthread_cond_t *cond, pthread_mutex_t *mutex)
>
> /* See __pthread_cond_wait_common. */
> int
> -__pthread_cond_timedwait (pthread_cond_t *cond, pthread_mutex_t *mutex,
> - const struct timespec *abstime)
> +__pthread_cond_timedwait64 (pthread_cond_t *cond, pthread_mutex_t *mutex,
> + const struct __timespec64 *abstime)
> {
> /* Check parameter validity. This should also tell the compiler that
> it can assume that abstime is not NULL. */
Ok.
> @@ -655,6 +655,20 @@ __pthread_cond_timedwait (pthread_cond_t *cond, pthread_mutex_t *mutex,
> ? CLOCK_MONOTONIC : CLOCK_REALTIME;
> return __pthread_cond_wait_common (cond, mutex, clockid, abstime);
> }
> +
> +#if __TIMESIZE != 64
> +libc_hidden_def (__pthread_cond_timedwait64)
It should be libpthread_hidden_def here.
> +
> +int
> +__pthread_cond_timedwait (pthread_cond_t *cond, pthread_mutex_t *mutex,
> + const struct timespec *abstime)
> +{
> + struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime);
> +
> + return __pthread_cond_timedwait64 (cond, mutex, &ts64);
> +}
> +#endif
> +
> versioned_symbol (libpthread, __pthread_cond_wait, pthread_cond_wait,
> GLIBC_2_3_2);
> versioned_symbol (libpthread, __pthread_cond_timedwait, pthread_cond_timedwait,
Ok, 'abstime' is marked as nonnull.
> @@ -662,18 +676,32 @@ versioned_symbol (libpthread, __pthread_cond_timedwait, pthread_cond_timedwait,
>
> /* See __pthread_cond_wait_common. */
> int
> -__pthread_cond_clockwait (pthread_cond_t *cond, pthread_mutex_t *mutex,
> - clockid_t clockid,
> - const struct timespec *abstime)
> +__pthread_cond_clockwait64 (pthread_cond_t *cond, pthread_mutex_t *mutex,
> + clockid_t clockid,
> + const struct __timespec64 *abstime)
> {
> /* Check parameter validity. This should also tell the compiler that
> it can assume that abstime is not NULL. */
> if (! valid_nanoseconds (abstime->tv_nsec))
> return EINVAL;
>
> - if (!futex_abstimed_supported_clockid (clockid))
> + if (! futex_abstimed_supported_clockid (clockid))
> return EINVAL;
Gratuitous change.
>
> return __pthread_cond_wait_common (cond, mutex, clockid, abstime);
> }
> +
> +#if __TIMESIZE != 64
> +libc_hidden_def (__pthread_cond_clockwait64)
> +
> +int
> +__pthread_cond_clockwait (pthread_cond_t *cond, pthread_mutex_t *mutex,
> + clockid_t clockid,
> + const struct timespec *abstime)
> +{
> + struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime);
> +
> + return __pthread_cond_clockwait64 (cond, mutex, clockid, &ts64);
> +}
> +#endif
> weak_alias (__pthread_cond_clockwait, pthread_cond_clockwait);
Ok, 'abstime' is marked as nonnull.
> diff --git a/sysdeps/nptl/Makefile b/sysdeps/nptl/Makefile
> index 0631a870c8..6d649a94a2 100644
> --- a/sysdeps/nptl/Makefile
> +++ b/sysdeps/nptl/Makefile
> @@ -17,7 +17,7 @@
> # <https://www.gnu.org/licenses/>.
>
> ifeq ($(subdir),nptl)
> -libpthread-sysdep_routines += errno-loc
> +libpthread-sysdep_routines += errno-loc futex-helpers
> endif
>
> ifeq ($(subdir),rt)
Ok.
> diff --git a/sysdeps/nptl/futex-helpers.c b/sysdeps/nptl/futex-helpers.c
> new file mode 100644
> index 0000000000..dfd8870d35
> --- /dev/null
> +++ b/sysdeps/nptl/futex-helpers.c
I think it should be named sysdeps/nptl/futex-internal.c since it implements
the usual inline function from the header.
> @@ -0,0 +1,89 @@
> +/* futex helper functions for glibc-internal use.
> + Copyright (C) 2020 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 <sysdep.h>
> +#include <time.h>
> +#include <futex-internal.h>
> +#include <kernel-features.h>
> +
> +int
> +futex_abstimed_wait_cancelable64 (unsigned int* futex_word,
> + unsigned int expected, clockid_t clockid,
> + const struct __timespec64* abstime,
> + int private)
As it was raised before, rename to __futex_abstimed_wait_cancelable64.
> +{
> + unsigned int clockbit;
> + int oldtype, err, op;
> +
> + /* Work around the fact that the kernel rejects negative timeout values
> + despite them being valid. */
> + if (__glibc_unlikely ((abstime != NULL) && (abstime->tv_sec < 0)))
> + return ETIMEDOUT;
> +
> + if (! lll_futex_supported_clockid (clockid))
> + return EINVAL;
> +
> + oldtype = __pthread_enable_asynccancel ();
> + clockbit = (clockid == CLOCK_REALTIME) ? FUTEX_CLOCK_REALTIME : 0;
> + op = __lll_private_flag (FUTEX_WAIT_BITSET | clockbit, private);
> +
> + err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op, expected,
> + abstime, NULL /* Unused. */,
> + FUTEX_BITSET_MATCH_ANY);
I think it would be better to just use INTERNAL_SYSCALL_CANCEL, since in the
long term to fiz the cancellation race (BZ#12683) it would require to get
rid of __*_{enable,disable}_asynccancel. It would incur in more code size,
but I think it is feasible.
> +#ifndef __ASSUME_TIME64_SYSCALLS
> + if (err == -ENOSYS)
> + {
> + struct timespec ts32;
Identation seems of here.
> + if (in_time_t_range (abstime->tv_sec))
> + {
> + ts32 = valid_timespec64_to_timespec (*abstime);
> +
> + err = INTERNAL_SYSCALL_CALL (futex, futex_word, op, expected,
> + &ts32, NULL /* Unused. */,
> + FUTEX_BITSET_MATCH_ANY);
> + }
> + else
> + err = -EOVERFLOW;
> + }
> +#endif
> + __pthread_disable_asynccancel (oldtype);
> +
> + switch (err)
> + {
> + case 0:
> + case -EAGAIN:
> + case -EINTR:
> + case -ETIMEDOUT:
> + case -EOVERFLOW: /* Passed absolute timeout uses 64 bit time_t type, but
> + underlying kernel does not support 64 bit time_t futex
> + syscalls. */
> + return -err;
> +
> + case -EFAULT: /* Must have been caused by a glibc or application bug. */
> + case -EINVAL: /* Either due to wrong alignment or due to the timeout not
> + being normalized. Must have been caused by a glibc or
> + application bug. */
> + case -ENOSYS: /* Must have been caused by a glibc bug. */
> + /* No other errors are documented at this time. */
> + default:
> + futex_fatal_error ();
> + }
> +}
> +
> +libpthread_hidden_def (futex_abstimed_wait_cancelable64)
There is no need to add a hidden def here, the function is internal to libpthread
so just mark as attribute_hidden.
> diff --git a/sysdeps/nptl/futex-internal.h b/sysdeps/nptl/futex-internal.h
> index 159aae82dc..aea01e5bcd 100644
> --- a/sysdeps/nptl/futex-internal.h
> +++ b/sysdeps/nptl/futex-internal.h
> @@ -519,4 +519,15 @@ futex_timed_wait_cancel64 (pid_t *tidp, pid_t tid,
> futex_fatal_error ();
> }
> }
> +
> +/* The futex_abstimed_wait_cancelable64 has been moved to a separate file
> + to avoid problems with exhausting available registers on some architectures
> + - e.g. on m68k architecture. */
> +int
> +futex_abstimed_wait_cancelable64 (unsigned int* futex_word,
> + unsigned int expected, clockid_t clockid,
> + const struct __timespec64* abstime,
> + int private);
> +libpthread_hidden_proto (futex_abstimed_wait_cancelable64)
> +
> #endif /* futex-internal.h */
Remove the libpthread_hidden_proto and move to attribute_hidden.
> diff --git a/sysdeps/unix/sysv/linux/m68k/Makefile b/sysdeps/unix/sysv/linux/m68k/Makefile
> index be40fae68a..f19c8c825d 100644
> --- a/sysdeps/unix/sysv/linux/m68k/Makefile
> +++ b/sysdeps/unix/sysv/linux/m68k/Makefile
> @@ -21,3 +21,8 @@ sysdep-dl-routines += dl-static
> sysdep-others += lddlibc4
> install-bin += lddlibc4
> endif
> +
> +ifeq ($(subdir),nptl)
> +libpthread-sysdep_routines += futex-helpers
There is no need to duplicate it here, you already do it on sysdeps/nptl/Makefile.
> +CFLAGS-futex-helpers.c += -fno-inline
> +endif
This is trick one that I would to avoid. One change to the generic code that did not
triggered the compiler issue is to move the 32-bit __NR_futex call to auxiliary function:
---
#ifndef __ASSUME_TIME64_SYSCALLS
static int
__futex_abstimed_wait_cancellable32 (unsigned int* futex_word,
unsigned int expected, clockid_t clockid,
const struct __timespec64* abstime,
int private)
{
if (! in_time_t_range (abstime->tv_sec))
return -EOVERFLOW;
unsigned int clockbit = (clockid == CLOCK_REALTIME)
? FUTEX_CLOCK_REALTIME : 0;
int op = __lll_private_flag (FUTEX_WAIT_BITSET | clockbit, private);
struct timespec ts32 = valid_timespec64_to_timespec (*abstime);
return INTERNAL_SYSCALL_CALL (futex, futex_word, op, expected,
&ts32, NULL /* Unused. */,
FUTEX_BITSET_MATCH_ANY);
}
#endif
[...]
int
__futex_abstimed_wait_cancelable64 (unsigned int* futex_word,
unsigned int expected, clockid_t clockid,
const struct __timespec64* abstime,
int private)
{
[...]
#ifndef __ASSUME_TIME64_SYSCALLS
if (err == -ENOSYS)
err = __futex_abstimed_wait_cancellable32 (futex_word, op, clockid,
abstime, private);
#endif
[...]
}
---
It builds file on gcc version 8.3.1 for m68k-linux-gnu and m68k-linux-gnu without
requiring extra compiler options.
> diff --git a/sysdeps/unix/sysv/linux/m68k/futex-helpers.c b/sysdeps/unix/sysv/linux/m68k/futex-helpers.c
> new file mode 100644
> index 0000000000..fb03b5d174
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/m68k/futex-helpers.c
> @@ -0,0 +1,19 @@
> +/* futex helper functions for glibc-internal use for m68k architecture
> + Copyright (C) 2020 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 <sysdeps/nptl/futex-helpers.c>
>
There file is not needed, sysdeps makefile rules will pick the nptl generic one.
More information about the Libc-alpha
mailing list