[v2,1/2] Y2038: make __mktime_internal compatible with __time64_t

Message ID 20190227112042.1794-1-lukma@denx.de
State Committed
Headers

Commit Message

Lukasz Majewski Feb. 27, 2019, 11:20 a.m. UTC
  From: Paul Eggert <eggert@cs.ucla.edu>

This implies also making its callers 64-bit-time compatible
(these are mktime/localtime and timegm) and providing wrappers
for 32-bit-time userland to call.

Tested with 'make check' on x86_64-linux-gnu and i686-linux.gnu.

* include/time.h (__mktime64): Add prototype.
* include/time.h (__localtime64): Likewise.
* include/time.h (fits_in_time_t): New static function.
* time/mktime.c (__mktime64): New function.
* time/timegm.c (__timegm64): Likewise.
* time/mktime.c (mktime) [__TIMESIZE]: New wrapper function.
* time/timegm.c (timegm) [__TIMESIZE]: Likewise.

---
Applicable on top of the newest glibc's master branch
SH1: aa0e46636a5b71b609a41e9ab97134cd76ac1522

Tested with make check for x86_64, i585 and ARM setups.

Changes for v2:
Add missing space (__set_errno (EOVERFLOW)) in src/time/timegm.c
Add missing space (__set_errno (EOVERFLOW)) in src/time/mktime.c
---
 include/time.h | 36 +++++++++++++++++++++-----
 time/mktime.c  | 80 ++++++++++++++++++++++++++++++++++++++++++----------------
 time/timegm.c  | 27 +++++++++++++++++++-
 3 files changed, 114 insertions(+), 29 deletions(-)
  

Comments

Lukasz Majewski March 6, 2019, 11:31 a.m. UTC | #1
Dear Joseph, Paul,

> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> This implies also making its callers 64-bit-time compatible
> (these are mktime/localtime and timegm) and providing wrappers
> for 32-bit-time userland to call.
> 
> Tested with 'make check' on x86_64-linux-gnu and i686-linux.gnu.
> 
> * include/time.h (__mktime64): Add prototype.
> * include/time.h (__localtime64): Likewise.
> * include/time.h (fits_in_time_t): New static function.
> * time/mktime.c (__mktime64): New function.
> * time/timegm.c (__timegm64): Likewise.
> * time/mktime.c (mktime) [__TIMESIZE]: New wrapper function.
> * time/timegm.c (timegm) [__TIMESIZE]: Likewise.
> 
> ---
> Applicable on top of the newest glibc's master branch
> SH1: aa0e46636a5b71b609a41e9ab97134cd76ac1522
> 
> Tested with make check for x86_64, i585 and ARM setups.
> 
> Changes for v2:
> Add missing space (__set_errno (EOVERFLOW)) in src/time/timegm.c
> Add missing space (__set_errno (EOVERFLOW)) in src/time/mktime.c

Gentle ping on this series.

> ---
>  include/time.h | 36 +++++++++++++++++++++-----
>  time/mktime.c  | 80
> ++++++++++++++++++++++++++++++++++++++++++----------------
> time/timegm.c  | 27 +++++++++++++++++++- 3 files changed, 114
> insertions(+), 29 deletions(-)
> 
> diff --git a/include/time.h b/include/time.h
> index 61dd9e180b..34e5e04820 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)
> @@ -16,6 +17,21 @@ libc_hidden_proto (localtime)
>  libc_hidden_proto (strftime)
>  libc_hidden_proto (strptime)
>  
> +#if __TIMESIZE == 64
> +# define __timegm64 timegm
> +# define __mktime64 mktime
> +# define __timelocal64 timelocal
> +#else
> +extern __time64_t __timegm64 (struct tm *__tp) __THROW;
> +extern __time64_t __mktime64 (struct tm *__tp) __THROW;
> +/* Another name for `__mktime64'.  */
> +extern __time64_t __timelocal64 (struct tm *__tp) __THROW;
> +
> +libc_hidden_proto (__mktime64)
> +libc_hidden_proto (__timelocal64)
> +#endif
> +
> +
>  extern __typeof (clock_getres) __clock_getres;
>  extern __typeof (clock_gettime) __clock_gettime;
>  libc_hidden_proto (__clock_gettime)
> @@ -49,13 +65,13 @@ extern void __tzset_parse_tz (const char *tz)
> attribute_hidden; 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
> -   normalize TP, given that a `struct tm *' maps to a `time_t' as
> performed +/* Subroutine of mktime.  Return the __time64_t
> representation of TP and
> +   normalize TP, given that a struct tm * maps to a __time64_t as
> performed by FUNC.  Record next guess for localtime-gmtime offset in
> *OFFSET.  */ -extern time_t __mktime_internal (struct tm *__tp,
> -				 struct tm *(*__func) (const time_t
> *,
> -						       struct tm *),
> -				 long int *__offset)
> attribute_hidden; +extern __time64_t __mktime_internal (struct tm
> *__tp,
> +				     struct tm *(*__func) (const
> __time64_t *,
> +							   struct tm
> *),
> +				     long int *__offset)
> attribute_hidden; 
>  #if __TIMESIZE == 64
>  # define __ctime64 ctime
> @@ -155,5 +171,13 @@ extern double __difftime (time_t time1, time_t
> time0); 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 t64)
> +{
> +  time_t t = t64;
> +  return t == t64;
> +}
> +
>  #endif
>  #endif
> diff --git a/time/mktime.c b/time/mktime.c
> index af43a6950d..5d3644e213 100644
> --- a/time/mktime.c
> +++ b/time/mktime.c
> @@ -112,11 +112,11 @@ my_tzset (void)
>     added to them, and then with another timestamp added, without
>     worrying about overflow.
>  
> -   Much of the code uses long_int to represent time_t values, to
> -   lessen the hassle of dealing with platforms where time_t is
> +   Much of the code uses long_int to represent __time64_t values, to
> +   lessen the hassle of dealing with platforms where __time64_t is
>     unsigned, and because long_int should suffice to represent all
> -   time_t values that mktime can generate even on platforms where
> -   time_t is excessively wide.  */
> +   __time64_t values that mktime can generate even on platforms where
> +   __time64_t is excessively wide.  */
>  
>  #if INT_MAX <= LONG_MAX / 4 / 366 / 24 / 60 / 60
>  typedef long int long_int;
> @@ -144,16 +144,17 @@ shr (long_int a, int b)
>  	  : a / (one << b) - (a % (one << b) < 0));
>  }
>  
> -/* Bounds for the intersection of time_t and long_int.  */
> +/* Bounds for the intersection of __time64_t and long_int.  */
>  
>  static long_int const mktime_min
> -  = ((TYPE_SIGNED (time_t) && TYPE_MINIMUM (time_t) < TYPE_MINIMUM
> (long_int))
> -     ? TYPE_MINIMUM (long_int) : TYPE_MINIMUM (time_t));
> +  = ((TYPE_SIGNED (__time64_t)
> +      && TYPE_MINIMUM (__time64_t) < TYPE_MINIMUM (long_int))
> +     ? TYPE_MINIMUM (long_int) : TYPE_MINIMUM (__time64_t));
>  static long_int const mktime_max
> -  = (TYPE_MAXIMUM (long_int) < TYPE_MAXIMUM (time_t)
> -     ? TYPE_MAXIMUM (long_int) : TYPE_MAXIMUM (time_t));
> +  = (TYPE_MAXIMUM (long_int) < TYPE_MAXIMUM (__time64_t)
> +     ? TYPE_MAXIMUM (long_int) : TYPE_MAXIMUM (__time64_t));
>  
> -verify (TYPE_IS_INTEGER (time_t));
> +verify (TYPE_IS_INTEGER (__time64_t));
>  
>  #define EPOCH_YEAR 1970
>  #define TM_YEAR_BASE 1900
> @@ -252,23 +253,23 @@ tm_diff (long_int year, long_int yday, int
> hour, int min, int sec, }
>  
>  /* Use CONVERT to convert T to a struct tm value in *TM.  T must be
> in
> -   range for time_t.  Return TM if successful, NULL (setting errno)
> on
> +   range for __time64_t.  Return TM if successful, NULL (setting
> errno) on failure.  */
>  static struct tm *
> -convert_time (struct tm *(*convert) (const time_t *, struct tm *),
> +convert_time (struct tm *(*convert) (const __time64_t *, struct tm
> *), long_int t, struct tm *tm)
>  {
> -  time_t x = t;
> +  __time64_t x = t;
>    return convert (&x, tm);
>  }
>  
>  /* Use CONVERT to convert *T to a broken down time in *TP.
>     If *T is out of range for conversion, adjust it so that
>     it is the nearest in-range value and then convert that.
> -   A value is in range if it fits in both time_t and long_int.
> +   A value is in range if it fits in both __time64_t and long_int.
>     Return TP on success, NULL (setting errno) on failure.  */
>  static struct tm *
> -ranged_convert (struct tm *(*convert) (const time_t *, struct tm *),
> +ranged_convert (struct tm *(*convert) (const __time64_t *, struct tm
> *), long_int *t, struct tm *tp)
>  {
>    long_int t1 = (*t < mktime_min ? mktime_min
> @@ -310,7 +311,7 @@ ranged_convert (struct tm *(*convert) (const
> time_t *, struct tm *), }
>  
>  
> -/* Convert *TP to a time_t value, inverting
> +/* Convert *TP to a __time64_t value, inverting
>     the monotonic and mostly-unit-linear conversion function CONVERT.
>     Use *OFFSET to keep track of a guess at the offset of the result,
>     compared to what the result would be for UTC without leap seconds.
> @@ -318,9 +319,9 @@ ranged_convert (struct tm *(*convert) (const
> time_t *, struct tm *), If successful, set *TP to the canonicalized
> struct tm; otherwise leave *TP alone, return ((time_t) -1) and set
> errno. This function is external because it is used also by
> timegm.c.  */ -time_t
> +__time64_t
>  __mktime_internal (struct tm *tp,
> -		   struct tm *(*convert) (const time_t *, struct tm
> *),
> +		   struct tm *(*convert) (const __time64_t *, struct
> tm *), mktime_offset_t *offset)
>  {
>    struct tm tm;
> @@ -520,10 +521,13 @@ __mktime_internal (struct tm *tp,
>  
>  #if defined _LIBC || NEED_MKTIME_WORKING || NEED_MKTIME_WINDOWS
>  
> -/* Convert *TP to a time_t value.  */
> -time_t
> -mktime (struct tm *tp)
> +/* Convert *TP to a __time64_t value.  */
> +__time64_t
> +__mktime64 (struct tm *tp)
>  {
> +  __time64_t t64;
> +  time_t t;
> +  struct tm tp0 = *tp;
>    /* POSIX.1 8.1.1 requires that whenever mktime() is called, the
>       time zone names contained in the external variable 'tzname'
> shall be set as if the tzset() function had been called.  */
> @@ -531,7 +535,15 @@ mktime (struct tm *tp)
>  
>  # if defined _LIBC || NEED_MKTIME_WORKING
>    static mktime_offset_t localtime_offset;
> -  return __mktime_internal (tp, __localtime_r, &localtime_offset);
> +  t64 = __mktime_internal (&tp0, __localtime64_r, &localtime_offset);
> +  t = t64;
> +  if (t != t64)
> +    {
> +      __set_errno (EOVERFLOW);
> +      return -1;
> +    }
> +  *tp = tp0;
> +  return t;
>  # else
>  #  undef mktime
>    return mktime (tp);
> @@ -540,6 +552,28 @@ mktime (struct tm *tp)
>  #endif /* _LIBC || NEED_MKTIME_WORKING || NEED_MKTIME_WINDOWS */
>  
>  #ifdef weak_alias
> +weak_alias (__mktime64, __timelocal64)
> +#endif
> +
> +#ifdef _LIBC
> +libc_hidden_def (__mktime64)
> +libc_hidden_weak (__timelocal64)
> +#endif
> +
> +#if __TIMESIZE != 64
> +
> +/* The 32-bit-time wrapper.  */
> +time_t
> +mktime (struct tm *tp)
> +{
> +  __time64_t t64 = __mktime64 (tp);
> +  if (fits_in_time_t (t64))
> +    return t64;
> +  __set_errno (EOVERFLOW);
> +  return -1;
> +}
> +
> +#ifdef weak_alias
>  weak_alias (mktime, timelocal)
>  #endif
>  
> @@ -547,3 +581,5 @@ weak_alias (mktime, timelocal)
>  libc_hidden_def (mktime)
>  libc_hidden_weak (timelocal)
>  #endif
> +
> +#endif
> diff --git a/time/timegm.c b/time/timegm.c
> index bfd36d0255..0cbe60a7fe 100644
> --- a/time/timegm.c
> +++ b/time/timegm.c
> @@ -22,13 +22,38 @@
>  #endif
>  
>  #include <time.h>
> +#include <errno.h>
>  
>  #include "mktime-internal.h"
> +#include <errno.h>
> +
> +__time64_t
> +__timegm64 (struct tm *tmp)
> +{
> +  static long int gmtime_offset;
> +  tmp->tm_isdst = 0;
> +  return __mktime_internal (tmp, __gmtime64_r, &gmtime_offset);
> +}
> +
> +#if __TIMESIZE != 64
>  
>  time_t
>  timegm (struct tm *tmp)
>  {
> +  time_t t;
> +  __time64_t t64;
> +  struct tm tmp0 = *tmp;
>    static mktime_offset_t gmtime_offset;
>    tmp->tm_isdst = 0;
> -  return __mktime_internal (tmp, __gmtime_r, &gmtime_offset);
> +  t64 = __mktime_internal (&tmp0, __gmtime64_r, &gmtime_offset);
> +  t = t64;
> +  if (t != t64)
> +    {
> +      __set_errno (EOVERFLOW);
> +      return -1;
> +    }
> +  *tmp = tmp0;
> +  return t;
>  }
> +
> +#endif




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
  
Paul Eggert March 12, 2019, 12:11 a.m. UTC | #2
On 2/27/19 3:20 AM, Lukasz Majewski wrote:
> +/* Another name for `__mktime64'.  */
> +extern __time64_t __timelocal64 (struct tm *__tp) __THROW;

In hindsight the name 'timelocal' was a mistake: it's not a portable
name and its use has not caught on. Although we need to keep 'timelocal'
for backwards compatibility, there's no need to define 'timelocal64', as
the very few people who need such a function can just call mktime64. So
I suggest removing all traces of timelocal64, __timelocal64, etc. from
the patch.


> +/* Check whether a time64_t value fits in a time_t.  */
> +static inline bool
> +fits_in_time_t (__time64_t t64)
> +{
> +  time_t t = t64;
> +  return t == t64;
> +}
> +

This function is used only in time/mktime.c, and so should be defined
there. This will help sharing with Gnulib, which doesn't have
include/time.h.

The function's name should not end with "_t" since that's in the
reserved namespace (important for Gnulib).


> -verify (TYPE_IS_INTEGER (time_t));
> +verify (TYPE_IS_INTEGER (__time64_t));

Please remove this line instead. It dates back to old POSIX, which
allowed time_t to be a floating-point type. POSIX no longer allows this
and we needn't worry about that possibility any more.


> +  __time64_t t64;
> +  time_t t;
> +  struct tm tp0 = *tp;
There is no need for both t and t64. Just declare "__time64_t t;" and
replace all uses of t64 with t. Also, please consistently rename tp0 to
tm0, since it's not a pointer.


> +  t = t64;
> +  if (t != t64)

Replace this with "if (! fits_in_time_t (t))" (but rename the function).


> +/* The 32-bit-time wrapper.  */
> +time_t
> +mktime (struct tm *tp)
> +{
> +  __time64_t t64 = __mktime64 (tp);
> +  if (fits_in_time_t (t64))
> +    return t64;
> +  __set_errno (EOVERFLOW);
> +  return -1;
> +}

Fix this so that it doesn not modify *TP when failing due to EOVERFLOW.
The manual says mktime doesn't change *TP on failure.


Similar comments apply to the implementation of timegm.
  
Joseph Myers March 12, 2019, 12:36 a.m. UTC | #3
On Mon, 11 Mar 2019, Paul Eggert wrote:

> On 2/27/19 3:20 AM, Lukasz Majewski wrote:
> > +/* Another name for `__mktime64'.  */
> > +extern __time64_t __timelocal64 (struct tm *__tp) __THROW;
> 
> In hindsight the name 'timelocal' was a mistake: it's not a portable
> name and its use has not caught on. Although we need to keep 'timelocal'
> for backwards compatibility, there's no need to define 'timelocal64', as
> the very few people who need such a function can just call mktime64. So
> I suggest removing all traces of timelocal64, __timelocal64, etc. from
> the patch.

Neither timelocal64 nor mktime64 will be a public API; the public API is 
to define _TIME_BITS=64 (and _FILE_OFFSET_BITS=64 because we don't want to 
support the combination of 64-bit times with 32-bit offsets) before 
including any system headers, then call the existing function names.

Given timelocal as part of the __USE_MISC API in time.h, it should be part 
of the __USE_MISC __TIME_BITS=64 API there as well - but it's fine for 
that case to redirect to __mktime64 (and thus not need __timelocal64 as 
another alias).
  
Lukasz Majewski March 12, 2019, 6:58 a.m. UTC | #4
Hi Paul,

> On 2/27/19 3:20 AM, Lukasz Majewski wrote:
> > +/* Another name for `__mktime64'.  */
> > +extern __time64_t __timelocal64 (struct tm *__tp) __THROW;  
> 
> In hindsight the name 'timelocal' was a mistake: it's not a portable
> name and its use has not caught on. Although we need to keep
> 'timelocal' for backwards compatibility, there's no need to define
> 'timelocal64', as the very few people who need such a function can
> just call mktime64. So I suggest removing all traces of timelocal64,
> __timelocal64, etc. from the patch.
> 
> 
> > +/* Check whether a time64_t value fits in a time_t.  */
> > +static inline bool
> > +fits_in_time_t (__time64_t t64)
> > +{
> > +  time_t t = t64;
> > +  return t == t64;
> > +}
> > +  
> 
> This function is used only in time/mktime.c, and so should be defined
> there. This will help sharing with Gnulib, which doesn't have
> include/time.h.

Some patches on top of this one (y2038 related) use fits_in_time_t() to
check if 64 bit wrapper on 32 bit version of function (like
__clock_gettime()) shall proceed or return EOVERFLOW.

The question is if there is a more suitable place than include/time.h
header to have fits_in_time() reused by both Glibc and Gnulib?

> 
> The function's name should not end with "_t" since that's in the
> reserved namespace (important for Gnulib).

Ok.

> 
> 
> > -verify (TYPE_IS_INTEGER (time_t));
> > +verify (TYPE_IS_INTEGER (__time64_t));  
> 
> Please remove this line instead. It dates back to old POSIX, which
> allowed time_t to be a floating-point type. POSIX no longer allows
> this and we needn't worry about that possibility any more.
> 
> 
> > +  __time64_t t64;
> > +  time_t t;
> > +  struct tm tp0 = *tp;  
> There is no need for both t and t64. Just declare "__time64_t t;" and
> replace all uses of t64 with t. Also, please consistently rename tp0
> to tm0, since it's not a pointer.
> 
> 
> > +  t = t64;
> > +  if (t != t64)  
> 
> Replace this with "if (! fits_in_time_t (t))" (but rename the
> function).
> 
> 
> > +/* The 32-bit-time wrapper.  */
> > +time_t
> > +mktime (struct tm *tp)
> > +{
> > +  __time64_t t64 = __mktime64 (tp);
> > +  if (fits_in_time_t (t64))
> > +    return t64;
> > +  __set_errno (EOVERFLOW);
> > +  return -1;
> > +}  
> 
> Fix this so that it doesn not modify *TP when failing due to
> EOVERFLOW. The manual says mktime doesn't change *TP on failure.
> 
> 
> Similar comments apply to the implementation of timegm.
> 

Thanks for your comments.


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
  
Lukasz Majewski March 17, 2019, 10:48 p.m. UTC | #5
Hi Joseph, Paul,

> On Mon, 11 Mar 2019, Paul Eggert wrote:
> 
> > On 2/27/19 3:20 AM, Lukasz Majewski wrote:  
> > > +/* Another name for `__mktime64'.  */
> > > +extern __time64_t __timelocal64 (struct tm *__tp) __THROW;  
> > 
> > In hindsight the name 'timelocal' was a mistake: it's not a portable
> > name and its use has not caught on. Although we need to keep
> > 'timelocal' for backwards compatibility, there's no need to define
> > 'timelocal64', as the very few people who need such a function can
> > just call mktime64. So I suggest removing all traces of
> > timelocal64, __timelocal64, etc. from the patch. 
> 
> Neither timelocal64 nor mktime64 will be a public API; the public API
> is to define _TIME_BITS=64 (and _FILE_OFFSET_BITS=64 because we don't
> want to support the combination of 64-bit times with 32-bit offsets)
> before including any system headers, then call the existing function
> names.

I do have a doubt if I understood the above comment(s). 

Please correct me if I'm wrong - this patch makes the
__mktime_internal compatible with __time_64 explicit 64 bit time data
structure.

It's goal is to replace all calls to mktime/timegm/localtime with
__mktime64/__timegm64/__localtime64 (alias to __mktime64) internal
counterparts (another level of abstraction with explicit 64 bit time
storage).

Then, the __mktime64() internal function is used to implement mktime
wrapper for 32-bit time (the same approach is for timegm).

This patch looks like a preparatory patch for further Y2038 conversion
(but doesn't provide one).

> 
> Given timelocal as part of the __USE_MISC API in time.h, it should be
> part of the __USE_MISC __TIME_BITS=64 API there as well - but it's

This patch doesn't introduce/use the _TIME_BITS=64 - instead it
operates on TIMESIZE=64.

> fine for that case to redirect to __mktime64 (and thus not need
> __timelocal64 as another alias).
> 


Do you require following code in include/time.h for timelocal:

#if defined __USE_MISC
# if defined(__REDIRECT)
  extern int __REDIRECT (timelocal, (struct tm *__tp),
                         __mktime64) __THROW);
# else
#  define timelocal __mktime64
# endif
#endif


The problem seems to be that Paul requested to remove {__}timelocal64
from this patch and you pointed out that timelocal is already supported
when __USE_MISC is defined?




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
  
Joseph Myers March 18, 2019, 4:27 p.m. UTC | #6
On Sun, 17 Mar 2019, Lukasz Majewski wrote:

> Do you require following code in include/time.h for timelocal:
> 
> #if defined __USE_MISC
> # if defined(__REDIRECT)
>   extern int __REDIRECT (timelocal, (struct tm *__tp),
>                          __mktime64) __THROW);
> # else
> #  define timelocal __mktime64
> # endif
> #endif

include/time.h is an *internal* header, so should not need to test __USE_* 
or use __REDIRECT.

Something like that would be appropriate in the *installed* header 
(time/time.h), under whatever __USE_* condition is set by _TIME_BITS=64, 
but only at the later point where we're ready to add support for 
_TIME_BITS=64 to the public headers and corresponding new public symbol 
versions for the new ABIs.

By using something like that in the installed header you avoid any need to 
define the name __timelocal64 anywhere.
  
Lukasz Majewski March 19, 2019, 10:53 a.m. UTC | #7
Hi Joseph,

> On Sun, 17 Mar 2019, Lukasz Majewski wrote:
> 
> > Do you require following code in include/time.h for timelocal:
> > 
> > #if defined __USE_MISC
> > # if defined(__REDIRECT)
> >   extern int __REDIRECT (timelocal, (struct tm *__tp),
> >                          __mktime64) __THROW);
> > # else
> > #  define timelocal __mktime64
> > # endif
> > #endif  
> 
> include/time.h is an *internal* header, so should not need to test
> __USE_* or use __REDIRECT.
> 
> Something like that would be appropriate in the *installed* header 
> (time/time.h), under whatever __USE_* condition is set by
> _TIME_BITS=64, but only at the later point where we're ready to add
> support for _TIME_BITS=64 to the public headers and corresponding new
> public symbol versions for the new ABIs.
> 
> By using something like that in the installed header you avoid any
> need to define the name __timelocal64 anywhere.
> 

The distinction between internal and external set of headers was
unclear for me.

Thanks for clarification.

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
  

Patch

diff --git a/include/time.h b/include/time.h
index 61dd9e180b..34e5e04820 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)
@@ -16,6 +17,21 @@  libc_hidden_proto (localtime)
 libc_hidden_proto (strftime)
 libc_hidden_proto (strptime)
 
+#if __TIMESIZE == 64
+# define __timegm64 timegm
+# define __mktime64 mktime
+# define __timelocal64 timelocal
+#else
+extern __time64_t __timegm64 (struct tm *__tp) __THROW;
+extern __time64_t __mktime64 (struct tm *__tp) __THROW;
+/* Another name for `__mktime64'.  */
+extern __time64_t __timelocal64 (struct tm *__tp) __THROW;
+
+libc_hidden_proto (__mktime64)
+libc_hidden_proto (__timelocal64)
+#endif
+
+
 extern __typeof (clock_getres) __clock_getres;
 extern __typeof (clock_gettime) __clock_gettime;
 libc_hidden_proto (__clock_gettime)
@@ -49,13 +65,13 @@  extern void __tzset_parse_tz (const char *tz) attribute_hidden;
 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
-   normalize TP, given that a `struct tm *' maps to a `time_t' as performed
+/* Subroutine of mktime.  Return the __time64_t representation of TP and
+   normalize TP, given that a struct tm * maps to a __time64_t as performed
    by FUNC.  Record next guess for localtime-gmtime offset in *OFFSET.  */
-extern time_t __mktime_internal (struct tm *__tp,
-				 struct tm *(*__func) (const time_t *,
-						       struct tm *),
-				 long int *__offset) attribute_hidden;
+extern __time64_t __mktime_internal (struct tm *__tp,
+				     struct tm *(*__func) (const __time64_t *,
+							   struct tm *),
+				     long int *__offset) attribute_hidden;
 
 #if __TIMESIZE == 64
 # define __ctime64 ctime
@@ -155,5 +171,13 @@  extern double __difftime (time_t time1, time_t time0);
    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 t64)
+{
+  time_t t = t64;
+  return t == t64;
+}
+
 #endif
 #endif
diff --git a/time/mktime.c b/time/mktime.c
index af43a6950d..5d3644e213 100644
--- a/time/mktime.c
+++ b/time/mktime.c
@@ -112,11 +112,11 @@  my_tzset (void)
    added to them, and then with another timestamp added, without
    worrying about overflow.
 
-   Much of the code uses long_int to represent time_t values, to
-   lessen the hassle of dealing with platforms where time_t is
+   Much of the code uses long_int to represent __time64_t values, to
+   lessen the hassle of dealing with platforms where __time64_t is
    unsigned, and because long_int should suffice to represent all
-   time_t values that mktime can generate even on platforms where
-   time_t is excessively wide.  */
+   __time64_t values that mktime can generate even on platforms where
+   __time64_t is excessively wide.  */
 
 #if INT_MAX <= LONG_MAX / 4 / 366 / 24 / 60 / 60
 typedef long int long_int;
@@ -144,16 +144,17 @@  shr (long_int a, int b)
 	  : a / (one << b) - (a % (one << b) < 0));
 }
 
-/* Bounds for the intersection of time_t and long_int.  */
+/* Bounds for the intersection of __time64_t and long_int.  */
 
 static long_int const mktime_min
-  = ((TYPE_SIGNED (time_t) && TYPE_MINIMUM (time_t) < TYPE_MINIMUM (long_int))
-     ? TYPE_MINIMUM (long_int) : TYPE_MINIMUM (time_t));
+  = ((TYPE_SIGNED (__time64_t)
+      && TYPE_MINIMUM (__time64_t) < TYPE_MINIMUM (long_int))
+     ? TYPE_MINIMUM (long_int) : TYPE_MINIMUM (__time64_t));
 static long_int const mktime_max
-  = (TYPE_MAXIMUM (long_int) < TYPE_MAXIMUM (time_t)
-     ? TYPE_MAXIMUM (long_int) : TYPE_MAXIMUM (time_t));
+  = (TYPE_MAXIMUM (long_int) < TYPE_MAXIMUM (__time64_t)
+     ? TYPE_MAXIMUM (long_int) : TYPE_MAXIMUM (__time64_t));
 
-verify (TYPE_IS_INTEGER (time_t));
+verify (TYPE_IS_INTEGER (__time64_t));
 
 #define EPOCH_YEAR 1970
 #define TM_YEAR_BASE 1900
@@ -252,23 +253,23 @@  tm_diff (long_int year, long_int yday, int hour, int min, int sec,
 }
 
 /* Use CONVERT to convert T to a struct tm value in *TM.  T must be in
-   range for time_t.  Return TM if successful, NULL (setting errno) on
+   range for __time64_t.  Return TM if successful, NULL (setting errno) on
    failure.  */
 static struct tm *
-convert_time (struct tm *(*convert) (const time_t *, struct tm *),
+convert_time (struct tm *(*convert) (const __time64_t *, struct tm *),
 	      long_int t, struct tm *tm)
 {
-  time_t x = t;
+  __time64_t x = t;
   return convert (&x, tm);
 }
 
 /* Use CONVERT to convert *T to a broken down time in *TP.
    If *T is out of range for conversion, adjust it so that
    it is the nearest in-range value and then convert that.
-   A value is in range if it fits in both time_t and long_int.
+   A value is in range if it fits in both __time64_t and long_int.
    Return TP on success, NULL (setting errno) on failure.  */
 static struct tm *
-ranged_convert (struct tm *(*convert) (const time_t *, struct tm *),
+ranged_convert (struct tm *(*convert) (const __time64_t *, struct tm *),
 		long_int *t, struct tm *tp)
 {
   long_int t1 = (*t < mktime_min ? mktime_min
@@ -310,7 +311,7 @@  ranged_convert (struct tm *(*convert) (const time_t *, struct tm *),
 }
 
 
-/* Convert *TP to a time_t value, inverting
+/* Convert *TP to a __time64_t value, inverting
    the monotonic and mostly-unit-linear conversion function CONVERT.
    Use *OFFSET to keep track of a guess at the offset of the result,
    compared to what the result would be for UTC without leap seconds.
@@ -318,9 +319,9 @@  ranged_convert (struct tm *(*convert) (const time_t *, struct tm *),
    If successful, set *TP to the canonicalized struct tm;
    otherwise leave *TP alone, return ((time_t) -1) and set errno.
    This function is external because it is used also by timegm.c.  */
-time_t
+__time64_t
 __mktime_internal (struct tm *tp,
-		   struct tm *(*convert) (const time_t *, struct tm *),
+		   struct tm *(*convert) (const __time64_t *, struct tm *),
 		   mktime_offset_t *offset)
 {
   struct tm tm;
@@ -520,10 +521,13 @@  __mktime_internal (struct tm *tp,
 
 #if defined _LIBC || NEED_MKTIME_WORKING || NEED_MKTIME_WINDOWS
 
-/* Convert *TP to a time_t value.  */
-time_t
-mktime (struct tm *tp)
+/* Convert *TP to a __time64_t value.  */
+__time64_t
+__mktime64 (struct tm *tp)
 {
+  __time64_t t64;
+  time_t t;
+  struct tm tp0 = *tp;
   /* POSIX.1 8.1.1 requires that whenever mktime() is called, the
      time zone names contained in the external variable 'tzname' shall
      be set as if the tzset() function had been called.  */
@@ -531,7 +535,15 @@  mktime (struct tm *tp)
 
 # if defined _LIBC || NEED_MKTIME_WORKING
   static mktime_offset_t localtime_offset;
-  return __mktime_internal (tp, __localtime_r, &localtime_offset);
+  t64 = __mktime_internal (&tp0, __localtime64_r, &localtime_offset);
+  t = t64;
+  if (t != t64)
+    {
+      __set_errno (EOVERFLOW);
+      return -1;
+    }
+  *tp = tp0;
+  return t;
 # else
 #  undef mktime
   return mktime (tp);
@@ -540,6 +552,28 @@  mktime (struct tm *tp)
 #endif /* _LIBC || NEED_MKTIME_WORKING || NEED_MKTIME_WINDOWS */
 
 #ifdef weak_alias
+weak_alias (__mktime64, __timelocal64)
+#endif
+
+#ifdef _LIBC
+libc_hidden_def (__mktime64)
+libc_hidden_weak (__timelocal64)
+#endif
+
+#if __TIMESIZE != 64
+
+/* The 32-bit-time wrapper.  */
+time_t
+mktime (struct tm *tp)
+{
+  __time64_t t64 = __mktime64 (tp);
+  if (fits_in_time_t (t64))
+    return t64;
+  __set_errno (EOVERFLOW);
+  return -1;
+}
+
+#ifdef weak_alias
 weak_alias (mktime, timelocal)
 #endif
 
@@ -547,3 +581,5 @@  weak_alias (mktime, timelocal)
 libc_hidden_def (mktime)
 libc_hidden_weak (timelocal)
 #endif
+
+#endif
diff --git a/time/timegm.c b/time/timegm.c
index bfd36d0255..0cbe60a7fe 100644
--- a/time/timegm.c
+++ b/time/timegm.c
@@ -22,13 +22,38 @@ 
 #endif
 
 #include <time.h>
+#include <errno.h>
 
 #include "mktime-internal.h"
+#include <errno.h>
+
+__time64_t
+__timegm64 (struct tm *tmp)
+{
+  static long int gmtime_offset;
+  tmp->tm_isdst = 0;
+  return __mktime_internal (tmp, __gmtime64_r, &gmtime_offset);
+}
+
+#if __TIMESIZE != 64
 
 time_t
 timegm (struct tm *tmp)
 {
+  time_t t;
+  __time64_t t64;
+  struct tm tmp0 = *tmp;
   static mktime_offset_t gmtime_offset;
   tmp->tm_isdst = 0;
-  return __mktime_internal (tmp, __gmtime_r, &gmtime_offset);
+  t64 = __mktime_internal (&tmp0, __gmtime64_r, &gmtime_offset);
+  t = t64;
+  if (t != t64)
+    {
+      __set_errno (EOVERFLOW);
+      return -1;
+    }
+  *tmp = tmp0;
+  return t;
 }
+
+#endif