Message ID | 20180418201819.15952-3-albert.aribaud@3adev.fr |
---|---|
State | New, archived |
Headers |
Received: (qmail 97251 invoked by alias); 18 Apr 2018 20:18:43 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 97139 invoked by uid 89); 18 Apr 2018 20:18:42 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.8 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, KAM_NUMSUBJECT, RCVD_IN_DNSWL_LOW, RCVD_IN_RP_RNBL autolearn=ham version=3.3.2 spammy=counterpart X-HELO: smtp3-g21.free.fr From: "Albert ARIBAUD (3ADEV)" <albert.aribaud@3adev.fr> To: libc-alpha@sourceware.org Cc: "Albert ARIBAUD (3ADEV)" <albert.aribaud@3adev.fr> Subject: [[PATCH RFC 2] 02/63] Y2038: add function __difftime64 Date: Wed, 18 Apr 2018 22:17:18 +0200 Message-Id: <20180418201819.15952-3-albert.aribaud@3adev.fr> In-Reply-To: <20180418201819.15952-2-albert.aribaud@3adev.fr> References: <20180418201819.15952-1-albert.aribaud@3adev.fr> <20180418201819.15952-2-albert.aribaud@3adev.fr> |
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
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.
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.
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
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
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.
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
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
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
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
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
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
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.
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.
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.
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
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.
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
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.
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.
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
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 --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); +}