diff mbox

[v8,1/3] y2038: Introduce internal for glibc struct __timespec64

Message ID 20190918211603.8444-2-lukma@denx.de
State Committed
Headers

Commit Message

Lukasz Majewski Sept. 18, 2019, 9:16 p.m. UTC 
  This type is a glibc's "internal" type similar to struct timespec but
whose tv_sec field is a __time64_t rather than a time_t, which makes it
Y2038-proof and usable to pass syscalls between user code and Y2038-proof
kernel.

To support passing this structure to the kernel - the tv_pad, 32 bit int,
has been introduced. The placement of it depends on endianness of the SoC.

Tested on x86_64 and ARM.

* include/time.h: Add struct __timespec64 definition

---
Changes for v8:
- Replace #if __WORDSIZE == 64 \
           || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
  condition with #if __TIMESIZE == 64
  to allow usage of struct __timespec64 aliased to timespec on ports
  supporting 64 bit time API from the outset.

  Architectures with __TIMESIZE == 32 will use struct __timespec64 with
  __time64_t tv_sec and __int32_t tv_pad and tv_nsec.

Changes for v7:
- None

Changes for v6:
- None

Changes for v5:
- Reword commit message
- Add missing preprocessor condition for x32 (to use timespec instead of
  __timespec64).

Changes for v4:
- Change tv_pad's type from named bitfield to 32 bit int

Changes for v3:
- Replace __TIMESIZE with __WORDSIZE (as architectures with __TIMESIZE==64
  will need to use this struct with 32 bit tv_nsec field).
- Update in-code comment

Changes for v2:
- None
---
 include/time.h | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

Comments

Joseph Myers Sept. 19, 2019, 8:14 p.m. UTC | #1 
On Wed, 18 Sep 2019, Lukasz Majewski wrote:

> This type is a glibc's "internal" type similar to struct timespec but
> whose tv_sec field is a __time64_t rather than a time_t, which makes it
> Y2038-proof and usable to pass syscalls between user code and Y2038-proof
> kernel.
> 
> To support passing this structure to the kernel - the tv_pad, 32 bit int,
> has been introduced. The placement of it depends on endianness of the SoC.
> 
> Tested on x86_64 and ARM.
> 
> * include/time.h: Add struct __timespec64 definition

This patch is OK.
Lukasz Majewski Sept. 23, 2019, 9:21 p.m. UTC | #2 
Hi Joseph,

> On Wed, 18 Sep 2019, Lukasz Majewski wrote:
> 
> > This type is a glibc's "internal" type similar to struct timespec
> > but whose tv_sec field is a __time64_t rather than a time_t, which
> > makes it Y2038-proof and usable to pass syscalls between user code
> > and Y2038-proof kernel.
> > 
> > To support passing this structure to the kernel - the tv_pad, 32
> > bit int, has been introduced. The placement of it depends on
> > endianness of the SoC.
> > 
> > Tested on x86_64 and ARM.
> > 
> > * include/time.h: Add struct __timespec64 definition  
> 
> This patch is OK.
> 

If I may ask - are there any issues with pulling this patch to glibc
-master branch?


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. 25, 2019, 12:47 a.m. UTC | #3 
On Mon, 23 Sep 2019, Lukasz Majewski wrote:

> > > * include/time.h: Add struct __timespec64 definition  
> > 
> > This patch is OK.
> > 
> 
> If I may ask - are there any issues with pulling this patch to glibc
> -master branch?

Unless there have been other concerns expressed about this patch in 
previous discussions, and unless you've discovered any problems with it 
yourself, I'm expecting you to commit it to master.  And that's generally 
the case for most patches - if someone has explicitly judged it OK for 
inclusion and there have been no other concerns expressed about it, it 
should be committed (unless in a release freeze period or it depends on 
some uncommitted patch, in which case the commit needs to be delayed).

I think it's generally for reviewers to say if their view is "I think this 
patch is OK but we should allow more time for other people to comment", 
rather than expecting patch contributors to judge when they need to wait 
further after a patch approval.
Lukasz Majewski Sept. 25, 2019, 7:45 a.m. UTC | #4 
Hi Joseph,

> On Mon, 23 Sep 2019, Lukasz Majewski wrote:
> 
> > > > * include/time.h: Add struct __timespec64 definition    
> > > 
> > > This patch is OK.
> > >   
> > 
> > If I may ask - are there any issues with pulling this patch to glibc
> > -master branch?  
> 
> Unless there have been other concerns expressed about this patch in 
> previous discussions, and unless you've discovered any problems with
> it yourself, I'm expecting you to commit it to master. 

Thanks for clarification - I thought that glibc follows kernel workflow
and maintainers of respective parts (or those who do the review) apply
patches.

> And that's
> generally the case for most patches - if someone has explicitly
> judged it OK for inclusion and there have been no other concerns
> expressed about it, it should be committed (unless in a release
> freeze period or it depends on some uncommitted patch, in which case
> the commit needs to be delayed).

Ok.

> 
> I think it's generally for reviewers to say if their view is "I think
> this patch is OK but we should allow more time for other people to
> comment", rather than expecting patch contributors to judge when they
> need to wait further after a patch approval.

Yes. I do understand.

If I may ask - what is the "acceptable" time for other people from
community to jump in and comment the patch before it shall be
applied?

Is it one week or more/less ?

> 

Thanks for your help.


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
Florian Weimer Sept. 25, 2019, 12:43 p.m. UTC | #5 
* Lukasz Majewski:

> This type is a glibc's "internal" type similar to struct timespec but
> whose tv_sec field is a __time64_t rather than a time_t, which makes it
> Y2038-proof and usable to pass syscalls between user code and Y2038-proof
> kernel.
>
> To support passing this structure to the kernel - the tv_pad, 32 bit int,
> has been introduced. The placement of it depends on endianness of the SoC.
>
> Tested on x86_64 and ARM.
>
> * include/time.h: Add struct __timespec64 definition

Please fix the subject before committing.  A word seems to be missing
after “internal”.  Thanks.
Florian Weimer Sept. 25, 2019, 12:51 p.m. UTC | #6 
* Lukasz Majewski:

>> I think it's generally for reviewers to say if their view is "I think
>> this patch is OK but we should allow more time for other people to
>> comment", rather than expecting patch contributors to judge when they
>> need to wait further after a patch approval.
>
> Yes. I do understand.
>
> If I may ask - what is the "acceptable" time for other people from
> community to jump in and comment the patch before it shall be
> applied?
>
> Is it one week or more/less ?

A week is more than enough, especially for patches that only touch
internals like this one.

Regarding the actual patch, I don't understand why tv_pad isn't an
*anonymous* bit field.  This seems to introduce unnecessary variance
between architectures and is incompatible with how glibc itself uses
struct timespec.  It's also inconsistent with the new comment in
include/time.h (named padding is only needed if you need to
zero-initialize the padding).
Lukasz Majewski Sept. 25, 2019, 1:06 p.m. UTC | #7 
Hi Florian,

> * Lukasz Majewski:
> 
> > This type is a glibc's "internal" type similar to struct timespec
> > but whose tv_sec field is a __time64_t rather than a time_t, which
> > makes it Y2038-proof and usable to pass syscalls between user code
> > and Y2038-proof kernel.
> >
> > To support passing this structure to the kernel - the tv_pad, 32
> > bit int, has been introduced. The placement of it depends on
> > endianness of the SoC.
> >
> > Tested on x86_64 and ARM.
> >
> > * include/time.h: Add struct __timespec64 definition  
> 
> Please fix the subject before committing.  A word seems to be missing
> after “internal”.  Thanks.

Is the below version more acceptable?

"y2038: Introduce struct __timespec64 - new internal glibc type"


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
Florian Weimer Sept. 25, 2019, 1:07 p.m. UTC | #8 
* Lukasz Majewski:

>> Please fix the subject before committing.  A word seems to be missing
>> after “internal”.  Thanks.
>
> Is the below version more acceptable?
>
> "y2038: Introduce struct __timespec64 - new internal glibc type"

Yes, looks fine to me.
Lukasz Majewski Sept. 25, 2019, 1:34 p.m. UTC | #9 
Hi Florian,

> * Lukasz Majewski:
> 
> >> I think it's generally for reviewers to say if their view is "I
> >> think this patch is OK but we should allow more time for other
> >> people to comment", rather than expecting patch contributors to
> >> judge when they need to wait further after a patch approval.  
> >
> > Yes. I do understand.
> >
> > If I may ask - what is the "acceptable" time for other people from
> > community to jump in and comment the patch before it shall be
> > applied?
> >
> > Is it one week or more/less ?  
> 
> A week is more than enough, especially for patches that only touch
> internals like this one.

Thanks for clarification.

> 
> Regarding the actual patch, I don't understand why tv_pad isn't an
> *anonymous* bit field. 

The reason for this is that we may need to clear this padding if we
plan to fix some issues - for example in kernel 5.1.0 - 5.1.4 there is
a bug for x32 which may require explicit clearing the padding.

> This seems to introduce unnecessary variance
> between architectures and is incompatible with how glibc itself uses
> struct timespec. 

The v3 of this patch had this field defined as anonymous padding.
However, there was strong objection for such approach [1].

> It's also inconsistent with the new comment in
> include/time.h (named padding is only needed if you need to
> zero-initialize the padding).

As explained above - some archs/kernels may require this named padding
for fixes.


Links:
[1] - https://sourceware.org/ml/libc-alpha/2019-05/msg00151.html

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
Florian Weimer Sept. 25, 2019, 1:40 p.m. UTC | #10 
* Lukasz Majewski:

>> Regarding the actual patch, I don't understand why tv_pad isn't an
>> *anonymous* bit field. 
>
> The reason for this is that we may need to clear this padding if we
> plan to fix some issues - for example in kernel 5.1.0 - 5.1.4 there is
> a bug for x32 which may require explicit clearing the padding.

I think we cannot support those kernels with reasonable effort.  So
cross-architecture source compatibility with existing practices is
more important.

Furthermore, I think the consensus for the public struct timespec64 is
that it should use an unnamed bitfield because of the prevalence of
incorrect (according to POSIX) initializers.

>> This seems to introduce unnecessary variance
>> between architectures and is incompatible with how glibc itself uses
>> struct timespec. 
>
> The v3 of this patch had this field defined as anonymous padding.
> However, there was strong objection for such approach [1].

> Links:
> [1] - https://sourceware.org/ml/libc-alpha/2019-05/msg00151.html

The patch has a named bitfield:

+  int tv_pad: 32;            /* Padding named for checking/setting */

As far as I can see, the discussion was about what was actually in the
patch, and not an unnamed bitfield.
Lukasz Majewski Sept. 25, 2019, 2:38 p.m. UTC | #11 
Hi Florian,

> * Lukasz Majewski:
> 
> >> Regarding the actual patch, I don't understand why tv_pad isn't an
> >> *anonymous* bit field.   
> >
> > The reason for this is that we may need to clear this padding if we
> > plan to fix some issues - for example in kernel 5.1.0 - 5.1.4 there
> > is a bug for x32 which may require explicit clearing the padding.  
> 
> I think we cannot support those kernels with reasonable effort.  So
> cross-architecture source compatibility with existing practices is
> more important.

I see your point.

> 
> Furthermore, I think the consensus for the public struct timespec64 is
> that it should use an unnamed bitfield because of the prevalence of
> incorrect (according to POSIX) initializers.

Yes. Correct.

> 
> >> This seems to introduce unnecessary variance
> >> between architectures and is incompatible with how glibc itself
> >> uses struct timespec.   
> >
> > The v3 of this patch had this field defined as anonymous padding.
> > However, there was strong objection for such approach [1].  
> 
> > Links:
> > [1] - https://sourceware.org/ml/libc-alpha/2019-05/msg00151.html  
> 
> The patch has a named bitfield:
> 
> +  int tv_pad: 32;            /* Padding named for checking/setting */
> 

Ach. Yes indeed - there was a _named_ bitfield in the patch. 

> As far as I can see, the discussion was about what was actually in the
> patch, and not an unnamed bitfield.

Yes, you seems to be right. Andreas objection was about the bitfield
usage.


Let's wait for Joseph's opinion if we shall replace named padding struct
members with unnamed bitfields (and drop potential support for fixing
issues, which would require explicit clearing of padding before passing
data to the kernel). 


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. 25, 2019, 4:28 p.m. UTC | #12 
On Wed, 25 Sep 2019, Lukasz Majewski wrote:

> Let's wait for Joseph's opinion if we shall replace named padding struct
> members with unnamed bitfields (and drop potential support for fixing
> issues, which would require explicit clearing of padding before passing
> data to the kernel). 

I'm not particularly concerned with whether this patch uses named padding, 
unnamed bit-field or 64-bit tv_nsec in the internal struct __timespec64.  
(I don't think a named bit-field would make sense there, however.)  If we 
use an unnamed bit-field now we can readily change it later to have a name 
if we decide to support clearing padding for compat syscalls for kernels 
5.1.0 to 5.1.4.
Lukasz Majewski Sept. 25, 2019, 8:03 p.m. UTC | #13 
Hi Joseph, Florian,

> On Wed, 25 Sep 2019, Lukasz Majewski wrote:
> 
> > Let's wait for Joseph's opinion if we shall replace named padding
> > struct members with unnamed bitfields (and drop potential support
> > for fixing issues, which would require explicit clearing of padding
> > before passing data to the kernel).   
> 
> I'm not particularly concerned with whether this patch uses named
> padding, unnamed bit-field or 64-bit tv_nsec in the internal struct
> __timespec64. (I don't think a named bit-field would make sense
> there, however.)  If we use an unnamed bit-field now we can readily
> change it later to have a name if we decide to support clearing
> padding for compat syscalls for kernels 5.1.0 to 5.1.4.
> 

Shall I assume that the consensus is to use unnamed bit-field to pad
the tv_nsec?


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
diff mbox

Patch

diff --git a/include/time.h b/include/time.h
index dcf91855ad..9792948744 100644
--- a/include/time.h
+++ b/include/time.h
@@ -5,6 +5,7 @@ 
 # include <bits/types/locale_t.h>
 # include <stdbool.h>
 # include <time/mktime-internal.h>
+# include <endian.h>
 
 extern __typeof (strftime_l) __strftime_l;
 libc_hidden_proto (__strftime_l)
@@ -49,6 +50,29 @@  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 __TIMESIZE == 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
+
 #if __TIMESIZE == 64
 # define __ctime64 ctime
 #else