[PATCH] Y2038: add struct __timespec64

[PATCH] Y2038: add struct __timespec64

[Albert ARIBAUD (3ADEV)]

* include/bits/types/struct_timespec64.h:
  Include time/bits/types/struct_timespec64.h
* include/time.h (valid_timeval_to_timespec64): Add.
* include/time.h (valid_timespec_to_timespec64): Likewise.
* include/time.h (valid_timespec64_to_timespec): Likewise.
* include/time.h (valid_timespec64_to_timeval): Likewise.
* include/time.h (IS_VALID_NANOSECONDS): Likewise.
* include/time.h (timespec_to_timespec64): Likewise.
* include/time.h (timespec64_to_timespec): Likewise.
* include/time.h (timespec64_to_timeval): Likewise.
* io/fcntl.h: Include bits/types/struct_timespec64.h.
* io/sys/poll.h: Likewise.
* io/sys/stat.h: Likewise.
* misc/sys/select.h: Likewise.
* posix/sched.h: Likewise.
* posix/netdb.h: Likewise.
* rt/aio.h: Likewise.
* rt/mqueue.h: Likewise.
* signal/signal.h: Likewise.
* sysdeps/nptl/pthread.h: Likewise.
* sysdeps/pthread/semaphore.h: Likewise.
* sysdeps/unix/sysv/linux/hppa/pthread.h: Likewise.
* sysvipc/sys/sem.h: Likewise.
* time/Makefile: Add bits/types/struct_timespec64.h
* time/bits/types/struct_itimerspec.h:
  Include bits/types/struct_timespec64.h
* time/bits/types/struct_itimerspec64.h: Add.
* time/time.h: Include bits/types/struct_timespec64.h
---
To be Y2038-proof, struct __timespec64 needs its tv_sec field to
be a __time64_t rather than a __time_t. However, the question is
which type should the tv_nsec field be.

Keeping tv_nsec a long (32-bit) would be compatible with Posix
requirements but would result in the GLIBC struct timespec being
binary-incompatible with the Linux 64-bit struct timespec, which
contains a 64-bit, not 32-bit, signed tv_nsec field.

In order to maintain Posix compatibility yet simplify conversion
between Posix and Linux struct timespec values, the Y2038-proof
struct time stores its tv_nsec field as a 32-bit signed integer
plus a padding which can serve as a 64-bit sign extension. This
both meets Posix requirements and makes the GLIBC and Linux
struct timespec binary compatible.

Note that in the API (which is not modified here, and will be
later alongside all Y2038-sensitive APIs), this padding is made
'invisible' by defining it as an anonymous bitfield, whereas
the struct __timespec64 introduced here has a named field for
the padding, allowing implementations to read and write it.

Also, provide static inline functions and macros for checking
and converting between 32-bit itimevals and timespecs to 64-bit
timespecs.

This patch is part of the Y2038 patch series, which is available at
<https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/aaribaud/y2038>.
Warning: this branch may be rebased on current master and/or updated based on
feedback from the list at any time.

 include/bits/types/struct_timespec64.h |  1 +
 include/time.h                         | 82 ++++++++++++++++++++++++++
 io/fcntl.h                             |  1 +
 io/sys/poll.h                          |  1 +
 io/sys/stat.h                          |  1 +
 misc/sys/select.h                      |  1 +
 posix/sched.h                          |  1 +
 resolv/netdb.h                         |  1 +
 rt/aio.h                               |  1 +
 rt/mqueue.h                            |  1 +
 signal/signal.h                        |  1 +
 sysdeps/nptl/pthread.h                 |  1 +
 sysdeps/pthread/semaphore.h            |  1 +
 sysdeps/unix/sysv/linux/hppa/pthread.h |  1 +
 sysvipc/sys/sem.h                      |  1 +
 time/Makefile                          |  2 +-
 time/bits/types/struct_itimerspec.h    |  1 +
 time/bits/types/struct_timespec64.h    | 29 +++++++++
 time/time.h                            |  1 +
 19 files changed, 128 insertions(+), 1 deletion(-)
 create mode 100644 include/bits/types/struct_timespec64.h
 create mode 100644 time/bits/types/struct_timespec64.h

diff --git a/include/bits/types/struct_timespec64.h b/include/bits/types/struct_timespec64.h
new file mode 100644
index 0000000000..42869680b8
--- /dev/null
+++ b/include/bits/types/struct_timespec64.h
@@ -0,0 +1 @@
+#include <time/bits/types/struct_timespec64.h>
diff --git a/include/time.h b/include/time.h
index 0c5c00218a..162afa1012 100644
--- a/include/time.h
+++ b/include/time.h
@@ -145,5 +145,87 @@ fits_in_time_t (__time64_t t64)
   return t == t64;
 }
 
+/* convert a known valid struct timeval into a struct __timespec64 */
+static inline void
+valid_timeval_to_timespec64 (const struct timeval *tv32,
+			     struct __timespec64 *ts64)
+{
+  ts64->tv_sec = tv32->tv_sec;
+  ts64->tv_nsec = tv32->tv_usec * 1000;
+}
+
+/* convert a known valid struct timespec into a struct __timespec64 */
+static inline void
+valid_timespec_to_timespec64 (const struct timespec *ts32,
+			      struct __timespec64 *ts64)
+{
+  ts64->tv_sec = ts32->tv_sec;
+  ts64->tv_nsec = ts32->tv_nsec;
+  /* we only need to zero ts64->tv_pad if we pass it to the kernel */
+}
+
+/* convert a known valid struct __timespec64 into a struct timespec */
+static inline void
+valid_timespec64_to_timespec (const struct __timespec64 *ts64,
+			      struct timespec *ts32)
+{
+  ts32->tv_sec = (time_t) ts64->tv_sec;
+  ts32->tv_nsec = ts64->tv_nsec;
+}
+
+/* convert a known valid struct __timespec64 into a struct timeval */
+static inline void
+valid_timespec64_to_timeval (const struct __timespec64 *ts64,
+			     struct timeval *tv32)
+{
+  tv32->tv_sec = (time_t) ts64->tv_sec;
+  tv32->tv_usec = ts64->tv_nsec / 1000;
+}
+
+/* check if a value lies with the valid nanoseconds range */
+#define IS_VALID_NANOSECONDS(ns) (ns >= 0 && ns <= 999999999)
+
+/* check and convert a struct timespec into a struct __timespec64 */
+static inline bool timespec_to_timespec64 (const struct timespec *ts32,
+					   struct __timespec64 *ts64)
+{
+  /* check that ts32 holds a valid count of nanoseconds */
+  if (! IS_VALID_NANOSECONDS (ts32->tv_nsec))
+    return false;
+  /* all ts32 fields can fit in ts64, so copy them */
+  valid_timespec_to_timespec64 (ts32, ts64);
+  /* we only need to zero ts64->tv_pad if we pass it to the kernel */
+  return true;
+}
+
+/* check and convert a struct __timespec64 into a struct timespec */
+static inline bool timespec64_to_timespec (const struct __timespec64 *ts64,
+					   struct timespec *ts32)
+{
+  /* check that tv_nsec holds a valid count of nanoseconds */
+  if (! IS_VALID_NANOSECONDS (ts64->tv_nsec))
+    return false;
+  /* check that tv_sec can fit in a __time_t */
+  if (! fits_in_time_t (ts64->tv_sec))
+    return false;
+  /* all ts64 fields can fit in ts32, so copy them */
+  valid_timespec64_to_timespec (ts64, ts32);
+  return true;
+}
+
+/* check and convert a struct __timespec64 into a struct timeval */
+static inline bool timespec64_to_timeval (const struct __timespec64 *ts64,
+					  struct timeval *tv32)
+{
+  /* check that tv_nsec holds a valid count of nanoseconds */
+  if (! IS_VALID_NANOSECONDS (ts64->tv_nsec))
+    return false;
+  /* check that tv_sec can fit in a __time_t */
+  if (! fits_in_time_t (ts64->tv_sec))
+    return false;
+  /* all ts64 fields can fit in tv32, so copy them */
+  valid_timespec64_to_timeval (ts64, tv32);
+  return true;
+}
 #endif
 #endif
diff --git a/io/fcntl.h b/io/fcntl.h
index 6b0e9fa1fa..db0064294f 100644
--- a/io/fcntl.h
+++ b/io/fcntl.h
@@ -73,6 +73,7 @@ typedef __pid_t pid_t;
 /* For XPG all symbols from <sys/stat.h> should also be available.  */
 #ifdef __USE_XOPEN2K8
 # include <bits/types/struct_timespec.h>
+# include <bits/types/struct_timespec64.h>
 #endif
 #if defined __USE_XOPEN || defined __USE_XOPEN2K8
 # include <bits/stat.h>
diff --git a/io/sys/poll.h b/io/sys/poll.h
index 68f68ca412..a4c2547dd6 100644
--- a/io/sys/poll.h
+++ b/io/sys/poll.h
@@ -26,6 +26,7 @@
 #ifdef __USE_GNU
 # include <bits/types/__sigset_t.h>
 # include <bits/types/struct_timespec.h>
+# include <bits/types/struct_timespec64.h>
 #endif
 
 
diff --git a/io/sys/stat.h b/io/sys/stat.h
index 762c8538ba..a1092f5fb4 100644
--- a/io/sys/stat.h
+++ b/io/sys/stat.h
@@ -28,6 +28,7 @@
 
 #ifdef __USE_XOPEN2K8
 # include <bits/types/struct_timespec.h>
+# include <bits/types/struct_timespec64.h>
 #endif
 
 #if defined __USE_XOPEN || defined __USE_XOPEN2K
diff --git a/misc/sys/select.h b/misc/sys/select.h
index 6dd0c83227..1285dc4a8b 100644
--- a/misc/sys/select.h
+++ b/misc/sys/select.h
@@ -37,6 +37,7 @@
 #include <bits/types/struct_timeval.h>
 #ifdef __USE_XOPEN2K
 # include <bits/types/struct_timespec.h>
+# include <bits/types/struct_timespec64.h>
 #endif
 
 #ifndef __suseconds_t_defined
diff --git a/posix/sched.h b/posix/sched.h
index 619b3b3a81..732c12baab 100644
--- a/posix/sched.h
+++ b/posix/sched.h
@@ -30,6 +30,7 @@
 
 #include <bits/types/time_t.h>
 #include <bits/types/struct_timespec.h>
+#include <bits/types/struct_timespec64.h>
 #ifndef __USE_XOPEN2K
 # include <time.h>
 #endif
diff --git a/resolv/netdb.h b/resolv/netdb.h
index 003800e882..96f675de75 100644
--- a/resolv/netdb.h
+++ b/resolv/netdb.h
@@ -35,6 +35,7 @@
 #ifdef __USE_GNU
 # include <bits/types/sigevent_t.h>
 # include <bits/types/struct_timespec.h>
+# include <bits/types/struct_timespec64.h>
 #endif
 
 #include <bits/netdb.h>
diff --git a/rt/aio.h b/rt/aio.h
index c3a1f4bd90..ffda0de906 100644
--- a/rt/aio.h
+++ b/rt/aio.h
@@ -27,6 +27,7 @@
 #include <bits/types/sigevent_t.h>
 #include <bits/sigevent-consts.h>
 #include <bits/types/struct_timespec.h>
+#include <bits/types/struct_timespec64.h>
 
 __BEGIN_DECLS
 
diff --git a/rt/mqueue.h b/rt/mqueue.h
index 5f354b4d76..39e2a243ca 100644
--- a/rt/mqueue.h
+++ b/rt/mqueue.h
@@ -23,6 +23,7 @@
 #include <fcntl.h>
 #include <bits/types/sigevent_t.h>
 #include <bits/types/struct_timespec.h>
+#include <bits/types/struct_timespec64.h>
 /* Get the definition of mqd_t and struct mq_attr.  */
 #include <bits/mqueue.h>
 
diff --git a/signal/signal.h b/signal/signal.h
index 87dc82a998..caa291587b 100644
--- a/signal/signal.h
+++ b/signal/signal.h
@@ -51,6 +51,7 @@ typedef __uid_t uid_t;
 #ifdef __USE_POSIX199309
 /* We need `struct timespec' later on.  */
 # include <bits/types/struct_timespec.h>
+# include <bits/types/struct_timespec64.h>
 #endif
 
 #if defined __USE_POSIX199309 || defined __USE_XOPEN_EXTENDED
diff --git a/sysdeps/nptl/pthread.h b/sysdeps/nptl/pthread.h
index df049abf74..41b915ec07 100644
--- a/sysdeps/nptl/pthread.h
+++ b/sysdeps/nptl/pthread.h
@@ -27,6 +27,7 @@
 #include <bits/setjmp.h>
 #include <bits/wordsize.h>
 #include <bits/types/struct_timespec.h>
+#include <bits/types/struct_timespec64.h>
 
 
 /* Detach state.  */
diff --git a/sysdeps/pthread/semaphore.h b/sysdeps/pthread/semaphore.h
index ff672ebd24..312433f544 100644
--- a/sysdeps/pthread/semaphore.h
+++ b/sysdeps/pthread/semaphore.h
@@ -22,6 +22,7 @@
 #include <sys/types.h>
 #ifdef __USE_XOPEN2K
 # include <bits/types/struct_timespec.h>
+# include <bits/types/struct_timespec64.h>
 #endif
 
 /* Get the definition for sem_t.  */
diff --git a/sysdeps/unix/sysv/linux/hppa/pthread.h b/sysdeps/unix/sysv/linux/hppa/pthread.h
index 11a024db59..e4640b3717 100644
--- a/sysdeps/unix/sysv/linux/hppa/pthread.h
+++ b/sysdeps/unix/sysv/linux/hppa/pthread.h
@@ -27,6 +27,7 @@
 #include <bits/setjmp.h>
 #include <bits/wordsize.h>
 #include <bits/types/struct_timespec.h>
+#include <bits/types/struct_timespec64.h>
 
 
 /* Detach state.  */
diff --git a/sysvipc/sys/sem.h b/sysvipc/sys/sem.h
index 200765bcba..b6a7eb2ada 100644
--- a/sysvipc/sys/sem.h
+++ b/sysvipc/sys/sem.h
@@ -31,6 +31,7 @@
 
 #ifdef __USE_GNU
 # include <bits/types/struct_timespec.h>
+# include <bits/types/struct_timespec64.h>
 #endif
 
 /* The following System V style IPC functions implement a semaphore
diff --git a/time/Makefile b/time/Makefile
index ec3e39dcea..61b8904dfc 100644
--- a/time/Makefile
+++ b/time/Makefile
@@ -27,7 +27,7 @@ headers := time.h sys/time.h sys/timeb.h bits/time.h			\
 	   bits/types/struct_itimerspec.h				\
 	   bits/types/struct_timespec.h bits/types/struct_timeval.h	\
 	   bits/types/struct_tm.h bits/types/timer_t.h			\
-	   bits/types/time_t.h
+	   bits/types/time_t.h bits/types/struct_timespec64.h
 
 routines := offtime asctime clock ctime ctime_r difftime \
 	    gmtime localtime mktime time		 \
diff --git a/time/bits/types/struct_itimerspec.h b/time/bits/types/struct_itimerspec.h
index 17cc1ac86d..d36295a830 100644
--- a/time/bits/types/struct_itimerspec.h
+++ b/time/bits/types/struct_itimerspec.h
@@ -3,6 +3,7 @@
 
 #include <bits/types.h>
 #include <bits/types/struct_timespec.h>
+#include <bits/types/struct_timespec64.h>
 
 /* POSIX.1b structure for timer start values and intervals.  */
 struct itimerspec
diff --git a/time/bits/types/struct_timespec64.h b/time/bits/types/struct_timespec64.h
new file mode 100644
index 0000000000..2221bed8bc
--- /dev/null
+++ b/time/bits/types/struct_timespec64.h
@@ -0,0 +1,29 @@
+#ifndef __timespec64_defined
+#define __timespec64_defined 1
+
+#include <bits/types.h>
+#include <endian.h>
+
+/* Y2036proof structure for a time value.  This is like a `struct timeval' but
+   has nanoseconds instead of microseconds.  To keep tings Posix-ish, we keep
+   the nanoseconds field a signed long, but since Linux has a 64-bit signed int,
+   we pad it with a 32-bit int, which should always be 0.
+   Note that the public type has an anonymous bitfield as padding, so that
+   it cannot be written into (or read from). */ 
+#if BYTE_ORDER == BIG_ENDIAN
+struct __timespec64
+{
+  __time64_t tv_sec;		/* Seconds */
+  int tv_pad: 32;		/* Padding named for checking/setting */
+  __syscall_slong_t tv_nsec;	/* Nanoseconds */
+};
+#else
+struct __timespec64
+{
+  __time64_t tv_sec;		/* Seconds */
+  __syscall_slong_t tv_nsec;	/* Nanoseconds */
+  int tv_pad: 32;		/* Padding named for checking/setting */
+};
+#endif
+
+#endif
diff --git a/time/time.h b/time/time.h
index 4b55e34402..c9282e2673 100644
--- a/time/time.h
+++ b/time/time.h
@@ -40,6 +40,7 @@
 
 #if defined __USE_POSIX199309 || defined __USE_ISOC11
 # include <bits/types/struct_timespec.h>
+# include <bits/types/struct_timespec64.h>
 #endif
 
 #ifdef __USE_POSIX199309
-- 
2.17.1

Re: [PATCH] Y2038: add struct __timespec64

[Joseph Myers]

On Wed, 19 Sep 2018, Albert ARIBAUD (3ADEV) wrote:

> diff --git a/time/bits/types/struct_timespec64.h b/time/bits/types/struct_timespec64.h
> new file mode 100644
> index 0000000000..2221bed8bc
> --- /dev/null
> +++ b/time/bits/types/struct_timespec64.h

Since it's actually defining struct __timespec64, the header needs to be 
bits/types/struct___timespec64.h.  bits/types/struct_timespec64.h would 
define a type struct timespec64, but we don't have such a type.

> +/* Y2036proof structure for a time value.  This is like a `struct timeval' but
> +   has nanoseconds instead of microseconds.  To keep tings Posix-ish, we keep
> +   the nanoseconds field a signed long, but since Linux has a 64-bit signed int,
> +   we pad it with a 32-bit int, which should always be 0.
> +   Note that the public type has an anonymous bitfield as padding, so that
> +   it cannot be written into (or read from). */ 

The bit-fields needs to be *anonymous* for initializers to work as 
intended.  It can't have the name tv_pad that it has in this patch, or any 
other name, with our without leading underscores.

> +#if BYTE_ORDER == BIG_ENDIAN

These macros are in the user namespace.  You have to use __BYTE_ORDER / 
__BIG_ENDIAN.

> +struct __timespec64
> +{
> +  __time64_t tv_sec;		/* Seconds */
> +  int tv_pad: 32;		/* Padding named for checking/setting */
> +  __syscall_slong_t tv_nsec;	/* Nanoseconds */
> +};

You also need a separate x86 version of the header that avoids inserting 
the padding for x32, given the (nonconforming) 64-bit __syscall_slong_t 
there.  Then, the wrapper in include/ will be wrong for x86, so the 
generic one probably needs to move from time/ to the top-level bits/ 
directory so the correct header is found in all cases.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Y2038: add struct __timespec64

[Joseph Myers]

Generally, what is the design for struct __timespec64 on configurations 
where time_t is already 64-bit - normal 64-bit configurations, not just 
x32?  I'd expect one of:

* #define __timespec64 timespec (and no separate structure defined at 
all).  This is probably also the right thing to do with x32 (if there are 
appropriate header conditionals already available, that may mean you don't 
need an x86-specific header variant at all).

* The header doesn't get included at all for such configurations and no 
APIs using the struct __timespec64 are declared at all for them.  That's 
probably harder to implement (needs more conditionals all over glibc), so 
the first approach seems better.

> +/* check if a value lies with the valid nanoseconds range */

Comments start with an uppercase letter, end with "." and two spaces.  
This needs fixing throughout the patch series.

> +static inline bool timespec_to_timespec64 (const struct timespec *ts32,

The "static inline bool" should go on a separate line; the function name 
in a definition always goes at the start of a line.  Likewise elsewhere.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Y2038: add struct __timespec64

[Albert ARIBAUD]

Hi Joseph,

On Wed, 19 Sep 2018 14:55:49 +0000, Joseph Myers
<joseph@codesourcery.com> wrote :

> Generally, what is the design for struct __timespec64 on configurations 
> where time_t is already 64-bit - normal 64-bit configurations, not just 
> x32?  I'd expect one of:
> 
> * #define __timespec64 timespec (and no separate structure defined at 
> all).  This is probably also the right thing to do with x32 (if there are 
> appropriate header conditionals already available, that may mean you don't 
> need an x86-specific header variant at all).

Correct.

> * The header doesn't get included at all for such configurations and no 
> APIs using the struct __timespec64 are declared at all for them.  That's 
> probably harder to implement (needs more conditionals all over glibc), so 
> the first approach seems better.
> 
> > +/* check if a value lies with the valid nanoseconds range */  
> 
> Comments start with an uppercase letter, end with "." and two spaces.  
> This needs fixing throughout the patch series.

Will fix.

> > +static inline bool timespec_to_timespec64 (const struct timespec *ts32,  
> 
> The "static inline bool" should go on a separate line; the function name 
> in a definition always goes at the start of a line.  Likewise elsewhere.

Ok.

Cordialement,
Albert ARIBAUD
3ADEV

Re: [PATCH] Y2038: add struct __timespec64

[Joseph Myers]

On Wed, 19 Sep 2018, Joseph Myers wrote:

> The bit-fields needs to be *anonymous* for initializers to work as 
> intended.  It can't have the name tv_pad that it has in this patch, or any 
> other name, with our without leading underscores.

Ignore this, I see you are distinguishing between an internal type here 
with a named field and a public type with an anonymous field.

*But* I wouldn't expect the internal type to be declared in any public 
header at all.  That is, this should not be a bits/ header at all, not an 
installed header, because it would only be used in implementation code, 
not from any public interface.  So you need to rename the header out of 
the bits/ namespace, which is purely for installed headers, and remove all 
the #includes from installed headers.

Regarding the comment you have on this type: 2038, not 2036; things, not 
tings; don't reference timeval at all (describing timespec in terms of 
timeval is a very historical way of writing a comment, that may have made 
sense when timeval was the established type and timespec was the new one, 
but doesn't make sense for over 20 years now; rather, comments on timeval 
should describe it as a legacy type, superseded by timespec).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Y2038: add struct __timespec64

[Albert ARIBAUD]

Hi Joseph,

On Wed, 19 Sep 2018 15:00:33 +0000, Joseph Myers
<joseph@codesourcery.com> wrote :

> On Wed, 19 Sep 2018, Joseph Myers wrote:
> 
> > The bit-fields needs to be *anonymous* for initializers to work as 
> > intended.  It can't have the name tv_pad that it has in this patch, or any 
> > other name, with our without leading underscores.  
> 
> Ignore this, I see you are distinguishing between an internal type here 
> with a named field and a public type with an anonymous field.

Indeed I am, and rather than ignoring your remark, I'll take it as a
sign that my comment is possibly confusing and needs to make it
explicit that there are two types, one private to glibc and one public.

> *But* I wouldn't expect the internal type to be declared in any public 
> header at all.  That is, this should not be a bits/ header at all, not an 
> installed header, because it would only be used in implementation code, 
> not from any public interface.  So you need to rename the header out of 
> the bits/ namespace, which is purely for installed headers, and remove all 
> the #includes from installed headers.

Ok.

> Regarding the comment you have on this type: 2038, not 2036; things, not 
> tings; don't reference timeval at all (describing timespec in terms of 
> timeval is a very historical way of writing a comment, that may have made 
> sense when timeval was the established type and timespec was the new one, 
> but doesn't make sense for over 20 years now; rather, comments on timeval 
> should describe it as a legacy type, superseded by timespec).

Will fix.

Cordialement,
Albert ARIBAUD
3ADEV

Re: [PATCH] Y2038: add struct __timespec64

[Joseph Myers]

On Wed, 19 Sep 2018, Albert ARIBAUD (3ADEV) wrote:

> To be Y2038-proof, struct __timespec64 needs its tv_sec field to
> be a __time64_t rather than a __time_t. However, the question is
> which type should the tv_nsec field be.

These comments are mainly about the proposed commit message, having 
commented on the code changes separately.  As before, the human-level part 
should come *before* the ChangeLog entry to be maximally helpful to 
reviewers.

I think this commit message is starting too deep into the details of the 
patch.  Before you delve into the details of the types of particular 
fields of particular structures, you need to explain what exactly those 
structures are for.

That is, your *first* paragraph needs to explain what struct __timespec64 
is (an *internal* type like struct timespec, where the seconds field is 
always 64-bit).  Don't hide the information that this is a purely internal 
type in the fourth paragraph as in the present commit message - it's 
critical information that needs to be right up front.  Once you've 
explained the function of the type, then you can discuss constraints on 
layout-compatibility with the public struct timespec when time_t is 64-bit 
(whether that is the default, or whether that results from _TIME_BITS=64), 
to avoid unnecessary copying at that interface boundary, and on 
layout-compatibility with kernel interfaces, again to avoid unnecessary 
copying.  Only then should you get into the tv_nsec discussions - until 
you've explained the function of and constraints on the struct 
__timespec64 type, it makes no sense to get into the details of such 
complications.

Having given the overall explanation of the layout requirements up front, 
the discussion ot tv_nsec is reasonable enough - provided you are clear, 
whenever you say things like

> Keeping tv_nsec a long (32-bit) would be compatible with Posix

about exactly what happens in the case where long is 64-bit and time_t is 
already 64-bit.  But there is another key piece of information missing 
from this commit message: what are the kernel interfaces for 64-bit times 
on 32-bit systems expected to look like?

We have, I think, made clear several times to the kernel people that the 
best thing for glibc would be that, at the kernel/userspace ABI 
boundaries, for ABIs where userspace long is 32-bit, only the low 32 bits 
of the nanoseconds field should be considered significant when coming from 
userspace to the kernel; the high bits should be considered as padding.  
Will the kernel be implementing this?

If the kernel is implementing that, then there's no obvious need for this 
structure with an explicit name for the padding at all, because there is 
no need to copy 64-bit timespec values and zero the padding to pass them 
into the kernel.  On the other hand, if the kernel is not implementing 
that - if the kernel is treating the nanoseconds field as a 64-bit 
integer, all bits significant, when it comes from userspace, then copying 
and zeroing padding is necessary when passing timespec values to the 
kernel.  So information about the kernel choice is critical information 
for the glibc commit message to justify the particular choices made in 
glibc.

Now if explicit zeroing is needed, the next question is whether the 
internal type should have named padding, as here, or whether it should 
have 64-bit nanoseconds.  In the first case, the zeroing when passing 
values to the kernel would be setting tv_pad to 0; in the second case, it 
would just be a tv_nsec assignment and the compiler would deal with 
setting the high part appropriately.

While the POSIX requirements are relevant for the *public* struct timespec 
with _TIME_BITS=64, they are less clearly relevant for the internal type.  
In either case, if pointers to one type are converted to pointers to the 
other, an explicit type cast will be needed as the types won't be 
compatible (except in the case where time_t is already 64-bit and long is 
64-bit and __timespec64 can be #defined to timespec).

Finally, having explained the overall nature of the changes in the patch, 
and then gone into the details of the particular choices of interest made 
(such as for the type of tv_nsec), the human-level message for any patch 
should then detail the testing done on that patch.  This would include 
testing (with the full glibc testsuite) for at least one 32-bit and one 
64-bit platform (for such 64-bit time patches; appropriate testing is very 
much dependent on the particular patch, and for many patches outside this 
area there's no need to test on more than one platform).  Then, after 
that, the ChangeLog entry.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Y2038: add struct __timespec64

[Albert ARIBAUD]

Hi Joseph,

On Wed, 19 Sep 2018 16:24:23 +0000, Joseph Myers
<joseph@codesourcery.com> wrote :

> We have, I think, made clear several times to the kernel people that the 
> best thing for glibc would be that, at the kernel/userspace ABI 
> boundaries, for ABIs where userspace long is 32-bit, only the low 32 bits 
> of the nanoseconds field should be considered significant when coming from 
> userspace to the kernel; the high bits should be considered as padding.  
> Will the kernel be implementing this?
> 
> If the kernel is implementing that, then there's no obvious need for this 
> structure with an explicit name for the padding at all, because there is 
> no need to copy 64-bit timespec values and zero the padding to pass them 
> into the kernel.  On the other hand, if the kernel is not implementing 
> that - if the kernel is treating the nanoseconds field as a 64-bit 
> integer, all bits significant, when it comes from userspace, then copying 
> and zeroing padding is necessary when passing timespec values to the 
> kernel.  So information about the kernel choice is critical information 
> for the glibc commit message to justify the particular choices made in 
> glibc.

I have just checked the Y2038 syscalls branch I am using currently, and
it seems to clear the 32 upper bits of tv_nsec when receiving it from
the userland, so yes, we actually don't need to clear it ourselves
(I'll have to update the design document, which still assumes
otherwise).
 
> Now if explicit zeroing is needed, the next question is whether the 
> internal type should have named padding, as here, or whether it should 
> have 64-bit nanoseconds.  In the first case, the zeroing when passing 
> values to the kernel would be setting tv_pad to 0; in the second case, it 
> would just be a tv_nsec assignment and the compiler would deal with 
> setting the high part appropriately.
> 
> While the POSIX requirements are relevant for the *public* struct timespec 
> with _TIME_BITS=64, they are less clearly relevant for the internal type.  
> In either case, if pointers to one type are converted to pointers to the 
> other, an explicit type cast will be needed as the types won't be 
> compatible (except in the case where time_t is already 64-bit and long is 
> 64-bit and __timespec64 can be #defined to timespec).

The Posix issue is indeed only on the public side, where computations
involving a tv_nsec may yield different effects if tv_nsec goes from
the possibly expected 32-bit long to an unexpected 64-bit signed int.

So I'll keep the public type with anonymous padding, but I will remove
padding and use 64 bits for tv_nsec in the internal type.

> Finally, having explained the overall nature of the changes in the patch, 
> and then gone into the details of the particular choices of interest made 
> (such as for the type of tv_nsec), the human-level message for any patch 
> should then detail the testing done on that patch.  This would include 
> testing (with the full glibc testsuite) for at least one 32-bit and one 
> 64-bit platform (for such 64-bit time patches; appropriate testing is very 
> much dependent on the particular patch, and for many patches outside this 
> area there's no need to test on more than one platform).  Then, after 
> that, the ChangeLog entry.

Ok.

Cordialement,
Albert ARIBAUD
3ADEV

Re: [PATCH] Y2038: add struct __timespec64

[Joseph Myers]

On Wed, 26 Sep 2018, Albert ARIBAUD wrote:

> I have just checked the Y2038 syscalls branch I am using currently, and
> it seems to clear the 32 upper bits of tv_nsec when receiving it from
> the userland, so yes, we actually don't need to clear it ourselves
> (I'll have to update the design document, which still assumes
> otherwise).

In that case, is there any reason for the contents of the internal type to 
differ at all from the contents of the public type?  Or would it be best 
for both to use the anonymous padding when building for 32-bit long?

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Y2038: add struct __timespec64

[Albert ARIBAUD]

Hi Joseph,

On Wed, 26 Sep 2018 16:28:21 +0000, Joseph Myers
<joseph@codesourcery.com> wrote :

> On Wed, 26 Sep 2018, Albert ARIBAUD wrote:
> 
> > I have just checked the Y2038 syscalls branch I am using currently, and
> > it seems to clear the 32 upper bits of tv_nsec when receiving it from
> > the userland, so yes, we actually don't need to clear it ourselves
> > (I'll have to update the design document, which still assumes
> > otherwise).  
> 
> In that case, is there any reason for the contents of the internal type to 
> differ at all from the contents of the public type?  Or would it be best 
> for both to use the anonymous padding when building for 32-bit long?

The only reason for a difference between an internal (64-bit tv_nsec)
and public (32-bit tv_nsec) type is that we want application source code
to build and run the same for 32-bit and 64-bit time, and Posix defines
tv_nsec as a long (for 32-bit time), so application source code /might/
expect it to be a long for 64-bit time too. However, the worse I can
foresee if we make tv_nsec 64-bit is that some application source code
might copy it into a long, which /may/ trigger gcc to warn if the
compiler options include -Wconversion.

If we consider this not to be a problem, then we can keep the 32-bit
tv_nsec plus anonymous padding in both the public and private struct
__timespec64 types.

Cordialement,
Albert ARIBAUD
3ADEV

Re: [PATCH] Y2038: add struct __timespec64

[Joseph Myers]

On Wed, 17 Oct 2018, Albert ARIBAUD wrote:

> > In that case, is there any reason for the contents of the internal type to 
> > differ at all from the contents of the public type?  Or would it be best 
> > for both to use the anonymous padding when building for 32-bit long?
> 
> The only reason for a difference between an internal (64-bit tv_nsec)
> and public (32-bit tv_nsec) type is that we want application source code
> to build and run the same for 32-bit and 64-bit time, and Posix defines
> tv_nsec as a long (for 32-bit time), so application source code /might/
> expect it to be a long for 64-bit time too. However, the worse I can
> foresee if we make tv_nsec 64-bit is that some application source code
> might copy it into a long, which /may/ trigger gcc to warn if the
> compiler options include -Wconversion.

I agree that the user-visible type should be long in all cases.  My 
question was why the internal tv_nsec should be 64-bit for the case of 
32-bit long, rather than making the internal type work the same as the 
POSIX one (long tv_nsec, anonymous padding when needed), if there is never 
any need to zero the padding explicitly.

-- 
Joseph S. Myers
joseph@codesourcery.com