Message ID | 20180618191443.9926-2-albert.aribaud@3adev.fr |
---|---|
State | New |
Headers | show |
Series | Y2038 support batch 1 - __time64_t and __tz_convert | expand |
On 06/18/2018 12:14 PM, Albert ARIBAUD (3ADEV) wrote: > +/* Check whether a time64_t value fits in a time_t. */ > +static inline bool > +fits_in_time_t (__time64_t t) > +{ > + return t == (time_t) t; > +} This static function is used nowhere in this patch series. Shouldn't its introduction be delayed to the first patch that actually needs it? Also, looking at the two future uses of this function, they're both of the form: __time64_t t64 = [something]; if (fits_in_time_t (t64)) return (time_t) t64; __set_errno (EOVERFLOW); return -1; Wouldn't it be better to have these uses do the following instead? This would be just as clear, and would avoid the need for casts and for the fits_in_time_t function. __time64_t t64 = [something]; time_t t = t64; if (t == t64) return t; __set_errno (EOVERFLOW); return -1;
Hi Paul, On Tue, 19 Jun 2018 16:25:41 -0700, Paul Eggert <eggert@cs.ucla.edu> wrote : > On 06/18/2018 12:14 PM, Albert ARIBAUD (3ADEV) wrote: > > +/* Check whether a time64_t value fits in a time_t. */ > > +static inline bool > > +fits_in_time_t (__time64_t t) > > +{ > > + return t == (time_t) t; > > +} > > This static function is used nowhere in this patch series. Shouldn't its > introduction be delayed to the first patch that actually needs it? > > Also, looking at the two future uses of this function, they're both of > the form: > > __time64_t t64 = [something]; > if (fits_in_time_t (t64)) > return (time_t) t64; > __set_errno (EOVERFLOW); > return -1; > > Wouldn't it be better to have these uses do the following instead? This > would be just as clear, and would avoid the need for casts and for the > fits_in_time_t function. > > __time64_t t64 = [something]; > time_t t = t64; > if (t == t64) > return t; > __set_errno (EOVERFLOW); > return -1; I can defer the function definition to within the second patch in the series. Regarding the cast, there is no way to reduce the /need/ for casts, as we /do/ need one here. What we can do is reduce the number of explicit casts, as in the example above. But I disagree that the resulting code would be as clear as the one in the patch: it would in fact be less clear, because the intent of the code would become implicit rather than explicit in two places: * replacing the call to (and consequently the definition of) function fits_in_time_t() with an in-line equality test obscures the reason why we do this test; * hiding the cast itself makes it harder to see that this cast is the important thing happening here. The goal of the code is to perform the cast, so it is best if said cast is explicit. Here, the end result of obscuring the function hiding an important cast would not be worth the added line and variable in every place where we need reduction to time_t. But we can do something that keeps the code explicit /and/ reduces it properly, based on the fact that we always call fits_in_time_t() to perform a cast to time_t or set errno and reduce to -1 if we cannot cast. We could put the consequence with the test, and instead of defining fits_in_time_t(), we could define reduce_to_time_t() as follows: /* Check whether a time64_t value fits in a time_t. */ static inline time_t reduce_to_time_t (__time64_t t) { if (t == (time_t) t) return t; __set_errno (EOVERFLOW); return -1; } Then wherever we would write time_t some_func (...) { ... if (fits_in_time_t (t64)) return (time_t) t64; __set_errno (EOVERFLOW); return -1; } .... we could now write time_t some_func (...) { ... return reduce_to_time_t (t64); } ... which would be as explicit (both in the definition and in the calls) and take up much less source code. Cordialement, Albert ARIBAUD 3ADEV
On Wed, 20 Jun 2018 08:04:27 +0200, Albert ARIBAUD <albert.aribaud@3adev.fr> wrote : > I can defer the function definition to within the second patch in the > series. s/patch/batch/ Cordialement, Albert ARIBAUD 3ADEV
On Wed, 20 Jun 2018 08:04:27 +0200, Albert ARIBAUD <albert.aribaud@3adev.fr> wrote : > But we can do something that keeps the code explicit /and/ reduces > it properly, based on the fact that we always call fits_in_time_t() to > perform a cast to time_t or set errno and reduce to -1 if we cannot > cast. > > We could put the consequence with the test, and instead of defining > fits_in_time_t(), we could define reduce_to_time_t() as follows: > > /* Check whether a time64_t value fits in a time_t. */ > static inline time_t > reduce_to_time_t (__time64_t t) > { > if (t == (time_t) t) > return t; > __set_errno (EOVERFLOW); > return -1; > } > Trying to define the function above in include/time.h fails because include/time.h needs to include include/errno.h to get the definition of __set_errno, but both header files include each other, and some C files include errno.h alone (or first), which results in the preprocessor meeting the definition of reduce_to_time_t() before that of __set_errno and rightly complaining that (function) __set_errno has not been declared yet. There is no easy way out of this, since the definition of __set_errno requires that of errno, which in turn requires tls.h. Or am I missing something? Cordialement, Albert ARIBAUD 3ADEV
Albert ARIBAUD wrote: > Regarding the cast, there is no way to reduce the /need/ for casts, as > we /do/ need one here. What we can do is reduce the number of explicit > casts A terminology point: the C Standard uses the word "cast" to describe what you're calling an "explicit cast", and it uses the word "conversion" to describe what you're calling a "cast". Let's stick to the standard terminology as it makes for less confusion. > But I disagree that the resulting code would be as clear as the one in > the patch: it would in fact be less clear, because the intent of the > code would become implicit rather than explicit in two places: First, in C, casts are more dangerous than other conversions because they allow more typos to go unchecked. If you mistakenly cast an integer to a pointer, the compiler often does not complain, but if you mistakenly convert an integer to a pointer without casting it, the compiler will catch your mistake and report an error. For this reason, C casts should not be used unless necessary (and they are not necessary here). Second, the code I proposed is completely obvious. One cannot read code like this: type1 x = ...; type2 y = x; if (y == x) ... without immediately knowing what's going on. Third, as you noted, the proposed fits_in_time_t function does a poor job of moving common code into a single function. To do a better job we would need something like the reduce_to_time_t function of your email. Unfortunately, as you noted in a later email, reduce_to_time_t doesn't work because of include problems. The exact same thought process went through my head when I wrote my review. That is, I thought "fits_in_time_t is a bad helper function, and trying to improve it by having a helper function that captures the actual idea won't work due to include hassles, so let's just do things directly; it's just as clear and it solves the problem better". So, let's just do things directly; it's just as clear and it solves the problem better.
Hi Paul, On Wed, 20 Jun 2018 09:01:08 -0700, Paul Eggert <eggert@cs.ucla.edu> wrote : > Albert ARIBAUD wrote: > > > Regarding the cast, there is no way to reduce the /need/ for casts, as > > we /do/ need one here. What we can do is reduce the number of explicit > > casts > > A terminology point: the C Standard uses the word "cast" to describe what you're > calling an "explicit cast", and it uses the word "conversion" to describe what > you're calling a "cast". Let's stick to the standard terminology as it makes for > less confusion. Agreed. > > But I disagree that the resulting code would be as clear as the one in > > the patch: it would in fact be less clear, because the intent of the > > code would become implicit rather than explicit in two places: > > First, in C, casts are more dangerous than other conversions because they allow > more typos to go unchecked. If you mistakenly cast an integer to a pointer, the > compiler often does not complain, but if you mistakenly convert an integer to a > pointer without casting it, the compiler will catch your mistake and report an > error. For this reason, C casts should not be used unless necessary (and they > are not necessary here). Indeed when the cast is /created/, there is a risk that it be wrong. But once it is created and reviewed and found correct, then in a function that risk is gone for good IMO since both the type being cast to and the type being cast from are known -- of course, I would stand firm against any cast being done to a macro argument, as you cannot tell what type it might have, if it has any. > Second, the code I proposed is completely obvious. One cannot read code like this: > > type1 x = ...; > type2 y = x; > if (y == x) ... > > without immediately knowing what's going on. Here, I would beg to differ on what one can or cannot immediately know what's going on without explicit clues. In my experience, what one immediately knows from a piece of code varies wildly across individuals, and Murphy is extremely efficient in ensuring that what goes without saying always ends up going wrong at some point. > Third, as you noted, the proposed fits_in_time_t function does a poor job of > moving common code into a single function. To do a better job we would need > something like the reduce_to_time_t function of your email. Unfortunately, as > you noted in a later email, reduce_to_time_t doesn't work because of include > problems. The exact same thought process went through my head when I wrote my > review. That is, I thought "fits_in_time_t is a bad helper function, and trying > to improve it by having a helper function that captures the actual idea won't > work due to include hassles, so let's just do things directly; it's just as > clear and it solves the problem better". > > So, let's just do things directly; it's just as clear and it solves the problem > better. There's the possibility of replacing function reduce_to_time_t() with a macro, which would postpone evaluation of __set_errno() until within the calling C file, but then, a cast in a macro is a worse idea than a cast in a function. I still dislike the idea of duplicating code, but here specifically, I can't see another way without doing bigger and deeper changes than I need to. I also still don't think the cast discussed here would be a risk or bad practice, but I'm ok with removing it provided the readers get a stronger hint on what's going on unsaid. So I'll go with the code you suggest, with a more explicit name for the temporary and a one-line short comment. /* Convert from __time64 to time_t or fail with EOVERFLOW. */ time_t t32 = t64; if (t32 == t64) return t32; __set_errno (EOVERFLOW); return -1; Cordialement, Albert ARIBAUD 3ADEV
On 06/20/2018 09:45 AM, Albert ARIBAUD wrote: > I'll go with the > code you suggest, with a more explicit name for the temporary and a > one-line short comment. > > /* Convert from __time64 to time_t or fail with EOVERFLOW. */ > time_t t32 = t64; > if (t32 == t64) > return t32; > __set_errno (EOVERFLOW); > return -1; That's OK, except for clarity please use "t" rather than "t32" since time_t is not necessarily 32-bit.
diff --git a/bits/timesize.h b/bits/timesize.h new file mode 100644 index 0000000000..0739a34df4 --- /dev/null +++ b/bits/timesize.h @@ -0,0 +1,23 @@ +/* Bit size of the time_t type at glibc build time, general case. + Copyright (C) 2018 Free Software Foundation, Inc. + This file is part of the GNU C Library. + Contributed by Ulrich Drepper <drepper@redhat.com>, 2002. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <http://www.gnu.org/licenses/>. */ + +#include <bits/wordsize.h> + +/* Size in bits of the 'time_t' type. */ +#define __TIMESIZE __WORDSIZE diff --git a/bits/timesizes.h b/bits/timesizes.h new file mode 100644 index 0000000000..0bbbfca8ff --- /dev/null +++ b/bits/timesizes.h @@ -0,0 +1,37 @@ +/* bits/timesizes.h -- underlying types for __time64_t. Generic version. + Copyright (C) 2018 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <http://www.gnu.org/licenses/>. */ + +#ifndef _BITS_TYPES_H +# error "Never include <bits/timesizes.h> directly; use <sys/types.h> instead." +#endif + +#ifndef _BITS_TIMESIZES_H +#define _BITS_TIMESIZES_H 1 + +/* See <bits/types.h> for the meaning of these macros. This file exists so + that <bits/types.h> need not vary across different GNU platforms. */ + +#if __TIMESIZE == 64 +/* If we already have 64-bit time then use it. */ +# define __TIME64_T_TYPE __TIME_T_TYPE +#else +/* Define a 64-bit type alongsize the 32-bit one. */ +# define __TIME64_T_TYPE __SQUAD_TYPE +#endif + +#endif /* bits/timesizes.h */ diff --git a/include/time.h b/include/time.h index 23d2580528..9ca2011f4c 100644 --- a/include/time.h +++ b/include/time.h @@ -3,6 +3,7 @@ #ifndef _ISOMAC # include <bits/types/locale_t.h> +# include <stdbool.h> extern __typeof (strftime_l) __strftime_l; libc_hidden_proto (__strftime_l) @@ -26,10 +27,6 @@ extern __typeof (clock_getcpuclockid) __clock_getcpuclockid; /* Now define the internal interfaces. */ struct tm; -/* time_t variant for representing time zone data, independent of - time_t. */ -typedef __int64_t internal_time_t; - /* Defined in mktime.c. */ extern const unsigned short int __mon_yday[2][13] attribute_hidden; @@ -43,7 +40,7 @@ extern int __use_tzfile attribute_hidden; extern void __tzfile_read (const char *file, size_t extra, char **extrap) attribute_hidden; -extern void __tzfile_compute (internal_time_t timer, int use_localtime, +extern void __tzfile_compute (__time64_t timer, int use_localtime, long int *leap_correct, int *leap_hit, struct tm *tp) attribute_hidden; extern void __tzfile_default (const char *std, const char *dst, @@ -101,10 +98,16 @@ extern char * __strptime_internal (const char *rp, const char *fmt, extern double __difftime (time_t time1, time_t time0); - /* Use in the clock_* functions. Size of the field representing the actual clock ID. */ #define CLOCK_IDFIELD_SIZE 3 +/* Check whether a time64_t value fits in a time_t. */ +static inline bool +fits_in_time_t (__time64_t t) +{ + return t == (time_t) t; +} + #endif #endif diff --git a/posix/bits/types.h b/posix/bits/types.h index 5e22ce41bf..66ccb0a6bc 100644 --- a/posix/bits/types.h +++ b/posix/bits/types.h @@ -25,6 +25,7 @@ #include <features.h> #include <bits/wordsize.h> +#include <bits/timesize.h> /* Convenience types. */ typedef unsigned char __u_char; @@ -138,6 +139,7 @@ __extension__ typedef unsigned long long int __uintmax_t; # error #endif #include <bits/typesizes.h> /* Defines __*_T_TYPE macros. */ +#include <bits/timesizes.h> /* Defines __TIME*_T_TYPE macros. */ __STD_TYPE __DEV_T_TYPE __dev_t; /* Type of device numbers. */ @@ -211,6 +213,12 @@ __STD_TYPE __U32_TYPE __socklen_t; It is not currently necessary for this to be machine-specific. */ typedef int __sig_atomic_t; +#if __TIMESIZE == 64 +# define __time64_t __time_t +#else +__STD_TYPE __TIME64_T_TYPE __time64_t; /* Seconds since the Epoch (_TIME_BITS==64). */ +#endif + #undef __STD_TYPE #endif /* bits/types.h */ diff --git a/stdlib/Makefile b/stdlib/Makefile index 808a8ceab7..c6ef3c8d06 100644 --- a/stdlib/Makefile +++ b/stdlib/Makefile @@ -29,7 +29,7 @@ headers := stdlib.h bits/stdlib.h bits/stdlib-ldbl.h bits/stdlib-float.h \ ucontext.h sys/ucontext.h \ alloca.h fmtmsg.h \ bits/stdlib-bsearch.h sys/random.h bits/stdint-intn.h \ - bits/stdint-uintn.h + bits/stdint-uintn.h bits/timesizes.h bits/timesize.h \ routines := \ atof atoi atol atoll \ diff --git a/sysdeps/unix/sysv/linux/x86/bits/timesize.h b/sysdeps/unix/sysv/linux/x86/bits/timesize.h new file mode 100644 index 0000000000..f8fe3929bc --- /dev/null +++ b/sysdeps/unix/sysv/linux/x86/bits/timesize.h @@ -0,0 +1,26 @@ +/* Bit size of the time_t type at glibc build time, x86-64 and x32 case. + Copyright (C) 2018 Free Software Foundation, Inc. + This file is part of the GNU C Library. + Contributed by Ulrich Drepper <drepper@redhat.com>, 2002. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <http://www.gnu.org/licenses/>. */ + +#if defined __x86_64__ && defined __ILP32__ +/* For x32, time is 64-bit even though word size is 32-bit. */ +# define __TIMESIZE 64 +#else +/* For others, time size is word size. */ +# define __TIMESIZE __WORDSIZE +#endif diff --git a/sysdeps/unix/sysv/linux/x86/bits/timesizes.h b/sysdeps/unix/sysv/linux/x86/bits/timesizes.h new file mode 100644 index 0000000000..6132ec86ab --- /dev/null +++ b/sysdeps/unix/sysv/linux/x86/bits/timesizes.h @@ -0,0 +1,40 @@ +/* bits/typesizes.h -- underlying types for __time64_t. Linux/x86-64 version. + Copyright (C) 2018 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <http://www.gnu.org/licenses/>. */ + +#ifndef _BITS_TYPES_H +# error "Never include <bits/timesizes.h> directly; use <sys/types.h> instead." +#endif + +#ifndef _BITS_TIMESIZES_H +#define _BITS_TIMESIZES_H 1 + +/* See <bits/types.h> for the meaning of these macros. This file exists so + that <bits/types.h> need not vary across different GNU platforms. */ + +#if defined __x86_64__ && defined __ILP32__ +/* For x32, time is 64-bit even though word size is 32-bit. */ +# define __TIME64_T_TYPE __SQUAD_TYPE +#elif __TIMESIZE == 64 +/* If we already have 64-bit time then use it. */ +# define __TIME64_T_TYPE __TIME_T_TYPE +#else +/* Define a 64-bit type alongsize the 32-bit one. */ +# define __TIME64_T_TYPE __SQUAD_TYPE +#endif + +#endif /* bits/timesizes.h */ diff --git a/time/tzfile.c b/time/tzfile.c index 2a385b92bc..d7e391c3a3 100644 --- a/time/tzfile.c +++ b/time/tzfile.c @@ -44,12 +44,12 @@ struct ttinfo struct leap { - internal_time_t transition; /* Time the transition takes effect. */ + __time64_t transition; /* Time the transition takes effect. */ long int change; /* Seconds of correction to apply. */ }; static size_t num_transitions; -libc_freeres_ptr (static internal_time_t *transitions); +libc_freeres_ptr (static __time64_t *transitions); static unsigned char *type_idxs; static size_t num_types; static struct ttinfo *types; @@ -113,8 +113,8 @@ __tzfile_read (const char *file, size_t extra, char **extrap) size_t tzspec_len; char *new = NULL; - _Static_assert (sizeof (internal_time_t) == 8, - "internal_time_t must be eight bytes"); + _Static_assert (sizeof (__time64_t) == 8, + "__time64_t must be eight bytes"); __use_tzfile = 0; @@ -220,9 +220,9 @@ __tzfile_read (const char *file, size_t extra, char **extrap) if (__builtin_expect (num_transitions > ((SIZE_MAX - (__alignof__ (struct ttinfo) - 1)) - / (sizeof (internal_time_t) + 1)), 0)) + / (sizeof (__time64_t) + 1)), 0)) goto lose; - total_size = num_transitions * (sizeof (internal_time_t) + 1); + total_size = num_transitions * (sizeof (__time64_t) + 1); total_size = ((total_size + __alignof__ (struct ttinfo) - 1) & ~(__alignof__ (struct ttinfo) - 1)); types_idx = total_size; @@ -279,7 +279,7 @@ __tzfile_read (const char *file, size_t extra, char **extrap) goto lose; type_idxs = (unsigned char *) transitions + (num_transitions - * sizeof (internal_time_t)); + * sizeof (__time64_t)); types = (struct ttinfo *) ((char *) transitions + types_idx); zone_names = (char *) types + num_types * sizeof (struct ttinfo); leaps = (struct leap *) ((char *) transitions + leaps_idx); @@ -580,7 +580,7 @@ __tzfile_default (const char *std, const char *dst, } void -__tzfile_compute (internal_time_t timer, int use_localtime, +__tzfile_compute (__time64_t timer, int use_localtime, long int *leap_correct, int *leap_hit, struct tm *tp) { @@ -669,7 +669,7 @@ __tzfile_compute (internal_time_t timer, int use_localtime, initial search spot from it. Half of a gregorian year has on average 365.2425 * 86400 / 2 = 15778476 seconds. The value i can be truncated if size_t is smaller than - internal_time_t, but this is harmless because it is just + __time64_t, but this is harmless because it is just a guess. */ i = (transitions[num_transitions - 1] - timer) / 15778476; if (i < num_transitions)