diff mbox

[[PATCH,RFC,2] 02/63] Y2038: add function __difftime64

Message ID 20180418201819.15952-3-albert.aribaud@3adev.fr
State New
Headers show

Commit Message

Albert ARIBAUD April 18, 2018, 8:17 p.m. UTC
Note:

1. The implementation expects __time64_t arguments, and could,
   in theory, require 64 bits to express the difference accurately;
   but it returns a double, which only provides about 55 significant
   bits.

   We could make it return a long double, which would be more than
   enough for 64 bits accuracy. But then, existing source code which
   uses difftime, and therefore stores difftime results in doubles,
   would need to change to long doubles. However, we want 64-bit time
   support to work without any application source code change.

   Besides, 55 bits allow for counting seconds accurately over 417
   billion years, which is arguably enough for most actual uses of
   difftime.

2. The implementation is simpler than its 32-bit counterpart, as it
   assumes that all __time64_t implementations are 64-bit integers.
---
 include/time.h  | 2 ++
 time/Versions   | 3 +++
 time/difftime.c | 9 +++++++++
 3 files changed, 14 insertions(+)

Comments

Paul Eggert April 18, 2018, 8:37 p.m. UTC | #1
On 04/18/2018 01:17 PM, Albert ARIBAUD (3ADEV) wrote:
> +/* Return the difference between 64-bit TIME1 and TIME0.  */
> +double
> +__difftime64 (__time64_t time1, __time64_t time0)
> +{
> +  /* Subtract the smaller integer from the larger, convert the difference to
> +     double, and then negate if needed.  */
> +  return time1 < time0 ? - (time0 - time1) : (time1 - time0);
> +}

This does not work correctly if there is an integer overflow in any of 
the three subtractions. Also, the comment disagrees with the code.

How about if you just #include difftime.c after defining a couple of 
macros, and then have difftime.c do its thing? That'd avoid problems 
like this. difftime.c would also need to be generalized a bit.
Joseph Myers April 18, 2018, 9:50 p.m. UTC | #2
On Wed, 18 Apr 2018, Albert ARIBAUD (3ADEV) wrote:

> diff --git a/time/Versions b/time/Versions
> index fd838181e4..57314b98c8 100644
> --- a/time/Versions
> +++ b/time/Versions
> @@ -65,4 +65,7 @@ libc {
>    GLIBC_2.16 {
>      timespec_get;
>    }
> +  GLIBC_2.27 {
> +    __difftime64;
> +  }

Please send any such patches as appropriate for current master, which 
means new symbols need to be at the version for the *next* release, 
currently 2.28.

Each patch should avoid regressions, meaning that it should include 
updates to all affected ABI baselines, unless there is a reason the series 
would need to be squashed for commit (the series as a whole should also be 
tested with build-many-glibcs.py).  Since there are no ABI baseline 
updates whatever in the series byeond two librt.abilist files and two 
libpthread.abilist files, but new ABIs are added to other libraries such 
as libc, either the series is seriously broken or the claim of no test 
regressions is incorrect.

As previously discussed, new ABI additions should be avoided for platforms 
where time_t is already 64-bit.  Obviously testing on such platforms is 
needed to make sure they continue to work just as they do now.
Albert ARIBAUD April 19, 2018, 12:27 p.m. UTC | #3
Hi Joseph,

On Wed, 18 Apr 2018 21:50:17 +0000, Joseph Myers
<joseph@codesourcery.com> wrote :

> On Wed, 18 Apr 2018, Albert ARIBAUD (3ADEV) wrote:
> 
> > diff --git a/time/Versions b/time/Versions
> > index fd838181e4..57314b98c8 100644
> > --- a/time/Versions
> > +++ b/time/Versions
> > @@ -65,4 +65,7 @@ libc {
> >    GLIBC_2.16 {
> >      timespec_get;
> >    }
> > +  GLIBC_2.27 {
> > +    __difftime64;
> > +  }  
> 
> Please send any such patches as appropriate for current master, which 
> means new symbols need to be at the version for the *next* release, 
> currently 2.28.

That's an overlook on my side; I'll bump up to 2.28.

> Each patch should avoid regressions, meaning that it should include 
> updates to all affected ABI baselines, unless there is a reason the series 
> would need to be squashed for commit (the series as a whole should also be 
> tested with build-many-glibcs.py).  Since there are no ABI baseline 
> updates whatever in the series byeond two librt.abilist files and two 
> libpthread.abilist files, but new ABIs are added to other libraries such 
> as libc, either the series is seriously broken or the claim of no test 
> regressions is incorrect.

The series is presented as a lot of small patches for clarity, with
only the final patch potentially modifying the public API (i.e., until
that patch, 'time_t' is always 32-bit, and only after the final patch
'time_t' /may/ be 32- /or/ 64-bit depending on _TIME_BITS). I expect
that the final series can and will be squashed indeed.

Re: test regressions: what I did was, on a target system, e.g. native
on an ARM system, to create two GLIBC trees, one with and one without
the patch set, configure them identically (except of course for the
build destination), run "make" and then run "make check" on both trees,
and compare the lists of errors, assuming that when the lists of errors
are the same there is no make check regression -- and then same for
'make xcheck'.

> As previously discussed, new ABI additions should be avoided for platforms 
> where time_t is already 64-bit.  Obviously testing on such platforms is 
> needed to make sure they continue to work just as they do now.

I'll add a 64-bit platform test on next run.

Cordialement,
Albert ARIBAUD
3ADEV
Albert ARIBAUD April 19, 2018, 1:04 p.m. UTC | #4
Hi Paul,

On Wed, 18 Apr 2018 13:37:41 -0700, Paul Eggert <eggert@cs.ucla.edu>
wrote :

> On 04/18/2018 01:17 PM, Albert ARIBAUD (3ADEV) wrote:
> > +/* Return the difference between 64-bit TIME1 and TIME0.  */
> > +double
> > +__difftime64 (__time64_t time1, __time64_t time0)
> > +{
> > +  /* Subtract the smaller integer from the larger, convert the difference to
> > +     double, and then negate if needed.  */
> > +  return time1 < time0 ? - (time0 - time1) : (time1 - time0);
> > +}  
> 
> This does not work correctly if there is an integer overflow in any of 
> the three subtractions. Also, the comment disagrees with the code.

Correct -- I'll fix this (and add the tests to cover the overflow
cases).

> How about if you just #include difftime.c after defining a couple of 
> macros, and then have difftime.c do its thing? That'd avoid problems 
> like this. difftime.c would also need to be generalized a bit.

One reason to have a separated implementation is that difftime.c has
quite complex logic covering many different implementations of time_t
(signed, unsigned, with padding bits) whereas here we only have signed
64-bit integer.

Cordialement,
Albert ARIBAUD
3ADEV
Joseph Myers April 19, 2018, 1:18 p.m. UTC | #5
On Thu, 19 Apr 2018, Albert ARIBAUD wrote:

> Re: test regressions: what I did was, on a target system, e.g. native
> on an ARM system, to create two GLIBC trees, one with and one without
> the patch set, configure them identically (except of course for the
> build destination), run "make" and then run "make check" on both trees,
> and compare the lists of errors, assuming that when the lists of errors
> are the same there is no make check regression -- and then same for
> 'make xcheck'.

That only works if you have a good baseline set of results.  If your 
baseline includes failures that aren't on the known list at 
<https://sourceware.org/glibc/wiki/Release/2.27>, they could hide problems 
in the testing of the patched glibc.

In particular, the baseline should have *no* ABI test failures (you need 
to configure --prefix=/usr to get the correct ABI).  And then you need to 
update the ABI test baselines (for *all* supported glibc platforms that 
currently have 32-bit time_t) to avoid introducing ABI test failures.
Paul Eggert April 19, 2018, 9:36 p.m. UTC | #6
On 04/19/2018 06:04 AM, Albert ARIBAUD wrote:
> One reason to have a separated implementation is that difftime.c has
> quite complex logic covering many different implementations of time_t
> (signed, unsigned, with padding bits) whereas here we only have signed
> 64-bit integer.

difftime.c is a general implementation that works regardless of the 
widths of time_t, int, long, etc. You tried reinventing its wheel but 
the reinvention was buggy. Although you could try reinventing its wheel 
again, I am afraid there will be another bug with reasonably high 
probability. Instead, let's reuse the wheel that we already have; that 
will be more likely to be correct, and will be easier to maintain in the 
future.

It's not just difftime.c. The same argument applies to mktime, etc. We 
shouldn't have two copies of these functions' source code: we should 
have just one generic copy, and then compile it once for 64-bit 
__time64_t and once for 32-bit time_t. The generic code already works 
for both 64-bit and 32-bit time_t, so it should also work for 64-bit 
__time64_t.

At bottom I am repeating the comment that Joseph Myers made here:

https://sourceware.org/ml/libc-alpha/2018-04/msg00376.html
Albert ARIBAUD May 2, 2018, 7:22 a.m. UTC | #7
Hi Paul,

On Thu, 19 Apr 2018 14:36:20 -0700, Paul Eggert <eggert@cs.ucla.edu>
wrote :

> On 04/19/2018 06:04 AM, Albert ARIBAUD wrote:
> > One reason to have a separated implementation is that difftime.c has
> > quite complex logic covering many different implementations of time_t
> > (signed, unsigned, with padding bits) whereas here we only have signed
> > 64-bit integer.  
> 
> difftime.c is a general implementation that works regardless of the 
> widths of time_t, int, long, etc. You tried reinventing its wheel but 
> the reinvention was buggy. Although you could try reinventing its wheel 
> again, I am afraid there will be another bug with reasonably high 
> probability. Instead, let's reuse the wheel that we already have; that 
> will be more likely to be correct, and will be easier to maintain in the 
> future.

I'd argue that the wheel we already have was built for the roads we
intended it to run on, but this change might be the occasion to update
to a wheel which will only run on a subset of roads where some of the
current wheel's properties become unneeded.

IOW: does the set of 32-bit architectures on which we will use
64-bit with 32-bit time need all the complexities of current difftime?
I'm thinking for instance about padding bits.

> It's not just difftime.c. The same argument applies to mktime, etc. We 
> shouldn't have two copies of these functions' source code: we should 
> have just one generic copy, and then compile it once for 64-bit 
> __time64_t and once for 32-bit time_t. The generic code already works 
> for both 64-bit and 32-bit time_t, so it should also work for 64-bit 
> __time64_t.

OK, so let me rephrase this to see if I am getting this :

1) Keep the original file untouched as the 32-bit-time compilation unit.
Compiling this file as it is now ensures that the ABI is untouched.

2) Compile the same original file with -D_TIME_BITS=64 (just like
application code would be compiled under 64-bit-time), producing a
second compilation unit which provides new symbols.

The problem with this approach is that it can be applied to some
implementations but that for many, the 64-bit version is *not*
functionally identical with the 32-bit one. At least the
syscall-dependent ones work differently: the 32-bit impelmentations
just use the 32-bit syscalls, while the 64-bit implementations have to
use the 64-bit syscalls *if availaible at run-time*, and be able to
fallback on the 32-bit syscalls if they have to.

Using a single source code for both 32-bit time and 64-bit time
would mean that 32-bit syscall-based implementation behavior would
change, which is contrary to one of the goals of Y2038; existing
GLIBC 32-bit time code should be unchanged.

I can respin the changes so that I apply your proposal for cases
where no syscalls are involved [1].

(Note: some time-related implementations are for protocols and file
formats. These will probably require extra care because we need to make
sure the protocol and file structures are actually independent on the
size of time_t.)

> At bottom I am repeating the comment that Joseph Myers made here:
> 
> https://sourceware.org/ml/libc-alpha/2018-04/msg00376.html

Cordialement,
Albert ARIBAUD
3ADEV
Albert ARIBAUD May 2, 2018, 7:24 a.m. UTC | #8
Hi Joseph,

On Thu, 19 Apr 2018 13:18:38 +0000, Joseph Myers
<joseph@codesourcery.com> wrote :

> On Thu, 19 Apr 2018, Albert ARIBAUD wrote:
> 
> > Re: test regressions: what I did was, on a target system, e.g. native
> > on an ARM system, to create two GLIBC trees, one with and one without
> > the patch set, configure them identically (except of course for the
> > build destination), run "make" and then run "make check" on both trees,
> > and compare the lists of errors, assuming that when the lists of errors
> > are the same there is no make check regression -- and then same for
> > 'make xcheck'.  
> 
> That only works if you have a good baseline set of results.  If your 
> baseline includes failures that aren't on the known list at 
> <https://sourceware.org/glibc/wiki/Release/2.27>, they could hide problems 
> in the testing of the patched glibc.

Noted. If I do a 'make check' from the git commit which corresponds to
this release, I should get the exact same list of failures as in the
release notes, right?

> In particular, the baseline should have *no* ABI test failures (you need 
> to configure --prefix=/usr to get the correct ABI).  And then you need to 
> update the ABI test baselines (for *all* supported glibc platforms that 
> currently have 32-bit time_t) to avoid introducing ABI test failures.

Will do in future tests.

Cordialement,
Albert ARIBAUD
3ADEV
Albert ARIBAUD May 2, 2018, 7:40 a.m. UTC | #9
On Wed, 2 May 2018 09:22:09 +0200, Albert ARIBAUD
<albert.aribaud@3adev.fr> wrote :

> OK, so let me rephrase this to see if I am getting this :
> 
> 1) Keep the original file untouched as the 32-bit-time compilation unit.
> Compiling this file as it is now ensures that the ABI is untouched.
> 
> 2) Compile the same original file with -D_TIME_BITS=64 (just like
> application code would be compiled under 64-bit-time), producing a
> second compilation unit which provides new symbols.

more to the point, it would be compiled *as if* under -D_TIME_BITS=64,
because TIME_BITS and __USE_TIME_BITS64 are public API macros which
are not defined at GLIBC build time -- and a git grep shows that no
GLIBC '.c' file ever references a __USE_* or _*_BITS macro defined in
features.h.

> The problem with this approach is that it can be applied to some
> implementations but that for many, the 64-bit version is *not*
> functionally identical with the 32-bit one. At least the
> syscall-dependent ones work differently: the 32-bit impelmentations
> just use the 32-bit syscalls, while the 64-bit implementations have to
> use the 64-bit syscalls *if availaible at run-time*, and be able to
> fallback on the 32-bit syscalls if they have to.

Also, 64-bit time types are not simply 32-bit time types with time_t
promoted to 64-bit signed, so there too, simply compiling the source
code would not always work.

Cordialement,
Albert ARIBAUD
3ADEV
Florian Weimer May 2, 2018, 7:46 a.m. UTC | #10
On 05/02/2018 09:22 AM, Albert ARIBAUD wrote:
> IOW: does the set of 32-bit architectures on which we will use
> 64-bit with 32-bit time need all the complexities of current difftime?
> I'm thinking for instance about padding bits.

Yes, you can simplify the code if you want, assuming that time_t doesn't 
have padding bits, and even hard-code the matching unsigned type.

Thanks,
Florian
Albert ARIBAUD May 2, 2018, 11:26 a.m. UTC | #11
Hi Florian,

On Wed, 2 May 2018 09:46:19 +0200, Florian Weimer <fweimer@redhat.com>
wrote :

> On 05/02/2018 09:22 AM, Albert ARIBAUD wrote:
> > IOW: does the set of 32-bit architectures on which we will use
> > 64-bit with 32-bit time need all the complexities of current difftime?
> > I'm thinking for instance about padding bits.  
> 
> Yes, you can simplify the code if you want, assuming that time_t doesn't 
> have padding bits, and even hard-code the matching unsigned type.

Watch out __time64_t is signed, not unsigned (if it were unsigned, that
would really simplify things, and we could just use subtraction).

> Thanks,
> Florian

Cordialement,
Albert ARIBAUD
3ADEV
Joseph Myers May 2, 2018, 3:45 p.m. UTC | #12
On Wed, 2 May 2018, Albert ARIBAUD wrote:

> 1) Keep the original file untouched as the 32-bit-time compilation unit.
> Compiling this file as it is now ensures that the ABI is untouched.
> 
> 2) Compile the same original file with -D_TIME_BITS=64 (just like
> application code would be compiled under 64-bit-time), producing a
> second compilation unit which provides new symbols.

3. Make the main implementation the 64-bit one.  Make difftime into a thin 
wrapper round __difftime64.  (In the case where time_t is already 64-bit, 
the function would just be built directly as difftime without any wrapper 
being involved.)

That's what I think it appropriate in all the cases where the function 
implementations are nontrivial.  It avoids duplication of complicated code 
in the sources, and reduces the code size increase from duplication in the 
binaries.

That's particularly important for e.g. the pthread functions where a 
previous patch series was duplicating hundreds of lines of code in some 
cases.

It would mean times sometimes go through two levels of validation / 
conversion, from 32-bit to 64-bit and then back to 32-bit if the kernel 
doesn't support the 64-bit interfaces, but I think the cost of that is 
negligible.
Paul Eggert May 2, 2018, 7:07 p.m. UTC | #13
On 05/02/2018 12:46 AM, Florian Weimer wrote:
>
> Yes, you can simplify the code if you want, assuming that time_t 
> doesn't have padding bits, and even hard-code the matching unsigned type.

The simplified code is already in difftime.c. All he needs to do is to 
compile difftime.c with the right flags, and perhaps make some trivial 
changes to it that won't affect its current use.

I'd be more impressed by his request to rewrite the code from scratch if 
his attempt to do so hadn't been so buggy. Really, we'll all be better 
off using the code we already have, as that code should work in this new 
environment with only trivial changes, and we can then focus on what our 
main problem is rather than have to delve back into the intricacies of 
time_t arithmetic and calendars and whatnot.
Paul Eggert May 2, 2018, 7:14 p.m. UTC | #14
On 05/02/2018 12:22 AM, Albert ARIBAUD wrote:
> At least the
> syscall-dependent ones work differently: the 32-bit impelmentations
> just use the 32-bit syscalls, while the 64-bit implementations have to
> use the 64-bit syscalls *if availaible at run-time*, and be able to
> fallback on the 32-bit syscalls if they have to.

That's fine, and we can have code protected by ifdef to do that as 
needed for the special case where the system has multiple time_t flavors 
some of which might not work. This doesn't mean we need to have separate 
copies of the entire module that needs to use such code.

And for functions like difftime that do not use time_t-related syscalls, 
things should be even simpler, as we shouldn't need to worry about this 
issue in those functions.

> Using a single source code for both 32-bit time and 64-bit time
> would mean that 32-bit syscall-based implementation behavior would
> change,

I don't see why it would, at least not if things were done as suggested 
above.
Albert ARIBAUD May 3, 2018, 5:30 p.m. UTC | #15
Hi Paul,

On Wed, 2 May 2018 12:07:32 -0700, Paul Eggert <eggert@cs.ucla.edu>
wrote :

> On 05/02/2018 12:46 AM, Florian Weimer wrote:
> >
> > Yes, you can simplify the code if you want, assuming that time_t 
> > doesn't have padding bits, and even hard-code the matching unsigned type.  
> 
> The simplified code is already in difftime.c. All he needs to do is to 
> compile difftime.c with the right flags, and perhaps make some trivial 
> changes to it that won't affect its current use.

Actually, the simpler code in difftime.c which 64-bit signed integer
needs is enclosed in run-time conditionals, not compile-time
conditionals, so there are no flags that can make the code compile to a
simpler form. It just does a lot of run-time tests (*) which only some
architectures or some time_t implementations actually need.

> I'd be more impressed by his request to rewrite the code from scratch if 
> his attempt to do so hadn't been so buggy. Really, we'll all be better 
> off using the code we already have, as that code should work in this new 
> environment with only trivial changes, and we can then focus on what our 
> main problem is rather than have to delve back into the intricacies of 
> time_t arithmetic and calendars and whatnot.

I would love that too, and one way of not delving into intricacies is
to remove them if this can be done safely in a given context. I will
gladly admit I might have removed slightly too much of the difftime
intricacies in 64-bit signed time context, and I do believe in the
"don't fix it if it ain't broken" approach, but I'm not aiming to fix
32-bit time here with all its implementations, integer or floating,
signed or unsigned, padded or not; I'm aiming to make 64-bit, integer,
unpadded time as devoid of undue intricacies as it can be.

(*) Which might possibly, but not will not necessarily, be optimized
away by some compilers based on the target architecture's time_t type
and/or signedness and/or size, or even on known architecture quirks --
I am choosing not to assume how clever compilers can be, and I consider
that putting a run-time test in some source code will be costlier in
execution time than not putting it there.

Cordialement,
Albert ARIBAUD
3ADEV
Paul Eggert May 3, 2018, 5:53 p.m. UTC | #16
On 05/03/2018 10:30 AM, Albert ARIBAUD wrote:
> the simpler code in difftime.c which 64-bit signed integer
> needs is enclosed in run-time conditionals, not compile-time
> conditionals


These run-time conditionals are optimized away, and have zero run-time 
overhead.

Really, you should use difftime.c and see what it does (look at the 
machine code that is generated if you like). You will see that it does 
the right thing and generates the same instructions that your copy 
would. Similarly for many of the other routines that you incorrectly 
think we need to make source-code copies of.


> I am choosing not to assume how clever compilers can be, and I consider
> that putting a run-time test in some source code will be costlier in
> execution time than not putting it there.

This is completely, 100%, backwards. It is routine in high-quality 
software nowadays to assume decent optimizing compilers; for example, 
that's why we now use functions rather than macros, since simple 
functions are routinely inlined. The glibc implementation assumes GCC, 
it is not our job to second-guess GCC, and we should not add complexity 
to glibc when GCC does a perfectly good job of optimizing simpler code.
Albert ARIBAUD May 22, 2018, 8:58 p.m. UTC | #17
Hi Paul,

On Thu, 3 May 2018 10:53:06 -0700, Paul Eggert <eggert@cs.ucla.edu>
wrote :

> On 05/03/2018 10:30 AM, Albert ARIBAUD wrote:
> > the simpler code in difftime.c which 64-bit signed integer
> > needs is enclosed in run-time conditionals, not compile-time
> > conditionals  
> 
> 
> These run-time conditionals are optimized away, and have zero run-time 
> overhead.
> 
> Really, you should use difftime.c and see what it does (look at the 
> machine code that is generated if you like). You will see that it does 
> the right thing and generates the same instructions that your copy 
> would. Similarly for many of the other routines that you incorrectly 
> think we need to make source-code copies of.
> 
> 
> > I am choosing not to assume how clever compilers can be, and I consider
> > that putting a run-time test in some source code will be costlier in
> > execution time than not putting it there.  
> 
> This is completely, 100%, backwards. It is routine in high-quality 
> software nowadays to assume decent optimizing compilers; for example, 
> that's why we now use functions rather than macros, since simple 
> functions are routinely inlined. The glibc implementation assumes GCC, 
> it is not our job to second-guess GCC, and we should not add complexity 
> to glibc when GCC does a perfectly good job of optimizing simpler code.

I make a difference between optimizing and optimizing *away*.

Optimizing: no problem -- let the compiler reorganize the code, factor
it, tweak it in whatever way preserves its semantics while making it
smaller or faster or both, I'm happy with it, and nowadays compilers do
a very good job of it indeed, I can see that whenever I'm trying to
step-debug with -O3.

But here we're talking about optimizing *away* code which the
compiler finds it won't use at all any more.

With the 32-bit time implementation, I do understand that it is
intended to work with different time_t types, and this implies the
presence of source code intended for one implementation but seen as
dead code for others. Simplifying the 32-bit difftime would indeed
imply removing some functionality required by some existing time_t
implementation, and is therefore out of question (plus, that's not on
my plate).

OTOH, the 64-bit implementation uses a single, well-enough-defined,
64-bit, signed integer time_t implementation across all architectures
where it will exist (endianness notwithstanding, and that's not an
issue for difftime) which can and should benefit from a single
implementation simpler that the 32-bit one. Any code in the 64-bit
difftime which assumes a time_t implementation other than a 64-bit
signed int is simply dead code in*all* cases, and it won't *ever* be
used. Yes, the compiler can optimize it away again and again, but why
keep true dead source code when we know it is truly dead? That's
keeping source code complexity where we could remove it, IMO.

[that is, of course, assuming that we *know* it is dead -- again, I may
have oversimplified my previous attempt. I am going through the code
again and triple-checking that the implementation works with a 64-bit
signed time_t.]

Cordialement,
Albert ARIBAUD
3ADEV
Paul Eggert May 22, 2018, 9:15 p.m. UTC | #18
On 05/22/2018 01:58 PM, Albert ARIBAUD wrote:
> But here we're talking about optimizing*away*  code which the
> compiler finds it won't use at all any more.

That's fine. GCC does this sort of thing all the time. It is a normal 
thing to write portable code that contains portions executed only on 
some platforms.

The proposed hand-optimization does not improve efficiency at all, so 
there is no good reason to insist on doing it by hand. Maintaining two 
copies of the source code would cause more maintenance work in the long 
run, and would increase the probability of bugs like the bug we already 
found in the "optimized" version. Although whether one should do an 
optimization is often a judgment call that could go either way, this 
particular case is a slam-dunk: we should let GCC do the optimization, 
and we should keep the overall source code simpler rather than maintain 
two copies of it, one "optimized" (but no faster).

As I recall, there were other unnecessary duplications in the proposed 
set of patches, so this is not just an issue of difftime.
Joseph Myers May 22, 2018, 10:09 p.m. UTC | #19
On Tue, 22 May 2018, Paul Eggert wrote:

> As I recall, there were other unnecessary duplications in the proposed set of
> patches, so this is not just an issue of difftime.

Indeed.  The global approach should be: the main implementation of any 
functionality that involves nontrivial implementation code in glibc is the 
one that uses 64-bit time, unconditionally, on all platforms.  On 
platforms that currently have 32-bit time_t, the public interfaces for 
32-bit times would become thin wrappers round those for 64-bit times while 
new public ABIs are added for 64-bit times (on platforms that currently 
have 64-bit time_t, there would be no new ABIs).  If kernel support for 
64-bit times is not known to be available for the (architecture, kernel 
version) pair for which glibc is built, the implementations in terms of 
64-bit times may then need to translate back to 32-bit times, with 
overflow checks, to call into the kernel.
Albert ARIBAUD June 11, 2018, 10:24 p.m. UTC | #20
Hi Joseph,

On Tue, 22 May 2018 22:09:55 +0000, Joseph Myers
<joseph@codesourcery.com> wrote :

> On Tue, 22 May 2018, Paul Eggert wrote:
> 
> > As I recall, there were other unnecessary duplications in the proposed set of
> > patches, so this is not just an issue of difftime.  
> 
> Indeed.  The global approach should be: the main implementation of any 
> functionality that involves nontrivial implementation code in glibc is the 
> one that uses 64-bit time, unconditionally, on all platforms.  On 
> platforms that currently have 32-bit time_t, the public interfaces for 
> 32-bit times would become thin wrappers round those for 64-bit times while 
> new public ABIs are added for 64-bit times (on platforms that currently 
> have 64-bit time_t, there would be no new ABIs).  If kernel support for 
> 64-bit times is not known to be available for the (architecture, kernel 
> version) pair for which glibc is built, the implementations in terms of 
> 64-bit times may then need to translate back to 32-bit times, with 
> overflow checks, to call into the kernel.

When a 64-bit time implementation needs to translate back to 32-bit
times before calling to the kernel, would it not be counter-productive
to make the 32-bit interface a wrapper around it? This would hit the
32-bit client code with two conversions (from the 32-bit interface
to the 64-bit implementation, and from the implementation to the 32-
bit kernel syscall) when the original 32-bit interface would make none
and call the kernel (as) directly (as possible).

Cordialement,
Albert ARIBAUD
3ADEV
Joseph Myers June 11, 2018, 10:39 p.m. UTC | #21
On Tue, 12 Jun 2018, Albert ARIBAUD wrote:

> When a 64-bit time implementation needs to translate back to 32-bit
> times before calling to the kernel, would it not be counter-productive
> to make the 32-bit interface a wrapper around it? This would hit the
> 32-bit client code with two conversions (from the 32-bit interface
> to the 64-bit implementation, and from the implementation to the 32-
> bit kernel syscall) when the original 32-bit interface would make none
> and call the kernel (as) directly (as possible).

If you're calling into the kernel, the cost of conversions is negligible.  
Even if you aren't calling into the kernel it's still a very small cost (a 
few instructions).  Increased code size from duplicate implementations 
also has a performance cost from increased cache usage.  And the interests 
of maintainability of glibc code point very strongly to having single 
implementations using 64-bit times, and thin wrappers round them for 
32-bit times - especially for complicated code such as some of the 
pthreads functions.
diff mbox

Patch

diff --git a/include/time.h b/include/time.h
index 7eda265a66..ba2c67a829 100644
--- a/include/time.h
+++ b/include/time.h
@@ -98,6 +98,8 @@  extern char * __strptime_internal (const char *rp, const char *fmt,
 
 extern double __difftime (time_t time1, time_t time0);
 
+extern double __difftime64 (__time64_t time1, __time64_t time0);
+
 /* Use in the clock_* functions.  Size of the field representing the
    actual clock ID.  */
 #define CLOCK_IDFIELD_SIZE	3
diff --git a/time/Versions b/time/Versions
index fd838181e4..57314b98c8 100644
--- a/time/Versions
+++ b/time/Versions
@@ -65,4 +65,7 @@  libc {
   GLIBC_2.16 {
     timespec_get;
   }
+  GLIBC_2.27 {
+    __difftime64;
+  }
 }
diff --git a/time/difftime.c b/time/difftime.c
index 7c5dd9898b..e68b9a2c81 100644
--- a/time/difftime.c
+++ b/time/difftime.c
@@ -119,3 +119,12 @@  __difftime (time_t time1, time_t time0)
   return time1 < time0 ? - subtract (time0, time1) : subtract (time1, time0);
 }
 strong_alias (__difftime, difftime)
+
+/* Return the difference between 64-bit TIME1 and TIME0.  */
+double
+__difftime64 (__time64_t time1, __time64_t time0)
+{
+  /* Subtract the smaller integer from the larger, convert the difference to
+     double, and then negate if needed.  */
+  return time1 < time0 ? - (time0 - time1) : (time1 - time0);
+}