Re: [PATCH 1/1] Y2038: add function __difftime64

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

Hi Paul,

On Wed, 20 Jun 2018 12:29:06 -0700, Paul Eggert <eggert@cs.ucla.edu>
wrote :

> On 06/20/2018 05:14 AM, Albert ARIBAUD (3ADEV) wrote:
> > This change batch creates a __difftime64 compatible with 64-bit time,
> > and makes difftime either an alias of it or a 32-bit wrapper around it.  
> 
> Since no glibc code calls difftime, can we assume that a later patch 
> will change will make __difftime64 and difftime both available to user 
> code? I'm not getting the big picture here.

Actually, this is not an assumption; this is explicitly stated in the
whole patch series posted as an RFC previously, twice (original and v2),
with a detailed cover letter explaining how the series was structured.
See e.g.

https://patchwork.ozlabs.org/cover/811194/ (original RFC series)
https://patchwork.ozlabs.org/patch/900333/ (second iteration)

These series were cut into many individual patches introducing new
internal (or at least private) type and functions plus a final patch to
update the public API.

> Which public development repository and branch are you using? I see lots 
> of glibc branches at sourceware.org named origin/aaribaud/y2038* but 
> none of them seem to correspond that the patches you're sending. If I 
> could see the current state of the entire set of patches you've 
> developed, that would save time reviewing.

The entire patch sets were available at RFC time as branches on the
sourceware repository.

Based on the relatively low amount of comments received on the second
RFC series, I thought I could post the series for good, with a fair
chance that any additional change request would be cosmetic, so I
packed related changes together to reduce the amount of patch sets and
went ahead.

What actually happened is, I am getting quite more feedback now than I
got with the RFCs.

This implies I am rewriting the series in near real-time, which means
the branches are ever-evolving now.

I'll provide two branches for reviewers as follows:

- the currently posted patches will be under aaribaud/y2038-posted.
  I will rebase this branch whenever I post a new series or a new
  version of an already posted series, or rebase above a more recent
  origin/master. This branch will undergo frequent tests and runs of
  build-many-glibcs.py

- the complete patch set, including the public API patch, will be
  under aaribaud/y2038-complete. I will keep int rebased above the
  aaribaud/y2038-posted branch, but it will not be tested as often, and
  may quite possibly not build at all at times.

Other branches under aaribaud/ may change at any time and be in any
state (for instance I use the -wip one to pass changes between my dev
machine and the one which runs the build-many-glibcs.py script).

> > 2. The 64-bit time implementation was obtained by duplicating the
> >     original 32-bit code then simplifying the source code based on
> >     the knowledge that __time64_t is a 64-bit signed integer  
> 
> This sort of simplification won't be possible in Gnulib, where 
> __time64_t will be an alias of time_t and therefore could be an unsigned 
> type. So let's not do that simplification. (Gnulib-using programs will 
> always ask for 64-bit time_t if that is an option and 32-bit is the 
> default, as the 32-bit default is silly if you have source code.)

We do currently have unsigned 32-bit time_t implementations, but do we
have unsigned 64-bit time_t? A signed 64-bit integer __time64_t was
a core assumption as early as the first of the seven revisions
of https://sourceware.org/glibc/wiki/Y2038ProofnessDesign; it's a pity
the existence of unsigned 64-bit time_t was never raised.

> It is OK to simplify difftime.c based on the assumption that time_t is 
> an integer type. Glibc difftime.c was written back when POSIX allowed 
> time_t to be floating-point, and some ancient implementations did that. 
> This was widely regarded to be a mistake, POSIX no longer allows 
> floating-point time_t and we don't need to worry about those old 
> implementations. However, this simplification should be done as a 
> separate patch (see first attachment).
>
> >     - in the difftime64 function, removal of code which handled
> >       time bitsize smaller than or equal to that of a double matissa
> >       (as __time64_t has more bits than a double mantissa can hold)  
> 
> This simplification is not possible in Gnulib either, as it's not 
> portable to assume that time_t has more bits than a double fraction can 
> hold. (They're fractions, not mantissas, by the way; mantissas are for 
> logarithms not for floating-point.)
>
> The patch needs a better commit message, as the commit message doesn't 
> say what exactly changed and has too much unnecessary talk about things 
> we aren't doing. Commit messages should focus on what actually changed; 
> the meat of any comments explaining the code should be in the code.
> 
> Attached is a proposed pair of (untested) patches that should reflect 
> the above comments. What do you think?
>
> PS. The mktime patches you sent have more problems like this. But that's 
> a bigger nut to crack, and let's get difftime done first.

If the proposed patches are OK, then the best is to apply them to
master and I will remove the corresponding ones from the Y2038 series
(or I can put them in the series with you as the commit author, although
I am worried that if I have to edit them I might accidentally reset the
authorship to me.

Cordialement,
Albert ARIBAUD
3ADEV