Re: [PATCH v5] y2038: Introduce __ASSUME_TIME64_SYSCALLS define

[Lukasz Majewski <lukma at denx dot de>]

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

Attachment: pgpYAjkNBgHN3.pgp
Description: OpenPGP digital signature