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

[Joseph Myers <joseph at codesourcery dot com>]

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, before the rest of this patch series can 
be properly reviewed.  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, and so it would be incorrect to define the macro in that case, 
but this patch defines it anyway.

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.

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).  Once 
the relevant abstraction is very clear, the reader can deduce the answer 
for each class of glibc ports.

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.  (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.)

-- 
Joseph S. Myers
joseph@codesourcery.com