Re: [PATCH v5] y2038: Introduce __ASSUME_TIME64_SYSCALLS define

[Lukasz Majewski <lukma at denx dot de>]

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

Attachment: pgpf3ZlHlcvpr.pgp
Description: OpenPGP digital signature