[PATCH v5 0/2] Y2038 support batch 1 - __time64_t and __tz_convert

[PATCH v5 0/2] Y2038 support batch 1 - __time64_t and __tz_convert

[Albert ARIBAUD (3ADEV)]

This is the first batch of Y2038 support patches.

The first patch introduces __TIMESIZE (which is the bit size of time_t
at glibc build time, and equals __WORDSIZE except for x32 where __WORDSIZE
is 32 but __TIMESIZE is 64) and __time64_t, the 64-bit counterpart of time_t,
to be used in 64-bit-time implementations of public APIs related to time.
Note that __time64_t replaces internal_time_t previously introduced.

The second patch makes __tz_convert compatible with 64-bit time. This
implies creating 64-bit-time versions of its callers and turning their
original versions into wrappers if they were 32-bit-time.

These patches are functionally tested as part of the whole Y2038 patch
series using the ad hoc framework at https://github.com/3adev/y2038 which
runs tests using times below, at and beyond the Y2038 limit.

These patches were also run through build-many-glibcs.py to ensure that
existing glibc configurations are not broken. The minimal configuration
set is x86_64-linux-gnu, aarch64-linux-gnu, powerpc-linux-gnu, and
arm-linux-gnueabi.

Albert ARIBAUD (3ADEV) (2):
  Y2038: Add 64-bit time for all architectures
  Y2038: make __tz_convert compatible with 64-bit-time

 bits/timesize.h                              | 23 +++++++++
 bits/timesizes.h                             | 37 ++++++++++++++
 include/time.h                               | 54 ++++++++++++++++----
 posix/bits/types.h                           |  8 +++
 stdlib/Makefile                              |  2 +-
 sysdeps/unix/sysv/linux/x86/bits/timesize.h  | 26 ++++++++++
 sysdeps/unix/sysv/linux/x86/bits/timesizes.h | 40 +++++++++++++++
 time/ctime.c                                 | 18 ++++++-
 time/ctime_r.c                               | 20 ++++++--
 time/gmtime.c                                | 40 +++++++++++++--
 time/localtime.c                             | 37 ++++++++++++--
 time/offtime.c                               | 12 ++---
 time/tzfile.c                                | 32 +++++-------
 time/tzset.c                                 | 30 +++++------
 14 files changed, 310 insertions(+), 69 deletions(-)
 create mode 100644 bits/timesize.h
 create mode 100644 bits/timesizes.h
 create mode 100644 sysdeps/unix/sysv/linux/x86/bits/timesize.h
 create mode 100644 sysdeps/unix/sysv/linux/x86/bits/timesizes.h

-- 
2.17.1

[PATCH v5 1/2] Y2038: Add 64-bit time for all architectures

[Albert ARIBAUD (3ADEV)]

* 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 type.

* Replace all occurrences of internal_time_t with
  __time64_t.

The  __time64_t type is public so that the public time_t type
can be a typedef or #define of __time64_t when we switch the
public API to 64-bit time.

Also, provide a function (inline) to check if a __time64_t
value fits in a (possibly 32-bit) time_t. This is used when
a 32-bit-time wrapper calls a glibc function which returns
a __time64_t, so that the wrapper can detect times which
it cannot properly return to its caller, and can flag an
EOVERFLOW accordingly.
---
 bits/timesize.h                              | 23 +++++++++++
 bits/timesizes.h                             | 37 ++++++++++++++++++
 include/time.h                               | 15 +++++---
 posix/bits/types.h                           |  8 ++++
 stdlib/Makefile                              |  2 +-
 sysdeps/unix/sysv/linux/x86/bits/timesize.h  | 26 +++++++++++++
 sysdeps/unix/sysv/linux/x86/bits/timesizes.h | 40 ++++++++++++++++++++
 time/tzfile.c                                | 18 ++++-----
 8 files changed, 153 insertions(+), 16 deletions(-)
 create mode 100644 bits/timesize.h
 create mode 100644 bits/timesizes.h
 create mode 100644 sysdeps/unix/sysv/linux/x86/bits/timesize.h
 create mode 100644 sysdeps/unix/sysv/linux/x86/bits/timesizes.h

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,
 }
 \f
 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)
-- 
2.17.1

Re: [PATCH v5 1/2] Y2038: Add 64-bit time for all architectures

[Paul Eggert]

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;

Re: [PATCH v5 1/2] Y2038: Add 64-bit time for all architectures

[Albert ARIBAUD]

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

Re: [PATCH v5 1/2] Y2038: Add 64-bit time for all architectures

[Albert ARIBAUD]

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

Re: [PATCH v5 1/2] Y2038: Add 64-bit time for all architectures

[Albert ARIBAUD]

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

Re: [PATCH v5 1/2] Y2038: Add 64-bit time for all architectures

[Paul Eggert]

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.

Re: [PATCH v5 1/2] Y2038: Add 64-bit time for all architectures

[Albert ARIBAUD]

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

Re: [PATCH v5 1/2] Y2038: Add 64-bit time for all architectures

[Paul Eggert]

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.

[PATCH v5 2/2] Y2038: make __tz_convert compatible with 64-bit-time

[Albert ARIBAUD (3ADEV)]

This implies that its callers be 64-bit-time compatible too.
It is done by creating 64-bit-time versions of these and
turning their original 32-bit-time versions into wrappers
(at a slight execution time cost).

The callers affected are:

  * localtime
  * localtime_r
  * ctime
  * ctime_r
  * gmtime
  * gmtime_r

Note that in time/tzfile.c we do not need to check for time_t
overflows anymore as introduced by commit fc79706a323 since we
now use internal_time_t.
---
 include/time.h   | 39 ++++++++++++++++++++++++++++++++++-----
 time/ctime.c     | 18 ++++++++++++++++--
 time/ctime_r.c   | 20 +++++++++++++++++---
 time/gmtime.c    | 40 +++++++++++++++++++++++++++++++++++-----
 time/localtime.c | 37 +++++++++++++++++++++++++++++++++----
 time/offtime.c   | 12 ++++++------
 time/tzfile.c    | 14 ++++----------
 time/tzset.c     | 30 ++++++++++++------------------
 8 files changed, 157 insertions(+), 53 deletions(-)

diff --git a/include/time.h b/include/time.h
index 9ca2011f4c..eaec898fac 100644
--- a/include/time.h
+++ b/include/time.h
@@ -47,7 +47,7 @@ extern void __tzfile_default (const char *std, const char *dst,
 			      long int stdoff, long int dstoff)
   attribute_hidden;
 extern void __tzset_parse_tz (const char *tz) attribute_hidden;
-extern void __tz_compute (time_t timer, struct tm *tm, int use_localtime)
+extern void __tz_compute (__time64_t timer, struct tm *tm, int use_localtime)
   __THROW attribute_hidden;
 
 /* Subroutine of `mktime'.  Return the `time_t' representation of TP and
@@ -57,18 +57,47 @@ extern time_t __mktime_internal (struct tm *__tp,
 				 struct tm *(*__func) (const time_t *,
 						       struct tm *),
 				 time_t *__offset) attribute_hidden;
+
+/* nis/nis_print.c needs ctime, so even if ctime is not declared here,
+   we define __ctime64 as ctime so that nis/nis_print.c can get linked
+   against a function called ctime. */
+#if __TIMESIZE == 64
+# define __ctime64 ctime
+#endif
+
+#if __TIMESIZE == 64
+# define __localtime64 localtime
+#else
+extern struct tm *__localtime64 (const __time64_t *__timer);
+#endif
+
 extern struct tm *__localtime_r (const time_t *__timer,
 				 struct tm *__tp) attribute_hidden;
 
+#if __TIMESIZE == 64
+# define __localtime64_r __localtime_r
+#else
+extern struct tm *__localtime64_r (const __time64_t *__timer,
+				   struct tm *__tp) attribute_hidden;
+#endif
+
 extern struct tm *__gmtime_r (const time_t *__restrict __timer,
 			      struct tm *__restrict __tp);
 libc_hidden_proto (__gmtime_r)
 
-/* Compute the `struct tm' representation of *T,
+#if __TIMESIZE == 64
+# define __gmtime64 gmtime
+# define __gmtime64_r __gmtime_r
+#else
+extern struct tm *__gmtime64_r (const __time64_t *__restrict __timer,
+			        struct tm *__restrict __tp);
+#endif
+
+/* Compute the `struct tm' representation of T,
    offset OFFSET seconds east of UTC,
    and store year, yday, mon, mday, wday, hour, min, sec into *TP.
    Return nonzero if successful.  */
-extern int __offtime (const time_t *__timer,
+extern int __offtime (const __time64_t __timer,
 		      long int __offset,
 		      struct tm *__tp) attribute_hidden;
 
@@ -77,8 +106,8 @@ extern char *__asctime_r (const struct tm *__tp, char *__buf)
 extern void __tzset (void) attribute_hidden;
 
 /* Prototype for the internal function to get information based on TZ.  */
-extern struct tm *__tz_convert (const time_t *timer, int use_localtime,
-				struct tm *tp) attribute_hidden;
+extern struct tm *__tz_convert (const __time64_t timer, int use_localtime,
+			        struct tm *tp) attribute_hidden;
 
 extern int __nanosleep (const struct timespec *__requested_time,
 			struct timespec *__remaining);
diff --git a/time/ctime.c b/time/ctime.c
index 1222614f29..fe6eb45cae 100644
--- a/time/ctime.c
+++ b/time/ctime.c
@@ -16,13 +16,27 @@
    <http://www.gnu.org/licenses/>.  */
 
 #include <time.h>
+#include <errno.h>
 
 /* Return a string as returned by asctime which
    is the representation of *T in that form.  */
 char *
-ctime (const time_t *t)
+__ctime64 (const __time64_t *t)
 {
   /* The C Standard says ctime (t) is equivalent to asctime (localtime (t)).
      In particular, ctime and asctime must yield the same pointer.  */
-  return asctime (localtime (t));
+  return asctime (__localtime64 (t));
 }
+
+/* Provide a 32-bit wrapper if needed */
+
+#if __TIMESIZE != 64
+
+char *
+ctime (const time_t *t)
+{
+  __time64_t t64 = *t;
+  return __ctime64 (&t64);
+}
+
+#endif
diff --git a/time/ctime_r.c b/time/ctime_r.c
index c111146d76..e731b53a77 100644
--- a/time/ctime_r.c
+++ b/time/ctime_r.c
@@ -18,12 +18,26 @@
    <http://www.gnu.org/licenses/>.  */
 
 #include <time.h>
+#include <errno.h>
 
 /* Return a string as returned by asctime which is the representation
-   of *T in that form.  Reentrant version.  */
+   of *T in that form.  */
 char *
-ctime_r (const time_t *t, char *buf)
+__ctime64_r (const __time64_t *t, char *buf)
 {
   struct tm tm;
-  return __asctime_r (__localtime_r (t, &tm), buf);
+  return __asctime_r (__localtime64_r (t, &tm), buf);
 }
+
+/* Provide a 32-bit wrapper if needed */
+
+#if __TIMESIZE != 64
+
+char *
+ctime_r (const time_t *t, char *buf)
+{
+  __time64_t t64 = *t;
+  return __ctime64_r (&t64, buf);
+}
+
+#endif
diff --git a/time/gmtime.c b/time/gmtime.c
index dc33b3e68a..689784d22e 100644
--- a/time/gmtime.c
+++ b/time/gmtime.c
@@ -17,21 +17,51 @@
    <http://www.gnu.org/licenses/>.  */
 
 #include <time.h>
+#include <errno.h>
+
+/* Return the `struct tm' representation of 64-bit-time *T
+   in UTC, using *TP to store the result.  */
+struct tm *
+__gmtime64_r (const __time64_t *t, struct tm *tp)
+{
+  return __tz_convert (*t, 0, tp);
+}
+
+/* Provide a 32-bit wrapper if needed */
+
+#if __TIMESIZE != 64
 
-/* Return the `struct tm' representation of *T in UTC,
-   using *TP to store the result.  */
 struct tm *
 __gmtime_r (const time_t *t, struct tm *tp)
 {
-  return __tz_convert (t, 0, tp);
+  __time64_t t64 = *t;
+  return __gmtime64_r (&t64, tp);
 }
+
+#endif
+
+/* This always works because either __TIMESIZE != 64 and __gmtime_r exists
+   or __TIMESIZE == 64 and the definition of __gmtime64_r above actually
+   defined __gmtime_r.  */
 libc_hidden_def (__gmtime_r)
 weak_alias (__gmtime_r, gmtime_r)
 
+/* Return the `struct tm' representation of 64-bit-time *T in UTC.	*/
+struct tm *
+__gmtime64 (const __time64_t *t)
+{
+  return __tz_convert (*t, 0, &_tmbuf);
+}
+
+/* Provide a 32-bit wrapper if needed */
+
+#if __TIMESIZE != 64
 
-/* Return the `struct tm' representation of *T in UTC.	*/
 struct tm *
 gmtime (const time_t *t)
 {
-  return __tz_convert (t, 0, &_tmbuf);
+  __time64_t t64 = *t;
+  return __gmtime64 (&t64);
 }
+
+#endif
diff --git a/time/localtime.c b/time/localtime.c
index 8684a8a971..029b58ff2a 100644
--- a/time/localtime.c
+++ b/time/localtime.c
@@ -17,25 +17,54 @@
    <http://www.gnu.org/licenses/>.  */
 
 #include <time.h>
+#include <errno.h>
 
 /* The C Standard says that localtime and gmtime return the same pointer.  */
 struct tm _tmbuf;
 
-
 /* Return the `struct tm' representation of *T in local time,
    using *TP to store the result.  */
+struct tm *
+__localtime64_r (const __time64_t *t, struct tm *tp)
+{
+  return __tz_convert (*t, 1, tp);
+}
+
+/* Provide a 32-bit wrapper if needed */
+
+#if __TIMESIZE != 64
+
 struct tm *
 __localtime_r (const time_t *t, struct tm *tp)
 {
-  return __tz_convert (t, 1, tp);
+  __time64_t t64 = *t;
+  return __localtime64_r (&t64, tp);
 }
-weak_alias (__localtime_r, localtime_r)
 
+#endif
+
+/* This always works because either __TIMESIZE != 64 and __localtime_r
+   exists or __TIMESIZE == 64 and the definition of __localtime64_r above
+   actually defined __localtime_r.  */
+weak_alias (__localtime_r, localtime_r)
 
 /* Return the `struct tm' representation of *T in local time.  */
+struct tm *
+__localtime64 (const __time64_t *t)
+{
+  return __tz_convert (*t, 1, &_tmbuf);
+}
+
+/* Provide a 32-bit wrapper if needed */
+
+#if __TIMESIZE != 64
+
 struct tm *
 localtime (const time_t *t)
 {
-  return __tz_convert (t, 1, &_tmbuf);
+  __time64_t t64 = *t;
+  return __localtime64 (&t64);
 }
 libc_hidden_def (localtime)
+
+#endif
diff --git a/time/offtime.c b/time/offtime.c
index 04c48389fc..ede23c418f 100644
--- a/time/offtime.c
+++ b/time/offtime.c
@@ -21,18 +21,18 @@
 #define	SECS_PER_HOUR	(60 * 60)
 #define	SECS_PER_DAY	(SECS_PER_HOUR * 24)
 
-/* Compute the `struct tm' representation of *T,
+/* Compute the `struct tm' representation of T,
    offset OFFSET seconds east of UTC,
    and store year, yday, mon, mday, wday, hour, min, sec into *TP.
    Return nonzero if successful.  */
 int
-__offtime (const time_t *t, long int offset, struct tm *tp)
+__offtime (const __time64_t t, long int offset, struct tm *tp)
 {
-  time_t days, rem, y;
+  __time64_t days, rem, y;
   const unsigned short int *ip;
 
-  days = *t / SECS_PER_DAY;
-  rem = *t % SECS_PER_DAY;
+  days = t / SECS_PER_DAY;
+  rem = t % SECS_PER_DAY;
   rem += offset;
   while (rem < 0)
     {
@@ -60,7 +60,7 @@ __offtime (const time_t *t, long int offset, struct tm *tp)
   while (days < 0 || days >= (__isleap (y) ? 366 : 365))
     {
       /* Guess a corrected year, assuming 365 days per year.  */
-      time_t yg = y + days / 365 - (days % 365 < 0);
+      __time64_t yg = y + days / 365 - (days % 365 < 0);
 
       /* Adjust DAYS and Y to match the guessed year.  */
       days -= ((yg - y) * 365
diff --git a/time/tzfile.c b/time/tzfile.c
index d7e391c3a3..972e3ff5cf 100644
--- a/time/tzfile.c
+++ b/time/tzfile.c
@@ -635,16 +635,10 @@ __tzfile_compute (__time64_t timer, int use_localtime,
 
 	  /* Convert to broken down structure.  If this fails do not
 	     use the string.  */
-	  {
-	    time_t truncated = timer;
-	    if (__glibc_unlikely (truncated != timer
-				  || ! __offtime (&truncated, 0, tp)))
-	      goto use_last;
-	  }
-
-	  /* Use the rules from the TZ string to compute the change.
-	     timer fits into time_t due to the truncation check
-	     above.  */
+	  if (__glibc_unlikely (! __offtime (timer, 0, tp)))
+	    goto use_last;
+
+	  /* Use the rules from the TZ string to compute the change.  */
 	  __tz_compute (timer, tp, 1);
 
 	  /* If tzspec comes from posixrules loaded by __tzfile_default,
diff --git a/time/tzset.c b/time/tzset.c
index a828b9fb75..4e91d9dca6 100644
--- a/time/tzset.c
+++ b/time/tzset.c
@@ -16,7 +16,6 @@
    <http://www.gnu.org/licenses/>.  */
 
 #include <ctype.h>
-#include <errno.h>
 #include <libc-lock.h>
 #include <stdbool.h>
 #include <stddef.h>
@@ -27,7 +26,7 @@
 
 #include <timezone/tzfile.h>
 
-#define SECSPERDAY ((time_t) 86400)
+#define SECSPERDAY ((__time64_t) 86400)
 
 char *__tzname[2] = { (char *) "GMT", (char *) "GMT" };
 int __daylight = 0;
@@ -55,7 +54,7 @@ typedef struct
 
     /* We cache the computed time of change for a
        given year so we don't have to recompute it.  */
-    time_t change;	/* When to change to this zone.  */
+    __time64_t change;	/* When to change to this zone.  */
     int computed_for;	/* Year above is computed for.  */
   } tz_rule;
 
@@ -416,7 +415,7 @@ tzset_internal (int always)
       tz_rules[0].name = tz_rules[1].name = "UTC";
       if (J0 != 0)
 	tz_rules[0].type = tz_rules[1].type = J0;
-      tz_rules[0].change = tz_rules[1].change = (time_t) -1;
+      tz_rules[0].change = tz_rules[1].change = -1;
       update_vars ();
       return;
     }
@@ -424,13 +423,13 @@ tzset_internal (int always)
   __tzset_parse_tz (tz);
 }
 \f
-/* Figure out the exact time (as a time_t) in YEAR
+/* Figure out the exact time (as a __time64_t) in YEAR
    when the change described by RULE will occur and
    put it in RULE->change, saving YEAR in RULE->computed_for.  */
 static void
 compute_change (tz_rule *rule, int year)
 {
-  time_t t;
+  __time64_t t;
 
   if (year != -1 && rule->computed_for == year)
     /* Operations on times in 2 BC will be slower.  Oh well.  */
@@ -514,9 +513,10 @@ compute_change (tz_rule *rule, int year)
 
 
 /* Figure out the correct timezone for TM and set `__tzname',
-   `__timezone', and `__daylight' accordingly.  */
+   `__timezone', and `__daylight' accordingly.
+   NOTE: this takes a __time64_t value, so passing a time_t value is OK. */
 void
-__tz_compute (time_t timer, struct tm *tm, int use_localtime)
+__tz_compute (__time64_t timer, struct tm *tm, int use_localtime)
 {
   compute_change (&tz_rules[0], 1900 + tm->tm_year);
   compute_change (&tz_rules[1], 1900 + tm->tm_year);
@@ -562,20 +562,14 @@ __tzset (void)
 }
 weak_alias (__tzset, tzset)
 \f
-/* Return the `struct tm' representation of *TIMER in the local timezone.
+/* Return the `struct tm' representation of TIMER in the local timezone.
    Use local time if USE_LOCALTIME is nonzero, UTC otherwise.  */
 struct tm *
-__tz_convert (const time_t *timer, int use_localtime, struct tm *tp)
+__tz_convert (const __time64_t timer, int use_localtime, struct tm *tp)
 {
   long int leap_correction;
   int leap_extra_secs;
 
-  if (timer == NULL)
-    {
-      __set_errno (EINVAL);
-      return NULL;
-    }
-
   __libc_lock_lock (tzset_lock);
 
   /* Update internal database according to current TZ setting.
@@ -584,14 +578,14 @@ __tz_convert (const time_t *timer, int use_localtime, struct tm *tp)
   tzset_internal (tp == &_tmbuf && use_localtime);
 
   if (__use_tzfile)
-    __tzfile_compute (*timer, use_localtime, &leap_correction,
+    __tzfile_compute (timer, use_localtime, &leap_correction,
 		      &leap_extra_secs, tp);
   else
     {
       if (! __offtime (timer, 0, tp))
 	tp = NULL;
       else
-	__tz_compute (*timer, tp, use_localtime);
+	__tz_compute (timer, tp, use_localtime);
       leap_correction = 0L;
       leap_extra_secs = 0;
     }
-- 
2.17.1

Re: [PATCH v5 2/2] Y2038: make __tz_convert compatible with 64-bit-time

[Paul Eggert]

On 06/18/2018 12:14 PM, Albert ARIBAUD (3ADEV) wrote:
> +#include <errno.h>

Why does time/ctime.h need this addition? I don't see where the rest of 
the patch is using anything that errno.h defines. Similarly for 
time/ctime_r.c, time/gmtime.c, time/localtime.c. I suspect this is a 
leftover of the older version of this patch that had __set_errno 
(EINVAL), and I suggest not including <errno.h> in these files.

>   /* Figure out the correct timezone for TM and set `__tzname',
> -   `__timezone', and `__daylight' accordingly.  */
> +   `__timezone', and `__daylight' accordingly.
> +   NOTE: this takes a __time64_t value, so passing a time_t value is OK. */
>   void
> -__tz_compute (time_t timer, struct tm *tm, int use_localtime)
> +__tz_compute (__time64_t timer, struct tm *tm, int use_localtime)

Why is that "NOTE:" comment line important or needed? The proposed set 
of patches never pass a time_t value to __tz_compute. I suspect this 
line is a leftover from an older version of the patch, and suggest 
removing that line.

Otherwise, the patch looks OK to me.

Re: [PATCH v5 2/2] Y2038: make __tz_convert compatible with 64-bit-time

[Albert ARIBAUD]

Hi Paul,

On Tue, 19 Jun 2018 16:35:53 -0700, Paul Eggert <eggert@cs.ucla.edu>
wrote :

> On 06/18/2018 12:14 PM, Albert ARIBAUD (3ADEV) wrote:
> > +#include <errno.h>  
> 
> Why does time/ctime.h need this addition? I don't see where the rest of 
> the patch is using anything that errno.h defines. Similarly for 
> time/ctime_r.c, time/gmtime.c, time/localtime.c. I suspect this is a 
> leftover of the older version of this patch that had __set_errno 
> (EINVAL), and I suggest not including <errno.h> in these files.

Indeed.

> >   /* Figure out the correct timezone for TM and set `__tzname',
> > -   `__timezone', and `__daylight' accordingly.  */
> > +   `__timezone', and `__daylight' accordingly.
> > +   NOTE: this takes a __time64_t value, so passing a time_t value is OK. */
> >   void
> > -__tz_compute (time_t timer, struct tm *tm, int use_localtime)
> > +__tz_compute (__time64_t timer, struct tm *tm, int use_localtime)  
> 
> Why is that "NOTE:" comment line important or needed? The proposed set 
> of patches never pass a time_t value to __tz_compute. I suspect this 
> line is a leftover from an older version of the patch, and suggest 
> removing that line.

You're right.

> Otherwise, the patch looks OK to me.

Thanks!

Cordialement,
Albert ARIBAUD
3ADEV