diff mbox

[v13,2/2] Y2038: make __tz_convert compatible with 64-bit-time

Message ID 20181024103532.3097-2-albert.aribaud@3adev.fr
State New, archived
Headers

Commit Message

Albert ARIBAUD Oct. 24, 2018, 10:35 a.m. UTC 
  Now that __time64_t exists, we can switch internal function
__tz_convert from 32-bit to 64-bit time. This involves switching
some other internal functions and turning some implementations
which use these into wrappers between public 32-bit and internal
64-bit time.

This patch was tested by running 'make check' on branch
master then applying this patch and its two predecessors and
running 'make check' again, and checking that both 'make check'
yield identical results. This was done on x86_64-linux-gnu and
i686-linux-gnu.

	* include/time.h
	(__tz_compute): Replace time_t with __time64_t.
	(__ctime64): Add.
	(__ctime64_r): Likewise.
	(__localtime64): Likewise.
	(__localtime64_r): Likewise.
	(__gmtime64): Likewise.
	(__gmtime64_r): Likewise.
	(__offtime): Replace const time_t* argument with __time64_t.
	(__tz_convert): Likewise.
	* time/ctime.c
	(__ctime64): Add.
	(__ctime): Turn into a wrapper.
	* time/ctime_r.c
	(__ctime64_r): Add.
	(__ctime_r): Turn into a wrapper.
	* time/gmtime.c
	(__gmtime64): Add.
	(__gmtime): Turn into a wrapper.
	(__gmtime64_r): Add.
	(__gmtime_r): Turn into a wrapper.
	* time/localtime.c
	(__localtime64): Add.
	(__localtime): Turn into a wrapper.
	(__localtime64_r): Add.
	(__localtime_r): Turn into a wrapper.
	* time/offtime.c: Replace all time_t occurrences with __time64_t.
	(__offtime): Replace const time_t* argument with __time64_t.
	* time/tzfile.c
	(__tzfile_compute): Adjust __offtime() call arguments.
	* time/tzset.c: Replace all time_t occurrences with __time64_t.
	(__tz_convert): Adjust __tzfile_compute and __tz_compute call arguments.
---
v13:
	- Fixed typo in commit message: __time_64_t => __time64_t
	- Reformatted commit message changelog

 include/time.h   | 44 +++++++++++++++++++++++++++++++++++++++-----
 time/ctime.c     | 17 +++++++++++++++--
 time/ctime_r.c   | 17 +++++++++++++++--
 time/gmtime.c    | 37 +++++++++++++++++++++++++++++++++----
 time/localtime.c | 37 +++++++++++++++++++++++++++++++++----
 time/offtime.c   | 12 ++++++------
 time/tzfile.c    | 14 ++++----------
 time/tzset.c     | 27 ++++++++++-----------------
 8 files changed, 155 insertions(+), 50 deletions(-)

Comments

Joseph Myers Oct. 24, 2018, 4:14 p.m. UTC | #1 
Does anyone see a way to make the code dealing with defining wrappers on 
systems with 32-bit time_t, and using #define to avoid the need for such 
wrappers on systems where time_t is only ever 64-bit, less repetitive?  My 
guess is that's hard to do on two counts: (a) because across all the 
functions in glibc that involve time, there are many differences of detail 
in the wrappers needed; and (b) because the header code, although more 
repetitive, involves #define so can't simply be generated from the 
expansion of some wrapper-generating macro.  ((b) is similar to various 
questions arising from ideas about symbol namespace that might involve 
many more such repetitive declarations in installed headers, depending on 
whether we eliminate any support for compilers without asm redirection 
support using installed glibc headers.)

In any case, this patch does two different things: it changes certain 
*internal* interfaces to use 64-bit time (and also to use time values 
passed directly rather than via pointers), and it sets up versions of 
*public* interfaces that use 64-bit time.  I'd rather keep the two 
separate, doing the internal change first (since the internal functions in 
question don't appear to call any of the public interfaces, only the other 
way round).  The internal change should be uncontroversial; the change to 
public interfaces at least could do with more people looking carefully at 
the design, as we're going to get many more such wrappers and so want to 
make sure the design is the best we can come up with before adding lots of 
repetitive code.
Albert ARIBAUD Oct. 24, 2018, 4:33 p.m. UTC | #2 
Hi Joseph,

Le Wed, 24 Oct 2018 16:14:54 +0000, Joseph Myers
<joseph@codesourcery.com> a écrit :

> Does anyone see a way to make the code dealing with defining wrappers on 
> systems with 32-bit time_t, and using #define to avoid the need for such 
> wrappers on systems where time_t is only ever 64-bit, less repetitive?  My 
> guess is that's hard to do on two counts: (a) because across all the 
> functions in glibc that involve time, there are many differences of detail 
> in the wrappers needed; and (b) because the header code, although more 
> repetitive, involves #define so can't simply be generated from the 
> expansion of some wrapper-generating macro.  ((b) is similar to various 
> questions arising from ideas about symbol namespace that might involve 
> many more such repetitive declarations in installed headers, depending on 
> whether we eliminate any support for compilers without asm redirection 
> support using installed glibc headers.)

I too have noticed how repetitive this is, but I can't see any way to
simplify this code apart from running it through some preprocessor
(other than the cpp it will run through as part of compilation, that
is), and that seems overkill to me.

> In any case, this patch does two different things: it changes certain 
> *internal* interfaces to use 64-bit time (and also to use time values 
> passed directly rather than via pointers), and it sets up versions of 
> *public* interfaces that use 64-bit time.  I'd rather keep the two 
> separate, doing the internal change first (since the internal functions in 
> question don't appear to call any of the public interfaces, only the other 
> way round).  The internal change should be uncontroversial; the change to 
> public interfaces at least could do with more people looking carefully at 
> the design, as we're going to get many more such wrappers and so want to 
> make sure the design is the best we can come up with before adding lots of 
> repetitive code.

I'm fine with splitting the patch, keeping the internal changes here and
moving the public interface changes further down the series.

Cordialement,
Albert ARIBAUD
3ADEV
Albert ARIBAUD Oct. 24, 2018, 9:09 p.m. UTC | #3 
On Wed, 24 Oct 2018 18:33:15 +0200, Albert ARIBAUD
<albert.aribaud@3adev.fr> wrote :

> I'm fine with splitting the patch, keeping the internal changes here and
> moving the public interface changes further down the series.

In fact, I could even break the internal changes patch further, into
one patch per internal function being changed. That would give:

- one patch to switch __offtime to 64-bit-time arguments and adjust
  calls to it from __tzfile_compute and __tz_convert;

- one patch to switch __tz_compute to 64-bit-time arguments and adjust
  calls to it from __tzfile_compute and __tz_convert;

- one patch to switch __tzfile_compute to 64-bit-time arguments and
  adjust the call to it from __tz_convert;

- one patch to switch __tz_convert to 64-bit-time argument and adjust
  the calls to it from gmtime, gmtime_r, localtime, and localtime_r.

Which is the preferred way? Only one, big-ish patch, or a set of
atomic ones?

Cordialement,
Albert ARIBAUD
3ADEV
Joseph Myers Oct. 25, 2018, 12:09 a.m. UTC | #4 
On Wed, 24 Oct 2018, Albert ARIBAUD wrote:

> On Wed, 24 Oct 2018 18:33:15 +0200, Albert ARIBAUD
> <albert.aribaud@3adev.fr> wrote :
> 
> > I'm fine with splitting the patch, keeping the internal changes here and
> > moving the public interface changes further down the series.
> 
> In fact, I could even break the internal changes patch further, into
> one patch per internal function being changed. That would give:
> 
> - one patch to switch __offtime to 64-bit-time arguments and adjust
>   calls to it from __tzfile_compute and __tz_convert;
> 
> - one patch to switch __tz_compute to 64-bit-time arguments and adjust
>   calls to it from __tzfile_compute and __tz_convert;
> 
> - one patch to switch __tzfile_compute to 64-bit-time arguments and
>   adjust the call to it from __tz_convert;
> 
> - one patch to switch __tz_convert to 64-bit-time argument and adjust
>   the calls to it from gmtime, gmtime_r, localtime, and localtime_r.
> 
> Which is the preferred way? Only one, big-ish patch, or a set of
> atomic ones?

I don't think splitting the internal changes by function is particularly 
beneficial to the review, so in this case I'd say one patch with the 
interface changes to all those functions.
diff mbox

Patch

diff --git a/include/time.h b/include/time.h
index 861b1acfb7..1bbfb10149 100644
--- a/include/time.h
+++ b/include/time.h
@@ -46,7 +46,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
@@ -56,18 +56,52 @@  extern time_t __mktime_internal (struct tm *__tp,
 				 struct tm *(*__func) (const time_t *,
 						       struct tm *),
 				 long int *__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 __ctime64_r ctime_r
+#endif
+
+#if __TIMESIZE == 64
+# define __localtime64 localtime
+#else
+extern struct tm *__localtime64 (const __time64_t *__timer);
+libc_hidden_proto (__localtime64)
+#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 (__time64_t __timer,
 		      long int __offset,
 		      struct tm *__tp) attribute_hidden;
 
@@ -76,8 +110,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 (__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..286c6b579c 100644
--- a/time/ctime.c
+++ b/time/ctime.c
@@ -20,9 +20,22 @@ 
 /* 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..84089599a7 100644
--- a/time/ctime_r.c
+++ b/time/ctime_r.c
@@ -22,8 +22,21 @@ 
 /* Return a string as returned by asctime which is the representation
    of *T in that form.  Reentrant version.  */
 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..d485a38c6f 100644
--- a/time/gmtime.c
+++ b/time/gmtime.c
@@ -18,20 +18,49 @@ 
 
 #include <time.h>
 
-/* Return the `struct tm' representation of *T in UTC,
+/* Return the `struct tm' representation of 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
+
 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..8e3b76e6cb 100644
--- a/time/localtime.c
+++ b/time/localtime.c
@@ -21,21 +21,50 @@ 
 /* 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);
+}
+libc_hidden_def (__localtime64)
+
+/* 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..3309fcd484 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 (__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 844a68de8c..3920525bf3 100644
--- a/time/tzfile.c
+++ b/time/tzfile.c
@@ -633,16 +633,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..834cc3ccec 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);
 }
 
-/* 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.  */
@@ -516,7 +515,7 @@  compute_change (tz_rule *rule, int year)
 /* Figure out the correct timezone for TM and set `__tzname',
    `__timezone', and `__daylight' accordingly.  */
 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 +561,14 @@  __tzset (void)
 }
 weak_alias (__tzset, tzset)
 
-/* 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 (__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 +577,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;
     }