mbox series

[v7,0/3] y2038: Linux: Introduce __clock_settime64 function

Message ID 20190906145911.30207-1-lukma@denx.de
Headers show
Series y2038: Linux: Introduce __clock_settime64 function | expand

Message

Lukasz Majewski Sept. 6, 2019, 2:59 p.m. UTC 
This patch set introduces the conversion of clock_settime to explicit
64 bit struct __timespec64 arguments. As a result this function is now
Y2038 safe.

This work is (loosely) based on a previous development/patches:
https://libc-alpha.sourceware.narkive.com/zniMOWui/rfc-patch-00-52-make-glibc-y2038-proof#post68

Github branch (including the y2038 conversion example):
https://github.com/lmajewski/y2038_glibc/commits/glibc__clock_settime-conversion-v7

Those patches have been applied on top of master branch:
SHA1: a26918cfda4bc4b9dad8aae1496e3ef7cbb63d96

Shall be used with provided meta-y2038 for development and testing:
https://github.com/lmajewski/meta-y2038

I've used guidelines from:
https://www.gnu.org/software/libc/manual/html_mono/libc.html
"D.2.1 64-bit time symbol handling in the GNU C Library"
to convert *clock_settime*.

and most notably from:
https://sourceware.org/glibc/wiki/Y2038ProofnessDesign#clock_gettime.28.29


Lukasz Majewski (3):
  y2038: Introduce internal for glibc struct __timespec64
  y2038: Provide conversion helpers for struct __timespec64
  y2038: linux: Provide __clock_settime64 implementation

 include/time.h                          | 116 ++++++++++++++++++++++++
 sysdeps/unix/sysv/linux/clock_settime.c |  38 +++++++-
 2 files changed, 150 insertions(+), 4 deletions(-)

Comments

Alistair Francis Sept. 6, 2019, 4:52 p.m. UTC | #1 
On Fri, Sep 6, 2019 at 7:59 AM Lukasz Majewski <lukma@denx.de> wrote:
>
> This patch set introduces the conversion of clock_settime to explicit
> 64 bit struct __timespec64 arguments. As a result this function is now
> Y2038 safe.
>
> This work is (loosely) based on a previous development/patches:
> https://libc-alpha.sourceware.narkive.com/zniMOWui/rfc-patch-00-52-make-glibc-y2038-proof#post68
>
> Github branch (including the y2038 conversion example):
> https://github.com/lmajewski/y2038_glibc/commits/glibc__clock_settime-conversion-v7
>
> Those patches have been applied on top of master branch:
> SHA1: a26918cfda4bc4b9dad8aae1496e3ef7cbb63d96
>
> Shall be used with provided meta-y2038 for development and testing:
> https://github.com/lmajewski/meta-y2038
>
> I've used guidelines from:
> https://www.gnu.org/software/libc/manual/html_mono/libc.html
> "D.2.1 64-bit time symbol handling in the GNU C Library"
> to convert *clock_settime*.
>
> and most notably from:
> https://sourceware.org/glibc/wiki/Y2038ProofnessDesign#clock_gettime.28.29

Thanks for the patches Lukassz!

With my RV32 port I see this failure when compiling:

In file included from ../sysdeps/generic/hp-timing.h:23,
                 from ../nptl/descr.h:27,
                 from ../sysdeps/riscv/nptl/tls.h:41,
                 from ../include/errno.h:25,
                 from ../sysdeps/unix/sysv/linux/clock_settime.c:18:
../include/time.h:129:28: error: conflicting types for '__clock_settime'
 # define __clock_settime64 __clock_settime
                            ^~~~~~~~~~~~~~~
../sysdeps/unix/sysv/linux/clock_settime.c:25:1: note: in expansion of
macro '__clock_settime64'
 __clock_settime64 (clockid_t clock_id, const struct __timespec64 *tp)
 ^~~~~~~~~~~~~~~~~
In file included from <command-line>:
../include/time.h:25:20: note: previous declaration of
'__clock_settime' was here
 libc_hidden_proto (__clock_settime)
                    ^~~~~~~~~~~~~~~
./../include/libc-symbols.h:598:33: note: in definition of macro
'__hidden_proto'
   extern thread __typeof (name) name __hidden_proto_hiddenattr (attrs);
                                 ^~~~
./../include/libc-symbols.h:617:44: note: in expansion of macro 'hidden_proto'
 # define libc_hidden_proto(name, attrs...) hidden_proto (name, ##attrs)
                                            ^~~~~~~~~~~~
../include/time.h:25:1: note: in expansion of macro 'libc_hidden_proto'
 libc_hidden_proto (__clock_settime)
 ^~~~~~~~~~~~~~~~~
../sysdeps/unix/sysv/linux/clock_settime.c:70:42: error: conflicting
types for 'clock_settime'
 versioned_symbol (libc, __clock_settime, clock_settime, GLIBC_2_17);
                                          ^~~~~~~~~~~~~
./../include/libc-symbols.h:152:26: note: in definition of macro '_weak_alias'
   extern __typeof (name) aliasname __attribute__ ((weak, alias (#name))) \
                          ^~~~~~~~~
../include/shlib-compat.h:74:3: note: in expansion of macro 'weak_alias'
   weak_alias (local, symbol)
   ^~~~~~~~~~
../sysdeps/unix/sysv/linux/clock_settime.c:70:1: note: in expansion of
macro 'versioned_symbol'
 versioned_symbol (libc, __clock_settime, clock_settime, GLIBC_2_17);
 ^~~~~~~~~~~~~~~~
In file included from ../include/time.h:2,
                 from ../sysdeps/generic/hp-timing.h:23,
                 from ../nptl/descr.h:27,
                 from ../sysdeps/riscv/nptl/tls.h:41,
                 from ../include/errno.h:25,
                 from ../sysdeps/unix/sysv/linux/clock_settime.c:18:
../time/time.h:222:12: note: previous declaration of 'clock_settime' was here
 extern int clock_settime (clockid_t __clock_id, const struct timespec *__tp)

Which I can fix with this diff:

diff --git a/include/time.h b/include/time.h
index 7ed3aa61d1d..28e2722de21 100644
--- a/include/time.h
+++ b/include/time.h
@@ -125,13 +125,9 @@ extern __time64_t __timegm64 (struct tm *__tp) __THROW;
 libc_hidden_proto (__timegm64)
 #endif

-#if __TIMESIZE == 64
-# define __clock_settime64 __clock_settime
-#else
 extern int __clock_settime64 (clockid_t clock_id,
                               const struct __timespec64 *tp);
 libc_hidden_proto (__clock_settime64)
-#endif

 /* Compute the `struct tm' representation of T,
    offset OFFSET seconds east of UTC,
diff --git a/sysdeps/unix/sysv/linux/clock_settime.c
b/sysdeps/unix/sysv/linux/clock_settime.c
index f5e084238a5..ab033c56ea9 100644
--- a/sysdeps/unix/sysv/linux/clock_settime.c
+++ b/sysdeps/unix/sysv/linux/clock_settime.c
@@ -54,7 +54,6 @@ __clock_settime64 (clockid_t clock_id, const struct
__timespec64 *tp)
 #endif
 }

-#if __TIMESIZE != 64
 int
 __clock_settime (clockid_t clock_id, const struct timespec *tp)
 {
@@ -63,7 +62,6 @@ __clock_settime (clockid_t clock_id, const struct
timespec *tp)
   valid_timespec_to_timespec64 (tp, &ts64);
   return __clock_settime64 (clock_id, &ts64);
 }
-#endif

 libc_hidden_def (__clock_settime)

Alistair

>
>
> Lukasz Majewski (3):
>   y2038: Introduce internal for glibc struct __timespec64
>   y2038: Provide conversion helpers for struct __timespec64
>   y2038: linux: Provide __clock_settime64 implementation
>
>  include/time.h                          | 116 ++++++++++++++++++++++++
>  sysdeps/unix/sysv/linux/clock_settime.c |  38 +++++++-
>  2 files changed, 150 insertions(+), 4 deletions(-)
>
> --
> 2.20.1
>
Joseph Myers Sept. 6, 2019, 9:28 p.m. UTC | #2 
On Fri, 6 Sep 2019, Alistair Francis wrote:

> Which I can fix with this diff:

This diff is not the right way to fix this build failure.

One of the design principles in the Y2038 support is that is __TIMESIZE == 
64, the time functions *aren't* trivial wrappers of time64 functions; 
rather, the time64 function definitions are remapped (via macros) so they 
define the function name with no "64".  For each case where there is a 
pair of functions (for different time_t types) in the __TIMESIZE == 32 
case, there should just be the one function when __TIMESIZE == 64.

This ensures that the Y2038 changes don't add any overhead at all in the 
glibc binaries on existing platforms with __TIMESIZE == 64.

You should look at exactly what the types in question are, that are being 
reported as conflicting in your build (probably by looking at preprocessed 
source).  __timespec64 and timespec are supposed to be the same type (via 
#define) when __TIMESIZE == 64, to avoid such incompatibilities.
Lukasz Majewski Sept. 6, 2019, 9:55 p.m. UTC | #3 
Hi Alistair,

> On Fri, Sep 6, 2019 at 7:59 AM Lukasz Majewski <lukma@denx.de> wrote:
> >
> > This patch set introduces the conversion of clock_settime to
> > explicit 64 bit struct __timespec64 arguments. As a result this
> > function is now Y2038 safe.
> >
> > This work is (loosely) based on a previous development/patches:
> > https://libc-alpha.sourceware.narkive.com/zniMOWui/rfc-patch-00-52-make-glibc-y2038-proof#post68
> >
> > Github branch (including the y2038 conversion example):
> > https://github.com/lmajewski/y2038_glibc/commits/glibc__clock_settime-conversion-v7
> >
> > Those patches have been applied on top of master branch:
> > SHA1: a26918cfda4bc4b9dad8aae1496e3ef7cbb63d96
> >
> > Shall be used with provided meta-y2038 for development and testing:
> > https://github.com/lmajewski/meta-y2038
> >
> > I've used guidelines from:
> > https://www.gnu.org/software/libc/manual/html_mono/libc.html
> > "D.2.1 64-bit time symbol handling in the GNU C Library"
> > to convert *clock_settime*.
> >
> > and most notably from:
> > https://sourceware.org/glibc/wiki/Y2038ProofnessDesign#clock_gettime.28.29
> >  
> 
> Thanks for the patches Lukassz!
> 
> With my RV32 port I see this failure when compiling:
> 

But I did not change the core code between v5 (to which you refer here):
https://patchwork.ozlabs.org/cover/1155397/
and then:
https://sourceware.org/ml/libc-alpha/2019-05/msg00661.html

and v7:
https://patchwork.ozlabs.org/patch/1159056/

(The only change in v6 was to use 32 bit call to clock_settime syscall
as requested by Arnd to facilitate validation on existing systems.
However, Joseph was against this change).

The notable change (but not in this patch set) was moving clock_settime
symbol solely to glibc (and it has been removed from librt).

> In file included from ../sysdeps/generic/hp-timing.h:23,
>                  from ../nptl/descr.h:27,
>                  from ../sysdeps/riscv/nptl/tls.h:41,
>                  from ../include/errno.h:25,
>                  from ../sysdeps/unix/sysv/linux/clock_settime.c:18:
> ../include/time.h:129:28: error: conflicting types for
> '__clock_settime' # define __clock_settime64 __clock_settime
>                             ^~~~~~~~~~~~~~~
> ../sysdeps/unix/sysv/linux/clock_settime.c:25:1: note: in expansion of
> macro '__clock_settime64'
>  __clock_settime64 (clockid_t clock_id, const struct __timespec64 *tp)
>  ^~~~~~~~~~~~~~~~~
> In file included from <command-line>:
> ../include/time.h:25:20: note: previous declaration of
> '__clock_settime' was here
>  libc_hidden_proto (__clock_settime)
>                     ^~~~~~~~~~~~~~~
> ./../include/libc-symbols.h:598:33: note: in definition of macro
> '__hidden_proto'
>    extern thread __typeof (name) name __hidden_proto_hiddenattr
> (attrs); ^~~~
> ./../include/libc-symbols.h:617:44: note: in expansion of macro
> 'hidden_proto' # define libc_hidden_proto(name, attrs...)
> hidden_proto (name, ##attrs) ^~~~~~~~~~~~
> ../include/time.h:25:1: note: in expansion of macro
> 'libc_hidden_proto' libc_hidden_proto (__clock_settime)
>  ^~~~~~~~~~~~~~~~~
> ../sysdeps/unix/sysv/linux/clock_settime.c:70:42: error: conflicting
> types for 'clock_settime'
>  versioned_symbol (libc, __clock_settime, clock_settime, GLIBC_2_17);
>                                           ^~~~~~~~~~~~~
> ./../include/libc-symbols.h:152:26: note: in definition of macro
> '_weak_alias' extern __typeof (name) aliasname __attribute__ ((weak,
> alias (#name))) \ ^~~~~~~~~
> ../include/shlib-compat.h:74:3: note: in expansion of macro
> 'weak_alias' weak_alias (local, symbol)
>    ^~~~~~~~~~
> ../sysdeps/unix/sysv/linux/clock_settime.c:70:1: note: in expansion of
> macro 'versioned_symbol'
>  versioned_symbol (libc, __clock_settime, clock_settime, GLIBC_2_17);
>  ^~~~~~~~~~~~~~~~
> In file included from ../include/time.h:2,
>                  from ../sysdeps/generic/hp-timing.h:23,
>                  from ../nptl/descr.h:27,
>                  from ../sysdeps/riscv/nptl/tls.h:41,
>                  from ../include/errno.h:25,
>                  from ../sysdeps/unix/sysv/linux/clock_settime.c:18:
> ../time/time.h:222:12: note: previous declaration of 'clock_settime'
> was here extern int clock_settime (clockid_t __clock_id, const struct
> timespec *__tp)
> 
> Which I can fix with this diff:
> 
> diff --git a/include/time.h b/include/time.h
> index 7ed3aa61d1d..28e2722de21 100644
> --- a/include/time.h
> +++ b/include/time.h
> @@ -125,13 +125,9 @@ extern __time64_t __timegm64 (struct tm *__tp)
> __THROW; libc_hidden_proto (__timegm64)
>  #endif
> 
> -#if __TIMESIZE == 64

What is the value of __TIMESIZE on your port? Is it 32 or 64 ?

Do you also #define __USE_TIME_BITS64	1 ?

> -# define __clock_settime64 __clock_settime
> -#else
>  extern int __clock_settime64 (clockid_t clock_id,
>                                const struct __timespec64 *tp);
>  libc_hidden_proto (__clock_settime64)
> -#endif
> 
>  /* Compute the `struct tm' representation of T,
>     offset OFFSET seconds east of UTC,
> diff --git a/sysdeps/unix/sysv/linux/clock_settime.c
> b/sysdeps/unix/sysv/linux/clock_settime.c
> index f5e084238a5..ab033c56ea9 100644
> --- a/sysdeps/unix/sysv/linux/clock_settime.c
> +++ b/sysdeps/unix/sysv/linux/clock_settime.c
> @@ -54,7 +54,6 @@ __clock_settime64 (clockid_t clock_id, const struct
> __timespec64 *tp)
>  #endif
>  }
> 
> -#if __TIMESIZE != 64

When I look on RISC-V patchset:
https://patchwork.ozlabs.org/patch/1155391/

for the __clock_nanosleep for example you follow the same convention.
And you don't need to remove #if __TIMESIZE != 64 for example.

One difference is that the clock_settime64 code will be Y2038 on e.g.
32 bit ARM only when I apply on top of it following patch:
https://github.com/lmajewski/y2038_glibc/commit/c6740c05ea9b224a552847d10402f98da8376994

(it enables redirect for clock_settime and support for -D_TIME_BITS=64
compilation flag).



>  int
>  __clock_settime (clockid_t clock_id, const struct timespec *tp)
>  {
> @@ -63,7 +62,6 @@ __clock_settime (clockid_t clock_id, const struct
> timespec *tp)
>    valid_timespec_to_timespec64 (tp, &ts64);
>    return __clock_settime64 (clock_id, &ts64);
>  }
> -#endif
> 
>  libc_hidden_def (__clock_settime)
> 
> Alistair
> 
> >
> >
> > Lukasz Majewski (3):
> >   y2038: Introduce internal for glibc struct __timespec64
> >   y2038: Provide conversion helpers for struct __timespec64
> >   y2038: linux: Provide __clock_settime64 implementation
> >
> >  include/time.h                          | 116
> > ++++++++++++++++++++++++ sysdeps/unix/sysv/linux/clock_settime.c |
> > 38 +++++++- 2 files changed, 150 insertions(+), 4 deletions(-)
> >
> > --
> > 2.20.1
> >  



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
Alistair Francis Sept. 6, 2019, 10:01 p.m. UTC | #4 
On Fri, Sep 6, 2019 at 2:55 PM Lukasz Majewski <lukma@denx.de> wrote:
>
> Hi Alistair,
>
> > On Fri, Sep 6, 2019 at 7:59 AM Lukasz Majewski <lukma@denx.de> wrote:
> > >
> > > This patch set introduces the conversion of clock_settime to
> > > explicit 64 bit struct __timespec64 arguments. As a result this
> > > function is now Y2038 safe.
> > >
> > > This work is (loosely) based on a previous development/patches:
> > > https://libc-alpha.sourceware.narkive.com/zniMOWui/rfc-patch-00-52-make-glibc-y2038-proof#post68
> > >
> > > Github branch (including the y2038 conversion example):
> > > https://github.com/lmajewski/y2038_glibc/commits/glibc__clock_settime-conversion-v7
> > >
> > > Those patches have been applied on top of master branch:
> > > SHA1: a26918cfda4bc4b9dad8aae1496e3ef7cbb63d96
> > >
> > > Shall be used with provided meta-y2038 for development and testing:
> > > https://github.com/lmajewski/meta-y2038
> > >
> > > I've used guidelines from:
> > > https://www.gnu.org/software/libc/manual/html_mono/libc.html
> > > "D.2.1 64-bit time symbol handling in the GNU C Library"
> > > to convert *clock_settime*.
> > >
> > > and most notably from:
> > > https://sourceware.org/glibc/wiki/Y2038ProofnessDesign#clock_gettime.28.29
> > >
> >
> > Thanks for the patches Lukassz!
> >
> > With my RV32 port I see this failure when compiling:
> >
>
> But I did not change the core code between v5 (to which you refer here):
> https://patchwork.ozlabs.org/cover/1155397/
> and then:
> https://sourceware.org/ml/libc-alpha/2019-05/msg00661.html
>
> and v7:
> https://patchwork.ozlabs.org/patch/1159056/
>
> (The only change in v6 was to use 32 bit call to clock_settime syscall
> as requested by Arnd to facilitate validation on existing systems.
> However, Joseph was against this change).
>
> The notable change (but not in this patch set) was moving clock_settime
> symbol solely to glibc (and it has been removed from librt).
>

Yes, I have had this issue for awhile.

> > In file included from ../sysdeps/generic/hp-timing.h:23,
> >                  from ../nptl/descr.h:27,
> >                  from ../sysdeps/riscv/nptl/tls.h:41,
> >                  from ../include/errno.h:25,
> >                  from ../sysdeps/unix/sysv/linux/clock_settime.c:18:
> > ../include/time.h:129:28: error: conflicting types for
> > '__clock_settime' # define __clock_settime64 __clock_settime
> >                             ^~~~~~~~~~~~~~~
> > ../sysdeps/unix/sysv/linux/clock_settime.c:25:1: note: in expansion of
> > macro '__clock_settime64'
> >  __clock_settime64 (clockid_t clock_id, const struct __timespec64 *tp)
> >  ^~~~~~~~~~~~~~~~~
> > In file included from <command-line>:
> > ../include/time.h:25:20: note: previous declaration of
> > '__clock_settime' was here
> >  libc_hidden_proto (__clock_settime)
> >                     ^~~~~~~~~~~~~~~
> > ./../include/libc-symbols.h:598:33: note: in definition of macro
> > '__hidden_proto'
> >    extern thread __typeof (name) name __hidden_proto_hiddenattr
> > (attrs); ^~~~
> > ./../include/libc-symbols.h:617:44: note: in expansion of macro
> > 'hidden_proto' # define libc_hidden_proto(name, attrs...)
> > hidden_proto (name, ##attrs) ^~~~~~~~~~~~
> > ../include/time.h:25:1: note: in expansion of macro
> > 'libc_hidden_proto' libc_hidden_proto (__clock_settime)
> >  ^~~~~~~~~~~~~~~~~
> > ../sysdeps/unix/sysv/linux/clock_settime.c:70:42: error: conflicting
> > types for 'clock_settime'
> >  versioned_symbol (libc, __clock_settime, clock_settime, GLIBC_2_17);
> >                                           ^~~~~~~~~~~~~
> > ./../include/libc-symbols.h:152:26: note: in definition of macro
> > '_weak_alias' extern __typeof (name) aliasname __attribute__ ((weak,
> > alias (#name))) \ ^~~~~~~~~
> > ../include/shlib-compat.h:74:3: note: in expansion of macro
> > 'weak_alias' weak_alias (local, symbol)
> >    ^~~~~~~~~~
> > ../sysdeps/unix/sysv/linux/clock_settime.c:70:1: note: in expansion of
> > macro 'versioned_symbol'
> >  versioned_symbol (libc, __clock_settime, clock_settime, GLIBC_2_17);
> >  ^~~~~~~~~~~~~~~~
> > In file included from ../include/time.h:2,
> >                  from ../sysdeps/generic/hp-timing.h:23,
> >                  from ../nptl/descr.h:27,
> >                  from ../sysdeps/riscv/nptl/tls.h:41,
> >                  from ../include/errno.h:25,
> >                  from ../sysdeps/unix/sysv/linux/clock_settime.c:18:
> > ../time/time.h:222:12: note: previous declaration of 'clock_settime'
> > was here extern int clock_settime (clockid_t __clock_id, const struct
> > timespec *__tp)
> >
> > Which I can fix with this diff:
> >
> > diff --git a/include/time.h b/include/time.h
> > index 7ed3aa61d1d..28e2722de21 100644
> > --- a/include/time.h
> > +++ b/include/time.h
> > @@ -125,13 +125,9 @@ extern __time64_t __timegm64 (struct tm *__tp)
> > __THROW; libc_hidden_proto (__timegm64)
> >  #endif
> >
> > -#if __TIMESIZE == 64
>
> What is the value of __TIMESIZE on your port? Is it 32 or 64 ?

It's 64.

>
> Do you also #define __USE_TIME_BITS64   1 ?

I don't. I can try with that, but I don't see if defined in the source.

Alistair

>
> > -# define __clock_settime64 __clock_settime
> > -#else
> >  extern int __clock_settime64 (clockid_t clock_id,
> >                                const struct __timespec64 *tp);
> >  libc_hidden_proto (__clock_settime64)
> > -#endif
> >
> >  /* Compute the `struct tm' representation of T,
> >     offset OFFSET seconds east of UTC,
> > diff --git a/sysdeps/unix/sysv/linux/clock_settime.c
> > b/sysdeps/unix/sysv/linux/clock_settime.c
> > index f5e084238a5..ab033c56ea9 100644
> > --- a/sysdeps/unix/sysv/linux/clock_settime.c
> > +++ b/sysdeps/unix/sysv/linux/clock_settime.c
> > @@ -54,7 +54,6 @@ __clock_settime64 (clockid_t clock_id, const struct
> > __timespec64 *tp)
> >  #endif
> >  }
> >
> > -#if __TIMESIZE != 64
>
> When I look on RISC-V patchset:
> https://patchwork.ozlabs.org/patch/1155391/
>
> for the __clock_nanosleep for example you follow the same convention.
> And you don't need to remove #if __TIMESIZE != 64 for example.
>
> One difference is that the clock_settime64 code will be Y2038 on e.g.
> 32 bit ARM only when I apply on top of it following patch:
> https://github.com/lmajewski/y2038_glibc/commit/c6740c05ea9b224a552847d10402f98da8376994
>
> (it enables redirect for clock_settime and support for -D_TIME_BITS=64
> compilation flag).
>
>
>
> >  int
> >  __clock_settime (clockid_t clock_id, const struct timespec *tp)
> >  {
> > @@ -63,7 +62,6 @@ __clock_settime (clockid_t clock_id, const struct
> > timespec *tp)
> >    valid_timespec_to_timespec64 (tp, &ts64);
> >    return __clock_settime64 (clock_id, &ts64);
> >  }
> > -#endif
> >
> >  libc_hidden_def (__clock_settime)
> >
> > Alistair
> >
> > >
> > >
> > > Lukasz Majewski (3):
> > >   y2038: Introduce internal for glibc struct __timespec64
> > >   y2038: Provide conversion helpers for struct __timespec64
> > >   y2038: linux: Provide __clock_settime64 implementation
> > >
> > >  include/time.h                          | 116
> > > ++++++++++++++++++++++++ sysdeps/unix/sysv/linux/clock_settime.c |
> > > 38 +++++++- 2 files changed, 150 insertions(+), 4 deletions(-)
> > >
> > > --
> > > 2.20.1
> > >
>
>
>
> 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
Lukasz Majewski Sept. 13, 2019, 2:36 p.m. UTC | #5 
Hi Alistair,

> On Fri, Sep 6, 2019 at 2:55 PM Lukasz Majewski <lukma@denx.de> wrote:
> >
> > Hi Alistair,
> >  
> > > On Fri, Sep 6, 2019 at 7:59 AM Lukasz Majewski <lukma@denx.de>
> > > wrote:  
> > > >
> > > > This patch set introduces the conversion of clock_settime to
> > > > explicit 64 bit struct __timespec64 arguments. As a result this
> > > > function is now Y2038 safe.
> > > >
> > > > This work is (loosely) based on a previous development/patches:
> > > > https://libc-alpha.sourceware.narkive.com/zniMOWui/rfc-patch-00-52-make-glibc-y2038-proof#post68
> > > >
> > > > Github branch (including the y2038 conversion example):
> > > > https://github.com/lmajewski/y2038_glibc/commits/glibc__clock_settime-conversion-v7
> > > >
> > > > Those patches have been applied on top of master branch:
> > > > SHA1: a26918cfda4bc4b9dad8aae1496e3ef7cbb63d96
> > > >
> > > > Shall be used with provided meta-y2038 for development and
> > > > testing: https://github.com/lmajewski/meta-y2038
> > > >
> > > > I've used guidelines from:
> > > > https://www.gnu.org/software/libc/manual/html_mono/libc.html
> > > > "D.2.1 64-bit time symbol handling in the GNU C Library"
> > > > to convert *clock_settime*.
> > > >
> > > > and most notably from:
> > > > https://sourceware.org/glibc/wiki/Y2038ProofnessDesign#clock_gettime.28.29
> > > >  
> > >
> > > Thanks for the patches Lukassz!
> > >
> > > With my RV32 port I see this failure when compiling:
> > >  
> >
> > But I did not change the core code between v5 (to which you refer
> > here): https://patchwork.ozlabs.org/cover/1155397/
> > and then:
> > https://sourceware.org/ml/libc-alpha/2019-05/msg00661.html
> >
> > and v7:
> > https://patchwork.ozlabs.org/patch/1159056/
> >
> > (The only change in v6 was to use 32 bit call to clock_settime
> > syscall as requested by Arnd to facilitate validation on existing
> > systems. However, Joseph was against this change).
> >
> > The notable change (but not in this patch set) was moving
> > clock_settime symbol solely to glibc (and it has been removed from
> > librt). 
> 
> Yes, I have had this issue for awhile.
> 
> > > In file included from ../sysdeps/generic/hp-timing.h:23,
> > >                  from ../nptl/descr.h:27,
> > >                  from ../sysdeps/riscv/nptl/tls.h:41,
> > >                  from ../include/errno.h:25,
> > >                  from
> > > ../sysdeps/unix/sysv/linux/clock_settime.c:18:
> > > ../include/time.h:129:28: error: conflicting types for
> > > '__clock_settime' # define __clock_settime64 __clock_settime
> > > ^~~~~~~~~~~~~~~ ../sysdeps/unix/sysv/linux/clock_settime.c:25:1:
> > > note: in expansion of macro '__clock_settime64'
> > >  __clock_settime64 (clockid_t clock_id, const struct __timespec64
> > > *tp) ^~~~~~~~~~~~~~~~~
> > > In file included from <command-line>:
> > > ../include/time.h:25:20: note: previous declaration of
> > > '__clock_settime' was here
> > >  libc_hidden_proto (__clock_settime)
> > >                     ^~~~~~~~~~~~~~~
> > > ./../include/libc-symbols.h:598:33: note: in definition of macro
> > > '__hidden_proto'
> > >    extern thread __typeof (name) name __hidden_proto_hiddenattr
> > > (attrs); ^~~~
> > > ./../include/libc-symbols.h:617:44: note: in expansion of macro
> > > 'hidden_proto' # define libc_hidden_proto(name, attrs...)
> > > hidden_proto (name, ##attrs) ^~~~~~~~~~~~
> > > ../include/time.h:25:1: note: in expansion of macro
> > > 'libc_hidden_proto' libc_hidden_proto (__clock_settime)
> > >  ^~~~~~~~~~~~~~~~~
> > > ../sysdeps/unix/sysv/linux/clock_settime.c:70:42: error:
> > > conflicting types for 'clock_settime'
> > >  versioned_symbol (libc, __clock_settime, clock_settime,
> > > GLIBC_2_17); ^~~~~~~~~~~~~
> > > ./../include/libc-symbols.h:152:26: note: in definition of macro
> > > '_weak_alias' extern __typeof (name) aliasname __attribute__
> > > ((weak, alias (#name))) \ ^~~~~~~~~
> > > ../include/shlib-compat.h:74:3: note: in expansion of macro
> > > 'weak_alias' weak_alias (local, symbol)
> > >    ^~~~~~~~~~
> > > ../sysdeps/unix/sysv/linux/clock_settime.c:70:1: note: in
> > > expansion of macro 'versioned_symbol'
> > >  versioned_symbol (libc, __clock_settime, clock_settime,
> > > GLIBC_2_17); ^~~~~~~~~~~~~~~~
> > > In file included from ../include/time.h:2,
> > >                  from ../sysdeps/generic/hp-timing.h:23,
> > >                  from ../nptl/descr.h:27,
> > >                  from ../sysdeps/riscv/nptl/tls.h:41,
> > >                  from ../include/errno.h:25,
> > >                  from
> > > ../sysdeps/unix/sysv/linux/clock_settime.c:18:
> > > ../time/time.h:222:12: note: previous declaration of
> > > 'clock_settime' was here extern int clock_settime (clockid_t
> > > __clock_id, const struct timespec *__tp)
> > >
> > > Which I can fix with this diff:
> > >
> > > diff --git a/include/time.h b/include/time.h
> > > index 7ed3aa61d1d..28e2722de21 100644
> > > --- a/include/time.h
> > > +++ b/include/time.h
> > > @@ -125,13 +125,9 @@ extern __time64_t __timegm64 (struct tm
> > > *__tp) __THROW; libc_hidden_proto (__timegm64)
> > >  #endif
> > >
> > > -#if __TIMESIZE == 64  
> >
> > What is the value of __TIMESIZE on your port? Is it 32 or 64 ?  
> 
> It's 64.
> 
> >
> > Do you also #define __USE_TIME_BITS64   1 ?  
> 
> I don't. I can try with that, but I don't see if defined in the
> source.

Alistair, have you managed to make your patches working on top of this
series?


> 
> Alistair
> 
> >  
> > > -# define __clock_settime64 __clock_settime
> > > -#else
> > >  extern int __clock_settime64 (clockid_t clock_id,
> > >                                const struct __timespec64 *tp);
> > >  libc_hidden_proto (__clock_settime64)
> > > -#endif
> > >
> > >  /* Compute the `struct tm' representation of T,
> > >     offset OFFSET seconds east of UTC,
> > > diff --git a/sysdeps/unix/sysv/linux/clock_settime.c
> > > b/sysdeps/unix/sysv/linux/clock_settime.c
> > > index f5e084238a5..ab033c56ea9 100644
> > > --- a/sysdeps/unix/sysv/linux/clock_settime.c
> > > +++ b/sysdeps/unix/sysv/linux/clock_settime.c
> > > @@ -54,7 +54,6 @@ __clock_settime64 (clockid_t clock_id, const
> > > struct __timespec64 *tp)
> > >  #endif
> > >  }
> > >
> > > -#if __TIMESIZE != 64  
> >
> > When I look on RISC-V patchset:
> > https://patchwork.ozlabs.org/patch/1155391/
> >
> > for the __clock_nanosleep for example you follow the same
> > convention. And you don't need to remove #if __TIMESIZE != 64 for
> > example.
> >
> > One difference is that the clock_settime64 code will be Y2038 on
> > e.g. 32 bit ARM only when I apply on top of it following patch:
> > https://github.com/lmajewski/y2038_glibc/commit/c6740c05ea9b224a552847d10402f98da8376994
> >
> > (it enables redirect for clock_settime and support for
> > -D_TIME_BITS=64 compilation flag).
> >
> >
> >  
> > >  int
> > >  __clock_settime (clockid_t clock_id, const struct timespec *tp)
> > >  {
> > > @@ -63,7 +62,6 @@ __clock_settime (clockid_t clock_id, const
> > > struct timespec *tp)
> > >    valid_timespec_to_timespec64 (tp, &ts64);
> > >    return __clock_settime64 (clock_id, &ts64);
> > >  }
> > > -#endif
> > >
> > >  libc_hidden_def (__clock_settime)
> > >
> > > Alistair
> > >  
> > > >
> > > >
> > > > Lukasz Majewski (3):
> > > >   y2038: Introduce internal for glibc struct __timespec64
> > > >   y2038: Provide conversion helpers for struct __timespec64
> > > >   y2038: linux: Provide __clock_settime64 implementation
> > > >
> > > >  include/time.h                          | 116
> > > > ++++++++++++++++++++++++
> > > > sysdeps/unix/sysv/linux/clock_settime.c | 38 +++++++- 2 files
> > > > changed, 150 insertions(+), 4 deletions(-)
> > > >
> > > > --
> > > > 2.20.1
> > > >  
> >
> >
> >
> > 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  



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
Alistair Francis Sept. 16, 2019, 9:50 p.m. UTC | #6 
On Fri, Sep 13, 2019 at 7:36 AM Lukasz Majewski <lukma@denx.de> wrote:
>
> Hi Alistair,
>
> > On Fri, Sep 6, 2019 at 2:55 PM Lukasz Majewski <lukma@denx.de> wrote:
> > >
> > > Hi Alistair,
> > >
> > > > On Fri, Sep 6, 2019 at 7:59 AM Lukasz Majewski <lukma@denx.de>
> > > > wrote:
> > > > >
> > > > > This patch set introduces the conversion of clock_settime to
> > > > > explicit 64 bit struct __timespec64 arguments. As a result this
> > > > > function is now Y2038 safe.
> > > > >
> > > > > This work is (loosely) based on a previous development/patches:
> > > > > https://libc-alpha.sourceware.narkive.com/zniMOWui/rfc-patch-00-52-make-glibc-y2038-proof#post68
> > > > >
> > > > > Github branch (including the y2038 conversion example):
> > > > > https://github.com/lmajewski/y2038_glibc/commits/glibc__clock_settime-conversion-v7
> > > > >
> > > > > Those patches have been applied on top of master branch:
> > > > > SHA1: a26918cfda4bc4b9dad8aae1496e3ef7cbb63d96
> > > > >
> > > > > Shall be used with provided meta-y2038 for development and
> > > > > testing: https://github.com/lmajewski/meta-y2038
> > > > >
> > > > > I've used guidelines from:
> > > > > https://www.gnu.org/software/libc/manual/html_mono/libc.html
> > > > > "D.2.1 64-bit time symbol handling in the GNU C Library"
> > > > > to convert *clock_settime*.
> > > > >
> > > > > and most notably from:
> > > > > https://sourceware.org/glibc/wiki/Y2038ProofnessDesign#clock_gettime.28.29
> > > > >
> > > >
> > > > Thanks for the patches Lukassz!
> > > >
> > > > With my RV32 port I see this failure when compiling:
> > > >
> > >
> > > But I did not change the core code between v5 (to which you refer
> > > here): https://patchwork.ozlabs.org/cover/1155397/
> > > and then:
> > > https://sourceware.org/ml/libc-alpha/2019-05/msg00661.html
> > >
> > > and v7:
> > > https://patchwork.ozlabs.org/patch/1159056/
> > >
> > > (The only change in v6 was to use 32 bit call to clock_settime
> > > syscall as requested by Arnd to facilitate validation on existing
> > > systems. However, Joseph was against this change).
> > >
> > > The notable change (but not in this patch set) was moving
> > > clock_settime symbol solely to glibc (and it has been removed from
> > > librt).
> >
> > Yes, I have had this issue for awhile.
> >
> > > > In file included from ../sysdeps/generic/hp-timing.h:23,
> > > >                  from ../nptl/descr.h:27,
> > > >                  from ../sysdeps/riscv/nptl/tls.h:41,
> > > >                  from ../include/errno.h:25,
> > > >                  from
> > > > ../sysdeps/unix/sysv/linux/clock_settime.c:18:
> > > > ../include/time.h:129:28: error: conflicting types for
> > > > '__clock_settime' # define __clock_settime64 __clock_settime
> > > > ^~~~~~~~~~~~~~~ ../sysdeps/unix/sysv/linux/clock_settime.c:25:1:
> > > > note: in expansion of macro '__clock_settime64'
> > > >  __clock_settime64 (clockid_t clock_id, const struct __timespec64
> > > > *tp) ^~~~~~~~~~~~~~~~~
> > > > In file included from <command-line>:
> > > > ../include/time.h:25:20: note: previous declaration of
> > > > '__clock_settime' was here
> > > >  libc_hidden_proto (__clock_settime)
> > > >                     ^~~~~~~~~~~~~~~
> > > > ./../include/libc-symbols.h:598:33: note: in definition of macro
> > > > '__hidden_proto'
> > > >    extern thread __typeof (name) name __hidden_proto_hiddenattr
> > > > (attrs); ^~~~
> > > > ./../include/libc-symbols.h:617:44: note: in expansion of macro
> > > > 'hidden_proto' # define libc_hidden_proto(name, attrs...)
> > > > hidden_proto (name, ##attrs) ^~~~~~~~~~~~
> > > > ../include/time.h:25:1: note: in expansion of macro
> > > > 'libc_hidden_proto' libc_hidden_proto (__clock_settime)
> > > >  ^~~~~~~~~~~~~~~~~
> > > > ../sysdeps/unix/sysv/linux/clock_settime.c:70:42: error:
> > > > conflicting types for 'clock_settime'
> > > >  versioned_symbol (libc, __clock_settime, clock_settime,
> > > > GLIBC_2_17); ^~~~~~~~~~~~~
> > > > ./../include/libc-symbols.h:152:26: note: in definition of macro
> > > > '_weak_alias' extern __typeof (name) aliasname __attribute__
> > > > ((weak, alias (#name))) \ ^~~~~~~~~
> > > > ../include/shlib-compat.h:74:3: note: in expansion of macro
> > > > 'weak_alias' weak_alias (local, symbol)
> > > >    ^~~~~~~~~~
> > > > ../sysdeps/unix/sysv/linux/clock_settime.c:70:1: note: in
> > > > expansion of macro 'versioned_symbol'
> > > >  versioned_symbol (libc, __clock_settime, clock_settime,
> > > > GLIBC_2_17); ^~~~~~~~~~~~~~~~
> > > > In file included from ../include/time.h:2,
> > > >                  from ../sysdeps/generic/hp-timing.h:23,
> > > >                  from ../nptl/descr.h:27,
> > > >                  from ../sysdeps/riscv/nptl/tls.h:41,
> > > >                  from ../include/errno.h:25,
> > > >                  from
> > > > ../sysdeps/unix/sysv/linux/clock_settime.c:18:
> > > > ../time/time.h:222:12: note: previous declaration of
> > > > 'clock_settime' was here extern int clock_settime (clockid_t
> > > > __clock_id, const struct timespec *__tp)
> > > >
> > > > Which I can fix with this diff:
> > > >
> > > > diff --git a/include/time.h b/include/time.h
> > > > index 7ed3aa61d1d..28e2722de21 100644
> > > > --- a/include/time.h
> > > > +++ b/include/time.h
> > > > @@ -125,13 +125,9 @@ extern __time64_t __timegm64 (struct tm
> > > > *__tp) __THROW; libc_hidden_proto (__timegm64)
> > > >  #endif
> > > >
> > > > -#if __TIMESIZE == 64
> > >
> > > What is the value of __TIMESIZE on your port? Is it 32 or 64 ?
> >
> > It's 64.
> >
> > >
> > > Do you also #define __USE_TIME_BITS64   1 ?
> >
> > I don't. I can try with that, but I don't see if defined in the
> > source.
>
> Alistair, have you managed to make your patches working on top of this
> series?

Hey Lukasz,

I have been travelling recently and haven't had a chance to look at
fixing this (besides the diff I provided already).

I should be able to look at it this week.

Alistair

>
>
> >
> > Alistair
> >
> > >
> > > > -# define __clock_settime64 __clock_settime
> > > > -#else
> > > >  extern int __clock_settime64 (clockid_t clock_id,
> > > >                                const struct __timespec64 *tp);
> > > >  libc_hidden_proto (__clock_settime64)
> > > > -#endif
> > > >
> > > >  /* Compute the `struct tm' representation of T,
> > > >     offset OFFSET seconds east of UTC,
> > > > diff --git a/sysdeps/unix/sysv/linux/clock_settime.c
> > > > b/sysdeps/unix/sysv/linux/clock_settime.c
> > > > index f5e084238a5..ab033c56ea9 100644
> > > > --- a/sysdeps/unix/sysv/linux/clock_settime.c
> > > > +++ b/sysdeps/unix/sysv/linux/clock_settime.c
> > > > @@ -54,7 +54,6 @@ __clock_settime64 (clockid_t clock_id, const
> > > > struct __timespec64 *tp)
> > > >  #endif
> > > >  }
> > > >
> > > > -#if __TIMESIZE != 64
> > >
> > > When I look on RISC-V patchset:
> > > https://patchwork.ozlabs.org/patch/1155391/
> > >
> > > for the __clock_nanosleep for example you follow the same
> > > convention. And you don't need to remove #if __TIMESIZE != 64 for
> > > example.
> > >
> > > One difference is that the clock_settime64 code will be Y2038 on
> > > e.g. 32 bit ARM only when I apply on top of it following patch:
> > > https://github.com/lmajewski/y2038_glibc/commit/c6740c05ea9b224a552847d10402f98da8376994
> > >
> > > (it enables redirect for clock_settime and support for
> > > -D_TIME_BITS=64 compilation flag).
> > >
> > >
> > >
> > > >  int
> > > >  __clock_settime (clockid_t clock_id, const struct timespec *tp)
> > > >  {
> > > > @@ -63,7 +62,6 @@ __clock_settime (clockid_t clock_id, const
> > > > struct timespec *tp)
> > > >    valid_timespec_to_timespec64 (tp, &ts64);
> > > >    return __clock_settime64 (clock_id, &ts64);
> > > >  }
> > > > -#endif
> > > >
> > > >  libc_hidden_def (__clock_settime)
> > > >
> > > > Alistair
> > > >
> > > > >
> > > > >
> > > > > Lukasz Majewski (3):
> > > > >   y2038: Introduce internal for glibc struct __timespec64
> > > > >   y2038: Provide conversion helpers for struct __timespec64
> > > > >   y2038: linux: Provide __clock_settime64 implementation
> > > > >
> > > > >  include/time.h                          | 116
> > > > > ++++++++++++++++++++++++
> > > > > sysdeps/unix/sysv/linux/clock_settime.c | 38 +++++++- 2 files
> > > > > changed, 150 insertions(+), 4 deletions(-)
> > > > >
> > > > > --
> > > > > 2.20.1
> > > > >
> > >
> > >
> > >
> > > 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
>
>
>
> 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
Alistair Francis Sept. 16, 2019, 10:45 p.m. UTC | #7 
On Fri, Sep 6, 2019 at 2:28 PM Joseph Myers <joseph@codesourcery.com> wrote:
>
> On Fri, 6 Sep 2019, Alistair Francis wrote:
>
> > Which I can fix with this diff:
>
> This diff is not the right way to fix this build failure.
>
> One of the design principles in the Y2038 support is that is __TIMESIZE ==
> 64, the time functions *aren't* trivial wrappers of time64 functions;
> rather, the time64 function definitions are remapped (via macros) so they
> define the function name with no "64".  For each case where there is a
> pair of functions (for different time_t types) in the __TIMESIZE == 32
> case, there should just be the one function when __TIMESIZE == 64.
>
> This ensures that the Y2038 changes don't add any overhead at all in the
> glibc binaries on existing platforms with __TIMESIZE == 64.
>
> You should look at exactly what the types in question are, that are being
> reported as conflicting in your build (probably by looking at preprocessed
> source).  __timespec64 and timespec are supposed to be the same type (via
> #define) when __TIMESIZE == 64, to avoid such incompatibilities.

Looking at the place where the__timespec64 is defined they aren't the
same for __TIMESIZE == 64 on a 32-bit system.

The code is below:

#if __WORDSIZE == 64 \
  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
# define __timespec64 timespec
#else
/* The glibc Y2038-proof struct __timespec64 structure for a time value.
   To keep things Posix-ish, we keep the nanoseconds field a 32-bit
   signed long, but since the Linux field is a 64-bit signed int, we
   pad our tv_nsec with a 32-bit int.

   As a general rule the Linux kernel is ignoring upper 32 bits of
   tv_nsec field.  */
struct __timespec64
{
  __time64_t tv_sec;         /* Seconds */
# if BYTE_ORDER == BIG_ENDIAN
  __int32_t tv_pad;          /* Padding */
  __int32_t tv_nsec;         /* Nanoseconds */
# else
  __int32_t tv_nsec;         /* Nanoseconds */
  __int32_t tv_pad;          /* Padding */
# endif
};
#endif

So looking at that with __TIMESIZE == 64 but __WORDSIZE == 32 (as on
RV32) we will specify a __timespec64 struct that is different to the
timespec struct.

Would the correct fix be to add __TIMESIZE == 64 to the #if mentioned above?

This diff fixes the build for me:

diff --git a/include/time.h b/include/time.h
index 5f7529f10e9..ff5f18ec56c 100644
--- a/include/time.h
+++ b/include/time.h
@@ -51,7 +51,8 @@ extern void __tz_compute (__time64_t timer, struct
tm *tm, int use_localtime)
   __THROW attribute_hidden;

 #if __WORDSIZE == 64 \
-  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
+  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64) || \
+  __TIMESIZE == 64
 # define __timespec64 timespec
 #else
 /* The glibc Y2038-proof struct __timespec64 structure for a time value.

Alistair

>
> --
> Joseph S. Myers
> joseph@codesourcery.com
Joseph Myers Sept. 17, 2019, 12:44 a.m. UTC | #8 
On Mon, 16 Sep 2019, Alistair Francis wrote:

> Would the correct fix be to add __TIMESIZE == 64 to the #if mentioned above?

Those other conditions imply __TIMESIZE == 64.  So replacing the existing 
conditions by __TIMESIZE == 64 might be correct, but not adding it while 
leaving the existing (then redundant) conditions there as well.

I think you should replace the conditional by __TIMESIZE == 64 (if that 
works), which I think should be safe for the reasons explained below.  
There is a possibility that we end up needing two different internal names 
instead of the present one, but I don't think that should block changing 
the conditional at this point to make further progress unless that causes 
problems with the existing patches on existing configurations.

The design for defining 64-bit-time functions with *64 names and then 
remapping those names to the public ones via #define if __TIMESIZE == 64 
implies that the type used in declaring / defining *64 functions needs to 
be mapped to struct timespec if __TIMESIZE == 64.

But if you need to set padding explicitly to zero when passing 64-bit 
timespecs into the kernel (the case of kernels 5.1.0 through 5.1.4, on 
32-bit architectures for which those kernel versions had a corresponding 
64-bit architecture with compat syscall support), you then need a type 
with named padding (whereas the public struct timespec needs to have an 
unnamed bit-field for the padding to stay compatible with existing sources 
with { tv_sec, tv_nsec } initializers).

So this links into what things should look like at the API level inside 
glibc for dealing with zeroing padding, if we do that at all.  However, 
unless we fix x32 to use "long int" for tv_nsec we don't need to deal with 
zeroing padding for any existing system with __TIMESIZE == 64 (and I don't 
think it's likely to be relevant for future 32-bit ports that use 
__TIMESIZE == 64 either, because the kernel issue is only for compat 
syscalls for 32-bit binaries under 64-bit kernels, which I don't think are 
relevant to any 32-bit architectures supported in 5.1 kernels but not yet 
in glibc).
Lukasz Majewski Sept. 17, 2019, 10:11 a.m. UTC | #9 
Hi Alistair, Joseph,

> On Fri, Sep 6, 2019 at 2:28 PM Joseph Myers <joseph@codesourcery.com>
> wrote:
> >
> > On Fri, 6 Sep 2019, Alistair Francis wrote:
> >  
> > > Which I can fix with this diff:  
> >
> > This diff is not the right way to fix this build failure.
> >
> > One of the design principles in the Y2038 support is that is
> > __TIMESIZE == 64, the time functions *aren't* trivial wrappers of
> > time64 functions; rather, the time64 function definitions are
> > remapped (via macros) so they define the function name with no
> > "64".  For each case where there is a pair of functions (for
> > different time_t types) in the __TIMESIZE == 32 case, there should
> > just be the one function when __TIMESIZE == 64.
> >
> > This ensures that the Y2038 changes don't add any overhead at all
> > in the glibc binaries on existing platforms with __TIMESIZE == 64.
> >
> > You should look at exactly what the types in question are, that are
> > being reported as conflicting in your build (probably by looking at
> > preprocessed source).  __timespec64 and timespec are supposed to be
> > the same type (via #define) when __TIMESIZE == 64, to avoid such
> > incompatibilities.  
> 
> Looking at the place where the__timespec64 is defined they aren't the
> same for __TIMESIZE == 64 on a 32-bit system.
> 
> The code is below:
> 
> #if __WORDSIZE == 64 \
>   || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
   ^^^^^^^^^^^^^ - [1]

> # define __timespec64 timespec
> #else
> /* The glibc Y2038-proof struct __timespec64 structure for a time
> value. To keep things Posix-ish, we keep the nanoseconds field a
> 32-bit signed long, but since the Linux field is a 64-bit signed int,
> we pad our tv_nsec with a 32-bit int.
> 
>    As a general rule the Linux kernel is ignoring upper 32 bits of
>    tv_nsec field.  */
> struct __timespec64
         ^^^^^^^^^^ - [3]

> {
>   __time64_t tv_sec;         /* Seconds */
> # if BYTE_ORDER == BIG_ENDIAN
>   __int32_t tv_pad;          /* Padding */
>   __int32_t tv_nsec;         /* Nanoseconds */
> # else
>   __int32_t tv_nsec;         /* Nanoseconds */
>   __int32_t tv_pad;          /* Padding */
> # endif
> };
> #endif
> 
> So looking at that with __TIMESIZE == 64 but __WORDSIZE == 32 (as on
> RV32) we will specify a __timespec64 struct that is different to the
> timespec struct.
> 
> Would the correct fix be to add __TIMESIZE == 64 to the #if mentioned
> above?

I've replaced the condition [1] with plain #if __TIMESIZE == 64 [2], but
then there was a concern that: 

Replace __TIMESIZE with __WORDSIZE (as architectures with
__TIMESIZE==64 will need to use this struct with 32 bit tv_nsec field).


Alistair - I guess that the size of RISC-V's struct timespec tv_nsec
is 4? (as it shall be long tv_nsec; [4]) ?

Then if you replace the condition [1] with #if __TIMESIZE == 64 you
would have:

struct timespec
{
  __time_t tv_sec;		/* Seconds.  */
  __syscall_slong_t tv_nsec;    /* Nanoseconds.  */
}

And then,

You would have tv_sec (8 bytes) and tv_nsec (4 bytes) [*]. Kernel's
clock_settime64 expects to receive two 8 bytes values.

In the best case you would leak glibc data to the kenel. In the worst
case kernel will modify this data, which may be some other struct's
field.

The condition [1] prevents from this for machines with __WORDSIZE == 32
(excluding x32).

Joseph is the above concern valid ?

> 
> This diff fixes the build for me:
> 
> diff --git a/include/time.h b/include/time.h
> index 5f7529f10e9..ff5f18ec56c 100644
> --- a/include/time.h
> +++ b/include/time.h
> @@ -51,7 +51,8 @@ extern void __tz_compute (__time64_t timer, struct
> tm *tm, int use_localtime)
>    __THROW attribute_hidden;
> 
>  #if __WORDSIZE == 64 \
> -  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
> +  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64) || \
> +  __TIMESIZE == 64
>  # define __timespec64 timespec
>  #else
>  /* The glibc Y2038-proof struct __timespec64 structure for a time
> value.
> 
> Alistair
> 
> >
> > --
> > Joseph S. Myers
> > joseph@codesourcery.com  

Note:

[2] - https://patchwork.ozlabs.org/patch/1092580/
[4] - https://linux.die.net/man/3/clock_gettime
[*] - what is the output of sizeof(__syscall_slong_t) on RISCV 32 bit?

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
Joseph Myers Sept. 17, 2019, 1:42 p.m. UTC | #10 
On Tue, 17 Sep 2019, Lukasz Majewski wrote:

> Then if you replace the condition [1] with #if __TIMESIZE == 64 you
> would have:
> 
> struct timespec
> {
>   __time_t tv_sec;		/* Seconds.  */
>   __syscall_slong_t tv_nsec;    /* Nanoseconds.  */
> }

The *public* struct timespec (defined in 
time/bits/types/struct_timespec.h) should be changed for ports that define 
__TIMESIZE == 64 while __SYSCALL_WORDSIZE == 32.

That is, if __TIMESIZE == 64, and if __SYSCALL_WORDSIZE (if defined) is 32 
or __WORDSIZE (if __SYSCALL_WORDSIZE is not defined), then struct timespec 
needs endian-dependent padding (defined as an *unnamed* 32-bit bit-field, 
so that it gets ignored for initializers).  (This is the same padding as 
would be needed in the case where __TIMESIZE == 32 but _TIME_BITS=64 is 
defined, but _TIME_BITS=64 support for headers comes later.)

RV32 has got away without that change to struct timespec because it's 
little-endian, and as long as __time_t is 8-byte-aligned implicit padding 
works as well as explicit in the little-endian case.  If BE, or if 8-byte 
__time_t is only 4-byte-aligned in structs (and so the struct ends up as 
12-byte without explicit padding), there would be problems.  I think it's 
cleanest to make the padding explicit even in the cases where in fact 
implicit padding would give the same layout.

RV32 does not need any support for clearing the padding before passing 
struct timespec to the kernel, because that's only relevant for compat 
syscalls in Linux 5.1.0 to 5.1.4 and the RISC-V kernel doesn't yet have 
compat syscall support for running RV32 binaries under RV64 kernels.
Lukasz Majewski Sept. 17, 2019, 3:53 p.m. UTC | #11 
Hi Joseph, Alistair,

> On Tue, 17 Sep 2019, Lukasz Majewski wrote:
> 
> > Then if you replace the condition [1] with #if __TIMESIZE == 64 you
> > would have:
> > 
> > struct timespec
> > {
> >   __time_t tv_sec;		/* Seconds.  */
> >   __syscall_slong_t tv_nsec;    /* Nanoseconds.  */
> > }  
> 
> The *public* struct timespec (defined in 
> time/bits/types/struct_timespec.h) should be changed for ports that
> define __TIMESIZE == 64 while __SYSCALL_WORDSIZE == 32.
> 
> That is, if __TIMESIZE == 64, and if __SYSCALL_WORDSIZE (if defined)
> is 32 or __WORDSIZE (if __SYSCALL_WORDSIZE is not defined), then
> struct timespec needs endian-dependent padding (defined as an
> *unnamed* 32-bit bit-field, so that it gets ignored for
> initializers). 

Ok, I will prepare proper patch with adjusting the *public* struct
timespec.

> (This is the same padding as would be needed in the
> case where __TIMESIZE == 32 but _TIME_BITS=64 is defined, but
> _TIME_BITS=64 support for headers comes later.)

As for example here [1].

Just for (my) confirmation:

- New 32 bits glibc ports (like RISC-V 32) will get __TIMESIZE ==
  64 (__WORDSIZE == 32) and no need to define the -D_TIME_BITS=64
  during the compilation. They will just get 64 bit time API support
  from the outset.

- Already supported 32 bits architectures (like armv7-a with __WORDSIZE
  == 32) will keep __TIMESIZE == 32 and require -D_TIME_BITS=64 for
  compilation. 
  After glibc sets the minimal supported kernel version to 5.1 and all
  conversions for syscalls to support 64 bit time API are done the
  __TIMESIZE will be set to 64 and -D_TIME_BITS=64 will not be required
  anymore for compilation.

  Until the above switch happens we will redirect time calls - like in
  [2].

> 
> RV32 has got away without that change to struct timespec because it's 
> little-endian, and as long as __time_t is 8-byte-aligned implicit
> padding works as well as explicit in the little-endian case. 

Ok.

> If BE,
> or if 8-byte __time_t is only 4-byte-aligned in structs (and so the
> struct ends up as 12-byte without explicit padding), there would be
> problems. 

Yes.

> I think it's cleanest to make the padding explicit even in
> the cases where in fact implicit padding would give the same layout.

Ok. So then we shall keep the condition:

#if __WORDSIZE == 64 \
  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
# define __timespec64 timespec
#else

// struct __timespec64 with endian dependent, explicit padding and
// __time64_t tv_sec;

#fi

And RV32 shall use the explicitly defined struct __timespec64 (from
#else) as presented in [3] ?

> 
> RV32 does not need any support for clearing the padding before
> passing struct timespec to the kernel, because that's only relevant
> for compat syscalls in Linux 5.1.0 to 5.1.4 and the RISC-V kernel
> doesn't yet have compat syscall support for running RV32 binaries
> under RV64 kernels.

Ok. Thanks for clarification.

> 


Note:

[1] -
https://github.com/lmajewski/y2038_glibc/commit/c6740c05ea9b224a552847d10402f98da8376994#diff-4ddbc47d3262d4f00f3825e4f3627dbbR10

[2] -
https://github.com/lmajewski/y2038_glibc/commit/c6740c05ea9b224a552847d10402f98da8376994#diff-07934c1fe09f0e6357413e138856c786R225

[3] -
https://github.com/lmajewski/y2038_glibc/commit/926634e657fa5a927152bf7eb06a62e8468f75ae#diff-5b9f1c6457e0e10079f657f283c19861R53

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
Joseph Myers Sept. 17, 2019, 4:51 p.m. UTC | #12 
On Tue, 17 Sep 2019, Lukasz Majewski wrote:

> - New 32 bits glibc ports (like RISC-V 32) will get __TIMESIZE ==
>   64 (__WORDSIZE == 32) and no need to define the -D_TIME_BITS=64
>   during the compilation. They will just get 64 bit time API support
>   from the outset.

Yes, at least if such ports wish to use 64-bit time; I don't think we've 
really discussed if we want to *require* 64-bit time for future ports 
(e.g. the next revised resubmissions of the ARC and NDS32 ports).  
Certainly the work required right now for ARC or NDS32 to use 64-bit time 
would be significantly more than the work for RV32 (because they also 
support older kernel versions without the 64-bit-time syscalls, so all the 
Y2038 work for fallback at runtime to older syscalls becomes relevant), 
unless they decide on 5.1 or later as minimum kernel version.

> - Already supported 32 bits architectures (like armv7-a with __WORDSIZE
>   == 32) will keep __TIMESIZE == 32 and require -D_TIME_BITS=64 for
>   compilation. 

Yes.

>   After glibc sets the minimal supported kernel version to 5.1 and all
>   conversions for syscalls to support 64 bit time API are done the
>   __TIMESIZE will be set to 64 and -D_TIME_BITS=64 will not be required
>   anymore for compilation.

No.  __TIMESIZE means the size of time_t in the unsuffixed ABIs in glibc, 
not the _TIME_BITS-dependent size of time_t in the current compilation.  
We hope in future to make _TIME_BITS=64 the default and only API supported 
for new compilations (which is independent of what the minimum kernel 
version is), but __TIMESIZE would still be 32, because the unsuffixed ABIs 
would remain compatible with existing binaries using 32-bit time.

> Ok. So then we shall keep the condition:
> 
> #if __WORDSIZE == 64 \
>   || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
> # define __timespec64 timespec
> #else

No.  __timespec64 should be defined to timespec whenever __TIMESIZE == 64.  
The timespec to which it is defined, in the public header, would gain 
padding.

The condition

#if __WORDSIZE == 64 \
  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)

is correct as a condition for struct timespec (in the public header) *not* 
to have padding.
Alistair Francis Sept. 18, 2019, 5:03 p.m. UTC | #13 
On Tue, Sep 17, 2019 at 9:51 AM Joseph Myers <joseph@codesourcery.com> wrote:
>
> On Tue, 17 Sep 2019, Lukasz Majewski wrote:
>
> > - New 32 bits glibc ports (like RISC-V 32) will get __TIMESIZE ==
> >   64 (__WORDSIZE == 32) and no need to define the -D_TIME_BITS=64
> >   during the compilation. They will just get 64 bit time API support
> >   from the outset.
>
> Yes, at least if such ports wish to use 64-bit time; I don't think we've
> really discussed if we want to *require* 64-bit time for future ports
> (e.g. the next revised resubmissions of the ARC and NDS32 ports).
> Certainly the work required right now for ARC or NDS32 to use 64-bit time
> would be significantly more than the work for RV32 (because they also
> support older kernel versions without the 64-bit-time syscalls, so all the
> Y2038 work for fallback at runtime to older syscalls becomes relevant),
> unless they decide on 5.1 or later as minimum kernel version.
>
> > - Already supported 32 bits architectures (like armv7-a with __WORDSIZE
> >   == 32) will keep __TIMESIZE == 32 and require -D_TIME_BITS=64 for
> >   compilation.
>
> Yes.
>
> >   After glibc sets the minimal supported kernel version to 5.1 and all
> >   conversions for syscalls to support 64 bit time API are done the
> >   __TIMESIZE will be set to 64 and -D_TIME_BITS=64 will not be required
> >   anymore for compilation.
>
> No.  __TIMESIZE means the size of time_t in the unsuffixed ABIs in glibc,
> not the _TIME_BITS-dependent size of time_t in the current compilation.
> We hope in future to make _TIME_BITS=64 the default and only API supported
> for new compilations (which is independent of what the minimum kernel
> version is), but __TIMESIZE would still be 32, because the unsuffixed ABIs
> would remain compatible with existing binaries using 32-bit time.
>
> > Ok. So then we shall keep the condition:
> >
> > #if __WORDSIZE == 64 \
> >   || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
> > # define __timespec64 timespec
> > #else
>
> No.  __timespec64 should be defined to timespec whenever __TIMESIZE == 64.
> The timespec to which it is defined, in the public header, would gain
> padding.
>
> The condition
>
> #if __WORDSIZE == 64 \
>   || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
>
> is correct as a condition for struct timespec (in the public header) *not*
> to have padding.

Are you going to incorporate this into your series Lukasz?

I currently have this diff which fixes the build failures for me:

diff --git a/include/time.h b/include/time.h
index 7ed3aa61d1d..91f6280eb4d 100644
--- a/include/time.h
+++ b/include/time.h
@@ -50,8 +50,7 @@ extern void __tzset_parse_tz (const char *tz)
attribute_hidden;
 extern void __tz_compute (__time64_t timer, struct tm *tm, int use_localtime)
   __THROW attribute_hidden;

-#if __WORDSIZE == 64 \
-  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
+#if __TIMESIZE == 64
 # define __timespec64 timespec
 #else
 /* The glibc Y2038-proof struct __timespec64 structure for a time value.
diff --git a/time/bits/types/struct_timespec.h
b/time/bits/types/struct_timespec.h
index 5b77c52b4f0..48405c4f08a 100644
--- a/time/bits/types/struct_timespec.h
+++ b/time/bits/types/struct_timespec.h
@@ -3,13 +3,25 @@
 #define _STRUCT_TIMESPEC 1

 #include <bits/types.h>
+#include <endian.h>

 /* POSIX.1b structure for a time value.  This is like a `struct timeval' but
    has nanoseconds instead of microseconds.  */
 struct timespec
 {
   __time_t tv_sec;             /* Seconds.  */
+#if __WORDSIZE == 64 \
+  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
   __syscall_slong_t tv_nsec;   /* Nanoseconds.  */
+#else
+# if BYTE_ORDER == BIG_ENDIAN
+  __int32_t tv_pad;           /* Padding */
+  __syscall_slong_t tv_nsec;  /* Nanoseconds */
+# else
+  __int32_t tv_nsec;          /* Nanoseconds */
+  __syscall_slong_t tv_pad;   /* Padding */
+# endif
+#endif
 };

 #endif

As well as that the timeval struct has the same issue. I'll have to
look into that and see what the solution is there.

Alistair

>
> --
> Joseph S. Myers
> joseph@codesourcery.com
Joseph Myers Sept. 18, 2019, 5:25 p.m. UTC | #14 
On Wed, 18 Sep 2019, Alistair Francis wrote:

> +#include <endian.h>
> 
>  /* POSIX.1b structure for a time value.  This is like a `struct timeval' but
>     has nanoseconds instead of microseconds.  */
>  struct timespec
>  {
>    __time_t tv_sec;             /* Seconds.  */
> +#if __WORDSIZE == 64 \
> +  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
>    __syscall_slong_t tv_nsec;   /* Nanoseconds.  */
> +#else
> +# if BYTE_ORDER == BIG_ENDIAN
> +  __int32_t tv_pad;           /* Padding */
> +  __syscall_slong_t tv_nsec;  /* Nanoseconds */
> +# else
> +  __int32_t tv_nsec;          /* Nanoseconds */
> +  __syscall_slong_t tv_pad;   /* Padding */
> +# endif
> +#endif

The padding must be an *unnamed bit-field* so that { tv_sec, tv_nsec } 
initializers (common in practice even if not officially supported by the 
standards) continue to work.  Also, I think you should just use "long int" 
for tv_nsec in the case where there is padding, as the standard-defined 
type (and then the padding can be "int: 32", so avoiding any dependence on 
whether compilers support non-int bit-fields).  Certainly the choice of 
types for tv_nsec and padding should not depend on the endianness (the 
patch above is using __int32_t for the first field and __syscall_slong_t 
for the second, regardless of which is tv_nsec and which is padding).

There are namespace issues when changing installed headers.  You can't use 
macros such as BYTE_ORDER or BIG_ENDIAN because they aren't in the 
standard-reserved namespaces.

Unfortunately the definitions of __LITTLE_ENDIAN and __BIG_ENDIAN are in 
<endian.h> (__BYTE_ORDER is in the architecture-specific <bits/endian.h>), 
and while the non-reserved names therein are all conditional on 
__USE_MISC, I don't think we really want to start exporting them from 
every header that uses struct timespec.  My inclination would be to have a 
separate bits/ header that only defines the __LITTLE_ENDIAN / __BIG_ENDIAN 
/ __PDP_ENDIAN macros (or that defines those and includes the 
architecture-specific header for __BYTE_ORDER), so that other headers can 
test endianness without bringing in all the other __USE_MISC 
endian-related macros from <endian.h>, but Zack might advise on how such 
changes would fit into his header cleanups.
Lukasz Majewski Sept. 18, 2019, 9:28 p.m. UTC | #15 
Hi Alistair,

> On Tue, Sep 17, 2019 at 9:51 AM Joseph Myers
> <joseph@codesourcery.com> wrote:
> >
> > On Tue, 17 Sep 2019, Lukasz Majewski wrote:
> >  
> > > - New 32 bits glibc ports (like RISC-V 32) will get __TIMESIZE ==
> > >   64 (__WORDSIZE == 32) and no need to define the -D_TIME_BITS=64
> > >   during the compilation. They will just get 64 bit time API
> > > support from the outset.  
> >
> > Yes, at least if such ports wish to use 64-bit time; I don't think
> > we've really discussed if we want to *require* 64-bit time for
> > future ports (e.g. the next revised resubmissions of the ARC and
> > NDS32 ports). Certainly the work required right now for ARC or
> > NDS32 to use 64-bit time would be significantly more than the work
> > for RV32 (because they also support older kernel versions without
> > the 64-bit-time syscalls, so all the Y2038 work for fallback at
> > runtime to older syscalls becomes relevant), unless they decide on
> > 5.1 or later as minimum kernel version. 
> > > - Already supported 32 bits architectures (like armv7-a with
> > > __WORDSIZE == 32) will keep __TIMESIZE == 32 and require
> > > -D_TIME_BITS=64 for compilation.  
> >
> > Yes.
> >  
> > >   After glibc sets the minimal supported kernel version to 5.1
> > > and all conversions for syscalls to support 64 bit time API are
> > > done the __TIMESIZE will be set to 64 and -D_TIME_BITS=64 will
> > > not be required anymore for compilation.  
> >
> > No.  __TIMESIZE means the size of time_t in the unsuffixed ABIs in
> > glibc, not the _TIME_BITS-dependent size of time_t in the current
> > compilation. We hope in future to make _TIME_BITS=64 the default
> > and only API supported for new compilations (which is independent
> > of what the minimum kernel version is), but __TIMESIZE would still
> > be 32, because the unsuffixed ABIs would remain compatible with
> > existing binaries using 32-bit time. 
> > > Ok. So then we shall keep the condition:
> > >
> > > #if __WORDSIZE == 64 \
> > >   || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
> > > # define __timespec64 timespec
> > > #else  
> >
> > No.  __timespec64 should be defined to timespec whenever __TIMESIZE
> > == 64. The timespec to which it is defined, in the public header,
> > would gain padding.
> >
> > The condition
> >
> > #if __WORDSIZE == 64 \
> >   || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
> >
> > is correct as a condition for struct timespec (in the public
> > header) *not* to have padding.  
> 
> Are you going to incorporate this into your series Lukasz?
> 
> I currently have this diff which fixes the build failures for me:
> 
> diff --git a/include/time.h b/include/time.h
> index 7ed3aa61d1d..91f6280eb4d 100644
> --- a/include/time.h
> +++ b/include/time.h
> @@ -50,8 +50,7 @@ extern void __tzset_parse_tz (const char *tz)
> attribute_hidden;
>  extern void __tz_compute (__time64_t timer, struct tm *tm, int
> use_localtime) __THROW attribute_hidden;
> 
> -#if __WORDSIZE == 64 \
> -  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
> +#if __TIMESIZE == 64

I've prepared v8 of __clock_settime64 conversion patches with the above
change:
https://patchwork.ozlabs.org/cover/1164209/

I've tested it with meta-y2038:
https://github.com/lmajewski/meta-y2038

 as well as
../src/scripts/build-many-glibcs.py

It seems to work as expected.	

>  # define __timespec64 timespec
>  #else
>  /* The glibc Y2038-proof struct __timespec64 structure for a time
> value. diff --git a/time/bits/types/struct_timespec.h
> b/time/bits/types/struct_timespec.h
> index 5b77c52b4f0..48405c4f08a 100644
> --- a/time/bits/types/struct_timespec.h
> +++ b/time/bits/types/struct_timespec.h
> @@ -3,13 +3,25 @@
>  #define _STRUCT_TIMESPEC 1
> 
>  #include <bits/types.h>
> +#include <endian.h>
> 
>  /* POSIX.1b structure for a time value.  This is like a `struct
> timeval' but has nanoseconds instead of microseconds.  */
>  struct timespec
>  {
>    __time_t tv_sec;             /* Seconds.  */
> +#if __WORDSIZE == 64 \
> +  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
>    __syscall_slong_t tv_nsec;   /* Nanoseconds.  */
> +#else
> +# if BYTE_ORDER == BIG_ENDIAN
> +  __int32_t tv_pad;           /* Padding */
> +  __syscall_slong_t tv_nsec;  /* Nanoseconds */
> +# else
> +  __int32_t tv_nsec;          /* Nanoseconds */
> +  __syscall_slong_t tv_pad;   /* Padding */
> +# endif
> +#endif
>  };

I did not incorporated the above change to v8 of __clock_settime64 as
there are some issues raised by Joseph.

Last but not least - we can get away with the above change as the
implicit padding works for RV32, and ARM32 (which both are LE).

> 
>  #endif
> 
> As well as that the timeval struct has the same issue. I'll have to
> look into that and see what the solution is there.
> 
> Alistair
> 
> >
> > --
> > Joseph S. Myers
> > joseph@codesourcery.com  




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
Lukasz Majewski Sept. 18, 2019, 9:37 p.m. UTC | #16 
Hi Joseph,

> On Wed, 18 Sep 2019, Alistair Francis wrote:
> 
> > +#include <endian.h>
> > 
> >  /* POSIX.1b structure for a time value.  This is like a `struct
> > timeval' but has nanoseconds instead of microseconds.  */
> >  struct timespec
> >  {
> >    __time_t tv_sec;             /* Seconds.  */
> > +#if __WORDSIZE == 64 \
> > +  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
> >    __syscall_slong_t tv_nsec;   /* Nanoseconds.  */
> > +#else
> > +# if BYTE_ORDER == BIG_ENDIAN
> > +  __int32_t tv_pad;           /* Padding */
> > +  __syscall_slong_t tv_nsec;  /* Nanoseconds */
> > +# else
> > +  __int32_t tv_nsec;          /* Nanoseconds */
> > +  __syscall_slong_t tv_pad;   /* Padding */
> > +# endif
> > +#endif  
> 

Just one more question - am I correct that the above code will increase
the overall sizeof(struct timespec) to 12 for machines with __WORDSIZE
== 32 ?

I guess that now such machine have sizeof(struct timespec) = 8 ?

However, this shouldn't be a problem (unless some user space SW uses
this data in an unusual way...).

> The padding must be an *unnamed bit-field* so that { tv_sec, tv_nsec
> } initializers (common in practice even if not officially supported
> by the standards) continue to work.  Also, I think you should just
> use "long int" for tv_nsec in the case where there is padding, as the
> standard-defined type (and then the padding can be "int: 32", so
> avoiding any dependence on whether compilers support non-int
> bit-fields).  Certainly the choice of types for tv_nsec and padding
> should not depend on the endianness (the patch above is using
> __int32_t for the first field and __syscall_slong_t for the second,
> regardless of which is tv_nsec and which is padding).
> 
> There are namespace issues when changing installed headers.  You
> can't use macros such as BYTE_ORDER or BIG_ENDIAN because they aren't
> in the standard-reserved namespaces.
> 
> Unfortunately the definitions of __LITTLE_ENDIAN and __BIG_ENDIAN are
> in <endian.h> (__BYTE_ORDER is in the architecture-specific
> <bits/endian.h>), and while the non-reserved names therein are all
> conditional on __USE_MISC, I don't think we really want to start
> exporting them from every header that uses struct timespec.  My
> inclination would be to have a separate bits/ header that only
> defines the __LITTLE_ENDIAN / __BIG_ENDIAN / __PDP_ENDIAN macros (or
> that defines those and includes the architecture-specific header for
> __BYTE_ORDER), so that other headers can test endianness without
> bringing in all the other __USE_MISC endian-related macros from
> <endian.h>, but Zack might advise on how such changes would fit into
> his header cleanups.
> 




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
Joseph Myers Sept. 18, 2019, 9:45 p.m. UTC | #17 
On Wed, 18 Sep 2019, Lukasz Majewski wrote:

> Just one more question - am I correct that the above code will increase
> the overall sizeof(struct timespec) to 12 for machines with __WORDSIZE
> == 32 ?

It looks like that's a bug in the patch, indeed.  (The padding should 
*only* be added when __TIMESIZE == 64 in addition to the other conditions 
given, not when __TIMESIZE == 32.)
Alistair Francis Sept. 18, 2019, 10:26 p.m. UTC | #18 
On Wed, Sep 18, 2019 at 2:28 PM Lukasz Majewski <lukma@denx.de> wrote:
>
> Hi Alistair,
>
> > On Tue, Sep 17, 2019 at 9:51 AM Joseph Myers
> > <joseph@codesourcery.com> wrote:
> > >
> > > On Tue, 17 Sep 2019, Lukasz Majewski wrote:
> > >
> > > > - New 32 bits glibc ports (like RISC-V 32) will get __TIMESIZE ==
> > > >   64 (__WORDSIZE == 32) and no need to define the -D_TIME_BITS=64
> > > >   during the compilation. They will just get 64 bit time API
> > > > support from the outset.
> > >
> > > Yes, at least if such ports wish to use 64-bit time; I don't think
> > > we've really discussed if we want to *require* 64-bit time for
> > > future ports (e.g. the next revised resubmissions of the ARC and
> > > NDS32 ports). Certainly the work required right now for ARC or
> > > NDS32 to use 64-bit time would be significantly more than the work
> > > for RV32 (because they also support older kernel versions without
> > > the 64-bit-time syscalls, so all the Y2038 work for fallback at
> > > runtime to older syscalls becomes relevant), unless they decide on
> > > 5.1 or later as minimum kernel version.
> > > > - Already supported 32 bits architectures (like armv7-a with
> > > > __WORDSIZE == 32) will keep __TIMESIZE == 32 and require
> > > > -D_TIME_BITS=64 for compilation.
> > >
> > > Yes.
> > >
> > > >   After glibc sets the minimal supported kernel version to 5.1
> > > > and all conversions for syscalls to support 64 bit time API are
> > > > done the __TIMESIZE will be set to 64 and -D_TIME_BITS=64 will
> > > > not be required anymore for compilation.
> > >
> > > No.  __TIMESIZE means the size of time_t in the unsuffixed ABIs in
> > > glibc, not the _TIME_BITS-dependent size of time_t in the current
> > > compilation. We hope in future to make _TIME_BITS=64 the default
> > > and only API supported for new compilations (which is independent
> > > of what the minimum kernel version is), but __TIMESIZE would still
> > > be 32, because the unsuffixed ABIs would remain compatible with
> > > existing binaries using 32-bit time.
> > > > Ok. So then we shall keep the condition:
> > > >
> > > > #if __WORDSIZE == 64 \
> > > >   || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
> > > > # define __timespec64 timespec
> > > > #else
> > >
> > > No.  __timespec64 should be defined to timespec whenever __TIMESIZE
> > > == 64. The timespec to which it is defined, in the public header,
> > > would gain padding.
> > >
> > > The condition
> > >
> > > #if __WORDSIZE == 64 \
> > >   || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
> > >
> > > is correct as a condition for struct timespec (in the public
> > > header) *not* to have padding.
> >
> > Are you going to incorporate this into your series Lukasz?
> >
> > I currently have this diff which fixes the build failures for me:
> >
> > diff --git a/include/time.h b/include/time.h
> > index 7ed3aa61d1d..91f6280eb4d 100644
> > --- a/include/time.h
> > +++ b/include/time.h
> > @@ -50,8 +50,7 @@ extern void __tzset_parse_tz (const char *tz)
> > attribute_hidden;
> >  extern void __tz_compute (__time64_t timer, struct tm *tm, int
> > use_localtime) __THROW attribute_hidden;
> >
> > -#if __WORDSIZE == 64 \
> > -  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
> > +#if __TIMESIZE == 64
>
> I've prepared v8 of __clock_settime64 conversion patches with the above
> change:
> https://patchwork.ozlabs.org/cover/1164209/
>
> I've tested it with meta-y2038:
> https://github.com/lmajewski/meta-y2038
>
>  as well as
> ../src/scripts/build-many-glibcs.py
>
> It seems to work as expected.
>
> >  # define __timespec64 timespec
> >  #else
> >  /* The glibc Y2038-proof struct __timespec64 structure for a time
> > value. diff --git a/time/bits/types/struct_timespec.h
> > b/time/bits/types/struct_timespec.h
> > index 5b77c52b4f0..48405c4f08a 100644
> > --- a/time/bits/types/struct_timespec.h
> > +++ b/time/bits/types/struct_timespec.h
> > @@ -3,13 +3,25 @@
> >  #define _STRUCT_TIMESPEC 1
> >
> >  #include <bits/types.h>
> > +#include <endian.h>
> >
> >  /* POSIX.1b structure for a time value.  This is like a `struct
> > timeval' but has nanoseconds instead of microseconds.  */
> >  struct timespec
> >  {
> >    __time_t tv_sec;             /* Seconds.  */
> > +#if __WORDSIZE == 64 \
> > +  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
> >    __syscall_slong_t tv_nsec;   /* Nanoseconds.  */
> > +#else
> > +# if BYTE_ORDER == BIG_ENDIAN
> > +  __int32_t tv_pad;           /* Padding */
> > +  __syscall_slong_t tv_nsec;  /* Nanoseconds */
> > +# else
> > +  __int32_t tv_nsec;          /* Nanoseconds */
> > +  __syscall_slong_t tv_pad;   /* Padding */
> > +# endif
> > +#endif
> >  };
>
> I did not incorporated the above change to v8 of __clock_settime64 as
> there are some issues raised by Joseph.

That's fine, I can fix up his comments and include that in my series.

>
> Last but not least - we can get away with the above change as the
> implicit padding works for RV32, and ARM32 (which both are LE).

RV32 is actually both BE and LE. The spec allows it to be either. At
the moment there are only LE implementations, but we should try to
handle both.

Alistair

>
> >
> >  #endif
> >
> > As well as that the timeval struct has the same issue. I'll have to
> > look into that and see what the solution is there.
> >
> > Alistair
> >
> > >
> > > --
> > > Joseph S. Myers
> > > joseph@codesourcery.com
>
>
>
>
> 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
Lukasz Majewski Sept. 19, 2019, 7:50 a.m. UTC | #19 
Hi Alistair,

> On Wed, Sep 18, 2019 at 2:28 PM Lukasz Majewski <lukma@denx.de> wrote:
> >
> > Hi Alistair,
> >  
> > > On Tue, Sep 17, 2019 at 9:51 AM Joseph Myers
> > > <joseph@codesourcery.com> wrote:  
> > > >
> > > > On Tue, 17 Sep 2019, Lukasz Majewski wrote:
> > > >  
> > > > > - New 32 bits glibc ports (like RISC-V 32) will get
> > > > > __TIMESIZE == 64 (__WORDSIZE == 32) and no need to define the
> > > > > -D_TIME_BITS=64 during the compilation. They will just get 64
> > > > > bit time API support from the outset.  
> > > >
> > > > Yes, at least if such ports wish to use 64-bit time; I don't
> > > > think we've really discussed if we want to *require* 64-bit
> > > > time for future ports (e.g. the next revised resubmissions of
> > > > the ARC and NDS32 ports). Certainly the work required right now
> > > > for ARC or NDS32 to use 64-bit time would be significantly more
> > > > than the work for RV32 (because they also support older kernel
> > > > versions without the 64-bit-time syscalls, so all the Y2038
> > > > work for fallback at runtime to older syscalls becomes
> > > > relevant), unless they decide on 5.1 or later as minimum kernel
> > > > version.  
> > > > > - Already supported 32 bits architectures (like armv7-a with
> > > > > __WORDSIZE == 32) will keep __TIMESIZE == 32 and require
> > > > > -D_TIME_BITS=64 for compilation.  
> > > >
> > > > Yes.
> > > >  
> > > > >   After glibc sets the minimal supported kernel version to 5.1
> > > > > and all conversions for syscalls to support 64 bit time API
> > > > > are done the __TIMESIZE will be set to 64 and -D_TIME_BITS=64
> > > > > will not be required anymore for compilation.  
> > > >
> > > > No.  __TIMESIZE means the size of time_t in the unsuffixed ABIs
> > > > in glibc, not the _TIME_BITS-dependent size of time_t in the
> > > > current compilation. We hope in future to make _TIME_BITS=64
> > > > the default and only API supported for new compilations (which
> > > > is independent of what the minimum kernel version is), but
> > > > __TIMESIZE would still be 32, because the unsuffixed ABIs would
> > > > remain compatible with existing binaries using 32-bit time.  
> > > > > Ok. So then we shall keep the condition:
> > > > >
> > > > > #if __WORDSIZE == 64 \
> > > > >   || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
> > > > > # define __timespec64 timespec
> > > > > #else  
> > > >
> > > > No.  __timespec64 should be defined to timespec whenever
> > > > __TIMESIZE == 64. The timespec to which it is defined, in the
> > > > public header, would gain padding.
> > > >
> > > > The condition
> > > >
> > > > #if __WORDSIZE == 64 \
> > > >   || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
> > > >
> > > > is correct as a condition for struct timespec (in the public
> > > > header) *not* to have padding.  
> > >
> > > Are you going to incorporate this into your series Lukasz?
> > >
> > > I currently have this diff which fixes the build failures for me:
> > >
> > > diff --git a/include/time.h b/include/time.h
> > > index 7ed3aa61d1d..91f6280eb4d 100644
> > > --- a/include/time.h
> > > +++ b/include/time.h
> > > @@ -50,8 +50,7 @@ extern void __tzset_parse_tz (const char *tz)
> > > attribute_hidden;
> > >  extern void __tz_compute (__time64_t timer, struct tm *tm, int
> > > use_localtime) __THROW attribute_hidden;
> > >
> > > -#if __WORDSIZE == 64 \
> > > -  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
> > > +#if __TIMESIZE == 64  
> >
> > I've prepared v8 of __clock_settime64 conversion patches with the
> > above change:
> > https://patchwork.ozlabs.org/cover/1164209/
> >
> > I've tested it with meta-y2038:
> > https://github.com/lmajewski/meta-y2038
> >
> >  as well as
> > ../src/scripts/build-many-glibcs.py
> >
> > It seems to work as expected.
> >  
> > >  # define __timespec64 timespec
> > >  #else
> > >  /* The glibc Y2038-proof struct __timespec64 structure for a time
> > > value. diff --git a/time/bits/types/struct_timespec.h
> > > b/time/bits/types/struct_timespec.h
> > > index 5b77c52b4f0..48405c4f08a 100644
> > > --- a/time/bits/types/struct_timespec.h
> > > +++ b/time/bits/types/struct_timespec.h
> > > @@ -3,13 +3,25 @@
> > >  #define _STRUCT_TIMESPEC 1
> > >
> > >  #include <bits/types.h>
> > > +#include <endian.h>
> > >
> > >  /* POSIX.1b structure for a time value.  This is like a `struct
> > > timeval' but has nanoseconds instead of microseconds.  */
> > >  struct timespec
> > >  {
> > >    __time_t tv_sec;             /* Seconds.  */
> > > +#if __WORDSIZE == 64 \
> > > +  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
> > >    __syscall_slong_t tv_nsec;   /* Nanoseconds.  */
> > > +#else
> > > +# if BYTE_ORDER == BIG_ENDIAN
> > > +  __int32_t tv_pad;           /* Padding */
> > > +  __syscall_slong_t tv_nsec;  /* Nanoseconds */
> > > +# else
> > > +  __int32_t tv_nsec;          /* Nanoseconds */
> > > +  __syscall_slong_t tv_pad;   /* Padding */
> > > +# endif
> > > +#endif
> > >  };  
> >
> > I did not incorporated the above change to v8 of __clock_settime64
> > as there are some issues raised by Joseph.  
> 
> That's fine, I can fix up his comments and include that in my series.
> 
> >
> > Last but not least - we can get away with the above change as the
> > implicit padding works for RV32, and ARM32 (which both are LE).  
> 
> RV32 is actually both BE and LE. The spec allows it to be either. 

Ok. I was not aware of this - and blindly assumed that it is LE.

> At
> the moment there are only LE implementations, but we should try to
> handle both.

Ok. Then if you don't mind, please add the above change to your series.

> 
> Alistair
> 
> >  
> > >
> > >  #endif
> > >
> > > As well as that the timeval struct has the same issue. I'll have
> > > to look into that and see what the solution is there.
> > >
> > > Alistair
> > >  
> > > >
> > > > --
> > > > Joseph S. Myers
> > > > joseph@codesourcery.com  
> >
> >
> >
> >
> > 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  




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
Alistair Francis Sept. 19, 2019, 9:56 p.m. UTC | #20 
On Wed, Sep 18, 2019 at 10:25 AM Joseph Myers <joseph@codesourcery.com> wrote:
>
> On Wed, 18 Sep 2019, Alistair Francis wrote:
>
> > +#include <endian.h>
> >
> >  /* POSIX.1b structure for a time value.  This is like a `struct timeval' but
> >     has nanoseconds instead of microseconds.  */
> >  struct timespec
> >  {
> >    __time_t tv_sec;             /* Seconds.  */
> > +#if __WORDSIZE == 64 \
> > +  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
> >    __syscall_slong_t tv_nsec;   /* Nanoseconds.  */
> > +#else
> > +# if BYTE_ORDER == BIG_ENDIAN
> > +  __int32_t tv_pad;           /* Padding */
> > +  __syscall_slong_t tv_nsec;  /* Nanoseconds */
> > +# else
> > +  __int32_t tv_nsec;          /* Nanoseconds */
> > +  __syscall_slong_t tv_pad;   /* Padding */
> > +# endif
> > +#endif
>
> The padding must be an *unnamed bit-field* so that { tv_sec, tv_nsec }
> initializers (common in practice even if not officially supported by the
> standards) continue to work.  Also, I think you should just use "long int"
> for tv_nsec in the case where there is padding, as the standard-defined
> type (and then the padding can be "int: 32", so avoiding any dependence on
> whether compilers support non-int bit-fields).  Certainly the choice of
> types for tv_nsec and padding should not depend on the endianness (the
> patch above is using __int32_t for the first field and __syscall_slong_t
> for the second, regardless of which is tv_nsec and which is padding).

Ok, I have fixed this up.

>
> There are namespace issues when changing installed headers.  You can't use
> macros such as BYTE_ORDER or BIG_ENDIAN because they aren't in the
> standard-reserved namespaces.
>
> Unfortunately the definitions of __LITTLE_ENDIAN and __BIG_ENDIAN are in
> <endian.h> (__BYTE_ORDER is in the architecture-specific <bits/endian.h>),
> and while the non-reserved names therein are all conditional on
> __USE_MISC, I don't think we really want to start exporting them from
> every header that uses struct timespec.  My inclination would be to have a
> separate bits/ header that only defines the __LITTLE_ENDIAN / __BIG_ENDIAN
> / __PDP_ENDIAN macros (or that defines those and includes the
> architecture-specific header for __BYTE_ORDER), so that other headers can
> test endianness without bringing in all the other __USE_MISC
> endian-related macros from <endian.h>, but Zack might advise on how such
> changes would fit into his header cleanups.

I think I understand what you mean, but it seems strange. I'm going to
send an RFC patch and we can discuss there.

Alistair

>
> --
> Joseph S. Myers
> joseph@codesourcery.com