This is the mail archive of the 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 v2 2/7] y2038: Introduce __ASSUME_64BIT_TIME define

Hi Joseph,

Thanks for your reply.

> On Mon, 29 Apr 2019, Lukasz Majewski wrote:
> > +/* Support for 64 bit version of clock_* Linux syscalls.
> > +
> > +   Support for following time related (and Y2038 safe) syscalls
> > has been added
> > +   in the 5.1 Linux kernel:
> > +
> > +   clock_gettime64 (nr. 403)
> > +   clock_settime64 (nr. 404)
> > +   clock_getres_time64 (nr. 406)
> > +   clock_nanosleep_time64 (nr. 407)
> > +  */
> > +#if __LINUX_KERNEL_VERSION >= 0x050100
> > +# define __ASSUME_64BIT_TIME 1
> > +#endif  
> This comment and macro definition are the key thing that need
> reviewing, probably over several iterations,

Ok. Please find my comments/concerns below regarding this __ASSUME

> before the rest of this
> patch series can be properly reviewed.

I would like to point out one thing - the rest of this patch series
also has an important goal - reviewing them would set the direction
(despite the __ASSUME discussion) for future Y2038 development and
conversion of other parts of glibc.

For example:

 - The decisions about the shape of internal/external struct timespec
   in glibc.

 - The need for explicit clearing padding when calling syscalls (as to
   be better safe than sorry in the future - there was related
   discussion started by Stepan).

 - If the conversion itself (__clock_settime64 vs clock_settime) is

Would greatly facilitate the development process and reduce the number
of iterations.

>  It is critical that the
> comment is completely clear and unambiguous about the exact macro
> semantics on the various relevant classes of architectures.
> See what I wrote in 
> <> and 
> <>.  I
> don't think the comment meets those requirements at present - that
> is, if you try to deduce from it what the macro definition should be
> on all the listed classes of architectures, either the conclusion is
> not clear or it sometimes conflicts with the actual definition.
> In particular, for existing 64-bit architectures, my understanding is
> that the kernel *does not* add new syscall names or numbers for the
> syscalls you list,

With the 5.1-rc6 it seems like 64 bit architectures are not add those
syscalls (like e.g. clock_settime64).

> and so it would be incorrect to define the macro
> in that case, but this patch defines it anyway.

You are right here - the 

#if __TIMESIZE != 64
# if __LINUX_KERNEL_VERSION >= 0x050100
#  define __ASSUME_64BIT_TIME 1
# endif

would do the trick.

> Note 1: the comment should not reference the above URLs; it should be 
> self-contained.  As stated in the second message above, it needs to
> be clear to people who haven't read any of the mailing list
> discussions around Y2038 issues.

Ok. I will prepare self contained comment.

> Note 2: if the comment actually needs to define the classes 1a, 1b,
> 2a, 2b, 3, it's probably using the wrong abstractions.  It should be
> very careful to refer to the abstraction that actually most reliably
> determines the presence or absence of the new syscalls (which might
> be the size of "long int" used in the syscall interface - glibc's
> __syscall_slong_t, which happens always to be the same size as
> __TIMESIZE for existing glibc ports but might not be for future ports
> - but make sure of that). 

The ABI (syscalls) compatibility was one of the concerns raised in this
patch series as a whole.

The glibc's internal struct __timespec64 passed to e.g. clock_settime64
syscall has explicit __time64_t (tv_sec), __int32_t (tv_nsec) and 32 bit

> Once the relevant abstraction is very
> clear, the reader can deduce the answer for each class of glibc ports.

IMHO, the abstraction would be:

1. The __ASSUME_64BIT_TIME is _never_ defined for 64 bit native systems

2. It is defined by default in:
sysdeps/unix/sysv/linux/kernel-features.h for 32 bit systems (and the
actual presence of the syscall is decided upon definitions of __NR_xxx*
(i.e. # ifdef __NR_clock_settime64).

As those syscalls are provided on almost every 32 bit system now
git grep -n "clock_settime64"

gives support for: arm, arm64 (compat mode), m68k, microblaze, mips,
parisc, powerpc, s390, sh, sparc, x86, xtensa

So it would be reasonable to just add this __ASSUME definition code to
sysdeps/unix/sysv/linux/kernel-features.h and #undef it for
architectures not supporting it (i.e. c-sky and riscv).

> Note 3: it's wrong to state the syscall numbers in the comment; that
> is not a relevant part of understanding the interface.  Stating the
> names, however, makes sense, provided you make sure not to use the
> __ASSUME_ macro for any *other* syscalls without updating the
> comment, and, at that time, reviewing whether the same definition
> conditions still work for all those syscalls. 

Correct, the names of supported syscalls also shall be written to the
comment (and the comment itself shall be extended with other, upcoming

> (Given that, as
> previously discussed, there might be *some* new syscalls even for
> architectures that already have 64-bit time, in order to provide
> timespec-based versions of syscalls currently using timeval.)

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:

Attachment: pgp9AnRFiSHe5.pgp
Description: OpenPGP digital signature

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