Re: [RFC v3 3/3] sys/types.h: Make snseconds_t user visible

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 08/12/2021 18:53, Alejandro Colomar (man-pages) wrote:
> Hi Adhemerval, Paul,
> 
> On 12/8/21 22:12, Adhemerval Zanella wrote:
>> We can even be more clever and use something similar to what musl does:
>>
>> diff --git a/time/bits/types/struct_timespec.h b/time/bits/types/struct_timespec.h
>> index 489e81136d..b962d3f95f 100644
>> --- a/time/bits/types/struct_timespec.h
>> +++ b/time/bits/types/struct_timespec.h
>> @@ -15,19 +15,9 @@ struct timespec
>>   #else
>>     __time_t tv_sec;             /* Seconds.  */
>>   #endif
>> -#if __WORDSIZE == 64 \
>> -  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64) \
>> -  || (__TIMESIZE == 32 && !defined __USE_TIME_BITS64)
>> -  __syscall_slong_t tv_nsec;   /* Nanoseconds.  */
>> -#else
>> -# if __BYTE_ORDER == __BIG_ENDIAN
>> -  int: 32;           /* Padding.  */
>> -  long int tv_nsec;  /* Nanoseconds.  */
>> -# else
>> -  long int tv_nsec;  /* Nanoseconds.  */
>> -  int: 32;           /* Padding.  */
>> -# endif
>> -#endif
>> +  int : 8 * (sizeof (time_t) - sizeof (long)) * (__BYTE_ORDER == __BIG_ENDIAN);
>> +  long int tv_nsec;
>> +  int : 8 * (sizeof (time_t) - sizeof (long)) * (__BYTE_ORDER == __LITTLE_ENDIAN);
>>   };
>>     #endif
>>
>>>
>>> and report back on the minimum kernel version for which this works correctly.
>>
>> The above does not show any regression on a recent 5.13, but I am not sure which
>> is the minimal version it does work correctly on x32.
>>
>> I don't mind bump the minimal kernel version for x32, however it would be good
>> to check with x86 maintainers.
> 
> If that works, this seems to me a clean workaround to the X32 bug (IMO both spec and kernel bug, in others' opinions kernel bug, in others' opinions spec bug, but we agree there's a bug).  We simply forget the bug ever existed, and hope no one triggers it again in a future implementation.  In the manual page we can add a small bug note for past implementations, noting that it doesn't apply anymore; instead of a prominent note that some implementations are currently buggy, which would be much worse.

It is clearly a bug [1], which was initially closed as not-implement but
I think we should fix it anyway.  I have sent a proposed fix [2], which
should cover old kernels that do not sanitize the high bits.

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=16437
[2] https://sourceware.org/pipermail/libc-alpha/2021-December/133919.html

> 
>>
>>>
>>> (I would still want the type changed in the standards, but only as futureproofing.)
>>>
> 
> Regarding this, I completely agree with Zack, that this is a bug spec, and since the standards simply wrote in stone existing practice, we can only conclude that existing practice wasn't ideal.  Having a contract so specific about the type of tv_nsec is unnecessary and in general C has moved in the other direction: use int or typedefs almost everywhere; rare are the interfaces that use long (and even more rarely or never short), and usually justified.  This case is just an accident of history.
> 
> I also see the value of keeping some accidents just for the sake of not accidentally making bigger accidents, and C has traditionally done this, so I could live with 'long' for tv_nsec, even if it is wrong.
> 
> In this case, in principle, snseconds_t would be backwards compatible, and it would allow future implementations (say 100 years from now?) to move to int if needed (maybe for ABI stability, maybe for other future reasons that we cannot foresee), or would allow to keep it defined as long forever if API stability is god.
> 
> But if you disagree with that, the above changes seem reasonable and enough to me.

Sorry but I still see no reason to push for snseconds_t, nor __snseconds_t.
The *main* motivation is clearly a *bad* decision to use a non-defined type 
on x32 for the sake of avoid fixing it on glibc (which I give you back then 
would require a lot of code changes).  Now that we are push for more code 
unification, the possible fix is not that complex.

Also the type is already futureproof, unless you have a future ABI that defined
long with less than 32-bit (which I think it will hit another issues than this
one).  Different time_t, the tv_nsec is really bounded by hard constraint (it
need to hold nanoseconds, instead of an unbounded time in future).

> 
> Paul:
> 
> On 12/8/21 19:12, Paul Eggert wrote:
>>
>> I'm still not seeing the need for a user-visible type "snseconds_t".
>>
> 
> I hope Zack and I made our reasons visible to you in our latest (and this) emails.
> 
>> In response to the earlier proposal "[RFC v2 1/2] sys/types.h: Define
>> new type: snseconds_t", Joseph suggested omitting snseconds_t[1], I
>> supported this suggestion[2], and I don't recall seeing any statement
>> explaining why this type needs to be user-visible.
> 
> Joseph suggested omitting it, and also suggested that if making it visible, do it in a separate patch for a separate discussion, and that's exactly what I did.
> 
> I enjoy these discussions, even when I know beforehand that my proposed changes will be rejected, because I (and also the other part) usually learn something, and hopefully fix other (related or unrelated) things that help all of us.  In this case we learnt some better code from musl, and cleaned up some mess of code (or are in the path to do so).
> 
> So please don't feel ignored in your rejection; I took it into account :)
> 
>>
>> This is a separate issue of whether we need a __snseconds_t type at all
>> (which is the disagreement that Rich Felker is expressing). Although the
>> type __snseconds_t may well be useful for glibc's own purposes, my
>> experience is that user code doesn't need a snseconds_t type without the
>> leading underscores. Arguably a user-visible snseconds_t type would
>> cause more confusion than it'd cure, and so would be a net negative.
> 
> Yes, and that's why in v3 they are in different patches, although since they have some relation, they are still in the same patch set.
> 
> Thanks,
> Alex
> 
>