Re: [PATCH v5] y2038: Introduce __ASSUME_TIME64_SYSCALLS define

[Lukasz Majewski <lukma at denx dot 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

Attachment: pgpNNF9mkTooO.pgp
Description: OpenPGP digital signature