Re: [PATCH v5] y2038: Introduce __ASSUME_TIME64_SYSCALLS define

[Stepan Golosunov <stepan at golosunov dot pp dot ru>]

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.