From patchwork Wed Jun 20 19:29:06 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paul Eggert X-Patchwork-Id: 27950 Received: (qmail 114616 invoked by alias); 20 Jun 2018 19:29:12 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 114606 invoked by uid 89); 20 Jun 2018 19:29:11 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.2 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_NUMSUBJECT, SPF_PASS, URIBL_RED autolearn=ham version=3.3.2 spammy= X-HELO: zimbra.cs.ucla.edu Subject: Re: [PATCH 1/1] Y2038: add function __difftime64 To: "Albert ARIBAUD (3ADEV)" , libc-alpha@sourceware.org References: <20180620121427.25397-1-albert.aribaud@3adev.fr> <20180620121427.25397-2-albert.aribaud@3adev.fr> From: Paul Eggert Openpgp: preference=signencrypt Message-ID: <9b02ad78-15cf-969e-b2ef-cae13d5a689e@cs.ucla.edu> Date: Wed, 20 Jun 2018 12:29:06 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <20180620121427.25397-2-albert.aribaud@3adev.fr> 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. From b6120a13a752335bc1c60d5d22a1cdb0b4403a7a Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Wed, 20 Jun 2018 11:37:54 -0700 Subject: [PATCH 2/2] difftime: new __time64_t variant * include/time.h (__difftime64): New macro or function decl. * time/difftime.c: Include "time-internal.h" if not glibc, for outside-glibc uses. (subtract, __difftime64) Use __time64_t, not time_t. (__difftime) [__TIMESIZAE != 64]: Turn into a wrapper function for __difftime64, a new function that contains the old body of __difftime. --- include/time.h | 6 ++++++ time/difftime.c | 36 ++++++++++++++++++++++++++---------- 2 files changed, 32 insertions(+), 10 deletions(-) diff --git a/include/time.h b/include/time.h index 5afb12305e..63cc855749 100644 --- a/include/time.h +++ b/include/time.h @@ -125,6 +125,12 @@ extern char * __strptime_internal (const char *rp, const char *fmt, struct tm *tm, void *statep, locale_t locparam) attribute_hidden; +#if __TIMESIZE == 64 +# define __difftime64 __difftime +#else +extern double __difftime64 (__time64_t time1, __time64_t time0); +#endif + extern double __difftime (time_t time1, time_t time0); /* Use in the clock_* functions. Size of the field representing the diff --git a/time/difftime.c b/time/difftime.c index 7727f5cdb5..4e765de624 100644 --- a/time/difftime.c +++ b/time/difftime.c @@ -23,19 +23,23 @@ #include #include +#ifndef _LIBC +# include "time-internal.h" +#endif + #define TYPE_BITS(type) (sizeof (type) * CHAR_BIT) #define TYPE_SIGNED(type) ((type) -1 < 0) /* Return the difference between TIME1 and TIME0, where TIME0 <= TIME1. */ static double -subtract (time_t time1, time_t time0) +subtract (__time64_t time1, __time64_t time0) { - if (! TYPE_SIGNED (time_t)) + if (! TYPE_SIGNED (__time64_t)) return time1 - time0; else { - /* Optimize the common special cases where time_t + /* Optimize the common special cases where __time64_t can be converted to uintmax_t without losing information. */ uintmax_t dt = (uintmax_t) time1 - (uintmax_t) time0; double delta = dt; @@ -43,7 +47,7 @@ subtract (time_t time1, time_t time0) if (UINTMAX_MAX / 2 < INTMAX_MAX) { /* This is a rare host where uintmax_t has padding bits, and possibly - information was lost when converting time_t to uintmax_t. + information was lost when converting __time64_t to uintmax_t. Check for overflow by comparing dt/2 to (time1/2 - time0/2). Overflow occurred if they differ by more than a small slop. Thanks to Clive D.W. Feather for detailed technical advice about @@ -74,9 +78,9 @@ subtract (time_t time1, time_t time0) 1 is unsigned in C, so it need not be compared to zero. */ uintmax_t hdt = dt / 2; - time_t ht1 = time1 / 2; - time_t ht0 = time0 / 2; - time_t dht = ht1 - ht0; + __time64_t ht1 = time1 / 2; + __time64_t ht0 = time0 / 2; + __time64_t dht = ht1 - ht0; if (2 < dht - hdt + 1) { @@ -97,17 +101,17 @@ subtract (time_t time1, time_t time0) /* Return the difference between TIME1 and TIME0. */ double -__difftime (time_t time1, time_t time0) +__difftime64 (__time64_t time1, __time64_t time0) { /* Convert to double and then subtract if no double-rounding error could result. */ - if (TYPE_BITS (time_t) <= DBL_MANT_DIG) + if (TYPE_BITS (__time64_t) <= DBL_MANT_DIG) return (double) time1 - (double) time0; /* Likewise for long double. */ - if (TYPE_BITS (time_t) <= LDBL_MANT_DIG) + if (TYPE_BITS (__time64_t) <= LDBL_MANT_DIG) return (long double) time1 - (long double) time0; /* Subtract the smaller integer from the larger, convert the difference to @@ -115,4 +119,16 @@ __difftime (time_t time1, time_t time0) return time1 < time0 ? - subtract (time0, time1) : subtract (time1, time0); } + +#if __TIMESIZE != 64 + +/* Return the difference between TIME1 and TIME0. */ +double +__difftime (time_t time1, time_t time0) +{ + return __difftime64 (time1, time0); +} + +#endif + strong_alias (__difftime, difftime) -- 2.17.1