Re: [PATCH v5] y2038: Introduce __ASSUME_TIME64_SYSCALLS define

[Lukasz Majewski <lukma at denx dot de>]

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

Attachment: pgpPCvfgsikUQ.pgp
Description: OpenPGP digital signature