[v5] y2038: Introduce __ASSUME_TIME64_SYSCALLS define

  This define indicates if the Linux kernel (5.1+) provides syscalls supporting
64 bit versions of struct timespec and timeval.

For architectures with __WORDSIZE==64 and __TIMESIZE==64 (e.g. x86_64, aarch64)
this flag is never defined (as those already use 64 bit versions of struct
timespec and timeval).

The __ASSUME_TIME64_SYSCALLS shall be only defined on systems with
__WORDSIZE==32.

For x32 this flag is explicitly undefined as this architecture has
__WORDSIZE==32 with __TIMESIZE==64. Despite having __WORDSIZE==32 the x32
has support for 64 bit time values and hence needs to undefine
__ASSUME_TIME64_SYSCALLS flag.


* sysdeps/unix/sysv/linux/kernel-features.h: (__ASSUME_TIME64_SYSCALLS):
[__LINUX_KERNEL_VERSION >= 0x050100]: Define.
* sysdeps/unix/sysv/linux/x86_64/kernel-features.h (__ASSUME_TIME64_SYSCALLS):
#undef the __ASSUME_TIME64_SYSCALLS for x32 architecture

---
Changes for v5:
- Rewrite the in-code comment (x32 description more precise)
- Change patch description (for x32)

Changes for v4:
- Exclude this patch from the clock_settime64 patch series
- Rewrite the in-code comment
- Change patch description

Changes for v3:
- Provide more detailed and verbose description
- Change name to __ASSUME_TIME64_SYSCALLS
- Undefine __ASSUME_TIME64_SYSCALLS on x32

Changes for v2:
- New patch
---
 sysdeps/unix/sysv/linux/kernel-features.h        | 38 ++++++++++++++++++++++++
 sysdeps/unix/sysv/linux/x86_64/kernel-features.h |  7 +++++
 2 files changed, 45 insertions(+)
  

Dear All,

> This define indicates if the Linux kernel (5.1+) provides syscalls
> supporting 64 bit versions of struct timespec and timeval.
> 
> For architectures with __WORDSIZE==64 and __TIMESIZE==64 (e.g.
> x86_64, aarch64) this flag is never defined (as those already use 64
> bit versions of struct timespec and timeval).
> 
> The __ASSUME_TIME64_SYSCALLS shall be only defined on systems with
> __WORDSIZE==32.
> 
> For x32 this flag is explicitly undefined as this architecture has
> __WORDSIZE==32 with __TIMESIZE==64. Despite having __WORDSIZE==32 the
> x32 has support for 64 bit time values and hence needs to undefine
> __ASSUME_TIME64_SYSCALLS flag.
> 
> 
> * sysdeps/unix/sysv/linux/kernel-features.h:
> (__ASSUME_TIME64_SYSCALLS): [__LINUX_KERNEL_VERSION >= 0x050100]:
> Define.
> * sysdeps/unix/sysv/linux/x86_64/kernel-features.h
> (__ASSUME_TIME64_SYSCALLS): #undef the __ASSUME_TIME64_SYSCALLS for
> x32 architecture

Gentle ping on this patch. Do you have any more comments?

> 
> ---
> Changes for v5:
> - Rewrite the in-code comment (x32 description more precise)
> - Change patch description (for x32)
> 
> Changes for v4:
> - Exclude this patch from the clock_settime64 patch series
> - Rewrite the in-code comment
> - Change patch description
> 
> Changes for v3:
> - Provide more detailed and verbose description
> - Change name to __ASSUME_TIME64_SYSCALLS
> - Undefine __ASSUME_TIME64_SYSCALLS on x32
> 
> Changes for v2:
> - New patch
> ---
>  sysdeps/unix/sysv/linux/kernel-features.h        | 38
> ++++++++++++++++++++++++
> sysdeps/unix/sysv/linux/x86_64/kernel-features.h |  7 +++++ 2 files
> changed, 45 insertions(+)
> 
> diff --git a/sysdeps/unix/sysv/linux/kernel-features.h
> b/sysdeps/unix/sysv/linux/kernel-features.h index
> bc5c959f58..30e77dd213 100644 ---
> a/sysdeps/unix/sysv/linux/kernel-features.h +++
> b/sysdeps/unix/sysv/linux/kernel-features.h @@ -143,3 +143,41 @@
>     */
>  
>  #define __ASSUME_CLONE_DEFAULT 1
> +
> +#include <bits/wordsize.h>
> +#if __WORDSIZE != 64
> +/* Support for Linux kernel syscalls, which are able to handle 64 bit
> +   time on 32 bit systems (with 'long' and __WORDSIZE equal to 32
> bits). +
> +   Linux kernel, as of version 5.1, provides following set of
> syscalls,
> +   which accept data based on struct timespec and timeval with 64 bit
> +   tv_sec:
> +
> +   clock_gettime64
> +   clock_settime64
> +   clock_adjtime64
> +   clock_getres_time64
> +   clock_nanosleep_time64
> +   timer_gettime64
> +   timer_settime64
> +   timerfd_gettime64
> +   timerfd_settime64
> +   utimensat_time64
> +   pselect6_time64
> +   ppoll_time64
> +   io_pgetevents_time64
> +   recvmmsg_time64
> +   mq_timedsend_time64
> +   mq_timedreceive_time64
> +   semtimedop_time64
> +   rt_sigtimedwait_time64
> +   futex_time64
> +   sched_rr_get_interval_time64
> +
> +   Above syscalls are supposed to replace legacy ones, which handle
> 32
> +   bit version of struct timespec and timeval (i.e. without the '64'
> +   suffix).  */
> +# if __LINUX_KERNEL_VERSION >= 0x050100
> +#  define __ASSUME_TIME64_SYSCALLS 1
> +# endif
> +#endif
> diff --git a/sysdeps/unix/sysv/linux/x86_64/kernel-features.h
> b/sysdeps/unix/sysv/linux/x86_64/kernel-features.h index
> 26280f57ec..179a9ae932 100644 ---
> a/sysdeps/unix/sysv/linux/x86_64/kernel-features.h +++
> b/sysdeps/unix/sysv/linux/x86_64/kernel-features.h @@ -24,3 +24,10 @@
>  #endif
>  
>  #include_next <kernel-features.h>
> +
> +/* For x32, which is a special case in respect to 64 bit time support
> +   (it has __WORDSIZE==32 but __TIMESIZE==64), the
> +   __ASSUME_TIME64_SYSCALLS flag needs to be explicitly undefined.
> */ +#ifdef __ILP32__
> +# undef __ASSUME_TIME64_SYSCALLS
> +#endif




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
  

15.05.2019 в 16:27:23 +0200 Lukasz Majewski написал:
> This define indicates if the Linux kernel (5.1+) provides syscalls supporting
> 64 bit versions of struct timespec and timeval.
> 
> For architectures with __WORDSIZE==64 and __TIMESIZE==64 (e.g. x86_64, aarch64)
> this flag is never defined (as those already use 64 bit versions of struct
> timespec and timeval).
> 
> The __ASSUME_TIME64_SYSCALLS shall be only defined on systems with
> __WORDSIZE==32.
> 
> For x32 this flag is explicitly undefined as this architecture has
> __WORDSIZE==32 with __TIMESIZE==64. Despite having __WORDSIZE==32 the x32
> has support for 64 bit time values and hence needs to undefine
> __ASSUME_TIME64_SYSCALLS flag.

What is not clear is how architectures where syscalls like
clock_settime are already using 64-bit time_t are supposed to be
identified.  Last patch for clock_settime seems to be using

#if __WORDSIZE != 32 || !defined __NR_clock_settime64 && defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64

to select code for these architectures.

This seems too complicated and potentially buggy.  Why not just
define __ASSUME_TIME64_SYSCALLS for this case too and then use

#if defined __ASSUME_TIME64_SYSCALLS && !defined __NR_clock_settime64

?

Especially given that in most cases the only difference in resulting
code with __ASSUME_TIME64_SYSCALLS defined would be the name of the
constant used (__NR_clock_settime64 when it's defined,
__NR_clock_settime otherwise).
  

Hi Stepan,

> 15.05.2019 в 16:27:23 +0200 Lukasz Majewski написал:
> > This define indicates if the Linux kernel (5.1+) provides syscalls
> > supporting 64 bit versions of struct timespec and timeval.
> > 
> > For architectures with __WORDSIZE==64 and __TIMESIZE==64 (e.g.
> > x86_64, aarch64) this flag is never defined (as those already use
> > 64 bit versions of struct timespec and timeval).
> > 
> > The __ASSUME_TIME64_SYSCALLS shall be only defined on systems with
> > __WORDSIZE==32.
> > 
> > For x32 this flag is explicitly undefined as this architecture has
> > __WORDSIZE==32 with __TIMESIZE==64. Despite having __WORDSIZE==32
> > the x32 has support for 64 bit time values and hence needs to
> > undefine __ASSUME_TIME64_SYSCALLS flag.  
> 
> What is not clear is how architectures where syscalls like
> clock_settime are already using 64-bit time_t are supposed to be
> identified.  Last patch for clock_settime seems to be using
> 
> #if __WORDSIZE != 32 || !defined __NR_clock_settime64 && defined
> __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64
> 

The check has been taken from:
sysdeps/unix/sysv/linux/bits/statvfs.h (line 24).


Considering your above comment - we would need to introduce two
flags:

1. New, glibc global - __ARCH_HAVE_TIME64_SYSCALLS (other names
possible: __ARCH_SUPPORT_TIME64_SYSCALLS, __ARCH_TIME64_SUPPORT)

#if (__WORDSIZE == 32 \
     && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32))
#define __ARCH_HAVE_TIME64_SYSCALLS
#endif

2. __ASSUME_TIME64_SYSCALLS as it is in this patch (to indicate
kernel support after 5.1+).


The __clock_settime64() pseudo code:

...
tv_nsec check
...

#if __ARCH_HAVE_TIME64_SYSCALLS
# ifdef __NR_clock_settime64
  int ret = INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
#  ifdef __ASSUME_TIME64_SYSCALLS
  return ret;
#  else
  if (ret == 0 || errno != ENOSYS)
    return ret;
#  endif
# endif
  if (! in_time_t_range (tp->tv_sec))
    {
      __set_errno (EOVERFLOW);
      return -1;
    }  
  struct timespec ts32;
  valid_timespec64_to_timespec (tp, &ts32);
  return INLINE_SYSCALL_CALL (clock_settime, clock_id, &ts32);
#else
  /* x32, x86_64 */
  return INLINE_SYSCALL_CALL (clock_settime, clock_id, tp);
#endif

> to select code for these architectures.
> 
> This seems too complicated and potentially buggy.  Why not just
> define __ASSUME_TIME64_SYSCALLS for this case too and then use
> 
> #if defined __ASSUME_TIME64_SYSCALLS && !defined __NR_clock_settime64
> 

As fair as I understood the __ASSUME_TIME64_SYSCALLS was supposed to
indicate in-kernel support for syscalls (similar to __ASSUME_STATX) -
which would result in simpler execution paths. 

> ?
> 
> Especially given that in most cases the only difference in resulting
> code with __ASSUME_TIME64_SYSCALLS defined would be the name of the
> constant used (__NR_clock_settime64 when it's defined,
> __NR_clock_settime otherwise).

And also the case where __clock_settime64() is called from
__clock_settime().


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 Thu, 23 May 2019, Stepan Golosunov wrote:

> This seems too complicated and potentially buggy.  Why not just
> define __ASSUME_TIME64_SYSCALLS for this case too and then use

There are multiple reasonable choices that could be made for the semantics 
of __ASSUME_TIME64_SYSCALLS.

One is the choice in this patch series at present, that it relates to the 
new syscalls *with their given names*.  Another possible choice would be 
the one you suggest, that it relates to *any syscalls whose semantics 
match those of the new syscalls, possibly under different names*.

If we use the latter choice, I think it would be best also to have 
#defines of the new to the old names (such as #define __NR_clock_settime64 
__NR_clock_settime) so the various places that use 
__ASSUME_TIME64_SYSCALLS don't all need their own conditionals.

Also, if we use the latter choice, we need to be very careful about 
ensuring the old syscalls really do have the right semantics, and really 
do exist on all kernels with at least the configured minimum version.  For 
example, we must not claim that semtimedop_time64 is available simply 
because a platform's syscall interface always had 64-bit time, if there 
wasn't an equivalent older syscall, or the equivalent older syscall had 
different semantics or was introduced more recently than the minimum 
kernel version for that glibc build.  This might well mean defining the 
macro to relate to a smaller set of syscalls for which this property can 
be more readily verified.
  

Dear Stepan, Joseph,

> Hi Stepan,
> 
> > 15.05.2019 в 16:27:23 +0200 Lukasz Majewski написал:  
> > > This define indicates if the Linux kernel (5.1+) provides syscalls
> > > supporting 64 bit versions of struct timespec and timeval.
> > > 
> > > For architectures with __WORDSIZE==64 and __TIMESIZE==64 (e.g.
> > > x86_64, aarch64) this flag is never defined (as those already use
> > > 64 bit versions of struct timespec and timeval).
> > > 
> > > The __ASSUME_TIME64_SYSCALLS shall be only defined on systems with
> > > __WORDSIZE==32.
> > > 
> > > For x32 this flag is explicitly undefined as this architecture has
> > > __WORDSIZE==32 with __TIMESIZE==64. Despite having __WORDSIZE==32
> > > the x32 has support for 64 bit time values and hence needs to
> > > undefine __ASSUME_TIME64_SYSCALLS flag.    
> > 
> > What is not clear is how architectures where syscalls like
> > clock_settime are already using 64-bit time_t are supposed to be
> > identified.  Last patch for clock_settime seems to be using
> > 
> > #if __WORDSIZE != 32 || !defined __NR_clock_settime64 && defined
> > __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64
> >   
> 
> The check has been taken from:
> sysdeps/unix/sysv/linux/bits/statvfs.h (line 24).
> 
> 
> Considering your above comment - we would need to introduce two
> flags:
> 
> 1. New, glibc global - __ARCH_HAVE_TIME64_SYSCALLS (other names
> possible: __ARCH_SUPPORT_TIME64_SYSCALLS, __ARCH_TIME64_SUPPORT)
> 
> #if (__WORDSIZE == 32 \
>      && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32))
> #define __ARCH_HAVE_TIME64_SYSCALLS
> #endif

I would provide more verbose description of course, but in short the
__ARCH_HAVE_TIME64_SYSCALLS would indicate all the archs (with
__WORDSIZE == 32) which would have a chance to have support for 64 bit
time.

> 
> 2. __ASSUME_TIME64_SYSCALLS as it is in this patch (to indicate
> kernel support after 5.1+).
> 
> 
> The __clock_settime64() pseudo code:
> 
> ...
> tv_nsec check
> ...
> 
> #if __ARCH_HAVE_TIME64_SYSCALLS
> # ifdef __NR_clock_settime64
>   int ret = INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
> #  ifdef __ASSUME_TIME64_SYSCALLS
>   return ret;
> #  else
>   if (ret == 0 || errno != ENOSYS)
>     return ret;
> #  endif
> # endif
>   if (! in_time_t_range (tp->tv_sec))
>     {
>       __set_errno (EOVERFLOW);
>       return -1;
>     }  
>   struct timespec ts32;
>   valid_timespec64_to_timespec (tp, &ts32);
>   return INLINE_SYSCALL_CALL (clock_settime, clock_id, &ts32);
> #else
>   /* x32, x86_64 */
>   return INLINE_SYSCALL_CALL (clock_settime, clock_id, tp);
> #endif

What do you think about this idea?

> 
> > to select code for these architectures.
> > 
> > This seems too complicated and potentially buggy.  Why not just
> > define __ASSUME_TIME64_SYSCALLS for this case too and then use
> > 
> > #if defined __ASSUME_TIME64_SYSCALLS && !defined
> > __NR_clock_settime64 
> 
> As fair as I understood the __ASSUME_TIME64_SYSCALLS was supposed to
> indicate in-kernel support for syscalls (similar to __ASSUME_STATX) -
> which would result in simpler execution paths. 
> 
> > ?
> > 
> > Especially given that in most cases the only difference in resulting
> > code with __ASSUME_TIME64_SYSCALLS defined would be the name of the
> > constant used (__NR_clock_settime64 when it's defined,
> > __NR_clock_settime otherwise).  
> 
> And also the case where __clock_settime64() is called from
> __clock_settime().
> 
> 
> 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
  

23.05.2019 в 11:35:30 +0200 Lukasz Majewski написал:
> Hi Stepan,
> 
> > 15.05.2019 в 16:27:23 +0200 Lukasz Majewski написал:
> > > This define indicates if the Linux kernel (5.1+) provides syscalls
> > > supporting 64 bit versions of struct timespec and timeval.
> > > 
> > > For architectures with __WORDSIZE==64 and __TIMESIZE==64 (e.g.
> > > x86_64, aarch64) this flag is never defined (as those already use
> > > 64 bit versions of struct timespec and timeval).
> > > 
> > > The __ASSUME_TIME64_SYSCALLS shall be only defined on systems with
> > > __WORDSIZE==32.
> > > 
> > > For x32 this flag is explicitly undefined as this architecture has
> > > __WORDSIZE==32 with __TIMESIZE==64. Despite having __WORDSIZE==32
> > > the x32 has support for 64 bit time values and hence needs to
> > > undefine __ASSUME_TIME64_SYSCALLS flag.  
> > 
> > What is not clear is how architectures where syscalls like
> > clock_settime are already using 64-bit time_t are supposed to be
> > identified.  Last patch for clock_settime seems to be using
> > 
> > #if __WORDSIZE != 32 || !defined __NR_clock_settime64 && defined
> > __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64
> > 
> 
> The check has been taken from:
> sysdeps/unix/sysv/linux/bits/statvfs.h (line 24).
> 
> 
> Considering your above comment - we would need to introduce two
> flags:
> 
> 1. New, glibc global - __ARCH_HAVE_TIME64_SYSCALLS (other names
> possible: __ARCH_SUPPORT_TIME64_SYSCALLS, __ARCH_TIME64_SUPPORT)
> 
> #if (__WORDSIZE == 32 \
>      && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32))
> #define __ARCH_HAVE_TIME64_SYSCALLS
> #endif
> 
> 2. __ASSUME_TIME64_SYSCALLS as it is in this patch (to indicate
> kernel support after 5.1+).
> 
> 
> The __clock_settime64() pseudo code:
> 
> ...
> tv_nsec check
> ...
> 
> #if __ARCH_HAVE_TIME64_SYSCALLS
> # ifdef __NR_clock_settime64
>   int ret = INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
> #  ifdef __ASSUME_TIME64_SYSCALLS
>   return ret;
> #  else
>   if (ret == 0 || errno != ENOSYS)
>     return ret;
> #  endif
> # endif

>   if (! in_time_t_range (tp->tv_sec))
>     {
>       __set_errno (EOVERFLOW);
>       return -1;
>     }  
>   struct timespec ts32;
>   valid_timespec64_to_timespec (tp, &ts32);
>   return INLINE_SYSCALL_CALL (clock_settime, clock_id, &ts32);

This part needs to be guarded by
#ifndef __ASSUME_TIME64_SYSCALLS

Not only it would be unreacheable when __ASSUME_TIME64_SYSCALLS is
defined, but also
INLINE_SYSCALL_CALL (clock_settime, clock_id, &ts32)
won't be compilable on some architectures.
__NR_clock_settime does not exist on new 32-bit architectures.

> #else
>   /* x32, x86_64 */
>   return INLINE_SYSCALL_CALL (clock_settime, clock_id, tp);
> #endif
> 
> > to select code for these architectures.
> > 
> > This seems too complicated and potentially buggy.  Why not just
> > define __ASSUME_TIME64_SYSCALLS for this case too and then use
> > 
> > #if defined __ASSUME_TIME64_SYSCALLS && !defined __NR_clock_settime64
> > 
> 
> As fair as I understood the __ASSUME_TIME64_SYSCALLS was supposed to
> indicate in-kernel support for syscalls (similar to __ASSUME_STATX) -
> which would result in simpler execution paths.

I proposed that __ASSUME_TIME64_SYSCALLS means what __ASSUME_* usually
mean: fallback (to old syscalls with 32-bit time) is not needed and
must not be compiled.

Which should be equivalent to: either the 20 time64 syscalls are
always available or traditional kernel interfaces for the same
functionality are already using 64-bit time_t.

It also allows to have simple answer to question on which syscalls
should be used.  When __ASSUME_TIME64_SYSCALLS is not defined both new
time64 and traditional syscall with 32-bit time_t should be tried.
When __ASSUME_TIME64_SYSCALLS is defined then new time64 syscall
should be used if it exists in kernel headers and traditional syscalls
(they would have 64-bit time_t) should be used otherwise.

In most cases code can look like this:

#ifdef __ASSUME_TIME64_SYSCALLS
#ifndef __NR_clock_settime64
#define __NR_clock_settime64 __NR_clock_settime
#endif
INLINE_SYSCALL_CALL (clock_settime64, …)
#else
try clock_settime64, if that fails (whether compiletime or runtime)
convert data to 32-bit, call clock_settime
#endif

(Yes, for semtimedop it won't be that simple.  Because traditional
syscall isn't semtimedop in some cases.  This also assumes that the
20 time64 syscall names won't suddenly be added to kernel headers for
64-bit ABIs.)



Defining __ASSUME_TIME64_SYSCALLS to mean "20 time64 syscalls are
guaranteed to be present at compile and runtime" and
__ARCH_HAVE_TIME64_SYSCALLS to mean "20 time64 syscalls are providing
the 64-bit time_t syscalls" would also work. But then:

1. Names like __ARCH_HAVE_* are currently not used in glibc.
2. It's easy to confuse __ASSUME_TIME64_SYSCALLS with
   __ARCH_HAVE_TIME64_SYSCALLS.
3. Code would probably be more complicated.
4. Care should be taken that __ASSUME_TIME64_SYSCALLS is not checked
   when __ARCH_HAVE_TIME64_SYSCALLS is not defined.  So that
   __ASSUME_TIME64_SYSCALLS can be easily removed when glibc will
   cease to support pre-5.1 kernels.
5. If 4. is done then defining __ASSUME_TIME64_SYSCALLS on 64-bit
   architectures and x32 won't break anything.  Then when pre-5.1
   kernels are no longer supported __ASSUME_TIME64_SYSCALLS will be
   defined unconditionally and its removal will be trivial.


> > ?
> > 
> > Especially given that in most cases the only difference in resulting
> > code with __ASSUME_TIME64_SYSCALLS defined would be the name of the
> > constant used (__NR_clock_settime64 when it's defined,
> > __NR_clock_settime otherwise).
> 
> And also the case where __clock_settime64() is called from
> __clock_settime().

That part is supposed to be guarded by __TIMESIZE and is not related
to __ASSUME_TIME64_SYSCALLS.
  

Hi Stepan, Joseph,

Thank you for your input.

> 23.05.2019 в 11:35:30 +0200 Lukasz Majewski написал:
> > Hi Stepan,
> >   
> > > 15.05.2019 в 16:27:23 +0200 Lukasz Majewski написал:  
> > > > This define indicates if the Linux kernel (5.1+) provides
> > > > syscalls supporting 64 bit versions of struct timespec and
> > > > timeval.
> > > > 
> > > > For architectures with __WORDSIZE==64 and __TIMESIZE==64 (e.g.
> > > > x86_64, aarch64) this flag is never defined (as those already
> > > > use 64 bit versions of struct timespec and timeval).
> > > > 
> > > > The __ASSUME_TIME64_SYSCALLS shall be only defined on systems
> > > > with __WORDSIZE==32.
> > > > 
> > > > For x32 this flag is explicitly undefined as this architecture
> > > > has __WORDSIZE==32 with __TIMESIZE==64. Despite having
> > > > __WORDSIZE==32 the x32 has support for 64 bit time values and
> > > > hence needs to undefine __ASSUME_TIME64_SYSCALLS flag.    
> > > 
> > > What is not clear is how architectures where syscalls like
> > > clock_settime are already using 64-bit time_t are supposed to be
> > > identified.  Last patch for clock_settime seems to be using
> > > 
> > > #if __WORDSIZE != 32 || !defined __NR_clock_settime64 && defined
> > > __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64
> > >   
> > 
> > The check has been taken from:
> > sysdeps/unix/sysv/linux/bits/statvfs.h (line 24).
> > 
> > 
> > Considering your above comment - we would need to introduce two
> > flags:
> > 
> > 1. New, glibc global - __ARCH_HAVE_TIME64_SYSCALLS (other names
> > possible: __ARCH_SUPPORT_TIME64_SYSCALLS, __ARCH_TIME64_SUPPORT)
> > 
> > #if (__WORDSIZE == 32 \
> >      && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32))
> > #define __ARCH_HAVE_TIME64_SYSCALLS
> > #endif
> > 
> > 2. __ASSUME_TIME64_SYSCALLS as it is in this patch (to indicate
> > kernel support after 5.1+).
> > 
> > 
> > The __clock_settime64() pseudo code:
> > 
> > ...
> > tv_nsec check
> > ...
> > 
> > #if __ARCH_HAVE_TIME64_SYSCALLS
> > # ifdef __NR_clock_settime64
> >   int ret = INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
> > #  ifdef __ASSUME_TIME64_SYSCALLS
> >   return ret;
> > #  else
> >   if (ret == 0 || errno != ENOSYS)
> >     return ret;
> > #  endif
> > # endif  
> 
> >   if (! in_time_t_range (tp->tv_sec))
> >     {
> >       __set_errno (EOVERFLOW);
> >       return -1;
> >     }  
> >   struct timespec ts32;
> >   valid_timespec64_to_timespec (tp, &ts32);
> >   return INLINE_SYSCALL_CALL (clock_settime, clock_id, &ts32);  
> 
> This part needs to be guarded by
> #ifndef __ASSUME_TIME64_SYSCALLS
> 
> Not only it would be unreacheable when __ASSUME_TIME64_SYSCALLS is
> defined, but also
> INLINE_SYSCALL_CALL (clock_settime, clock_id, &ts32)
> won't be compilable on some architectures.
> __NR_clock_settime does not exist on new 32-bit architectures.

The above code path is added for two use cases:

1. The fallback on systems where both __NR_clock_settime and
__NR_clock_settime64 are defined, but for some reason the call to
latter syscall is not supported (by having older kernel than headers
used for glibc compilation).

2. The __TIMESIZE != 64 execution patch for 32 bit systems (the glibc
configuration to not support Y2038 time).

However, I do prefer the described below solution.

> 
> > #else
> >   /* x32, x86_64 */
> >   return INLINE_SYSCALL_CALL (clock_settime, clock_id, tp);
> > #endif
> >   
> > > to select code for these architectures.
> > > 
> > > This seems too complicated and potentially buggy.  Why not just
> > > define __ASSUME_TIME64_SYSCALLS for this case too and then use
> > > 
> > > #if defined __ASSUME_TIME64_SYSCALLS && !defined
> > > __NR_clock_settime64 
> > 
> > As fair as I understood the __ASSUME_TIME64_SYSCALLS was supposed to
> > indicate in-kernel support for syscalls (similar to __ASSUME_STATX)
> > - which would result in simpler execution paths.  
> 
> I proposed that __ASSUME_TIME64_SYSCALLS means what __ASSUME_* usually
> mean: fallback (to old syscalls with 32-bit time) is not needed and
> must not be compiled.
> 
> Which should be equivalent to: either the 20 time64 syscalls are
> always available or traditional kernel interfaces for the same
> functionality are already using 64-bit time_t.
> 

Shall the __ASSUME_TIME64_SYSCALLS be defined as:
(@ sysdeps/unix/sysv/linux/kernel-features.h):

#if __WORDSIZE == 32
# if __LINUX_KERNEL_VERSION >= 0x050100
#  define __ASSUME_TIME64_SYSCALLS 1
# endif
#endif

And also for __TIMESIZE==64
(@ include/time.h)

#if __TIMESIZE==64
# define __ASSUME_TIME64_SYSCALLS 1
#endif



Then the code would be (it is easier for me to understand the
execution paths when providing the pseudo code):

__clock_settime64(....)
{
...

#ifdef __ASSUME_TIME64_SYSCALLS
#  ifndef __NR_clock_settime64
#    define __NR_clock_settime64 __NR_clock_settime [1]
#  endif
  INLINE_SYSCALL_CALL (clock_settime64, …) [2]
#else [3]
  int ret = INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
  if (ret == 0 || errno != ENOSYS)
	return ret;
  struct timespec ts32;
  valid_timespec64_to_timespec (tp, &ts32);
  return INLINE_SYSCALL_CALL (clock_settime, clock_id, &ts32);	
#endif

}


Notes:
======

[1] - x32/x86_64 - The __NR_clock_settime64 is NOT defined. The syscall
itself is called in [2].

[1] - arm32 - linux kernel headers (used for glibc build) are not
providing the __NR_clock_settime64 for some reason.

[3] - systems with old headers or kernel. Also fallback code when
__TIMESIZE != 64 and the __clock_settime64() is called from
clock_settime() on legacy, non 64 bit time supporting systems.

> It also allows to have simple answer to question on which syscalls
> should be used.  When __ASSUME_TIME64_SYSCALLS is not defined both new
> time64 and traditional syscall with 32-bit time_t should be tried.
> When __ASSUME_TIME64_SYSCALLS is defined then new time64 syscall
> should be used if it exists in kernel headers and traditional syscalls
> (they would have 64-bit time_t) should be used otherwise.
> 
> In most cases code can look like this:
> 
> #ifdef __ASSUME_TIME64_SYSCALLS
> #ifndef __NR_clock_settime64
> #define __NR_clock_settime64 __NR_clock_settime
> #endif
> INLINE_SYSCALL_CALL (clock_settime64, …)
> #else
> try clock_settime64, if that fails (whether compiletime or runtime)
> convert data to 32-bit, call clock_settime
> #endif
> 
> (Yes, for semtimedop it won't be that simple.  Because traditional
> syscall isn't semtimedop in some cases.  This also assumes that the
> 20 time64 syscall names won't suddenly be added to kernel headers for
> 64-bit ABIs.)
> 
> 
> 
> Defining __ASSUME_TIME64_SYSCALLS to mean "20 time64 syscalls are
> guaranteed to be present at compile and runtime" and
> __ARCH_HAVE_TIME64_SYSCALLS to mean "20 time64 syscalls are providing
> the 64-bit time_t syscalls" would also work. But then:
> 
> 1. Names like __ARCH_HAVE_* are currently not used in glibc.
> 2. It's easy to confuse __ASSUME_TIME64_SYSCALLS with
>    __ARCH_HAVE_TIME64_SYSCALLS.
> 3. Code would probably be more complicated.
> 4. Care should be taken that __ASSUME_TIME64_SYSCALLS is not checked
>    when __ARCH_HAVE_TIME64_SYSCALLS is not defined.  So that
>    __ASSUME_TIME64_SYSCALLS can be easily removed when glibc will
>    cease to support pre-5.1 kernels.
> 5. If 4. is done then defining __ASSUME_TIME64_SYSCALLS on 64-bit
>    architectures and x32 won't break anything.  Then when pre-5.1
>    kernels are no longer supported __ASSUME_TIME64_SYSCALLS will be
>    defined unconditionally and its removal will be trivial.
> 

I think that your approach with __ASSUME_TIME64_SYSCALLS meaning:
"either the 20 time64 syscalls are always available or traditional
kernel interfaces for the same functionality are already using 64-bit
time_t." is better and shall be used as it provides more concise code.

Please correct me if I made a mistake in the above pseudo code and the
__ASSUME_TIME64_SYSCALLS definition itself. I've tried to "implement"
your idea.

> 
> > > ?
> > > 
> > > Especially given that in most cases the only difference in
> > > resulting code with __ASSUME_TIME64_SYSCALLS defined would be the
> > > name of the constant used (__NR_clock_settime64 when it's defined,
> > > __NR_clock_settime otherwise).  
> > 
> > And also the case where __clock_settime64() is called from
> > __clock_settime().  
> 
> That part is supposed to be guarded by __TIMESIZE and is not related
> to __ASSUME_TIME64_SYSCALLS.

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
  

Hi Stepan, Joseph,

> Hi Stepan, Joseph,
> 
> Thank you for your input.
> 
> > 23.05.2019 в 11:35:30 +0200 Lukasz Majewski написал:  
> > > Hi Stepan,
> > >     
> > > > 15.05.2019 в 16:27:23 +0200 Lukasz Majewski написал:    
> > > > > This define indicates if the Linux kernel (5.1+) provides
> > > > > syscalls supporting 64 bit versions of struct timespec and
> > > > > timeval.
> > > > > 
> > > > > For architectures with __WORDSIZE==64 and __TIMESIZE==64 (e.g.
> > > > > x86_64, aarch64) this flag is never defined (as those already
> > > > > use 64 bit versions of struct timespec and timeval).
> > > > > 
> > > > > The __ASSUME_TIME64_SYSCALLS shall be only defined on systems
> > > > > with __WORDSIZE==32.
> > > > > 
> > > > > For x32 this flag is explicitly undefined as this architecture
> > > > > has __WORDSIZE==32 with __TIMESIZE==64. Despite having
> > > > > __WORDSIZE==32 the x32 has support for 64 bit time values and
> > > > > hence needs to undefine __ASSUME_TIME64_SYSCALLS flag.      
> > > > 
> > > > What is not clear is how architectures where syscalls like
> > > > clock_settime are already using 64-bit time_t are supposed to be
> > > > identified.  Last patch for clock_settime seems to be using
> > > > 
> > > > #if __WORDSIZE != 32 || !defined __NR_clock_settime64 && defined
> > > > __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64
> > > >     
> > > 
> > > The check has been taken from:
> > > sysdeps/unix/sysv/linux/bits/statvfs.h (line 24).
> > > 
> > > 
> > > Considering your above comment - we would need to introduce two
> > > flags:
> > > 
> > > 1. New, glibc global - __ARCH_HAVE_TIME64_SYSCALLS (other names
> > > possible: __ARCH_SUPPORT_TIME64_SYSCALLS, __ARCH_TIME64_SUPPORT)
> > > 
> > > #if (__WORDSIZE == 32 \
> > >      && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32))
> > > #define __ARCH_HAVE_TIME64_SYSCALLS
> > > #endif
> > > 
> > > 2. __ASSUME_TIME64_SYSCALLS as it is in this patch (to indicate
> > > kernel support after 5.1+).
> > > 
> > > 
> > > The __clock_settime64() pseudo code:
> > > 
> > > ...
> > > tv_nsec check
> > > ...
> > > 
> > > #if __ARCH_HAVE_TIME64_SYSCALLS
> > > # ifdef __NR_clock_settime64
> > >   int ret = INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
> > > #  ifdef __ASSUME_TIME64_SYSCALLS
> > >   return ret;
> > > #  else
> > >   if (ret == 0 || errno != ENOSYS)
> > >     return ret;
> > > #  endif
> > > # endif    
> >   
> > >   if (! in_time_t_range (tp->tv_sec))
> > >     {
> > >       __set_errno (EOVERFLOW);
> > >       return -1;
> > >     }  
> > >   struct timespec ts32;
> > >   valid_timespec64_to_timespec (tp, &ts32);
> > >   return INLINE_SYSCALL_CALL (clock_settime, clock_id, &ts32);    
> > 
> > This part needs to be guarded by
> > #ifndef __ASSUME_TIME64_SYSCALLS
> > 
> > Not only it would be unreacheable when __ASSUME_TIME64_SYSCALLS is
> > defined, but also
> > INLINE_SYSCALL_CALL (clock_settime, clock_id, &ts32)
> > won't be compilable on some architectures.
> > __NR_clock_settime does not exist on new 32-bit architectures.  
> 
> The above code path is added for two use cases:
> 
> 1. The fallback on systems where both __NR_clock_settime and
> __NR_clock_settime64 are defined, but for some reason the call to
> latter syscall is not supported (by having older kernel than headers
> used for glibc compilation).
> 
> 2. The __TIMESIZE != 64 execution patch for 32 bit systems (the glibc
> configuration to not support Y2038 time).
> 
> However, I do prefer the described below solution.
> 
> >   
> > > #else
> > >   /* x32, x86_64 */
> > >   return INLINE_SYSCALL_CALL (clock_settime, clock_id, tp);
> > > #endif
> > >     
> > > > to select code for these architectures.
> > > > 
> > > > This seems too complicated and potentially buggy.  Why not just
> > > > define __ASSUME_TIME64_SYSCALLS for this case too and then use
> > > > 
> > > > #if defined __ASSUME_TIME64_SYSCALLS && !defined
> > > > __NR_clock_settime64   
> > > 
> > > As fair as I understood the __ASSUME_TIME64_SYSCALLS was supposed
> > > to indicate in-kernel support for syscalls (similar to
> > > __ASSUME_STATX)
> > > - which would result in simpler execution paths.    
> > 
> > I proposed that __ASSUME_TIME64_SYSCALLS means what __ASSUME_*
> > usually mean: fallback (to old syscalls with 32-bit time) is not
> > needed and must not be compiled.
> > 
> > Which should be equivalent to: either the 20 time64 syscalls are
> > always available or traditional kernel interfaces for the same
> > functionality are already using 64-bit time_t.
> >   
> 
> Shall the __ASSUME_TIME64_SYSCALLS be defined as:
> (@ sysdeps/unix/sysv/linux/kernel-features.h):
> 
> #if __WORDSIZE == 32
> # if __LINUX_KERNEL_VERSION >= 0x050100
> #  define __ASSUME_TIME64_SYSCALLS 1
> # endif
> #endif
> 
> And also for __TIMESIZE==64
> (@ include/time.h)
> 
> #if __TIMESIZE==64
> # define __ASSUME_TIME64_SYSCALLS 1
> #endif
> 
> 
> 
> Then the code would be (it is easier for me to understand the
> execution paths when providing the pseudo code):
> 
> __clock_settime64(....)
> {
> ...
> 
> #ifdef __ASSUME_TIME64_SYSCALLS
> #  ifndef __NR_clock_settime64
> #    define __NR_clock_settime64 __NR_clock_settime [1]
> #  endif
>   INLINE_SYSCALL_CALL (clock_settime64, …) [2]
> #else [3]
>   int ret = INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
>   if (ret == 0 || errno != ENOSYS)
> 	return ret;
>   struct timespec ts32;
>   valid_timespec64_to_timespec (tp, &ts32);
>   return INLINE_SYSCALL_CALL (clock_settime, clock_id, &ts32);	
> #endif
> 
> }
> 
> 
> Notes:
> ======
> 
> [1] - x32/x86_64 - The __NR_clock_settime64 is NOT defined. The
> syscall itself is called in [2].
> 
> [1] - arm32 - linux kernel headers (used for glibc build) are not
> providing the __NR_clock_settime64 for some reason.
> 
> [3] - systems with old headers or kernel. Also fallback code when
> __TIMESIZE != 64 and the __clock_settime64() is called from
> clock_settime() on legacy, non 64 bit time supporting systems.
> 
> > It also allows to have simple answer to question on which syscalls
> > should be used.  When __ASSUME_TIME64_SYSCALLS is not defined both
> > new time64 and traditional syscall with 32-bit time_t should be
> > tried. When __ASSUME_TIME64_SYSCALLS is defined then new time64
> > syscall should be used if it exists in kernel headers and
> > traditional syscalls (they would have 64-bit time_t) should be used
> > otherwise.
> > 
> > In most cases code can look like this:
> > 
> > #ifdef __ASSUME_TIME64_SYSCALLS
> > #ifndef __NR_clock_settime64
> > #define __NR_clock_settime64 __NR_clock_settime
> > #endif
> > INLINE_SYSCALL_CALL (clock_settime64, …)
> > #else
> > try clock_settime64, if that fails (whether compiletime or runtime)
> > convert data to 32-bit, call clock_settime
> > #endif
> > 
> > (Yes, for semtimedop it won't be that simple.  Because traditional
> > syscall isn't semtimedop in some cases.  This also assumes that the
> > 20 time64 syscall names won't suddenly be added to kernel headers
> > for 64-bit ABIs.)
> > 
> > 
> > 
> > Defining __ASSUME_TIME64_SYSCALLS to mean "20 time64 syscalls are
> > guaranteed to be present at compile and runtime" and
> > __ARCH_HAVE_TIME64_SYSCALLS to mean "20 time64 syscalls are
> > providing the 64-bit time_t syscalls" would also work. But then:
> > 
> > 1. Names like __ARCH_HAVE_* are currently not used in glibc.
> > 2. It's easy to confuse __ASSUME_TIME64_SYSCALLS with
> >    __ARCH_HAVE_TIME64_SYSCALLS.
> > 3. Code would probably be more complicated.
> > 4. Care should be taken that __ASSUME_TIME64_SYSCALLS is not checked
> >    when __ARCH_HAVE_TIME64_SYSCALLS is not defined.  So that
> >    __ASSUME_TIME64_SYSCALLS can be easily removed when glibc will
> >    cease to support pre-5.1 kernels.
> > 5. If 4. is done then defining __ASSUME_TIME64_SYSCALLS on 64-bit
> >    architectures and x32 won't break anything.  Then when pre-5.1
> >    kernels are no longer supported __ASSUME_TIME64_SYSCALLS will be
> >    defined unconditionally and its removal will be trivial.
> >   
> 
> I think that your approach with __ASSUME_TIME64_SYSCALLS meaning:
> "either the 20 time64 syscalls are always available or traditional
> kernel interfaces for the same functionality are already using 64-bit
> time_t." is better and shall be used as it provides more concise code.
> 
> Please correct me if I made a mistake in the above pseudo code and the
> __ASSUME_TIME64_SYSCALLS definition itself. I've tried to "implement"
> your idea.
> 
> >   
> > > > ?
> > > > 
> > > > Especially given that in most cases the only difference in
> > > > resulting code with __ASSUME_TIME64_SYSCALLS defined would be
> > > > the name of the constant used (__NR_clock_settime64 when it's
> > > > defined, __NR_clock_settime otherwise).    
> > > 
> > > And also the case where __clock_settime64() is called from
> > > __clock_settime().    
> > 
> > That part is supposed to be guarded by __TIMESIZE and is not related
> > to __ASSUME_TIME64_SYSCALLS.  
> 
> Ok.
> 

Could you share your thoughts regarding the above idea?
Thanks in advance.

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

On Sat, 25 May 2019, Stepan Golosunov wrote:

> In most cases code can look like this:
> 
> #ifdef __ASSUME_TIME64_SYSCALLS
> #ifndef __NR_clock_settime64
> #define __NR_clock_settime64 __NR_clock_settime
> #endif
> INLINE_SYSCALL_CALL (clock_settime64, …)
> #else
> try clock_settime64, if that fails (whether compiletime or runtime)
> convert data to 32-bit, call clock_settime
> #endif
> 
> (Yes, for semtimedop it won't be that simple.  Because traditional
> syscall isn't semtimedop in some cases.  This also assumes that the

If we follow this approach, it would be reasonable to say that 
__ASSUME_TIME64_SYSCALLS is *only* for those syscalls where the old 
syscall, on platforms that had 64-bit time in their syscall interface all 
along, is exactly equivalent (and to list that subset) - not for 
semtimedop.  Then there could be a separate __ASSUME_SEMTIMEDOP_TIME64.

> 20 time64 syscall names won't suddenly be added to kernel headers for
> 64-bit ABIs.)

Rather, that they won't be added *with new syscall numbers*.  If the 
kernel adds them as macro aliases for the old syscall numbers, code like 
that would work just fine.  If the kernel adds them as new syscall 
numbers, the definition of __ASSUME_TIME64_SYSCALLS (that assumes it can 
always be true for existing 64-bit syscall ABIs) would suddenly become 
wrong when using new kernel headers, because it would result in glibc code 
using new syscall numbers without regard to a possibly older runtime 
kernel version.
  

On Sun, 26 May 2019, Lukasz Majewski wrote:

> Shall the __ASSUME_TIME64_SYSCALLS be defined as:
> (@ sysdeps/unix/sysv/linux/kernel-features.h):
> 
> #if __WORDSIZE == 32
> # if __LINUX_KERNEL_VERSION >= 0x050100
> #  define __ASSUME_TIME64_SYSCALLS 1
> # endif
> #endif
> 
> And also for __TIMESIZE==64
> (@ include/time.h)
> 
> #if __TIMESIZE==64
> # define __ASSUME_TIME64_SYSCALLS 1
> #endif

__ASSUME_* should *only* be defined in kernel-features.h.  A definition in 
include/* would be incorrect.

It's not clear to me that __TIMESIZE == 64 would be the right condition 
(in kernel-features.h) for defining __ASSUME_TIME64_SYSCALLS independent 
of kernel version, because it would get wrong the case of 32-bit 
architectures with old kernel support and glibc support added in future 
with 64-bit time only (if we decide that any such future glibc port should 
use only 64-bit time in userspace, but without also requiring a new kernel 
for such a port).  Rather, __WORDSIZE == 64 (with a special case for x32) 
is, as previously discussed, closer to what we want (which is that the 
"long" in the syscall ABI is 64-bit, which corresponds to __WORDSIZE and 
the userspace "long" in all cases except for x32).

> Then the code would be (it is easier for me to understand the
> execution paths when providing the pseudo code):
> 
> __clock_settime64(....)
> {
> ...
> 
> #ifdef __ASSUME_TIME64_SYSCALLS
> #  ifndef __NR_clock_settime64
> #    define __NR_clock_settime64 __NR_clock_settime [1]
> #  endif
>   INLINE_SYSCALL_CALL (clock_settime64, …) [2]
> #else [3]
>   int ret = INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
>   if (ret == 0 || errno != ENOSYS)
> 	return ret;

This part of the code inside the #else needs to be conditional, as well, 
on "#ifdef __NR_clock_settime64".  (Unless we require very new kernel 
headers to build glibc.  But we obviously can't right now - there's no 
existing kernel release whose headers both include these syscalls and are 
suitable for testing glibc, because of the fds_bits namespace issue.)
  

Hi Joseph,

> On Sat, 25 May 2019, Stepan Golosunov wrote:
> 
> > In most cases code can look like this:
> > 
> > #ifdef __ASSUME_TIME64_SYSCALLS
> > #ifndef __NR_clock_settime64
> > #define __NR_clock_settime64 __NR_clock_settime
> > #endif
> > INLINE_SYSCALL_CALL (clock_settime64, …)
> > #else
> > try clock_settime64, if that fails (whether compiletime or runtime)
> > convert data to 32-bit, call clock_settime
> > #endif
> > 
> > (Yes, for semtimedop it won't be that simple.  Because traditional
> > syscall isn't semtimedop in some cases.  This also assumes that
> > the  
> 
> If we follow this approach, it would be reasonable to say that 
> __ASSUME_TIME64_SYSCALLS is *only* for those syscalls where the old 
> syscall, on platforms that had 64-bit time in their syscall interface
> all along, is exactly equivalent (and to list that subset) 

Yes, I do agree. Also those syscalls shall be explicitly listed in the
commit message and code (as comments).

> - not for 
> semtimedop.  Then there could be a separate
> __ASSUME_SEMTIMEDOP_TIME64.
> 
> > 20 time64 syscall names won't suddenly be added to kernel headers
> > for 64-bit ABIs.)  
> 
> Rather, that they won't be added *with new syscall numbers*.  If the 
> kernel adds them as macro aliases for the old syscall numbers, code
> like that would work just fine.  If the kernel adds them as new
> syscall numbers, the definition of __ASSUME_TIME64_SYSCALLS (that
> assumes it can always be true for existing 64-bit syscall ABIs) would
> suddenly become wrong when using new kernel headers, because it would
> result in glibc code using new syscall numbers without regard to a
> possibly older runtime kernel version.
> 

Maybe I'm missing something, but would there be any use case
(possibility) that for archs supporting 64 bit time ABI (and using e.g.
__NR_clock_settime now) the new syscalls (like __NR_clock_settime64),
with the same functionality, would be added with different numbers?

Why already perfectly working __NR_clock_settime would be replaced by
other syscall (like __NR_clock_settime64) not with alias but different
number (on e.g x86_64)?

I always thought that __NR_clock_settime64 would be added on archs with
__WORDSIZE==32 to allow them to work after Y2038 and hence there would
be no chance for such collision.


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, 29 May 2019, Lukasz Majewski wrote:

> Maybe I'm missing something, but would there be any use case
> (possibility) that for archs supporting 64 bit time ABI (and using e.g.
> __NR_clock_settime now) the new syscalls (like __NR_clock_settime64),
> with the same functionality, would be added with different numbers?

I don't see a use case for it, but it's one of many ideas that were 
discussed in the past (to have the same syscalls on all systems with the 
same numbers as far as possible).
  

Hi Joseph

> On Sun, 26 May 2019, Lukasz Majewski wrote:
> 
> > Shall the __ASSUME_TIME64_SYSCALLS be defined as:
> > (@ sysdeps/unix/sysv/linux/kernel-features.h):
> > 
> > #if __WORDSIZE == 32
> > # if __LINUX_KERNEL_VERSION >= 0x050100
> > #  define __ASSUME_TIME64_SYSCALLS 1
> > # endif
> > #endif
> > 
> > And also for __TIMESIZE==64
> > (@ include/time.h)
> > 
> > #if __TIMESIZE==64
> > # define __ASSUME_TIME64_SYSCALLS 1
> > #endif  
> 
> __ASSUME_* should *only* be defined in kernel-features.h.  A
> definition in include/* would be incorrect.

Ok.

> 
> It's not clear to me that __TIMESIZE == 64 would be the right
> condition (in kernel-features.h) for defining
> __ASSUME_TIME64_SYSCALLS independent of kernel version, because it
> would get wrong the case of 32-bit architectures with old kernel
> support and glibc support added in future with 64-bit time only (if
> we decide that any such future glibc port should use only 64-bit time
> in userspace, but without also requiring a new kernel for such a
> port). 

In the above case we would switch to __TIMESIZE == 64 in some point.

> Rather, __WORDSIZE == 64 (with a special case for x32) is, as
> previously discussed, closer to what we want (which is that the
> "long" in the syscall ABI is 64-bit, which corresponds to __WORDSIZE
> and the userspace "long" in all cases except for x32).
> 

Shall the __ASSUME_TIME64_SYSCALLS be defined as:
(@ sysdeps/unix/sysv/linux/kernel-features.h):

#if (__WORDSIZE == 32 && \
	((__LINUX_KERNEL_VERSION >= 0x050100 || \
	 (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64))) || \
    (__WORDSIZE == 64)
#  define __ASSUME_TIME64_SYSCALLS 1
# endif
#endif

The above statement supports:

- 32 bit __WORDSIZE devices with 64 bit time ABI syscalls after 5.1
  kernel (e.g. clock_settime64)

- x32 (which defines __SYSCALL_WORDSIZE == 64)

- 64 bit systems (with 64 bit time ABI) - the __WORDSIZE == 64

> > Then the code would be (it is easier for me to understand the
> > execution paths when providing the pseudo code):
> > 
> > __clock_settime64(....)
> > {
> > ...
> > 
> > #ifdef __ASSUME_TIME64_SYSCALLS
> > #  ifndef __NR_clock_settime64
> > #    define __NR_clock_settime64 __NR_clock_settime [1]
> > #  endif
> >   INLINE_SYSCALL_CALL (clock_settime64, …) [2]
> > #else [3]
> >   int ret = INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
> >   if (ret == 0 || errno != ENOSYS)
> > 	return ret;  
> 
> This part of the code inside the #else needs to be conditional, as
> well, on "#ifdef __NR_clock_settime64".  (Unless we require very new
> kernel headers to build glibc.  But we obviously can't right now -
> there's no existing kernel release whose headers both include these
> syscalls and are suitable for testing glibc, because of the fds_bits
> namespace issue.)
> 

I assume that the "#ifdef __NR_clock_settime64" would prevent from
unneeded call to clock_settime64 (as we would end up in the fallback
path) on setup with old kernel and glibc build with old headers (by
'old' I mean one not supporting 64 bit time).

Also it will prevent from described above situation where we do have
kernel 5.1+ but glibc is build with kernel headers older than 5.1.


The corrected version of clock_settime64:

__clock_settime64(....)
{
...

#ifdef __ASSUME_TIME64_SYSCALLS
#  ifndef __NR_clock_settime64
#    define __NR_clock_settime64 __NR_clock_settime [1]
#  endif
  INLINE_SYSCALL_CALL (clock_settime64, …) [2]
#else [3]
# ifdef __NR_clock_settime64
  int ret = INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
  if (ret == 0 || errno != ENOSYS)
	return ret;
# endif
  struct timespec ts32;
  valid_timespec64_to_timespec (tp, &ts32);
  return INLINE_SYSCALL_CALL (clock_settime, clock_id, &ts32);	
#endif



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, 29 May 2019, Lukasz Majewski wrote:

> Shall the __ASSUME_TIME64_SYSCALLS be defined as:
> (@ sysdeps/unix/sysv/linux/kernel-features.h):
> 
> #if (__WORDSIZE == 32 && \
> 	((__LINUX_KERNEL_VERSION >= 0x050100 || \
> 	 (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64))) || \
>     (__WORDSIZE == 64)
> #  define __ASSUME_TIME64_SYSCALLS 1
> # endif
> #endif

That's not correctly formatted (break lines before not after operators), 
but it seems like the right sort of idea.

> I assume that the "#ifdef __NR_clock_settime64" would prevent from
> unneeded call to clock_settime64 (as we would end up in the fallback

It would prevent a *compilation failure* from trying to call a syscall 
whose syscall number is not defined.