Re: [PATCH v5] y2038: Introduce __ASSUME_TIME64_SYSCALLS define

[Lukasz Majewski <lukma at denx dot de>]

Hi Joseph

> On Sun, 26 May 2019, Lukasz Majewski wrote:
> 
> > 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  
> 
> __ASSUME_* should *only* be defined in kernel-features.h.  A
> definition in include/* would be incorrect.

Ok.

> 
> It's not clear to me that __TIMESIZE == 64 would be the right
> condition (in kernel-features.h) for defining
> __ASSUME_TIME64_SYSCALLS independent of kernel version, because it
> would get wrong the case of 32-bit architectures with old kernel
> support and glibc support added in future with 64-bit time only (if
> we decide that any such future glibc port should use only 64-bit time
> in userspace, but without also requiring a new kernel for such a
> port). 

In the above case we would switch to __TIMESIZE == 64 in some point.

> Rather, __WORDSIZE == 64 (with a special case for x32) is, as
> previously discussed, closer to what we want (which is that the
> "long" in the syscall ABI is 64-bit, which corresponds to __WORDSIZE
> and the userspace "long" in all cases except for x32).
> 

Shall the __ASSUME_TIME64_SYSCALLS be defined as:
(@ sysdeps/unix/sysv/linux/kernel-features.h):

#if (__WORDSIZE == 32 && \
	((__LINUX_KERNEL_VERSION >= 0x050100 || \
	 (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64))) || \
    (__WORDSIZE == 64)
#  define __ASSUME_TIME64_SYSCALLS 1
# endif
#endif

The above statement supports:

- 32 bit __WORDSIZE devices with 64 bit time ABI syscalls after 5.1
  kernel (e.g. clock_settime64)

- x32 (which defines __SYSCALL_WORDSIZE == 64)

- 64 bit systems (with 64 bit time ABI) - the __WORDSIZE == 64

> > 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;  
> 
> This part of the code inside the #else needs to be conditional, as
> well, on "#ifdef __NR_clock_settime64".  (Unless we require very new
> kernel headers to build glibc.  But we obviously can't right now -
> there's no existing kernel release whose headers both include these
> syscalls and are suitable for testing glibc, because of the fds_bits
> namespace issue.)
> 

I assume that the "#ifdef __NR_clock_settime64" would prevent from
unneeded call to clock_settime64 (as we would end up in the fallback
path) on setup with old kernel and glibc build with old headers (by
'old' I mean one not supporting 64 bit time).

Also it will prevent from described above situation where we do have
kernel 5.1+ but glibc is build with kernel headers older than 5.1.


The corrected version of clock_settime64:

__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]
# ifdef __NR_clock_settime64
  int ret = INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
  if (ret == 0 || errno != ENOSYS)
	return ret;
# endif
  struct timespec ts32;
  valid_timespec64_to_timespec (tp, &ts32);
  return INLINE_SYSCALL_CALL (clock_settime, clock_id, &ts32);	
#endif



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: pgpY4ocORSbAS.pgp
Description: OpenPGP digital signature