[RFC] y2038: nptl: Convert pthread_cond_{clock|timed}wait to support 64 bit time
Commit Message
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.
---
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
Comments
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
@@ -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,
@@ -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);
@@ -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)
new file mode 100644
@@ -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)
@@ -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 */
@@ -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
new file mode 100644
@@ -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>