Re: [PATCH] Y2038: add struct __timespec64

[Albert ARIBAUD <albert dot aribaud at 3adev dot fr>]

Hi Joseph,

On Wed, 19 Sep 2018 16:24:23 +0000, Joseph Myers
<joseph@codesourcery.com> wrote :

> We have, I think, made clear several times to the kernel people that the 
> best thing for glibc would be that, at the kernel/userspace ABI 
> boundaries, for ABIs where userspace long is 32-bit, only the low 32 bits 
> of the nanoseconds field should be considered significant when coming from 
> userspace to the kernel; the high bits should be considered as padding.  
> Will the kernel be implementing this?
> 
> If the kernel is implementing that, then there's no obvious need for this 
> structure with an explicit name for the padding at all, because there is 
> no need to copy 64-bit timespec values and zero the padding to pass them 
> into the kernel.  On the other hand, if the kernel is not implementing 
> that - if the kernel is treating the nanoseconds field as a 64-bit 
> integer, all bits significant, when it comes from userspace, then copying 
> and zeroing padding is necessary when passing timespec values to the 
> kernel.  So information about the kernel choice is critical information 
> for the glibc commit message to justify the particular choices made in 
> glibc.

I have just checked the Y2038 syscalls branch I am using currently, and
it seems to clear the 32 upper bits of tv_nsec when receiving it from
the userland, so yes, we actually don't need to clear it ourselves
(I'll have to update the design document, which still assumes
otherwise).
 
> Now if explicit zeroing is needed, the next question is whether the 
> internal type should have named padding, as here, or whether it should 
> have 64-bit nanoseconds.  In the first case, the zeroing when passing 
> values to the kernel would be setting tv_pad to 0; in the second case, it 
> would just be a tv_nsec assignment and the compiler would deal with 
> setting the high part appropriately.
> 
> While the POSIX requirements are relevant for the *public* struct timespec 
> with _TIME_BITS=64, they are less clearly relevant for the internal type.  
> In either case, if pointers to one type are converted to pointers to the 
> other, an explicit type cast will be needed as the types won't be 
> compatible (except in the case where time_t is already 64-bit and long is 
> 64-bit and __timespec64 can be #defined to timespec).

The Posix issue is indeed only on the public side, where computations
involving a tv_nsec may yield different effects if tv_nsec goes from
the possibly expected 32-bit long to an unexpected 64-bit signed int.

So I'll keep the public type with anonymous padding, but I will remove
padding and use 64 bits for tv_nsec in the internal type.

> Finally, having explained the overall nature of the changes in the patch, 
> and then gone into the details of the particular choices of interest made 
> (such as for the type of tv_nsec), the human-level message for any patch 
> should then detail the testing done on that patch.  This would include 
> testing (with the full glibc testsuite) for at least one 32-bit and one 
> 64-bit platform (for such 64-bit time patches; appropriate testing is very 
> much dependent on the particular patch, and for many patches outside this 
> area there's no need to test on more than one platform).  Then, after 
> that, the ChangeLog entry.

Ok.

Cordialement,
Albert ARIBAUD
3ADEV