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

[Albert ARIBAUD <albert dot aribaud at 3adev dot fr>]

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