[PATCH 1/1] Y2038: add function __difftime64

[Paul Eggert]

On 06/20/2018 05:14 AM, Albert ARIBAUD (3ADEV) wrote:
> This change batch creates a __difftime64 compatible with 64-bit time,
> and makes difftime either an alias of it or a 32-bit wrapper around it.

Since no glibc code calls difftime, can we assume that a later patch 
will change will make __difftime64 and difftime both available to user 
code? I'm not getting the big picture here.

Which public development repository and branch are you using? I see lots 
of glibc branches at sourceware.org named origin/aaribaud/y2038* but 
none of them seem to correspond that the patches you're sending. If I 
could see the current state of the entire set of patches you've 
developed, that would save time reviewing.

>     We could make it return a long double

It's not just that existing programs expect difftime to return 'double'; 
it's also that C11 and POSIX both require it to return 'double'.

> 2. The 64-bit time implementation was obtained by duplicating the
>     original 32-bit code then simplifying the source code based on
>     the knowledge that __time64_t is a 64-bit signed integer

This sort of simplification won't be possible in Gnulib, where 
__time64_t will be an alias of time_t and therefore could be an unsigned 
type. So let's not do that simplification. (Gnulib-using programs will 
always ask for 64-bit time_t if that is an option and 32-bit is the 
default, as the 32-bit default is silly if you have source code.)

It is OK to simplify difftime.c based on the assumption that time_t is 
an integer type. Glibc difftime.c was written back when POSIX allowed 
time_t to be floating-point, and some ancient implementations did that. 
This was widely regarded to be a mistake, POSIX no longer allows 
floating-point time_t and we don't need to worry about those old 
implementations. However, this simplification should be done as a 
separate patch (see first attachment).

>     - in the difftime64 function, removal of code which handled
>       time bitsize smaller than or equal to that of a double matissa
>       (as __time64_t has more bits than a double mantissa can hold)

This simplification is not possible in Gnulib either, as it's not 
portable to assume that time_t has more bits than a double fraction can 
hold. (They're fractions, not mantissas, by the way; mantissas are for 
logarithms not for floating-point.)

> -static double
> -subtract (time_t time1, time_t time0)
> +static double subtract (__time64_t time1, __time64_t time0)

Don't change indentation in cases like this; just stick to the glibc style.

>   {
> -  if (! TYPE_SIGNED (time_t))
> -    return time1 - time0;

Let's leave that TYPE_SIGNED test in, for the benefit of non-glibc uses 
where time_t is unsigned.

> -  if (TYPE_BITS (time_t) <= DBL_MANT_DIG
> -      || (TYPE_FLOATING (time_t) && sizeof (time_t) < sizeof (long double)))
> -    return (double) time1 - (double) time0;

Likewise, this needs to stay in, for portability.

The patch needs a better commit message, as the commit message doesn't 
say what exactly changed and has too much unnecessary talk about things 
we aren't doing. Commit messages should focus on what actually changed; 
the meat of any comments explaining the code should be in the code.

Attached is a proposed pair of (untested) patches that should reflect 
the above comments. What do you think?

PS. The mktime patches you sent have more problems like this. But that's 
a bigger nut to crack, and let's get difftime done first.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-difftime-do-not-worry-about-floating-point-time_t.patch
Type: text/x-patch
Size: 2157 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20180620/03b00c7a/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-difftime-new-__time64_t-variant.patch
Type: text/x-patch
Size: 4140 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20180620/03b00c7a/attachment-0001.bin>