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

  * 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
  

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;
  

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
  

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
  

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
  

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.
  

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
  

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.