[PATCH] [RFC] y2038: nptl: Convert pthread_cond_{clock|timed}wait to support 64 bit time

[PATCH] [RFC] y2038: nptl: Convert pthread_cond_{clock|timed}wait to support 64 bit time

[Lukasz Majewski]

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

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>
-- 
2.20.1

Re: [PATCH] [RFC] y2038: nptl: Convert pthread_cond_{clock|timed}wait to support 64 bit time

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 3394 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] [RFC] y2038: nptl: Convert pthread_cond_{clock|timed}wait to support 64 bit time

[Florian Weimer]

* 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

Re: [PATCH] [RFC] y2038: nptl: Convert pthread_cond_{clock|timed}wait to support 64 bit time

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 964 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] [RFC] y2038: nptl: Convert pthread_cond_{clock|timed}wait to support 64 bit time

[Andreas Schwab]

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.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] [RFC] y2038: nptl: Convert pthread_cond_{clock|timed}wait to support 64 bit time

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 1150 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] [RFC] y2038: nptl: Convert pthread_cond_{clock|timed}wait to support 64 bit time

[Andreas Schwab]

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.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] [RFC] y2038: nptl: Convert pthread_cond_{clock|timed}wait to support 64 bit time

[Adhemerval Zanella]

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.

Re: [PATCH] [RFC] y2038: nptl: Convert pthread_cond_{clock|timed}wait to support 64 bit time

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 1781 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] [RFC] y2038: nptl: Convert pthread_cond_{clock|timed}wait to support 64 bit time

[Andreas Schwab]

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.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] [RFC] y2038: nptl: Convert pthread_cond_{clock|timed}wait to support 64 bit time

[Adhemerval Zanella]

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.

Re: [PATCH] [RFC] y2038: nptl: Convert pthread_cond_{clock|timed}wait to support 64 bit time

[Andreas Schwab]

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.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] [RFC] y2038: nptl: Convert pthread_cond_{clock|timed}wait to support 64 bit time

[Adhemerval Zanella]

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.

Re: [PATCH] [RFC] y2038: nptl: Convert pthread_cond_{clock|timed}wait to support 64 bit time

[Adhemerval Zanella]

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

Re: [PATCH] [RFC] y2038: nptl: Convert pthread_cond_{clock|timed}wait to support 64 bit time

[Adhemerval Zanella]

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. 

Re: [PATCH] [RFC] y2038: nptl: Convert pthread_cond_{clock|timed}wait to support 64 bit time

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 19896 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] [RFC] y2038: nptl: Convert pthread_cond_{clock|timed}wait to support 64 bit time

[Florian Weimer]

* 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

Re: [PATCH] [RFC] y2038: nptl: Convert pthread_cond_{clock|timed}wait to support 64 bit time

[Adhemerval Zanella]

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.

Re: [PATCH] [RFC] y2038: nptl: Convert pthread_cond_{clock|timed}wait to support 64 bit time

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 2339 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]