Re: [PATCH v2 0/9] Y2038 struct tm related patches

[Joseph Myers <joseph at codesourcery dot com>]

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