This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v8 1/2] Y2038: Add 64-bit time for all architectures


Hi Florian,

On Wed, 26 Sep 2018 10:39:59 +0200, Florian Weimer <fweimer@redhat.com>
wrote :

> * Albert ARIBAUD:
> 
> > diff --git a/posix/bits/types.h b/posix/bits/types.h
> > index 5e22ce41bf..cda0a70dd8 100644
> > --- a/posix/bits/types.h
> > +++ b/posix/bits/types.h  
> 
> > @@ -211,6 +213,12 @@ __STD_TYPE __U32_TYPE __socklen_t;
> >     It is not currently necessary for this to be machine-specific.  */
> >  typedef int __sig_atomic_t;
> >  
> > +#if __TIMESIZE == 64
> > +# define __time64_t __time_t
> > +#else
> > +__STD_TYPE __TIME64_T_TYPE __time64_t;	/* Seconds since the
> > Epoch.  */ +#endif  
> 
> Should the __TIMESIZE == 64 case use typedef as well?

It was requested that the Y038 code cause as little change as possible
at the object level in the TIMESIZE == 64 case, hence the #define which
ensures that after the preprocessor stage, the compiler can only see
'time_t', not 'time64_t'.

> > diff --git a/sysdeps/unix/sysv/linux/x86/bits/time64.h b/sysdeps/unix/sysv/linux/x86/bits/time64.h
> > new file mode 100644
> > index 0000000000..81de09e23f
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/x86/bits/time64.h  
> 
> > +#if defined __x86_64__ && defined __ILP32__
> > +/* For x32, time is 64-bit even though word size is 32-bit.  */
> > +# define __TIME64_T_TYPE		__SQUAD_TYPE
> > +#elif __TIMESIZE == 64
> > +/* If we already have 64-bit time then use it.  */
> > +# define __TIME64_T_TYPE		__TIME_T_TYPE
> > +#else
> > +/* Define a 64-bit type alongsize the 32-bit one.  */
> > +# define __TIME64_T_TYPE		__SQUAD_TYPE
> > +#endif  
> 
> Perhaps I'm missing something, but I think this should be put into a
> generic header and could be written like this:
> 
> #if __TIMESIZE == 64
> # define __TIME64_T_TYPE __TIME_T_TYPE
> #else
> # if __WORDSIZE != 32
> #  error "32-bit word size expected for non-64-bit time_t"
> # endif
> # define __TIME64_T_TYPE __SQUAD_TYPE
> #endif
> 
> I don't think there is a 64-bit port with a 32-bit time_t.

The patch does provide definitions for __TIMESIZE and __TIME64_T_TYPE
in generic headers (bits/timesize and bits/time64.h respectively)
which cover all cases for all 64-bit architectures and all 32-bit
architectures.

Since the only exception is X32, it makes sense that the definitions
which take this exception into account be put in the most X32 specific
header, which is the x86 one.

This organization stems from previous comments by Joseph Myers:

https://patchwork.ozlabs.org/patch/930876/#1935670

> > diff --git a/sysdeps/unix/sysv/linux/x86/bits/timesize.h b/sysdeps/unix/sysv/linux/x86/bits/timesize.h
> > new file mode 100644
> > index 0000000000..8b88ab84b0
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/x86/bits/timesize.h  
> 
> > +#if defined __x86_64__ && defined __ILP32__
> > +/* For x32, time is 64-bit even though word size is 32-bit.  */
> > +# define __TIMESIZE	64
> > +#else
> > +/* For others, time size is word size.  */
> > +# define __TIMESIZE	__WORDSIZE
> > +#endif  
> 
> I think writing this as
> 
> #ifdef __x86_64__
> /* This includes x32, where time_t is 64-bit even though the word size
>    is 32-bit.  */
> # define __TIMESIZE	64
> #else
> # define __TIMESIZE	32
> #endif
> 
> is much clearer.  It's not that there's going to be a different x86-64
> API with yet another time_t size any time soon.

I prefer the #if/#else condition to explicitly involve x32
(through the symbol __ILP32__) rather than implicitly.

> > index 72ef75f074..844a68de8c 100644
> > --- a/time/tzfile.c
> > +++ b/time/tzfile.c  
> 
> The tzfile.c changes look okay to me.

Thanks!

> Thanks,
> Florian

Cordialement,
Albert ARIBAUD
3ADEV


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]