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

[Alistair Francis <alistair23 at gmail dot com>]

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