On Tue, 18 Sep 2018, Albert ARIBAUD (3ADEV) wrote:
> * Add macro __TIMESIZE equal to the bit size of time_t.
> It equals the architecture __WORDSIZE except for x32
> where it equals 64.
> * Add type __time64_t which is always 64-bit. On 64-bit
> architectures and on x32, it is #defined as time_t.
> On other architectures, it has its own definition.
> * Replace all occurrences of internal_time_t with
> __time64_t.
These comments are primarily on the proposed commit message, not the code
changes, although they do have implications for comments in the code.
First: I am assuming this patch and commit message are proposed for commit
as-is, since it isn't marked as an RFC; if something is known not to be
ready for commit as-is then it's best to mark it as an RFC and be clear
about the things you know are not yet ready.
We've agreed that commit messages need to be reviewed just as much as code
changes. They are the primary piece of information about a change people
will be looking at when looking at that change in future, so they need to
be written to make sense to those unknown future readers, who may not have
the context we have now - as well as providing an explanation to the
reviewers now.
A commit message generally needs to include both (a) the human-level
explanation of what the change does and why, and (b) the proposed
ChangeLog entry. (a) would normally be some number of paragraphs,
depending on the nature and complexity of the change; it's only for some
very simple changes that the Subject: line of the patch submission is
sufficient without further paragraphs.
In this patch submission, you're missing the ChangeLog entry, In some
others, you have it first, which is confusing to readers; the ChangeLog
entry is expected to be the last thing before the patch itself, so if it's
first that gives the impression there is no human-level explanation at
all.
"bit size of time_t" is ambiguous. Do you mean the size of time_t *for
the default ABI for this libc*, or the size for the current compilation?
Once _TIME_BITS=64 is supported, those can differ. And since this patch
is part of preparing for supporting _TIME_BITS=64, it's critical to know
which is meant, so that __TIMESIZE conditionals in this or subsequent
patches can be properly reviewed according to whether they would be
correct in the context of _TIME_BITS=64. I'm guessing that you mean for
the default ABI for this libc (so it becomes a suitable condition for
whether the default time_t ABIs are aliases to the 64-bit ones or wrappers
round them), but that needs to be explicit, both in the commit message and
in the comment on the default definition of this macro.
Then, I'd expect some high-level description of the purpose of the patch
before discussing anything about the individual macros / types added.
That is, something along the lines of:
glibc support for 64-bit time_t on 32-bit architectures will involve
primarily using 64-bit times inside glibc, with conversions to and from
32-bit times taking place as necessary for interfaces using such times.
This requires a glibc-internal name for a type for times that are always
64-bit. To determine whether the default time_t interfaces are 32-bit
and so need conversions, or are 64-bit and so are compatible with the
internal 64-bit type without conversions, a macro giving the size of the
default time_t is also required. Given the new type, it can then
replace uses of internal_time_t.
After that, the descriptions of the individual macros / types added could
be given (whether as paragraphs or as bullet points).
> +#ifndef _BITS_TYPES_H
> +# error "Never include <bits/time64.h> directly; use <sys/types.h> instead."
> +#endif
> +
> +#ifndef _BITS_TIME64_H
> +#define _BITS_TIME64_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. */
I don't think this comment (copied from bits/typesizes.h) is applicable
here as-is. It's one macro not multiple macros, and that reason is the
reason for bits/typesizes.h to exist, not so much the reason for
bits/time64.h to exist. When it's just one macro, you may as well give
the meaning directly rather than pointing to another file for it.
> +/* Size in bits of the 'time_t' type. */
> +#define __TIMESIZE __WORDSIZE
This is what needs clarifying as being about the default ABI not the
current compilation.
> +#if __TIMESIZE == 64
> +# define __time64_t __time_t
> +#else
> +__STD_TYPE __TIME64_T_TYPE __time64_t; /* Seconds since the Epoch (_TIME_BITS==64). */
I don't think the reference to _TIME_BITS==64 is helpful at this point;
right now, this is the internal type for times (which would also be used
for time_t when _TIME_BITS=64).
> +#ifndef _BITS_TIME64_H
> +#define _BITS_TIME64_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. */
Another repeat of the same comment that I don't think is helpful.
new file mode 100644
@@ -0,0 +1,37 @@
+/* bits/time64.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/time64.h> directly; use <sys/types.h> instead."
+#endif
+
+#ifndef _BITS_TIME64_H
+#define _BITS_TIME64_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/time64.h */
new file mode 100644
@@ -0,0 +1,22 @@
+/* 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.
+
+ 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
@@ -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,7 +98,6 @@ 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
@@ -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/time64.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 */
@@ -29,7 +29,7 @@ headers := stdlib.h bits/stdlib.h bits/stdlib-ldbl.h bits/stdlib-float.h \
ucontext.h sys/ucontext.h bits/indirect-return.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/time64.h bits/timesize.h \
routines := \
atof atoi atol atoll \
new file mode 100644
@@ -0,0 +1,40 @@
+/* bits/time64.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/time64.h> directly; use <sys/types.h> instead."
+#endif
+
+#ifndef _BITS_TIME64_H
+#define _BITS_TIME64_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/time64.h */
new file mode 100644
@@ -0,0 +1,25 @@
+/* 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.
+
+ 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
@@ -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)