Re: [PATCH v2 2/7] y2038: Introduce __ASSUME_64BIT_TIME define

[Lukasz Majewski <lukma at denx dot de>]

Hi Joseph,

Thanks for your reply.

> On Mon, 29 Apr 2019, Lukasz Majewski wrote:
> 
> > +/* Support for 64 bit version of clock_* Linux syscalls.
> > +
> > +   Support for following time related (and Y2038 safe) syscalls
> > has been added
> > +   in the 5.1 Linux kernel:
> > +
> > +   clock_gettime64 (nr. 403)
> > +   clock_settime64 (nr. 404)
> > +   clock_getres_time64 (nr. 406)
> > +   clock_nanosleep_time64 (nr. 407)
> > +  */
> > +#if __LINUX_KERNEL_VERSION >= 0x050100
> > +# define __ASSUME_64BIT_TIME 1
> > +#endif  
> 
> This comment and macro definition are the key thing that need
> reviewing, probably over several iterations,

Ok. Please find my comments/concerns below regarding this __ASSUME
define.

> before the rest of this
> patch series can be properly reviewed.

I would like to point out one thing - the rest of this patch series
also has an important goal - reviewing them would set the direction
(despite the __ASSUME discussion) for future Y2038 development and
conversion of other parts of glibc.

For example:

 - The decisions about the shape of internal/external struct timespec
   in glibc.

 - The need for explicit clearing padding when calling syscalls (as to
   be better safe than sorry in the future - there was related
   discussion started by Stepan).

 - If the conversion itself (__clock_settime64 vs clock_settime) is
   correct/acceptable.

Would greatly facilitate the development process and reduce the number
of iterations.

>  It is critical that the
> comment is completely clear and unambiguous about the exact macro
> semantics on the various relevant classes of architectures.
> 
> See what I wrote in 
> <https://sourceware.org/ml/libc-alpha/2018-09/msg00448.html> and 
> <https://sourceware.org/ml/libc-alpha/2018-12/msg00568.html>.  I
> don't think the comment meets those requirements at present - that
> is, if you try to deduce from it what the macro definition should be
> on all the listed classes of architectures, either the conclusion is
> not clear or it sometimes conflicts with the actual definition.
> 
> In particular, for existing 64-bit architectures, my understanding is
> that the kernel *does not* add new syscall names or numbers for the
> syscalls you list,

With the 5.1-rc6 it seems like 64 bit architectures are not add those
syscalls (like e.g. clock_settime64).

> and so it would be incorrect to define the macro
> in that case, but this patch defines it anyway.

You are right here - the 

#if __TIMESIZE != 64
# if __LINUX_KERNEL_VERSION >= 0x050100
#  define __ASSUME_64BIT_TIME 1
# endif
#endif

would do the trick.

> 
> Note 1: the comment should not reference the above URLs; it should be 
> self-contained.  As stated in the second message above, it needs to
> be clear to people who haven't read any of the mailing list
> discussions around Y2038 issues.

Ok. I will prepare self contained comment.

> 
> Note 2: if the comment actually needs to define the classes 1a, 1b,
> 2a, 2b, 3, it's probably using the wrong abstractions.  It should be
> very careful to refer to the abstraction that actually most reliably
> determines the presence or absence of the new syscalls (which might
> be the size of "long int" used in the syscall interface - glibc's
> __syscall_slong_t, which happens always to be the same size as
> __TIMESIZE for existing glibc ports but might not be for future ports
> - but make sure of that). 

The ABI (syscalls) compatibility was one of the concerns raised in this
patch series as a whole.

The glibc's internal struct __timespec64 passed to e.g. clock_settime64
syscall has explicit __time64_t (tv_sec), __int32_t (tv_nsec) and 32 bit
padding.


> Once the relevant abstraction is very
> clear, the reader can deduce the answer for each class of glibc ports.

IMHO, the abstraction would be:

1. The __ASSUME_64BIT_TIME is _never_ defined for 64 bit native systems

2. It is defined by default in:
sysdeps/unix/sysv/linux/kernel-features.h for 32 bit systems (and the
actual presence of the syscall is decided upon definitions of __NR_xxx*
(i.e. # ifdef __NR_clock_settime64).

As those syscalls are provided on almost every 32 bit system now
(5.1-rc6):
git grep -n "clock_settime64"

gives support for: arm, arm64 (compat mode), m68k, microblaze, mips,
parisc, powerpc, s390, sh, sparc, x86, xtensa

So it would be reasonable to just add this __ASSUME definition code to
sysdeps/unix/sysv/linux/kernel-features.h and #undef it for
architectures not supporting it (i.e. c-sky and riscv).

> 
> Note 3: it's wrong to state the syscall numbers in the comment; that
> is not a relevant part of understanding the interface.  Stating the
> names, however, makes sense, provided you make sure not to use the
> __ASSUME_ macro for any *other* syscalls without updating the
> comment, and, at that time, reviewing whether the same definition
> conditions still work for all those syscalls. 

Correct, the names of supported syscalls also shall be written to the
comment (and the comment itself shall be extended with other, upcoming
patches).

> (Given that, as
> previously discussed, there might be *some* new syscalls even for
> architectures that already have 64-bit time, in order to provide
> timespec-based versions of syscalls currently using timeval.)
> 



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