[v2,2/3] Y2038: nptl: Provide futex_abstimed_wait64 supporting 64 bit time

  This is the helper function, which uses struct __timespec64
to provide 64 bit absolute time to futex syscalls.

The aim of this function is to move convoluted pre-processor
macro code from sysdeps/nptl/lowlevellock-futex.h to C
function in futex-internal.c

The futex_abstimed_wait64 function has been put into a separate
file on the purpose - to avoid issues apparent on the m68k
architecture related to small number of available registers (there
is not enough registers to put all necessary arguments in them if
the above function would be added to futex-internal.h with
__always_inline attribute).

Additional precautions for m68k port have been taken - the
futex-internal.c file will be compiled with -fno-inline flag.

---
Changes for v2:
- Handle the case when *abstime pointer is NULL
---
 sysdeps/nptl/futex-internal.c         | 70 +++++++++++++++++++++++++++
 sysdeps/nptl/futex-internal.h         |  6 +++
 sysdeps/unix/sysv/linux/m68k/Makefile |  2 +
 3 files changed, 78 insertions(+)
  

On Sat, Sep 19, 2020 at 6:08 AM Lukasz Majewski <lukma@denx.de> wrote:
>
> This is the helper function, which uses struct __timespec64
> to provide 64 bit absolute time to futex syscalls.
>
> The aim of this function is to move convoluted pre-processor
> macro code from sysdeps/nptl/lowlevellock-futex.h to C
> function in futex-internal.c
>
> The futex_abstimed_wait64 function has been put into a separate
> file on the purpose - to avoid issues apparent on the m68k
> architecture related to small number of available registers (there
> is not enough registers to put all necessary arguments in them if
> the above function would be added to futex-internal.h with
> __always_inline attribute).
>
> Additional precautions for m68k port have been taken - the
> futex-internal.c file will be compiled with -fno-inline flag.

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

>
> ---
> Changes for v2:
> - Handle the case when *abstime pointer is NULL
> ---
>  sysdeps/nptl/futex-internal.c         | 70 +++++++++++++++++++++++++++
>  sysdeps/nptl/futex-internal.h         |  6 +++
>  sysdeps/unix/sysv/linux/m68k/Makefile |  2 +
>  3 files changed, 78 insertions(+)
>
> diff --git a/sysdeps/nptl/futex-internal.c b/sysdeps/nptl/futex-internal.c
> index 3366aac162..3211b4c94f 100644
> --- a/sysdeps/nptl/futex-internal.c
> +++ b/sysdeps/nptl/futex-internal.c
> @@ -45,6 +45,29 @@ __futex_abstimed_wait_cancelable32 (unsigned int* futex_word,
>                                    abstime != NULL ? &ts32 : NULL,
>                                    NULL /* Unused.  */, FUTEX_BITSET_MATCH_ANY);
>  }
> +
> +static int
> +__futex_abstimed_wait32 (unsigned int* futex_word,
> +                         unsigned int expected, clockid_t clockid,
> +                         const struct __timespec64* abstime,
> +                         int private)
> +{
> +  struct timespec ts32;
> +
> +  if (abstime != NULL && ! 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);
> +
> +  if (abstime != NULL)
> +    ts32 = valid_timespec64_to_timespec (*abstime);
> +
> +  return INTERNAL_SYSCALL_CALL (futex, futex_word, op, expected,
> +                                abstime != NULL ? &ts32 : NULL,
> +                                NULL /* Unused.  */, FUTEX_BITSET_MATCH_ANY);
> +}
>  #endif
>
>  int
> @@ -97,3 +120,50 @@ __futex_abstimed_wait_cancelable64 (unsigned int* futex_word,
>        futex_fatal_error ();
>      }
>  }
> +
> +int
> +__futex_abstimed_wait64 (unsigned int* futex_word, unsigned int expected,
> +                         clockid_t clockid,
> +                         const struct __timespec64* abstime, int private)
> +{
> +  unsigned int clockbit;
> +  int err;
> +
> +  /* 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;
> +
> +  clockbit = (clockid == CLOCK_REALTIME) ? FUTEX_CLOCK_REALTIME : 0;
> +  int 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)
> +    err = __futex_abstimed_wait32 (futex_word, expected,
> +                                   clockid, abstime, private);
> +#endif
> +  switch (err)
> +    {
> +    case 0:
> +    case -EAGAIN:
> +    case -EINTR:
> +    case -ETIMEDOUT:
> +      return -err;
> +
> +    case -EFAULT: /* Must have been caused by a glibc or application bug.  */
> +    case -EINVAL: /* Either due to wrong alignment, unsupported
> +                    clockid 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 ();
> +    }
> +}
> diff --git a/sysdeps/nptl/futex-internal.h b/sysdeps/nptl/futex-internal.h
> index 7f3910ad98..1ba0d61938 100644
> --- a/sysdeps/nptl/futex-internal.h
> +++ b/sysdeps/nptl/futex-internal.h
> @@ -529,4 +529,10 @@ __futex_abstimed_wait_cancelable64 (unsigned int* futex_word,
>                                      const struct __timespec64* abstime,
>                                      int private) attribute_hidden;
>
> +int
> +__futex_abstimed_wait64 (unsigned int* futex_word, unsigned int expected,
> +                         clockid_t clockid,
> +                         const struct __timespec64* abstime,
> +                         int private) attribute_hidden;
> +
>  #endif  /* futex-internal.h */
> diff --git a/sysdeps/unix/sysv/linux/m68k/Makefile b/sysdeps/unix/sysv/linux/m68k/Makefile
> index be40fae68a..65164c5752 100644
> --- a/sysdeps/unix/sysv/linux/m68k/Makefile
> +++ b/sysdeps/unix/sysv/linux/m68k/Makefile
> @@ -21,3 +21,5 @@ sysdep-dl-routines += dl-static
>  sysdep-others += lddlibc4
>  install-bin += lddlibc4
>  endif
> +
> +CFLAGS-futex-internal.c += -fno-inline
> --
> 2.20.1
>
  

Dear Community,

> This is the helper function, which uses struct __timespec64
> to provide 64 bit absolute time to futex syscalls.
> 
> The aim of this function is to move convoluted pre-processor
> macro code from sysdeps/nptl/lowlevellock-futex.h to C
> function in futex-internal.c
> 
> The futex_abstimed_wait64 function has been put into a separate
> file on the purpose - to avoid issues apparent on the m68k
> architecture related to small number of available registers (there
> is not enough registers to put all necessary arguments in them if
> the above function would be added to futex-internal.h with
> __always_inline attribute).
> 
> Additional precautions for m68k port have been taken - the
> futex-internal.c file will be compiled with -fno-inline flag.
> 

Do we have more comments regarding this patch? Or shall I pull it to
-master?

> ---
> Changes for v2:
> - Handle the case when *abstime pointer is NULL
> ---
>  sysdeps/nptl/futex-internal.c         | 70
> +++++++++++++++++++++++++++ sysdeps/nptl/futex-internal.h         |
> 6 +++ sysdeps/unix/sysv/linux/m68k/Makefile |  2 +
>  3 files changed, 78 insertions(+)
> 
> diff --git a/sysdeps/nptl/futex-internal.c
> b/sysdeps/nptl/futex-internal.c index 3366aac162..3211b4c94f 100644
> --- a/sysdeps/nptl/futex-internal.c
> +++ b/sysdeps/nptl/futex-internal.c
> @@ -45,6 +45,29 @@ __futex_abstimed_wait_cancelable32 (unsigned int*
> futex_word, abstime != NULL ? &ts32 : NULL,
>                                    NULL /* Unused.  */,
> FUTEX_BITSET_MATCH_ANY); }
> +
> +static int
> +__futex_abstimed_wait32 (unsigned int* futex_word,
> +                         unsigned int expected, clockid_t clockid,
> +                         const struct __timespec64* abstime,
> +                         int private)
> +{
> +  struct timespec ts32;
> +
> +  if (abstime != NULL && ! 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); +
> +  if (abstime != NULL)
> +    ts32 = valid_timespec64_to_timespec (*abstime);
> +
> +  return INTERNAL_SYSCALL_CALL (futex, futex_word, op, expected,
> +                                abstime != NULL ? &ts32 : NULL,
> +                                NULL /* Unused.  */,
> FUTEX_BITSET_MATCH_ANY); +}
>  #endif
>  
>  int
> @@ -97,3 +120,50 @@ __futex_abstimed_wait_cancelable64 (unsigned int*
> futex_word, futex_fatal_error ();
>      }
>  }
> +
> +int
> +__futex_abstimed_wait64 (unsigned int* futex_word, unsigned int
> expected,
> +                         clockid_t clockid,
> +                         const struct __timespec64* abstime, int
> private) +{
> +  unsigned int clockbit;
> +  int err;
> +
> +  /* 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;
> +
> +  clockbit = (clockid == CLOCK_REALTIME) ? FUTEX_CLOCK_REALTIME : 0;
> +  int 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)
> +    err = __futex_abstimed_wait32 (futex_word, expected,
> +                                   clockid, abstime, private);
> +#endif
> +  switch (err)
> +    {
> +    case 0:
> +    case -EAGAIN:
> +    case -EINTR:
> +    case -ETIMEDOUT:
> +      return -err;
> +
> +    case -EFAULT: /* Must have been caused by a glibc or application
> bug.  */
> +    case -EINVAL: /* Either due to wrong alignment, unsupported
> +		     clockid 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 ();
> +    }
> +}
> diff --git a/sysdeps/nptl/futex-internal.h
> b/sysdeps/nptl/futex-internal.h index 7f3910ad98..1ba0d61938 100644
> --- a/sysdeps/nptl/futex-internal.h
> +++ b/sysdeps/nptl/futex-internal.h
> @@ -529,4 +529,10 @@ __futex_abstimed_wait_cancelable64 (unsigned
> int* futex_word, const struct __timespec64* abstime,
>                                      int private) attribute_hidden;
>  
> +int
> +__futex_abstimed_wait64 (unsigned int* futex_word, unsigned int
> expected,
> +                         clockid_t clockid,
> +                         const struct __timespec64* abstime,
> +                         int private) attribute_hidden;
> +
>  #endif  /* futex-internal.h */
> diff --git a/sysdeps/unix/sysv/linux/m68k/Makefile
> b/sysdeps/unix/sysv/linux/m68k/Makefile index be40fae68a..65164c5752
> 100644 --- a/sysdeps/unix/sysv/linux/m68k/Makefile
> +++ b/sysdeps/unix/sysv/linux/m68k/Makefile
> @@ -21,3 +21,5 @@ sysdep-dl-routines += dl-static
>  sysdep-others += lddlibc4
>  install-bin += lddlibc4
>  endif
> +
> +CFLAGS-futex-internal.c += -fno-inline




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 19/09/2020 10:07, Lukasz Majewski wrote:
> This is the helper function, which uses struct __timespec64
> to provide 64 bit absolute time to futex syscalls.
> 
> The aim of this function is to move convoluted pre-processor
> macro code from sysdeps/nptl/lowlevellock-futex.h to C
> function in futex-internal.c
> 
> The futex_abstimed_wait64 function has been put into a separate
> file on the purpose - to avoid issues apparent on the m68k
> architecture related to small number of available registers (there
> is not enough registers to put all necessary arguments in them if
> the above function would be added to futex-internal.h with
> __always_inline attribute).
> 
> Additional precautions for m68k port have been taken - the
> futex-internal.c file will be compiled with -fno-inline flag.

LGTM with a nit regarding style and a clarification about 
'-fno-inline'.

> 
> ---
> Changes for v2:
> - Handle the case when *abstime pointer is NULL
> ---
>  sysdeps/nptl/futex-internal.c         | 70 +++++++++++++++++++++++++++
>  sysdeps/nptl/futex-internal.h         |  6 +++
>  sysdeps/unix/sysv/linux/m68k/Makefile |  2 +
>  3 files changed, 78 insertions(+)
> 
> diff --git a/sysdeps/nptl/futex-internal.c b/sysdeps/nptl/futex-internal.c
> index 3366aac162..3211b4c94f 100644
> --- a/sysdeps/nptl/futex-internal.c
> +++ b/sysdeps/nptl/futex-internal.c
> @@ -45,6 +45,29 @@ __futex_abstimed_wait_cancelable32 (unsigned int* futex_word,
>                                    abstime != NULL ? &ts32 : NULL,
>                                    NULL /* Unused.  */, FUTEX_BITSET_MATCH_ANY);
>  }
> +
> +static int
> +__futex_abstimed_wait32 (unsigned int* futex_word,
> +                         unsigned int expected, clockid_t clockid,
> +                         const struct __timespec64* abstime,
> +                         int private)
> +{
> +  struct timespec ts32;
> +
> +  if (abstime != NULL && ! 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);
> +
> +  if (abstime != NULL)
> +    ts32 = valid_timespec64_to_timespec (*abstime);
> +
> +  return INTERNAL_SYSCALL_CALL (futex, futex_word, op, expected,
> +                                abstime != NULL ? &ts32 : NULL,
> +                                NULL /* Unused.  */, FUTEX_BITSET_MATCH_ANY);
> +}
>  #endif
>  
>  int

Ok.

> @@ -97,3 +120,50 @@ __futex_abstimed_wait_cancelable64 (unsigned int* futex_word,
>        futex_fatal_error ();
>      }
>  }
> +
> +int
> +__futex_abstimed_wait64 (unsigned int* futex_word, unsigned int expected,
> +                         clockid_t clockid,
> +                         const struct __timespec64* abstime, int private)
> +{
> +  unsigned int clockbit;
> +  int err;
> +
> +  /* 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;

Space before parentheses. 

> +
> +  clockbit = (clockid == CLOCK_REALTIME) ? FUTEX_CLOCK_REALTIME : 0;
> +  int 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)
> +    err = __futex_abstimed_wait32 (futex_word, expected,
> +                                   clockid, abstime, private);
> +#endif
> +  switch (err)
> +    {
> +    case 0:
> +    case -EAGAIN:
> +    case -EINTR:
> +    case -ETIMEDOUT:
> +      return -err;
> +
> +    case -EFAULT: /* Must have been caused by a glibc or application bug.  */
> +    case -EINVAL: /* Either due to wrong alignment, unsupported
> +		     clockid 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 ();
> +    }
> +}

Ok.

> diff --git a/sysdeps/nptl/futex-internal.h b/sysdeps/nptl/futex-internal.h
> index 7f3910ad98..1ba0d61938 100644
> --- a/sysdeps/nptl/futex-internal.h
> +++ b/sysdeps/nptl/futex-internal.h
> @@ -529,4 +529,10 @@ __futex_abstimed_wait_cancelable64 (unsigned int* futex_word,
>                                      const struct __timespec64* abstime,
>                                      int private) attribute_hidden;
>  
> +int
> +__futex_abstimed_wait64 (unsigned int* futex_word, unsigned int expected,
> +                         clockid_t clockid,
> +                         const struct __timespec64* abstime,
> +                         int private) attribute_hidden;
> +
>  #endif  /* futex-internal.h */

Ok.

> diff --git a/sysdeps/unix/sysv/linux/m68k/Makefile b/sysdeps/unix/sysv/linux/m68k/Makefile
> index be40fae68a..65164c5752 100644
> --- a/sysdeps/unix/sysv/linux/m68k/Makefile
> +++ b/sysdeps/unix/sysv/linux/m68k/Makefile
> @@ -21,3 +21,5 @@ sysdep-dl-routines += dl-static
>  sysdep-others += lddlibc4
>  install-bin += lddlibc4
>  endif
> +
> +CFLAGS-futex-internal.c += -fno-inline
> 

Do we still need it?
  

Hi Adhemerval,

> On 19/09/2020 10:07, Lukasz Majewski wrote:
> > This is the helper function, which uses struct __timespec64
> > to provide 64 bit absolute time to futex syscalls.
> > 
> > The aim of this function is to move convoluted pre-processor
> > macro code from sysdeps/nptl/lowlevellock-futex.h to C
> > function in futex-internal.c
> > 
> > The futex_abstimed_wait64 function has been put into a separate
> > file on the purpose - to avoid issues apparent on the m68k
> > architecture related to small number of available registers (there
> > is not enough registers to put all necessary arguments in them if
> > the above function would be added to futex-internal.h with
> > __always_inline attribute).
> > 
> > Additional precautions for m68k port have been taken - the
> > futex-internal.c file will be compiled with -fno-inline flag.  
> 
> LGTM with a nit regarding style and a clarification about 
> '-fno-inline'.

Please find the explanation below.

> 
> > 
> > ---
> > Changes for v2:
> > - Handle the case when *abstime pointer is NULL
> > ---
> >  sysdeps/nptl/futex-internal.c         | 70
> > +++++++++++++++++++++++++++ sysdeps/nptl/futex-internal.h         |
> >  6 +++ sysdeps/unix/sysv/linux/m68k/Makefile |  2 +
> >  3 files changed, 78 insertions(+)
> > 
> > diff --git a/sysdeps/nptl/futex-internal.c
> > b/sysdeps/nptl/futex-internal.c index 3366aac162..3211b4c94f 100644
> > --- a/sysdeps/nptl/futex-internal.c
> > +++ b/sysdeps/nptl/futex-internal.c
> > @@ -45,6 +45,29 @@ __futex_abstimed_wait_cancelable32 (unsigned
> > int* futex_word, abstime != NULL ? &ts32 : NULL,
> >                                    NULL /* Unused.  */,
> > FUTEX_BITSET_MATCH_ANY); }
> > +
> > +static int
> > +__futex_abstimed_wait32 (unsigned int* futex_word,
> > +                         unsigned int expected, clockid_t clockid,
> > +                         const struct __timespec64* abstime,
> > +                         int private)
> > +{
> > +  struct timespec ts32;
> > +
> > +  if (abstime != NULL && ! 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); +
> > +  if (abstime != NULL)
> > +    ts32 = valid_timespec64_to_timespec (*abstime);
> > +
> > +  return INTERNAL_SYSCALL_CALL (futex, futex_word, op, expected,
> > +                                abstime != NULL ? &ts32 : NULL,
> > +                                NULL /* Unused.  */,
> > FUTEX_BITSET_MATCH_ANY); +}
> >  #endif
> >  
> >  int  
> 
> Ok.
> 
> > @@ -97,3 +120,50 @@ __futex_abstimed_wait_cancelable64 (unsigned
> > int* futex_word, futex_fatal_error ();
> >      }
> >  }
> > +
> > +int
> > +__futex_abstimed_wait64 (unsigned int* futex_word, unsigned int
> > expected,
> > +                         clockid_t clockid,
> > +                         const struct __timespec64* abstime, int
> > private) +{
> > +  unsigned int clockbit;
> > +  int err;
> > +
> > +  /* 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;  
> 
> Space before parentheses. 

Ok. Fixed.

> 
> > +
> > +  clockbit = (clockid == CLOCK_REALTIME) ? FUTEX_CLOCK_REALTIME :
> > 0;
> > +  int 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)
> > +    err = __futex_abstimed_wait32 (futex_word, expected,
> > +                                   clockid, abstime, private);
> > +#endif
> > +  switch (err)
> > +    {
> > +    case 0:
> > +    case -EAGAIN:
> > +    case -EINTR:
> > +    case -ETIMEDOUT:
> > +      return -err;
> > +
> > +    case -EFAULT: /* Must have been caused by a glibc or
> > application bug.  */
> > +    case -EINVAL: /* Either due to wrong alignment, unsupported
> > +		     clockid 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 ();
> > +    }
> > +}  
> 
> Ok.
> 
> > diff --git a/sysdeps/nptl/futex-internal.h
> > b/sysdeps/nptl/futex-internal.h index 7f3910ad98..1ba0d61938 100644
> > --- a/sysdeps/nptl/futex-internal.h
> > +++ b/sysdeps/nptl/futex-internal.h
> > @@ -529,4 +529,10 @@ __futex_abstimed_wait_cancelable64 (unsigned
> > int* futex_word, const struct __timespec64* abstime,
> >                                      int private) attribute_hidden;
> >  
> > +int
> > +__futex_abstimed_wait64 (unsigned int* futex_word, unsigned int
> > expected,
> > +                         clockid_t clockid,
> > +                         const struct __timespec64* abstime,
> > +                         int private) attribute_hidden;
> > +
> >  #endif  /* futex-internal.h */  
> 
> Ok.
> 
> > diff --git a/sysdeps/unix/sysv/linux/m68k/Makefile
> > b/sysdeps/unix/sysv/linux/m68k/Makefile index
> > be40fae68a..65164c5752 100644 ---
> > a/sysdeps/unix/sysv/linux/m68k/Makefile +++
> > b/sysdeps/unix/sysv/linux/m68k/Makefile @@ -21,3 +21,5 @@
> > sysdep-dl-routines += dl-static sysdep-others += lddlibc4
> >  install-bin += lddlibc4
> >  endif
> > +
> > +CFLAGS-futex-internal.c += -fno-inline
> >   
> 
> Do we still need it?

Unfortunately, yes. The m68k architecture has the issue with properly
compiling this code (in futex-internal.c) as it has very limited number
of registers (8 x 32 bit IIRC).

I did some investigation and it looks like static inline
in_time_t_range() function affects the compiler capabilities to
generate correct code.

It can be easily inlined on any architecture but m68k.

As m68k has issues with this code compilation - it is IMHO better to
disable inlining (-fno-inline) on this particular port.

As a result we would have slower execution only on relevant functions,
but 64 bit time support would be provided. 


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 Sep 30 2020, Lukasz Majewski wrote:

> Unfortunately, yes. The m68k architecture has the issue with properly
> compiling this code (in futex-internal.c) as it has very limited number
> of registers (8 x 32 bit IIRC).

The m68k has 16 registers, which is plenty (x86 has only 8).

Andreas.
  

On 30/09/2020 09:47, Lukasz Majewski wrote:
> Hi Adhemerval,
> 
>> On 19/09/2020 10:07, Lukasz Majewski wrote:
>>> This is the helper function, which uses struct __timespec64
>>> to provide 64 bit absolute time to futex syscalls.
>>>
>>> The aim of this function is to move convoluted pre-processor
>>> macro code from sysdeps/nptl/lowlevellock-futex.h to C
>>> function in futex-internal.c
>>>
>>> The futex_abstimed_wait64 function has been put into a separate
>>> file on the purpose - to avoid issues apparent on the m68k
>>> architecture related to small number of available registers (there
>>> is not enough registers to put all necessary arguments in them if
>>> the above function would be added to futex-internal.h with
>>> __always_inline attribute).
>>>
>>> Additional precautions for m68k port have been taken - the
>>> futex-internal.c file will be compiled with -fno-inline flag.  
>>
>> LGTM with a nit regarding style and a clarification about 
>> '-fno-inline'.
> 
> Please find the explanation below.
> 
>>
>>>
>>> ---
>>> Changes for v2:
>>> - Handle the case when *abstime pointer is NULL
>>> ---
>>>  sysdeps/nptl/futex-internal.c         | 70
>>> +++++++++++++++++++++++++++ sysdeps/nptl/futex-internal.h         |
>>>  6 +++ sysdeps/unix/sysv/linux/m68k/Makefile |  2 +
>>>  3 files changed, 78 insertions(+)
>>>
>>> diff --git a/sysdeps/nptl/futex-internal.c
>>> b/sysdeps/nptl/futex-internal.c index 3366aac162..3211b4c94f 100644
>>> --- a/sysdeps/nptl/futex-internal.c
>>> +++ b/sysdeps/nptl/futex-internal.c
>>> @@ -45,6 +45,29 @@ __futex_abstimed_wait_cancelable32 (unsigned
>>> int* futex_word, abstime != NULL ? &ts32 : NULL,
>>>                                    NULL /* Unused.  */,
>>> FUTEX_BITSET_MATCH_ANY); }
>>> +
>>> +static int
>>> +__futex_abstimed_wait32 (unsigned int* futex_word,
>>> +                         unsigned int expected, clockid_t clockid,
>>> +                         const struct __timespec64* abstime,
>>> +                         int private)
>>> +{
>>> +  struct timespec ts32;
>>> +
>>> +  if (abstime != NULL && ! 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); +
>>> +  if (abstime != NULL)
>>> +    ts32 = valid_timespec64_to_timespec (*abstime);
>>> +
>>> +  return INTERNAL_SYSCALL_CALL (futex, futex_word, op, expected,
>>> +                                abstime != NULL ? &ts32 : NULL,
>>> +                                NULL /* Unused.  */,
>>> FUTEX_BITSET_MATCH_ANY); +}
>>>  #endif
>>>  
>>>  int  
>>
>> Ok.
>>
>>> @@ -97,3 +120,50 @@ __futex_abstimed_wait_cancelable64 (unsigned
>>> int* futex_word, futex_fatal_error ();
>>>      }
>>>  }
>>> +
>>> +int
>>> +__futex_abstimed_wait64 (unsigned int* futex_word, unsigned int
>>> expected,
>>> +                         clockid_t clockid,
>>> +                         const struct __timespec64* abstime, int
>>> private) +{
>>> +  unsigned int clockbit;
>>> +  int err;
>>> +
>>> +  /* 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;  
>>
>> Space before parentheses. 
> 
> Ok. Fixed.
> 
>>
>>> +
>>> +  clockbit = (clockid == CLOCK_REALTIME) ? FUTEX_CLOCK_REALTIME :
>>> 0;
>>> +  int 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)
>>> +    err = __futex_abstimed_wait32 (futex_word, expected,
>>> +                                   clockid, abstime, private);
>>> +#endif
>>> +  switch (err)
>>> +    {
>>> +    case 0:
>>> +    case -EAGAIN:
>>> +    case -EINTR:
>>> +    case -ETIMEDOUT:
>>> +      return -err;
>>> +
>>> +    case -EFAULT: /* Must have been caused by a glibc or
>>> application bug.  */
>>> +    case -EINVAL: /* Either due to wrong alignment, unsupported
>>> +		     clockid 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 ();
>>> +    }
>>> +}  
>>
>> Ok.
>>
>>> diff --git a/sysdeps/nptl/futex-internal.h
>>> b/sysdeps/nptl/futex-internal.h index 7f3910ad98..1ba0d61938 100644
>>> --- a/sysdeps/nptl/futex-internal.h
>>> +++ b/sysdeps/nptl/futex-internal.h
>>> @@ -529,4 +529,10 @@ __futex_abstimed_wait_cancelable64 (unsigned
>>> int* futex_word, const struct __timespec64* abstime,
>>>                                      int private) attribute_hidden;
>>>  
>>> +int
>>> +__futex_abstimed_wait64 (unsigned int* futex_word, unsigned int
>>> expected,
>>> +                         clockid_t clockid,
>>> +                         const struct __timespec64* abstime,
>>> +                         int private) attribute_hidden;
>>> +
>>>  #endif  /* futex-internal.h */  
>>
>> Ok.
>>
>>> diff --git a/sysdeps/unix/sysv/linux/m68k/Makefile
>>> b/sysdeps/unix/sysv/linux/m68k/Makefile index
>>> be40fae68a..65164c5752 100644 ---
>>> a/sysdeps/unix/sysv/linux/m68k/Makefile +++
>>> b/sysdeps/unix/sysv/linux/m68k/Makefile @@ -21,3 +21,5 @@
>>> sysdep-dl-routines += dl-static sysdep-others += lddlibc4
>>>  install-bin += lddlibc4
>>>  endif
>>> +
>>> +CFLAGS-futex-internal.c += -fno-inline
>>>   
>>
>> Do we still need it?
> 
> Unfortunately, yes. The m68k architecture has the issue with properly
> compiling this code (in futex-internal.c) as it has very limited number
> of registers (8 x 32 bit IIRC).
> 
> I did some investigation and it looks like static inline
> in_time_t_range() function affects the compiler capabilities to
> generate correct code.
> 
> It can be easily inlined on any architecture but m68k.
> 
> As m68k has issues with this code compilation - it is IMHO better to
> disable inlining (-fno-inline) on this particular port.
> 
> As a result we would have slower execution only on relevant functions,
> but 64 bit time support would be provided. 

I recall this was need on first patch for the cancellable 64-bit
futex_abstimed_wait where it embedded the 32-bit fallback in the 
__futex_abstimed_wait_cancelable64. And my suggestion to move it to 
an external function seems to avoid the extra compiler flag.

This patchset uses the same strategy and I checked both patches
and it seems that -fno-inline is not required to build m68k.
  

Hi Andreas,

> On Sep 30 2020, Lukasz Majewski wrote:
> 
> > Unfortunately, yes. The m68k architecture has the issue with
> > properly compiling this code (in futex-internal.c) as it has very
> > limited number of registers (8 x 32 bit IIRC).  
> 
> The m68k has 16 registers, which is plenty (x86 has only 8).

This is what I got from wiki (and no I'm not the expert with m68k, so
corrections are welcome):
https://en.wikipedia.org/wiki/Motorola_68000_series (8 x 32 bit data
registers)

And I cannot say why it is not able to generate the correct code...

The proposed flag (-fno-inline) seems to be the most pragmatic solution
(the code in question is in futex-interna.c and the flag only affects
this port).

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

Hi Adhemerval,

> On 30/09/2020 09:47, Lukasz Majewski wrote:
> > Hi Adhemerval,
> >   
> >> On 19/09/2020 10:07, Lukasz Majewski wrote:  
> >>> This is the helper function, which uses struct __timespec64
> >>> to provide 64 bit absolute time to futex syscalls.
> >>>
> >>> The aim of this function is to move convoluted pre-processor
> >>> macro code from sysdeps/nptl/lowlevellock-futex.h to C
> >>> function in futex-internal.c
> >>>
> >>> The futex_abstimed_wait64 function has been put into a separate
> >>> file on the purpose - to avoid issues apparent on the m68k
> >>> architecture related to small number of available registers (there
> >>> is not enough registers to put all necessary arguments in them if
> >>> the above function would be added to futex-internal.h with
> >>> __always_inline attribute).
> >>>
> >>> Additional precautions for m68k port have been taken - the
> >>> futex-internal.c file will be compiled with -fno-inline flag.    
> >>
> >> LGTM with a nit regarding style and a clarification about 
> >> '-fno-inline'.  
> > 
> > Please find the explanation below.
> >   
> >>  
> >>>
> >>> ---
> >>> Changes for v2:
> >>> - Handle the case when *abstime pointer is NULL
> >>> ---
> >>>  sysdeps/nptl/futex-internal.c         | 70
> >>> +++++++++++++++++++++++++++ sysdeps/nptl/futex-internal.h
> >>> | 6 +++ sysdeps/unix/sysv/linux/m68k/Makefile |  2 +
> >>>  3 files changed, 78 insertions(+)
> >>>
> >>> diff --git a/sysdeps/nptl/futex-internal.c
> >>> b/sysdeps/nptl/futex-internal.c index 3366aac162..3211b4c94f
> >>> 100644 --- a/sysdeps/nptl/futex-internal.c
> >>> +++ b/sysdeps/nptl/futex-internal.c
> >>> @@ -45,6 +45,29 @@ __futex_abstimed_wait_cancelable32 (unsigned
> >>> int* futex_word, abstime != NULL ? &ts32 : NULL,
> >>>                                    NULL /* Unused.  */,
> >>> FUTEX_BITSET_MATCH_ANY); }
> >>> +
> >>> +static int
> >>> +__futex_abstimed_wait32 (unsigned int* futex_word,
> >>> +                         unsigned int expected, clockid_t
> >>> clockid,
> >>> +                         const struct __timespec64* abstime,
> >>> +                         int private)
> >>> +{
> >>> +  struct timespec ts32;
> >>> +
> >>> +  if (abstime != NULL && ! 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); +
> >>> +  if (abstime != NULL)
> >>> +    ts32 = valid_timespec64_to_timespec (*abstime);
> >>> +
> >>> +  return INTERNAL_SYSCALL_CALL (futex, futex_word, op, expected,
> >>> +                                abstime != NULL ? &ts32 : NULL,
> >>> +                                NULL /* Unused.  */,
> >>> FUTEX_BITSET_MATCH_ANY); +}
> >>>  #endif
> >>>  
> >>>  int    
> >>
> >> Ok.
> >>  
> >>> @@ -97,3 +120,50 @@ __futex_abstimed_wait_cancelable64 (unsigned
> >>> int* futex_word, futex_fatal_error ();
> >>>      }
> >>>  }
> >>> +
> >>> +int
> >>> +__futex_abstimed_wait64 (unsigned int* futex_word, unsigned int
> >>> expected,
> >>> +                         clockid_t clockid,
> >>> +                         const struct __timespec64* abstime, int
> >>> private) +{
> >>> +  unsigned int clockbit;
> >>> +  int err;
> >>> +
> >>> +  /* 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;    
> >>
> >> Space before parentheses.   
> > 
> > Ok. Fixed.
> >   
> >>  
> >>> +
> >>> +  clockbit = (clockid == CLOCK_REALTIME) ? FUTEX_CLOCK_REALTIME :
> >>> 0;
> >>> +  int 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)
> >>> +    err = __futex_abstimed_wait32 (futex_word, expected,
> >>> +                                   clockid, abstime, private);
> >>> +#endif
> >>> +  switch (err)
> >>> +    {
> >>> +    case 0:
> >>> +    case -EAGAIN:
> >>> +    case -EINTR:
> >>> +    case -ETIMEDOUT:
> >>> +      return -err;
> >>> +
> >>> +    case -EFAULT: /* Must have been caused by a glibc or
> >>> application bug.  */
> >>> +    case -EINVAL: /* Either due to wrong alignment, unsupported
> >>> +		     clockid 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 ();
> >>> +    }
> >>> +}    
> >>
> >> Ok.
> >>  
> >>> diff --git a/sysdeps/nptl/futex-internal.h
> >>> b/sysdeps/nptl/futex-internal.h index 7f3910ad98..1ba0d61938
> >>> 100644 --- a/sysdeps/nptl/futex-internal.h
> >>> +++ b/sysdeps/nptl/futex-internal.h
> >>> @@ -529,4 +529,10 @@ __futex_abstimed_wait_cancelable64 (unsigned
> >>> int* futex_word, const struct __timespec64* abstime,
> >>>                                      int private)
> >>> attribute_hidden; 
> >>> +int
> >>> +__futex_abstimed_wait64 (unsigned int* futex_word, unsigned int
> >>> expected,
> >>> +                         clockid_t clockid,
> >>> +                         const struct __timespec64* abstime,
> >>> +                         int private) attribute_hidden;
> >>> +
> >>>  #endif  /* futex-internal.h */    
> >>
> >> Ok.
> >>  
> >>> diff --git a/sysdeps/unix/sysv/linux/m68k/Makefile
> >>> b/sysdeps/unix/sysv/linux/m68k/Makefile index
> >>> be40fae68a..65164c5752 100644 ---
> >>> a/sysdeps/unix/sysv/linux/m68k/Makefile +++
> >>> b/sysdeps/unix/sysv/linux/m68k/Makefile @@ -21,3 +21,5 @@
> >>> sysdep-dl-routines += dl-static sysdep-others += lddlibc4
> >>>  install-bin += lddlibc4
> >>>  endif
> >>> +
> >>> +CFLAGS-futex-internal.c += -fno-inline
> >>>     
> >>
> >> Do we still need it?  
> > 
> > Unfortunately, yes. The m68k architecture has the issue with
> > properly compiling this code (in futex-internal.c) as it has very
> > limited number of registers (8 x 32 bit IIRC).
> > 
> > I did some investigation and it looks like static inline
> > in_time_t_range() function affects the compiler capabilities to
> > generate correct code.
> > 
> > It can be easily inlined on any architecture but m68k.
> > 
> > As m68k has issues with this code compilation - it is IMHO better to
> > disable inlining (-fno-inline) on this particular port.
> > 
> > As a result we would have slower execution only on relevant
> > functions, but 64 bit time support would be provided.   
> 
> I recall this was need on first patch for the cancellable 64-bit
> futex_abstimed_wait where it embedded the 32-bit fallback in the 
> __futex_abstimed_wait_cancelable64. And my suggestion to move it to 
> an external function seems to avoid the extra compiler flag.
> 
> This patchset uses the same strategy and I checked both patches
> and it seems that -fno-inline is not required to build m68k.

As fair as I can tell (at the time I tested it) - it was necessary to
have -fno-inline when this patch was applied on top of the one which is
now in the -master branch (in futex-internal.c).

I will double check it now with:

glibc/glibc-many-build --keep failed glibcs


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 Wed, 30 Sep 2020 15:39:53 +0200
Lukasz Majewski <lukma@denx.de> wrote:

> Hi Adhemerval,
> 
> > On 30/09/2020 09:47, Lukasz Majewski wrote:  
> > > Hi Adhemerval,
> > >     
> > >> On 19/09/2020 10:07, Lukasz Majewski wrote:    
> > >>> This is the helper function, which uses struct __timespec64
> > >>> to provide 64 bit absolute time to futex syscalls.
> > >>>
> > >>> The aim of this function is to move convoluted pre-processor
> > >>> macro code from sysdeps/nptl/lowlevellock-futex.h to C
> > >>> function in futex-internal.c
> > >>>
> > >>> The futex_abstimed_wait64 function has been put into a separate
> > >>> file on the purpose - to avoid issues apparent on the m68k
> > >>> architecture related to small number of available registers
> > >>> (there is not enough registers to put all necessary arguments
> > >>> in them if the above function would be added to
> > >>> futex-internal.h with __always_inline attribute).
> > >>>
> > >>> Additional precautions for m68k port have been taken - the
> > >>> futex-internal.c file will be compiled with -fno-inline flag.
> > >>>    
> > >>
> > >> LGTM with a nit regarding style and a clarification about 
> > >> '-fno-inline'.    
> > > 
> > > Please find the explanation below.
> > >     
> > >>    
> > >>>
> > >>> ---
> > >>> Changes for v2:
> > >>> - Handle the case when *abstime pointer is NULL
> > >>> ---
> > >>>  sysdeps/nptl/futex-internal.c         | 70
> > >>> +++++++++++++++++++++++++++ sysdeps/nptl/futex-internal.h
> > >>> | 6 +++ sysdeps/unix/sysv/linux/m68k/Makefile |  2 +
> > >>>  3 files changed, 78 insertions(+)
> > >>>
> > >>> diff --git a/sysdeps/nptl/futex-internal.c
> > >>> b/sysdeps/nptl/futex-internal.c index 3366aac162..3211b4c94f
> > >>> 100644 --- a/sysdeps/nptl/futex-internal.c
> > >>> +++ b/sysdeps/nptl/futex-internal.c
> > >>> @@ -45,6 +45,29 @@ __futex_abstimed_wait_cancelable32 (unsigned
> > >>> int* futex_word, abstime != NULL ? &ts32 : NULL,
> > >>>                                    NULL /* Unused.  */,
> > >>> FUTEX_BITSET_MATCH_ANY); }
> > >>> +
> > >>> +static int
> > >>> +__futex_abstimed_wait32 (unsigned int* futex_word,
> > >>> +                         unsigned int expected, clockid_t
> > >>> clockid,
> > >>> +                         const struct __timespec64* abstime,
> > >>> +                         int private)
> > >>> +{
> > >>> +  struct timespec ts32;
> > >>> +
> > >>> +  if (abstime != NULL && ! 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); +
> > >>> +  if (abstime != NULL)
> > >>> +    ts32 = valid_timespec64_to_timespec (*abstime);
> > >>> +
> > >>> +  return INTERNAL_SYSCALL_CALL (futex, futex_word, op,
> > >>> expected,
> > >>> +                                abstime != NULL ? &ts32 : NULL,
> > >>> +                                NULL /* Unused.  */,
> > >>> FUTEX_BITSET_MATCH_ANY); +}
> > >>>  #endif
> > >>>  
> > >>>  int      
> > >>
> > >> Ok.
> > >>    
> > >>> @@ -97,3 +120,50 @@ __futex_abstimed_wait_cancelable64 (unsigned
> > >>> int* futex_word, futex_fatal_error ();
> > >>>      }
> > >>>  }
> > >>> +
> > >>> +int
> > >>> +__futex_abstimed_wait64 (unsigned int* futex_word, unsigned int
> > >>> expected,
> > >>> +                         clockid_t clockid,
> > >>> +                         const struct __timespec64* abstime,
> > >>> int private) +{
> > >>> +  unsigned int clockbit;
> > >>> +  int err;
> > >>> +
> > >>> +  /* 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;      
> > >>
> > >> Space before parentheses.     
> > > 
> > > Ok. Fixed.
> > >     
> > >>    
> > >>> +
> > >>> +  clockbit = (clockid == CLOCK_REALTIME) ?
> > >>> FUTEX_CLOCK_REALTIME : 0;
> > >>> +  int 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)
> > >>> +    err = __futex_abstimed_wait32 (futex_word, expected,
> > >>> +                                   clockid, abstime, private);
> > >>> +#endif
> > >>> +  switch (err)
> > >>> +    {
> > >>> +    case 0:
> > >>> +    case -EAGAIN:
> > >>> +    case -EINTR:
> > >>> +    case -ETIMEDOUT:
> > >>> +      return -err;
> > >>> +
> > >>> +    case -EFAULT: /* Must have been caused by a glibc or
> > >>> application bug.  */
> > >>> +    case -EINVAL: /* Either due to wrong alignment, unsupported
> > >>> +		     clockid 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 ();
> > >>> +    }
> > >>> +}      
> > >>
> > >> Ok.
> > >>    
> > >>> diff --git a/sysdeps/nptl/futex-internal.h
> > >>> b/sysdeps/nptl/futex-internal.h index 7f3910ad98..1ba0d61938
> > >>> 100644 --- a/sysdeps/nptl/futex-internal.h
> > >>> +++ b/sysdeps/nptl/futex-internal.h
> > >>> @@ -529,4 +529,10 @@ __futex_abstimed_wait_cancelable64
> > >>> (unsigned int* futex_word, const struct __timespec64* abstime,
> > >>>                                      int private)
> > >>> attribute_hidden; 
> > >>> +int
> > >>> +__futex_abstimed_wait64 (unsigned int* futex_word, unsigned int
> > >>> expected,
> > >>> +                         clockid_t clockid,
> > >>> +                         const struct __timespec64* abstime,
> > >>> +                         int private) attribute_hidden;
> > >>> +
> > >>>  #endif  /* futex-internal.h */      
> > >>
> > >> Ok.
> > >>    
> > >>> diff --git a/sysdeps/unix/sysv/linux/m68k/Makefile
> > >>> b/sysdeps/unix/sysv/linux/m68k/Makefile index
> > >>> be40fae68a..65164c5752 100644 ---
> > >>> a/sysdeps/unix/sysv/linux/m68k/Makefile +++
> > >>> b/sysdeps/unix/sysv/linux/m68k/Makefile @@ -21,3 +21,5 @@
> > >>> sysdep-dl-routines += dl-static sysdep-others += lddlibc4
> > >>>  install-bin += lddlibc4
> > >>>  endif
> > >>> +
> > >>> +CFLAGS-futex-internal.c += -fno-inline
> > >>>       
> > >>
> > >> Do we still need it?    
> > > 
> > > Unfortunately, yes. The m68k architecture has the issue with
> > > properly compiling this code (in futex-internal.c) as it has very
> > > limited number of registers (8 x 32 bit IIRC).
> > > 
> > > I did some investigation and it looks like static inline
> > > in_time_t_range() function affects the compiler capabilities to
> > > generate correct code.
> > > 
> > > It can be easily inlined on any architecture but m68k.
> > > 
> > > As m68k has issues with this code compilation - it is IMHO better
> > > to disable inlining (-fno-inline) on this particular port.
> > > 
> > > As a result we would have slower execution only on relevant
> > > functions, but 64 bit time support would be provided.     
> > 
> > I recall this was need on first patch for the cancellable 64-bit
> > futex_abstimed_wait where it embedded the 32-bit fallback in the 
> > __futex_abstimed_wait_cancelable64. And my suggestion to move it to 
> > an external function seems to avoid the extra compiler flag.
> > 
> > This patchset uses the same strategy and I checked both patches
> > and it seems that -fno-inline is not required to build m68k.  
> 
> As fair as I can tell (at the time I tested it) - it was necessary to
> have -fno-inline when this patch was applied on top of the one which
> is now in the -master branch (in futex-internal.c).
> 
> I will double check it now with:
> 
> glibc/glibc-many-build --keep failed glibcs

Indeed with current -master the issue is gone:

../src/scripts/build-many-glibcs.py
/home/lukma/work/glibc/glibc-many-build --keep failed glibcs
m68k-linux-gnu m68k-linux-gnu-coldfire m68k-linux-gnu-coldfire-soft
x86_64-linux-gnu

Is all OK with the -fno-inline removed.... Strange.

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




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