Message ID | 20200824164000.1619-1-lukma@denx.de |
---|---|
State | New |
Headers | show |
Series | [RFC] y2038: nptl: Convert pthread_cond_{clock|timed}wait to support 64 bit time | expand |
Dear Community, > 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. Unfortunately, having the futex-helpers.c as a separate file caused issue with linknamespace tests: /tmp/tmpiql0gye3/undef.c:33:1: warning: ‘pthread_attr_getstackaddr’ is deprecated [-Wdeprecated-declarations] 33 | void *__glibc_test_pthread_attr_getstackaddr = (void *) &pthread_attr_getstackaddr; | ^~~~ In file included from ../include/pthread.h:1, from /tmp/tmpiql0gye3/undef.c:1: ../sysdeps/nptl/pthread.h:332:12: note: declared here 332 | extern int pthread_attr_getstackaddr (const pthread_attr_t *__restrict | ^~~~~~~~~~~~~~~~~~~~~~~~~ /tmp/tmpiql0gye3/undef.c:42:1: warning: ‘pthread_attr_setstackaddr’ is deprecated [-Wdeprecated-declarations] 42 | void *__glibc_test_pthread_attr_setstackaddr = (void *) &pthread_attr_setstackaddr; | ^~~~ In file included from ../include/pthread.h:1, from /tmp/tmpiql0gye3/undef.c:1: ../sysdeps/nptl/pthread.h:340:12: note: declared here 340 | extern int pthread_attr_setstackaddr (pthread_attr_t *__attr, | ^~~~~~~~~~~~~~~~~~~~~~~~~ I'm a bit puzzled here. It is a warning in fact despite I've guarded the function in question with: libpthread_hidden_{proto|def} (futex_abstimed_wait_cancelable64) Even more strange is that the check passes when the same function is added to futex-internals.h with "static __always_inline" attribute. (But then the problem with small number of regs for m68k is apparent). As fair as I understood - the linknamespace check is to build sample program with proper header (like pthread.h) and check if symbols from linked library (in our case libpthread.*) are exported properly. In the above case the "test" complains about using obsolete functions (i.e. - pthread_attr_setstackaddr). Is this part of the check itself? Or is it a side effect? Any help appreciated. Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
* Lukasz Majewski: > I'm a bit puzzled here. It is a warning in fact despite I've guarded > the function in question with: > libpthread_hidden_{proto|def} (futex_abstimed_wait_cancelable64) I think the linknamespace test attempts to descend into futex_abstimed_wait_cancelable64 because it its name is in the public (non-implementation) namespace. Try naming it __futex_abstimed_wait_cancelable64. Thanks, Florian
Hi Florian, > * Lukasz Majewski: > > > I'm a bit puzzled here. It is a warning in fact despite I've guarded > > the function in question with: > > libpthread_hidden_{proto|def} (futex_abstimed_wait_cancelable64) > > I think the linknamespace test attempts to descend into > futex_abstimed_wait_cancelable64 because it its name is in the public > (non-implementation) namespace. > > Try naming it __futex_abstimed_wait_cancelable64. Indeed, adding the "__" prefix fixed the issue. And this "__" prefix is not needed in futex-internal.h functions as those are "pasted" into source code by C preprocessor .... Thanks Florian for help :-) > > Thanks, > Florian > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Aug 24 2020, Lukasz Majewski wrote: > 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 Why do you need that twice? > +CFLAGS-futex-helpers.c += -fno-inline Why do you need that? Andreas.
Hi Andreas, > On Aug 24 2020, Lukasz Majewski wrote: > > > 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 > > Why do you need that twice? This is to fix following issue: https://marc.info/?l=glibc-alpha&m=159730587416436&w=2 > > > +CFLAGS-futex-helpers.c += -fno-inline > > Why do you need that? This solves problem with small number general purpose registers on m68k when inlining this patch. This problem only happens on m68k. > > Andreas. > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Aug 25 2020, Lukasz Majewski wrote: > Hi Andreas, > >> On Aug 24 2020, Lukasz Majewski wrote: >> >> > 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 >> >> Why do you need that twice? > > This is to fix following issue: > https://marc.info/?l=glibc-alpha&m=159730587416436&w=2 But why *twice*? >> >> > +CFLAGS-futex-helpers.c += -fno-inline >> >> Why do you need that? > > This solves problem with small number general purpose registers on m68k > when inlining this patch. What do you mean with "inlining this patch"? Andreas.
On 25/08/2020 07:34, Andreas Schwab wrote: > On Aug 25 2020, Lukasz Majewski wrote: > >> Hi Andreas, >> >>> On Aug 24 2020, Lukasz Majewski wrote: >>> >>>> 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 >>> >>> Why do you need that twice? >> >> This is to fix following issue: >> https://marc.info/?l=glibc-alpha&m=159730587416436&w=2 > > But why *twice*? > >>> >>>> +CFLAGS-futex-helpers.c += -fno-inline >>> >>> Why do you need that? >> >> This solves problem with small number general purpose registers on m68k >> when inlining this patch. > > What do you mean with "inlining this patch"? It is a mitigation for a compiler issue, without it the build fails with: ../sysdeps/nptl/futex-helpers.c: In function ‘__futex_abstimed_wait_cancelable64’: ../sysdeps/nptl/futex-helpers.c:87:1: error: unable to find a register to spill in class ‘DATA_REGS’ } ^ ../sysdeps/nptl/futex-helpers.c:87:1: error: this is the insn: (insn 98 97 99 8 (parallel [ (set (cc0) (compare (reg:DI 37 [ _13 ]) (reg:DI 12 %a4 [orig:54 s+-4 ] [54]))) (clobber (scratch:DI)) ]) "../sysdeps/nptl/futex-helpers.c":53 13 {*cmpdi_internal} (expr_list:REG_DEAD (reg:DI 12 %a4 [orig:54 s+-4 ] [54]) (nil))) ../sysdeps/nptl/futex-helpers.c:87: confused by earlier errors, bailing out I think a better strategy would to move the 32-bit futex time_t a helper function. I will write it down on my review.
Hi Andreas, > On Aug 25 2020, Lukasz Majewski wrote: > > > Hi Andreas, > > > >> On Aug 24 2020, Lukasz Majewski wrote: > >> > >> > 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 > >> > >> Why do you need that twice? > > > > This is to fix following issue: > > https://marc.info/?l=glibc-alpha&m=159730587416436&w=2 > > But why *twice*? Ok. So the following snippet: + ifeq ($(subdir),nptl) [*] + libpthread-sysdep_routines += futex-helpers Is not needed as it is already placed in /npt/Makefile. Am I correct? > > >> > >> > +CFLAGS-futex-helpers.c += -fno-inline So, only this modified (without ifeq above [*]) CFLAGS for futex-helper.c compilation is needed? > >> > >> Why do you need that? > > > > This solves problem with small number general purpose registers on > > m68k when inlining this patch. > > What do you mean with "inlining this patch"? When I put futex_abstimed_wait_cancelable64() as static __always_inline to futex-internals.h then the aforementioned problem on m68k emerge. > > Andreas. > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
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.
On Aug 25 2020, Lukasz Majewski wrote: > When I put futex_abstimed_wait_cancelable64() as static __always_inline > to futex-internals.h then the aforementioned problem on m68k emerge. But futex-helpers.c contains a single function. There is nothing to inline. Andreas.
On 25/08/2020 11:47, Andreas Schwab wrote: > On Aug 25 2020, Lukasz Majewski wrote: > >> When I put futex_abstimed_wait_cancelable64() as static __always_inline >> to futex-internals.h then the aforementioned problem on m68k emerge. > > But futex-helpers.c contains a single function. There is nothing to > inline. > > Andreas. > in_time_t_range and valid_timespec64_to_timespec.
On Aug 25 2020, Adhemerval Zanella via Libc-alpha wrote: > On 25/08/2020 11:47, Andreas Schwab wrote: >> On Aug 25 2020, Lukasz Majewski wrote: >> >>> When I put futex_abstimed_wait_cancelable64() as static __always_inline >>> to futex-internals.h then the aforementioned problem on m68k emerge. >> >> But futex-helpers.c contains a single function. There is nothing to >> inline. >> >> Andreas. >> > > in_time_t_range and valid_timespec64_to_timespec. But you don't want to uninline them. Andreas.
On 25/08/2020 12:26, Andreas Schwab wrote: > On Aug 25 2020, Adhemerval Zanella via Libc-alpha wrote: > >> On 25/08/2020 11:47, Andreas Schwab wrote: >>> On Aug 25 2020, Lukasz Majewski wrote: >>> >>>> When I put futex_abstimed_wait_cancelable64() as static __always_inline >>>> to futex-internals.h then the aforementioned problem on m68k emerge. >>> >>> But futex-helpers.c contains a single function. There is nothing to >>> inline. >>> >>> Andreas. >>> >> >> in_time_t_range and valid_timespec64_to_timespec. > > But you don't want to uninline them. Yes, that it seems exactly what is triggering gcc ICE.
On 25/08/2020 12:31, Adhemerval Zanella wrote: > > > On 25/08/2020 12:26, Andreas Schwab wrote: >> On Aug 25 2020, Adhemerval Zanella via Libc-alpha wrote: >> >>> On 25/08/2020 11:47, Andreas Schwab wrote: >>>> On Aug 25 2020, Lukasz Majewski wrote: >>>> >>>>> When I put futex_abstimed_wait_cancelable64() as static __always_inline >>>>> to futex-internals.h then the aforementioned problem on m68k emerge. >>>> >>>> But futex-helpers.c contains a single function. There is nothing to >>>> inline. >>>> >>>> Andreas. >>>> >>> >>> in_time_t_range and valid_timespec64_to_timespec. >> >> But you don't want to uninline them. > > Yes, that it seems exactly what is triggering gcc ICE. > And my suggestion to try workaround this issue is to move 32-bit futex call to its own function [1]. [1] https://sourceware.org/pipermail/libc-alpha/2020-August/117254.html
Hi Adhemerval, > 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. Yes, correct. I've already enabled the "gnu" coding style (c-set-style) in emacs. It automatically replaces some spaces with tabs. However, with some earlier patches - such approach was not accepted and using 2 spaces for indentation like: if (foo) { if (baz) flag = 1; } was recommended. Which indentation style is correct then? As the manual: https://www.gnu.org/prep/standards/standards.html did not say about it explicitly. Also here: https://sourceware.org/glibc/wiki/Style_and_Conventions it is not said explicitly. The only recommendation I had from Joseph on the very beginning of my involvement in the project: https://sourceware.org/pipermail/libc-alpha/2019-March/102274.html > > > --- > > 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. Ok. I will remove it > > > #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. Ok > > > + > > +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. Yes, I've double checked it now :-) > > > @@ -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. Yes, this change is not related to this change. However, IIRC the if (! futex*...) for '!' (a space after it) is the correct coding style. > > > > > 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. Yes. > > > 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. Ok. > > > @@ -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. > I've already done it. This works and linknamespace tests pass. > > +{ > > + 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. > Ok. > > +#ifndef __ASSUME_TIME64_SYSCALLS > > + if (err == -ENOSYS) > > + { > > + struct timespec ts32; > > > Identation seems of here. Ok. Fixed. > > > + 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. Ok. > > > 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. > Ok. > > 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. > Ok. > > +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. I've also tested it on my side - seems to work correctly. > > > 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. I will remove this file from this patch. Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
* Lukasz Majewski: >> Some comments below. As a general comment, I think you are using 8 >> whitespace instead of tab. > > Yes, correct. I've already enabled the "gnu" coding style > (c-set-style) in emacs. It automatically replaces some spaces with tabs. > > However, with some earlier patches - such approach was not accepted and > using 2 spaces for indentation like: > > if (foo) > { > if (baz) > flag = 1; > > } For new files, I prefer using spaces instead of tabs because the diffs show the correct indentation. With tabs, they sometimes do not because the leading /+/- does not shift a following tab one column right (like it would do for eight spaces). Thanks, Florian
On 26/08/2020 14:59, Lukasz Majewski wrote: > Hi Adhemerval, > [..] > was recommended. Which indentation style is correct then? As the manual: > https://www.gnu.org/prep/standards/standards.html > > did not say about it explicitly. > > Also here: > https://sourceware.org/glibc/wiki/Style_and_Conventions > > it is not said explicitly. > > The only recommendation I had from Joseph on the very beginning of my > involvement in the project: > https://sourceware.org/pipermail/libc-alpha/2019-March/102274.html I usually follow current file practice of two-column indentation with tab for 8 columns. But as Florian has added, new files could use whitespace instead of tabs. >> >>> @@ -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. > > Yes, this change is not related to this change. However, IIRC the if (! > futex*...) for '!' (a space after it) is the correct coding style. It not wrong indeed, but usually for style change it is better to send a separate patch.
Hi Adhemerval, > On 26/08/2020 14:59, Lukasz Majewski wrote: > > Hi Adhemerval, > > [..] > > was recommended. Which indentation style is correct then? As the > > manual: https://www.gnu.org/prep/standards/standards.html > > > > did not say about it explicitly. > > > > Also here: > > https://sourceware.org/glibc/wiki/Style_and_Conventions > > > > it is not said explicitly. > > > > The only recommendation I had from Joseph on the very beginning of > > my involvement in the project: > > https://sourceware.org/pipermail/libc-alpha/2019-March/102274.html > > I usually follow current file practice of two-column indentation with > tab for 8 columns. But as Florian has added, new files could use > whitespace instead of tabs. Thanks for clarification. I will use whitespaces where possible. > > >> > >>> @@ -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. > > > > Yes, this change is not related to this change. However, IIRC the > > if (! futex*...) for '!' (a space after it) is the correct coding > > style. > > It not wrong indeed, but usually for style change it is better to > send a separate patch. Ok. Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
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, @@ -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, 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> + #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); } @@ -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. */ @@ -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) + +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, @@ -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; 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); 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) 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 @@ -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) +{ + 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); +#ifndef __ASSUME_TIME64_SYSCALLS + if (err == -ENOSYS) + { + struct timespec ts32; + 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) 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 */ 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 +CFLAGS-futex-helpers.c += -fno-inline +endif 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>