[v7,1/2] Y2038: Add 64-bit time for all architectures

Message ID 20180918204249.29026-1-albert.aribaud@3adev.fr
State New
Headers show
Series
  • [v7,1/2] Y2038: Add 64-bit time for all architectures
Related show

Commit Message

Albert ARIBAUD Sept. 18, 2018, 8:42 p.m.
* 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.
---
 bits/time64.h                               | 37 +++++++++++++++++++
 bits/timesize.h                             | 22 ++++++++++++
 include/time.h                              |  8 ++---
 posix/bits/types.h                          |  8 +++++
 stdlib/Makefile                             |  2 +-
 sysdeps/unix/sysv/linux/x86/bits/time64.h   | 40 +++++++++++++++++++++
 sysdeps/unix/sysv/linux/x86/bits/timesize.h | 25 +++++++++++++
 time/tzfile.c                               | 18 +++++-----
 8 files changed, 144 insertions(+), 16 deletions(-)
 create mode 100644 bits/time64.h
 create mode 100644 bits/timesize.h
 create mode 100644 sysdeps/unix/sysv/linux/x86/bits/time64.h
 create mode 100644 sysdeps/unix/sysv/linux/x86/bits/timesize.h

Comments

Joseph Myers Sept. 19, 2018, 4:06 p.m. | #1
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.

Patch

diff --git a/bits/time64.h b/bits/time64.h
new file mode 100644
index 0000000000..a524588586
--- /dev/null
+++ b/bits/time64.h
@@ -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 */
diff --git a/bits/timesize.h b/bits/timesize.h
new file mode 100644
index 0000000000..d8a7804989
--- /dev/null
+++ b/bits/timesize.h
@@ -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
diff --git a/include/time.h b/include/time.h
index e30c5fc3b1..082b5746e3 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,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
diff --git a/posix/bits/types.h b/posix/bits/types.h
index 5e22ce41bf..f36338141f 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/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 */
diff --git a/stdlib/Makefile b/stdlib/Makefile
index 01194bbf7c..bdb0a18295 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 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						      \
diff --git a/sysdeps/unix/sysv/linux/x86/bits/time64.h b/sysdeps/unix/sysv/linux/x86/bits/time64.h
new file mode 100644
index 0000000000..a5e97a0d05
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/x86/bits/time64.h
@@ -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 */
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..8b88ab84b0
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/x86/bits/timesize.h
@@ -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
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)