[v2,0/9] Y2038 struct tm related patches

Message ID 20181129221010.19589-1-albert.aribaud@3adev.fr
Headers show
Series
  • Y2038 struct tm related patches
Related show

Message

Albert ARIBAUD (3ADEV) Nov. 29, 2018, 10:10 p.m.
This series introduces Y2038-proof struct tm related functions.

First, it makes __tz_convert 64-bit-time compatible, as this function
is used in several struct tm related public interfaces. Then it adds
Y2038-proof versions of said interfaces.

v2:
- each commit message now mention how the patch was tested
- a ninth patch is added for mktime.c simplification and GNULIB compat.

Albert ARIBAUD (3ADEV) (7):
  Y2038: make __tz_convert compatible with 64-bit-time
  Y2038: add function __localtime64
  Y2038: add function __localtime64_r
  Y2038: add function __gmtime64
  Y2038: add function __gmtime64_r
  Y2038: add function __ctime64
  Y2038: add function __ctime64_r

Paul Eggert (2):
  Y2038: make __mktime_internal compatible with __time64_t
  Fix time/mktime.c and time/gmtime.c for gnulib compatibility

 include/time.h   | 81 ++++++++++++++++++++++++++++++++++++++++++------
 time/ctime.c     | 14 +++++++++
 time/ctime_r.c   | 13 ++++++++
 time/gmtime.c    | 33 ++++++++++++++++++--
 time/localtime.c | 35 ++++++++++++++++++---
 time/mktime.c    | 79 ++++++++++++++++++++++++++++++----------------
 time/offtime.c   | 12 +++----
 time/timegm.c    | 27 +++++++++++++++-
 time/tzfile.c    | 14 +++------
 time/tzset.c     | 27 ++++++----------
 10 files changed, 258 insertions(+), 77 deletions(-)

-- 
2.17.1

Comments

Albert ARIBAUD (3ADEV) Dec. 5, 2018, 12:11 a.m. | #1
On Thu, 29 Nov 2018 23:10:01 +0100, "Albert ARIBAUD (3ADEV)"
<albert.aribaud@3adev.fr> wrote :

> This series introduces Y2038-proof struct tm related functions.

> 

> First, it makes __tz_convert 64-bit-time compatible, as this function

> is used in several struct tm related public interfaces. Then it adds

> Y2038-proof versions of said interfaces.

> 

> v2:

> - each commit message now mention how the patch was tested

> - a ninth patch is added for mktime.c simplification and GNULIB compat.


Any comments on these?

Cordialement,
Albert ARIBAUD
3ADEV
Joseph Myers Dec. 5, 2018, 4:49 p.m. | #2
Patch 1 is OK.

For most of the rest of the series, I think there is a general issue to 
resolve regarding how we document (internally) the symbol handling for 
time-related symbols.  In patch 5, for example, you have:

> +/* 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.  */


But this is nothing to do with those particular symbols; it's completely 
generic, as part of the way time-related symbols are handled in this 
series, that each such function for time_t is either defined explicitly, 
or via a #define of the name used with __time64_t.  So it doesn't deserve 
a comment there, any more than each use of libc_hidden_def or weak_alias 
deserves a comment on the generic symbol handling conventions in glibc 
that result in those uses (only those with something unusual about them 
should have such comments).

Now, those conventions probably *do* deserve to be documented somewhere 
(in the glibc sources, not just on the wiki), but somewhere not tied to 
one particular function.  I think a better place might be maint.texi: add 
a section "Symbol Handling" or similar, which initially would just discuss 
how symbols for 64-bit time are handled (both on systems that had it all 
along, and on systems where support is being added alongside existing 
support for 32-bit time), but could be expanded in future to discuss the 
various other symbol handling issues in glibc.

Patch 6 has a similarly problematic comment:

> +/* 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. */


(a) __ctime64 is defined as ctime as part of the general symbol 
conventions for such symbols, since ctime is unconditionally part of the 
glibc ABI and API.  It's nothing to do with a particular use in 
nis/nis_print.c.  (Once that comment is accordingly removed, no further 
action is needed for (b) and (c) below; they are just general observations 
on related issues.)

(b) Any such non-obvious reference in a comment from one part of the glibc 
source to another really needs a corresponding comment pointing in the 
other direction, so that someone changing nis/nis_print.c would know they 
might need to update the other comment.

(c) In general, code internal to glibc should change to use 64-bit time 
explicitly rather than the old time_t / ctime.  In this particular case, 
(i) it probably can't do so yet because you can't changes users outside of 
libc.so until libc.so exports the new interfaces, and (ii) the change 
might not provide much benefit, if the 32-bit times are part of the NIS 
protocol (which would mean NIS is obsolete by Y2038) - although it may be 
a good idea anyway to get rid of glibc-internal uses of the 32-bit 
interfaces.

-- 
Joseph S. Myers
joseph@codesourcery.com
Albert ARIBAUD (3ADEV) Dec. 5, 2018, 6:40 p.m. | #3
Hi Joseph,

On Wed, 5 Dec 2018 16:49:26 +0000, Joseph Myers
<joseph@codesourcery.com> wrote :

> Patch 1 is OK.


Thanks! Accordingly, I'll apply this patch on master and re-spin the
series above it.

> For most of the rest of the series, I think there is a general issue to 

> resolve regarding how we document (internally) the symbol handling for 

> time-related symbols.  In patch 5, for example, you have:

> 

> > +/* 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.  */  

> 

> But this is nothing to do with those particular symbols; it's completely 

> generic, as part of the way time-related symbols are handled in this 

> series, that each such function for time_t is either defined explicitly, 

> or via a #define of the name used with __time64_t.  So it doesn't deserve 

> a comment there, any more than each use of libc_hidden_def or weak_alias 

> deserves a comment on the generic symbol handling conventions in glibc 

> that result in those uses (only those with something unusual about them 

> should have such comments).

> 

> Now, those conventions probably *do* deserve to be documented somewhere 

> (in the glibc sources, not just on the wiki), but somewhere not tied to 

> one particular function.  I think a better place might be maint.texi: add 

> a section "Symbol Handling" or similar, which initially would just discuss 

> how symbols for 64-bit time are handled (both on systems that had it all 

> along, and on systems where support is being added alongside existing 

> support for 32-bit time), but could be expanded in future to discuss the 

> various other symbol handling issues in glibc.


I will prepend a patch to the (re-spun) series to add a section
"Symbol handling" to maint.texi, with a subsection "Y2038 symbol
handling" in which I will document these conventions.

> Patch 6 has a similarly problematic comment:

> 

> > +/* 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. */  

> 

> (a) __ctime64 is defined as ctime as part of the general symbol 

> conventions for such symbols, since ctime is unconditionally part of the 

> glibc ABI and API.  It's nothing to do with a particular use in 

> nis/nis_print.c.  (Once that comment is accordingly removed, no further 

> action is needed for (b) and (c) below; they are just general observations 

> on related issues.)

> 

> (b) Any such non-obvious reference in a comment from one part of the glibc 

> source to another really needs a corresponding comment pointing in the 

> other direction, so that someone changing nis/nis_print.c would know they 

> might need to update the other comment.

> 

> (c) In general, code internal to glibc should change to use 64-bit time 

> explicitly rather than the old time_t / ctime.  In this particular case, 

> (i) it probably can't do so yet because you can't changes users outside of 

> libc.so until libc.so exports the new interfaces, and (ii) the change 

> might not provide much benefit, if the 32-bit times are part of the NIS 

> protocol (which would mean NIS is obsolete by Y2038) - although it may be 

> a good idea anyway to get rid of glibc-internal uses of the 32-bit 

> interfaces.


Understood.

Cordialement,
Albert ARIBAUD
3ADEV