[RFC,00/12,RFC] y2038: Convert timespec_{sub|add|create} in support to be Y2038 safe

  This patch set is the first attempt to convert functions utilized by nptl and
pthreads to support 64 bit time.

First timespec* functions are renamed to have common "__" prefix for internal
functions. This is a preparatory work for further conversion.

One question with this patch set is how shall we tackle the conversion
of time related functions (like e.g. ./support/timespec_add) used internally in
glibc as helper functions for internal code or tests.
I could follow the approach used for glibc syscalls (like clock_settime) to have
two versions depending on __TIMESIZE and __WORDSIZE, but this seems to be an
overkill for internally used functions.

Patches with "Convert:" prefix replace struct timespec with struct __timespec64
in timespec_* functions. This is necessary to have in-glibc proper support for
64 bit time.
The problem is with instant replacement of struct timespec with struct 
__timespec64 in glibc internal code (and tests). 

For replacing timesize with __timesize64 I could:
- Replace its occurences in relevant directories - like ./nptl or
  ./sysdeps/pthreads

- Replace them in functions (tests) and use explicit conversion functions - like
  valid_timespec_to_timespec64()

- Replace _all_ occurences of struct timespec with struct __timespec64 at once
  with using sed on glibc tree.

The last option seems to be the most appealing as we already use __timespec64
(with its aliassing) for some core system syscalls (like clock_gettime).
However, such patch shall be applied just after the release of new stable
version to have time for potential fixes.

Lukasz Majewski (12):
  doc: Fix wording and formattine in ./support/README
  y2038: Rename timespec_compare to __timespec_compare
  y2038: Rename make_timespec to __make_timespec
  y2038: Rename xclock_gettime to __xclock_gettime
  y2038: Rename xclock_now to __xclock_now
  y2038: Rename timespec_sub to __timespec_sub
  y2038: Rename timespec_add to __timespec_add
  y2038: Convert __make_timespec to be Y2038 safe
  y2038: Convert __xclock_gettime to be Y2038 safe
  y2038: Convert __xclock_now to be Y2038 safe
  y2038: Convert timespec* files in ./support to be Y2038 safe
  y2038: Convert timespec_* from posix-timer.h to be Y2038 safe

 nptl/tst-eintr2.c                |  4 ++--
 nptl/tst-eintr5.c                |  4 ++--
 nptl/tst-rwlock6.c               | 14 +++++++-------
 nptl/tst-rwlock7.c               | 10 +++++-----
 nptl/tst-rwlock9.c               | 10 +++++-----
 nptl/tst-sem17.c                 |  2 +-
 nptl/tst-sem5.c                  |  4 ++--
 support/README                   |  4 ++--
 support/timespec-add.c           | 12 ++++++------
 support/timespec-sub.c           | 18 +++++++++---------
 support/timespec.c               | 12 ++++++------
 support/timespec.h               | 27 ++++++++++++++-------------
 support/xclock_gettime.c         |  5 ++---
 support/xtime.h                  |  9 +++++----
 sysdeps/mach/clock_nanosleep.c   |  4 ++--
 sysdeps/pthread/posix-timer.h    | 11 ++++++-----
 sysdeps/pthread/timer_gettime.c  |  4 ++--
 sysdeps/pthread/timer_routines.c | 14 +++++++-------
 sysdeps/pthread/timer_settime.c  |  4 ++--
 sysdeps/pthread/tst-cond11.c     |  2 +-
 sysdeps/pthread/tst-cond26.c     |  2 +-
 sysdeps/pthread/tst-cond27.c     |  4 ++--
 sysdeps/pthread/tst-join14.c     |  2 +-
 sysdeps/pthread/tst-join3.c      |  8 ++++----
 sysdeps/pthread/tst-join5.c      | 12 ++++++------
 sysdeps/pthread/tst-mutex5.c     | 12 ++++++------
 sysdeps/pthread/tst-mutex9.c     |  4 ++--
 sysdeps/pthread/tst-rwlock14.c   |  2 +-
 28 files changed, 111 insertions(+), 109 deletions(-)
  

On Mon, 1 Jun 2020, Lukasz Majewski wrote:

> First timespec* functions are renamed to have common "__" prefix for internal
> functions. This is a preparatory work for further conversion.

Leading "__" is *only* needed when the name is used in contexts where it 
could conflict with a user identifier.  For example, in installed headers 
or with external linkage.

In particular, static inline functions in non-installed headers never need 
a leading "__".  So there is no justification for renaming 
timespec_compare unless you plan to make it an extern, non-inline 
function, in which case you should say so explicitly in that patch's 
commit message.

xclock_gettime is inherently unsuitable for use in installed libraries, 
because it exits (FAIL_EXIT1) on error, which is not suitable for library 
code.  So there is no need to rename that function; any installed library 
code that uses it has to be fixed not to use it and instead to do 
appropriate error checks on the result of clock_gettime (returning an 
error from the caller if appropriate) itself; library code should almost 
never exit the process on error.  Likewise xclock_now, because it calls 
xclock_gettime, must not be used in installed libraries.

These function naming changes are only appropriate for external linkage 
functions whose semantics are appropriate for use in installed libraries 
and that are actually used in such libraries or that you intend to be used 
in such libraries.  Please review all those changes to make sure that you 
don't rename functions for which such library use is not appropriate or 
not planned.
  

Hi Joseph,

> On Mon, 1 Jun 2020, Lukasz Majewski wrote:
> 
> > First timespec* functions are renamed to have common "__" prefix
> > for internal functions. This is a preparatory work for further
> > conversion.  
> 
> Leading "__" is *only* needed when the name is used in contexts where
> it could conflict with a user identifier.  For example, in installed
> headers or with external linkage.
> 
> In particular, static inline functions in non-installed headers never
> need a leading "__".  So there is no justification for renaming 
> timespec_compare unless you plan to make it an extern, non-inline 
> function, in which case you should say so explicitly in that patch's 
> commit message.
> 
> xclock_gettime is inherently unsuitable for use in installed
> libraries, because it exits (FAIL_EXIT1) on error, which is not
> suitable for library code.  So there is no need to rename that
> function; any installed library code that uses it has to be fixed not
> to use it and instead to do appropriate error checks on the result of
> clock_gettime (returning an error from the caller if appropriate)
> itself; library code should almost never exit the process on error.
> Likewise xclock_now, because it calls xclock_gettime, must not be
> used in installed libraries.
> 
> These function naming changes are only appropriate for external
> linkage functions whose semantics are appropriate for use in
> installed libraries and that are actually used in such libraries or
> that you intend to be used in such libraries.  Please review all
> those changes to make sure that you don't rename functions for which
> such library use is not appropriate or not planned.
> 

Thanks for very detailed explanation. Considering the above arguments -
there is no point in converting timespec_* and xclock_* functions as
those are only used internally in glibc - either as helper functions or
for writing tests.

I will drop patches 02-07.


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
  

Dear Community,

I would like to bring up one relevant Y2038 support issue  - to be more
specific - the conversion/replacement of struct timespec to struct
__timespec64 in glibc internal code (source tree).

For example - I would like to convert nptl and pthreads to be Y2038
safe. To do that the timespec_* helpers and some functions, which use
futex_time64 syscall, (from 5.1+) need to use struct __timespec64. 

The problem is with instant replacement of struct timespec with
struct __timespec64 in glibc internal code (and tests). 


To do it I could:
- Replace its occurences in relevant directories - like ./nptl or
  ./sysdeps/pthread - i.e. rename all occurrences in a single directory
 
- Replace them in functions (tests) and use explicit conversion
  functions - like valid_timespec_to_timespec64() before passing struct
  __timespec64 arguments (like ones for futex_time64 for nptl).

- Replace _all_ occurrences in glibc tree of struct timespec with struct
  __timespec64 at once with using sed on the glibc tree.

The last option seems to be the most appealing as we already use
__timespec64 (with its aliasing) for some core system syscalls (like
clock_gettime). 
However, such patch shall be applied just after the release of new
stable glibc version (August 2020?) to have time for potential fixes.


Which options shall we use?



A few more related questions:
- Shall tests in ./nptl and ./sysdeps/pthread [*] use struct
  __timespec64 or struct timespec? 
  From my understanding tests (like ./nptl/tst-*) use exported headers
  so struct timespec for them is struct __timespec64 anyway for archs
  with __WORDSIZE == 32 and __TIMESIZE !=64.

- Other time related structures needs to be converted as well - like
  struct itimerspec. 


[*] - from running scripts/build-many-glibcs.py it looks like only
i686-gnu port (HURD) is using code in ./sysdeps/pthread. Will
./sysdeps/pthread be replaced by nptl in some near time in the future
(and removed)? 



Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
  

On Wed, 3 Jun 2020, Lukasz Majewski wrote:

> To do it I could:
> - Replace its occurences in relevant directories - like ./nptl or
>   ./sysdeps/pthread - i.e. rename all occurrences in a single directory
>  
> - Replace them in functions (tests) and use explicit conversion
>   functions - like valid_timespec_to_timespec64() before passing struct
>   __timespec64 arguments (like ones for futex_time64 for nptl).
> 
> - Replace _all_ occurrences in glibc tree of struct timespec with struct
>   __timespec64 at once with using sed on the glibc tree.

Using sed obviously won't work, since external interfaces need different 
handling from purely internal uses.  I think you need to change things bit 
by bit, in sufficiently small patches for convenient review.

Regarding tests, I expect many tests of time-related interfaces should end 
up being built and run twice on systems that currently use 32-bit time, 
once to test the interfaces with 32-bit time and once to test the 
interfaces with 64-bit time.  Also, tests can't generally use 64-bit time 
interfaces from libc until _TIME_BITS=64 support is actually implemented.  
So I think tests would be one of the last places to change (and similarly 
installed executables).  Whereas any internal use of time in a function in 
the libraries that does not involve time in its interface can be updated 
more or less independently of any other such use, provided the relevant 
internal interfaces using 64-bit time are now available.
  

Hi Joseph,

> On Wed, 3 Jun 2020, Lukasz Majewski wrote:
> 
> > To do it I could:
> > - Replace its occurences in relevant directories - like ./nptl or
> >   ./sysdeps/pthread - i.e. rename all occurrences in a single
> > directory 
> > - Replace them in functions (tests) and use explicit conversion
> >   functions - like valid_timespec_to_timespec64() before passing
> > struct __timespec64 arguments (like ones for futex_time64 for nptl).
> > 
> > - Replace _all_ occurrences in glibc tree of struct timespec with
> > struct __timespec64 at once with using sed on the glibc tree.  
> 
> Using sed obviously won't work, since external interfaces need
> different handling from purely internal uses.  I think you need to
> change things bit by bit, in sufficiently small patches for
> convenient review.

Considering the above comment - it seems like it would be best to
replace struct timespec with struct __timespec64 in directories - like
./nptl and omit tests from converison.

> 
> Regarding tests, I expect many tests of time-related interfaces
> should end up being built and run twice on systems that currently use
> 32-bit time, once to test the interfaces with 32-bit time and once to
> test the interfaces with 64-bit time.  Also, tests can't generally
> use 64-bit time interfaces from libc until _TIME_BITS=64 support is
> actually implemented. So I think tests would be one of the last
> places to change (and similarly installed executables).  Whereas any
> internal use of time in a function in the libraries that does not
> involve time in its interface can be updated more or less
> independently of any other such use, provided the relevant internal
> interfaces using 64-bit time are now available.
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
  

Hello,

Lukasz Majewski, le mer. 03 juin 2020 14:53:47 +0200, a ecrit:
> [*] - from running scripts/build-many-glibcs.py it looks like only
> i686-gnu port (HURD) is using code in ./sysdeps/pthread.

? No, nptl also uses e.g. the C11 threads implementation that is there.

> Will ./sysdeps/pthread be replaced by nptl in some near time in the
> future (and removed)?

Ideally nptl and htl (and fbtl of the freebsd port) would be merged.
Apparently nptl is not as linux-specific as I could fear, and now
that gnumach has a futex-like interface (gsync) possibly it could
just be used, but it won't be just a snap, htl implements at least
some cancel support that we need in userland filesystem support
(pthread_hurd_cond_timedwait_np), and also the pthread cancel, signal,
TLS supports need to be plugged. I tend to make htl interface like nptl
with the libc, so that should be feasibly long-term.

Samuel
  

Hi Joseph,

> On Wed, 3 Jun 2020, Lukasz Majewski wrote:
> 
> > To do it I could:
> > - Replace its occurences in relevant directories - like ./nptl or
> >   ./sysdeps/pthread - i.e. rename all occurrences in a single
> > directory 
> > - Replace them in functions (tests) and use explicit conversion
> >   functions - like valid_timespec_to_timespec64() before passing
> > struct __timespec64 arguments (like ones for futex_time64 for nptl).
> > 
> > - Replace _all_ occurrences in glibc tree of struct timespec with
> > struct __timespec64 at once with using sed on the glibc tree.  
> 
> Using sed obviously won't work, since external interfaces need
> different handling from purely internal uses.  I think you need to
> change things bit by bit, in sufficiently small patches for
> convenient review.
> 
> Regarding tests, I expect many tests of time-related interfaces
> should end up being built and run twice on systems that currently use
> 32-bit time, once to test the interfaces with 32-bit time and once to
> test the interfaces with 64-bit time.  

Does it mean that I shall NOT make the struct timespec to struct
__timespec64 conversion in glibc tests (e.g.
./sysdeps/pthread/tst-mutex5.c). Would those tests build infrastructure
be changed to build them w/ _TIME_BITS=64 support?


One thing puzzles me though - it seems like xclock_gettime, xclock_now,
timespec_add, timespec_sub are used by glibc tests (#include
<support/timespec.h>).

Am I correct that those functions are not exported and used solely
inside glibc?

The problem is that I do need to convert them to support 64 bit time as
pthreads use them (sysdeps/pthread/timer_settime.c). This also means
that I would need to update tests (otherwise I would experience
build/tests errors), which you don't recommend before the _TIME_BITS=64
support is added.

How to proceed in this case? Two big parts of Y2038 support are still
missing:

- One is stat and friends (which may be simplified when we move forward
  with minimal kernel version supported)

- And second are pthreads (nptl).

Pthreads are important for user space applications - so they shall be
converted sooner than latter IMHO.

> Also, tests can't generally
> use 64-bit time interfaces from libc until _TIME_BITS=64 support is
> actually implemented. 

Does it mean that tests - like sysdeps/pthread/tst-mutex5.c - will
always use exported struct timespec? (aliased to 64 bit struct 
__timespec64 when needed)?

> So I think tests would be one of the last
> places to change (and similarly installed executables). 

> Whereas any
> internal use of time in a function in the libraries that does not
> involve time in its interface can be updated more or less
> independently of any other such use, provided the relevant internal
> interfaces using 64-bit time are now available.
> 

As I stated above - e.g. timespec_sub() helper function (from /support)
is used by both pthreats and tests.

Despite it is internal function - shall I follow the pattern and
rewrite it as:

__timespec_add64(... struct __timespec64)
{

..

}
#if __TIMESIZE != 64
libc_hidden_def (__timespec_add64)

__timespec_add(... struct timespec64)
{
	call __timespec_add64();

}

However, the above approach seems to me like an overkill to do this for
internally used support function.



Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
  

On Jun 24 2020, Lukasz Majewski wrote:

> As I stated above - e.g. timespec_sub() helper function (from /support)
> is used by both pthreats and tests.

There are two definitions of timespec_sub, in support/timespec-sub.c and
in sysdeps/pthread/posix-timer.h.  The former is for use in tests, the
latter is used by libpthread.

The functions in support/timespec-*.c should probably be renamed to
xtimespec_*.

Andreas.
  

On Wed, 24 Jun 2020, Lukasz Majewski wrote:

> > Also, tests can't generally
> > use 64-bit time interfaces from libc until _TIME_BITS=64 support is
> > actually implemented. 
> 
> Does it mean that tests - like sysdeps/pthread/tst-mutex5.c - will
> always use exported struct timespec? (aliased to 64 bit struct 
> __timespec64 when needed)?

Yes.  Tests normally use public interfaces, not internal ones.  Where a 
test needs to use internal interfaces related to time, such interfaces may 
end up needing variants for different sizes of time_t (but unless they 
actually result in functions defined in installed shared libraries and 
used both from those shared libraries and from other installed libraries 
or executables, the hidden_def mechanism is not relevant for such 
interfaces).
  

Hi Andreas,

> On Jun 24 2020, Lukasz Majewski wrote:
> 
> > As I stated above - e.g. timespec_sub() helper function (from
> > /support) is used by both pthreats and tests.  
> 
> There are two definitions of timespec_sub, in support/timespec-sub.c
> and in sysdeps/pthread/posix-timer.h.  The former is for use in
> tests, the latter is used by libpthread.

So if I understood it correctly - the support/* functions are to
facilitate writing tests?

> 
> The functions in support/timespec-*.c should probably be renamed to
> xtimespec_*.

And the 'x' prefix means that those functions are supposed to be used
for testing?

> 
> Andreas.
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
  

On Wed, 24 Jun 2020, Lukasz Majewski wrote:

> > The functions in support/timespec-*.c should probably be renamed to
> > xtimespec_*.
> 
> And the 'x' prefix means that those functions are supposed to be used
> for testing?

The 'x' prefix is in the style of xmalloc as used in many GNU packages - 
checking for errors in a safer way than the default libc API.

Most of the 'x' functions are thereby unsuitable for use in library code, 
but may be used in tests and in code that goes into the various 
executables shipped with glibc, because they exit the program on error and 
library code should generally not do that.  (The 'x' names are also not in 
the implementation namespace; non-static internal functions used within 
library code need to have reserved names such as __*.)