[RFC,1/7] y2038: Introduce struct __timespec64
diff mbox series

Message ID 20190327085210.22019-2-lukma@denx.de
State New
Headers show
Series
  • y2038: clock_settime rework to be Y2038 safe on 32bit systems
Related show

Commit Message

Lukasz Majewski March 27, 2019, 8:52 a.m. UTC
This type is a glibc's 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 between user
code and Y2038-proof kernel syscalls (e.g. clock_gettime()).

On 64-bit architectures, and on X32, struct __timespec64 is
just an alias of struct timespec, which is already 64-bit.
On other architectures, it must be explicitly defined.

When passing this structure to the kernel - it ensures that the
higher half of tv_nsec is always 0, which means that glibc
can reuse the public type (as tv_pad is zeroed anyway).

Moreover, the tv_nsec has an anonymous 32 bit padding (ordered
according to the endianness of the architecture) to prevent user space
programs, depending on long, 32 bit, tv_nsec from breaking.

Tested on x86_64 and ARM.

* time/bits/types/struct___timespec64.h:
  Create new file time/bits/types/struct___timespec64.h
* include/time.h: Add # include <time/bits/types/struct___timespec64.h>
* time/Makefile:
  Add bits/types/struct___timespec64.h to Makefile's headers
---
 include/time.h                        |  1 +
 time/Makefile                         |  2 +-
 time/bits/types/struct___timespec64.h | 39 +++++++++++++++++++++++++++++++++++
 3 files changed, 41 insertions(+), 1 deletion(-)
 create mode 100644 time/bits/types/struct___timespec64.h

Comments

Joseph Myers March 27, 2019, 1:41 p.m. UTC | #1
On Wed, 27 Mar 2019, Lukasz Majewski wrote:

> This type is a glibc's 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 between user
> code and Y2038-proof kernel syscalls (e.g. clock_gettime()).

Could you please give a detailed explanation of why this needs to go in an 
installed header, as opposed to purely in a glibc-internal header?  (All 
bits/ headers should be installed headers; the set of installed headers is 
determined by the "headers" settings in makefiles.  Since you're not 
actually including this from the public time.h in this patch, only the 
internal one, there doesn't seem to be a reason here for either the naming 
or installing the header.)
Lukasz Majewski March 27, 2019, 3:13 p.m. UTC | #2
Hi Joseph,

Thanks for a prompt response.

> On Wed, 27 Mar 2019, Lukasz Majewski wrote:
> 
> > This type is a glibc's 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 between user
> > code and Y2038-proof kernel syscalls (e.g. clock_gettime()).  
> 
> Could you please give a detailed explanation of why this needs to go
> in an installed header, as opposed to purely in a glibc-internal
> header? 

For Y2038 safe code on 32 bit machines we do need 64 bit representation
for some legacy data structures: time_t, struct timespec, etc.

Linux kernel (5.0-rc2) is using (expecting):
struct __kernel_timespec {
	__kernel_time64_t tv_sec; /* seconds */
	long long tv_nsec; /* nanoseconds */
};

in its syscalls.

With glibc I could introduce an "internal"  struct timespec64 (with 64
bit tv_sec field) which would match the above kernel definition and
"installed" one which would be posix compliant (to be still long - 32
bits on 32 bits systems).

Instead - only one ("installed") struct __timespec64 has been
introduced to be also used as glibc internal and OS installed
one to be used by user space programs. 

> (All bits/ headers should be installed headers; the set of
> installed headers is determined by the "headers" settings in
> makefiles.

Yes. The struct___timespec64.h has been added to "headers" and on
purpose made "installed".

>  Since you're not actually including this from the public
> time.h in this patch, only the internal one, there doesn't seem to be
> a reason here for either the naming or installing the header.)
> 

The idea with this exported/installed struct __timespec64 is as
follows:

User space programs on 32 bit machines use struct timespec, which when
one passes -D_TIME_BITS==64 -D_FILE_OFFSET_BITS==64 during compilation
is replaced by struct __timespec64, which correctly handles time after
Y2038.
Then glibc functions work on struct __timespec64 and provide results to
user space program.



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 March 27, 2019, 5:04 p.m. UTC | #3
On Wed, 27 Mar 2019, Lukasz Majewski wrote:

> User space programs on 32 bit machines use struct timespec, which when
> one passes -D_TIME_BITS==64 -D_FILE_OFFSET_BITS==64 during compilation
> is replaced by struct __timespec64, which correctly handles time after
> Y2038.
> Then glibc functions work on struct __timespec64 and provide results to
> user space program.

By comparison, for _FILE_OFFSET_BITS=64 the headers e.g. continue to 
define "struct stat", but with different contents in that case.  So it 
would be possible to do the same with "struct timespec" rather than 
defining it to __timespec64.

There are arguments either way (e.g. defining timespec to __timespec64 
results in different C++ name mangling when _TIME_BITS=64 is used, which 
may be a good idea).  But given the question recently raised about whether 
__time64_t belongs in the installed headers, there needs to be a careful 
justification given for the particular approach chosen, whichever approach 
it ends up being.
Lukasz Majewski March 28, 2019, 7:24 a.m. UTC | #4
Hi Joseph,

> On Wed, 27 Mar 2019, Lukasz Majewski wrote:
> 
> > User space programs on 32 bit machines use struct timespec, which
> > when one passes -D_TIME_BITS==64 -D_FILE_OFFSET_BITS==64 during
> > compilation is replaced by struct __timespec64, which correctly
> > handles time after Y2038.
> > Then glibc functions work on struct __timespec64 and provide
> > results to user space program.  
> 
> By comparison, for _FILE_OFFSET_BITS=64 the headers e.g. continue to 
> define "struct stat", but with different contents in that case.  So
> it would be possible to do the same with "struct timespec" rather
> than defining it to __timespec64.

Ok. I've looked into the ./bits/stat.h source code:

#ifndef __USE_FILE_OFFSET64
    __ino_t st_ino;             /* File serial number.  */
#else
    __ino64_t st_ino;           /* File serial number.  */
#endif

In this case explicit 64 bit type is used (__ino64_t).

On important information from the above - it is allowed to use
__USE_FILE_OFFSET64 (which corresponds to __USE_TIME_BITS64 for time).

> 
> There are arguments either way (e.g. defining timespec to
> __timespec64 results in different C++ name mangling when
> _TIME_BITS=64 is used, which may be a good idea).  But given the
> question recently raised about whether __time64_t belongs in the
> installed headers, there needs to be a careful justification given
> for the particular approach chosen, whichever approach it ends up
> being.
> 

I will state more details about this problem in the other mail - one
related to Paul's mktime rework (and hiding __time64_t to be only
glibc/gnulib private 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
Joseph Myers March 28, 2019, 4:19 p.m. UTC | #5
On Thu, 28 Mar 2019, Lukasz Majewski wrote:

> Ok. I've looked into the ./bits/stat.h source code:
> 
> #ifndef __USE_FILE_OFFSET64
>     __ino_t st_ino;             /* File serial number.  */
> #else
>     __ino64_t st_ino;           /* File serial number.  */
> #endif
> 
> In this case explicit 64 bit type is used (__ino64_t).

Yes, consistency with that is an argument for having __time64_t in the 
installed headers (__ino_t and __ino64_t always exist, and their types 
don't depend on _FILE_OFFSET_BITS, but which one is used to define ino_t 
does depend on _FILE_OFFSET_BITS - so consistency with that approach would 
suggest __time64_t in installed headers even if no __timespec64 in 
installed headers, with __time_t continuing to have the same type it has 
at present).

There should not, however, be a public interface time64_t (or 
corresponding non-reserved function names), to avoid excess complexity 
around having explicit 64-bit-time public interfaces for each function as 
well as the _TIME_BITS=64 interfaces.
Paul Eggert March 28, 2019, 4:35 p.m. UTC | #6
On 3/28/19 9:19 AM, Joseph Myers wrote:
>> In this case explicit 64 bit type is used (__ino64_t).
> Yes, consistency with that is an argument for having __time64_t in the 
> installed headers

But the two cases are not consistent. Glibc exposes the type ino64_t to
user code, because ino_t is part of the file API and Glibc attempts to
support mixed-mode 32-bit programs that use both 32-bit ino_t and 64-bit
ino64_t in the same program.

> There should not, however, be a public interface time64_t
Yes, and that's why the two cases are not consistent. We're planning to
do time_t differently: we are not attempting to support mixed-mode user
code, so there is no need to export either __time64_t or time64_t to
user code.
Joseph Myers March 28, 2019, 4:39 p.m. UTC | #7
On Thu, 28 Mar 2019, Paul Eggert wrote:

> > There should not, however, be a public interface time64_t
> Yes, and that's why the two cases are not consistent. We're planning to
> do time_t differently: we are not attempting to support mixed-mode user
> code, so there is no need to export either __time64_t or time64_t to
> user code.

I'm not convinced that the absence of one set of interfaces (public time64 
functions, time64_t, etc.) is a reason for inconsistency relating to a 
piece of internals that is present in both cases (__time_t, __off_t, 
etc.).  I think having __time_t handled differently from __off_t in that 
regard (by making the definition depend on _TIME_BITS, rather than only 
having the definition of time_t depend on _TIME_BITS) would be liable to 
confuse people reading glibc headers.

It's more plausible that the headers should not define __time_t at all in 
the _TIME_BITS=64 case (if there are no interfaces that would use it).
Paul Eggert March 28, 2019, 5:07 p.m. UTC | #8
On 3/28/19 9:39 AM, Joseph Myers wrote:
>
> having __time_t handled differently from __off_t in that 
> regard (by making the definition depend on _TIME_BITS, rather than only 
> having the definition of time_t depend on _TIME_BITS) would be liable to 
> confuse people reading glibc headers.

It depends on what sort of confusion is more likely to cause trouble.
I'm worried about confusion in user code mistakenly using __time64_t -
something that *will* happen, I'm afraid (we've seen that with
__off64_t). The confusion you're worried about would be limited to
people reading glibc source code (i.e., code not installed under
/usr/include) and so it is considerably more limited in scope and can be
addressed by a comment as needed.


> It's more plausible that the headers should not define __time_t at all in 
> the _TIME_BITS=64 case (if there are no interfaces that would use it).
>
Several installed .h files use __time_t now, e.g., <bits/shm.h>. Are you
suggesting that it's plausible to change them to use time_t instead?
Something like this should work, yes. But then when _TIME_BITS=64 public
headers won't need to define __time64_t either, for the same reason they
won't need to define __time_t. I.e., if _TIME_BITS=64 then neither
__time_t nor __time64_t would be defined.

That would be OK for me (as I intend to use _TIME_BITS=64 in all my
apps, and by 2038 _TIME_BITS=64 will be the default). However, I'm still
puzzled as to why glibc should expose __time_t and __time64_t to
_TIME_BITS!=64 user code, given that we don't want to support mixed-mode
user code.
Joseph Myers March 28, 2019, 5:25 p.m. UTC | #9
On Thu, 28 Mar 2019, Paul Eggert wrote:

> > It's more plausible that the headers should not define __time_t at all in 
> > the _TIME_BITS=64 case (if there are no interfaces that would use it).
> >
> Several installed .h files use __time_t now, e.g., <bits/shm.h>. Are you
> suggesting that it's plausible to change them to use time_t instead?

I'm suggesting that such structures will end up having contents that 
depend on _TIME_BITS.  What is the Linux kernel using as the 64-bit time 
version of shmid_ds?  Note that the existing padding around the time 
fields there (where present) does *not* generally correspond to 
endianness, i.e. a separate structure layout is needed.

It may well be the case that these headers can and should use the public 
*_t types more.  Historically POSIX didn't always require the *_t types to 
be defined in all headers which had interfaces using them, but they are 
always reserved in POSIX so it's OK to define them anyway in such headers, 
and POSIX has moved towards generally requiring the types to be defined in 
such cases.

> That would be OK for me (as I intend to use _TIME_BITS=64 in all my
> apps, and by 2038 _TIME_BITS=64 will be the default). However, I'm still
> puzzled as to why glibc should expose __time_t and __time64_t to
> _TIME_BITS!=64 user code, given that we don't want to support mixed-mode
> user code.

I don't think we should be particularly concerned by what __* names are 
visible to user code; the visibility of such names is simply a side-effect 
of how the C language works.  They should be present, or not, however is 
convenient for implementing and maintaining the installed headers (which 
includes keeping those headers consistent between different *_t typedefs 
in most cases).
Paul Eggert March 28, 2019, 5:40 p.m. UTC | #10
On 3/28/19 10:25 AM, Joseph Myers wrote:
>
> I don't think we should be particularly concerned by what __* names are 
> visible to user code; the visibility of such names is simply a side-effect 
> of how the C language works.

Hmm, well, our opinions differ, as I've seen too many people abusing the
C-language rules and would rather avoid these problems when it's easy,
as it is here.

However, as long as __time64_t and __time_t are not visible to user code
when _TIME_BITS=64, this will address most of my own practical concerns
and should be OK as a compromise.

> They should be present, or not, however is 
> convenient for implementing and maintaining the installed headers (which 
> includes keeping those headers consistent between different *_t typedefs 
> in most cases).
My impression is that it will be more convenient to put __time64_t in a
private header, to help remind maintainers that that the situation with
time_t is different, as there is no intent to support mixed-mode user
code with time_t. But I understand if your opinion differs.
Lukasz Majewski March 30, 2019, 2:58 p.m. UTC | #11
Hi Paul, Joseph,

Thank you for your feedback.

> On 3/28/19 10:25 AM, Joseph Myers wrote:
> >
> > I don't think we should be particularly concerned by what __* names
> > are visible to user code; the visibility of such names is simply a
> > side-effect of how the C language works.  
> 
> Hmm, well, our opinions differ, as I've seen too many people abusing
> the C-language rules and would rather avoid these problems when it's
> easy, as it is here.
> 
> However, as long as __time64_t and __time_t are not visible to user
> code when _TIME_BITS=64, this will address most of my own practical
> concerns and should be OK as a compromise.

To sum up:
----------

1. __time64_t and __time_t are "exported" (visible in /usr/include/*)
always (similar to __off_t and __off64_t).

2. The "exported" struct timespec would look like:

struct timespec
{
/* Use the original definition for 64-bit arches
   or when 64-bit-time by default has *not* been requested */
#if __WORDSIZE > 32 || ! defined(__USE_TIME_BITS64)
  __time_t tv_sec; /* Seconds. */
#else
  __time64_t tv_sec;
#end
  __syscall_slong_t tv_nsec;
};

The same code would be use with e.g. struct timeval

3. Put the internal struct __timespec64

struct __timespec64
{
  __time64_t tv_sec;
  __int64_t tv_nsec;
}

into include/time.h and don't introduce separate
include/bits/types/struct___timespec64.h 

The same idea would be applied to struct timeval

4. Syscalls conversion - starting with Linux only - performed as
described in: "D.2.1 64-bit time symbol handling in the GNU C Library"
https://www.gnu.org/software/libc/manual/html_mono/libc.html

(As done with clock_settime() in this RFC patch set).

5. Add some glibc tests (which could be also easily adapted to test
after Y2038 scenario) after converting some calls (like e.g.
clock_{settime|gettime|nanosleep|getres,etc}).

> 
> > They should be present, or not, however is 
> > convenient for implementing and maintaining the installed headers
> > (which includes keeping those headers consistent between different
> > *_t typedefs in most cases).  
> My impression is that it will be more convenient to put __time64_t in
> a private header, to help remind maintainers that that the situation
> with time_t is different, as there is no intent to support mixed-mode
> user code with time_t. But I understand if your opinion differs.
> 




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
Paul Eggert March 30, 2019, 4:24 p.m. UTC | #12
>> However, as long as __time64_t and __time_t are not visible to user
>> code when _TIME_BITS=64, this will address most of my own practical
>> concerns and should be OK as a compromise.
> 
> To sum up:
> ----------
> 
> 1. __time64_t and __time_t are "exported" (visible in /usr/include/*)
> always (similar to __off_t and __off64_t).

No, the compromise is that these types are "exported" only when _TIME_BITS<64. 
Exporting them when _TIME_BITS==64 is unnecessary and confusing. (It's also 
unnecessary and confusing when _TIME_BITS<64, but I'm willing to compromise 
there because the _TIME_BITS<64 case is obsolescent and will eventually go away.)

> 2. The "exported" struct timespec would look like:
> 
> struct timespec
> {
> /* Use the original definition for 64-bit arches
>     or when 64-bit-time by default has *not* been requested */
> #if __WORDSIZE > 32 || ! defined(__USE_TIME_BITS64)
>    __time_t tv_sec; /* Seconds. */
> #else
>    __time64_t tv_sec;
> #end
>    __syscall_slong_t tv_nsec;
> };

Sorry, I'm not following this. Why can't this be the natural 'struct timespec { 
time_t tv_sec; ...; }'? The two forms are equivalent, regardless of whether 
__WORDSIZE > 32 || ! defined(__USE_TIME_BITS64) and regardless of what we do 
about (1), so why use the more-complicated and more-confusing definition?

As Joseph mentioned, there may need to be ifdeffery around tv_nsec due to the 
issue of endian-dependent padding around tv_nsec. However, there does not need 
to be ifdeffery around tv_sec. Just use time_t there; problem solved.

> 3. Put the internal struct __timespec64 ...
> into include/time.h and don't introduce separate
> include/bits/types/struct___timespec64.h
> 
> The same idea would be applied to struct timeval

Yes.
Lukasz Majewski April 1, 2019, 5:30 a.m. UTC | #13
Hi Paul,

> >> However, as long as __time64_t and __time_t are not visible to user
> >> code when _TIME_BITS=64, this will address most of my own practical
> >> concerns and should be OK as a compromise.  
> > 
> > To sum up:
> > ----------
> > 
> > 1. __time64_t and __time_t are "exported" (visible
> > in /usr/include/*) always (similar to __off_t and __off64_t).  
> 
> No, the compromise is that these types are "exported" only when
> _TIME_BITS<64. Exporting them when _TIME_BITS==64 is unnecessary and
> confusing. (It's also unnecessary and confusing when _TIME_BITS<64,
> but I'm willing to compromise there because the _TIME_BITS<64 case is
> obsolescent and will eventually go away.)

Ok. As __time64_t has been "hidden" in your previous mktime patch set,
I suppose that this would be changed in the next version?

> 
> > 2. The "exported" struct timespec would look like:
> > 
> > struct timespec
> > {
> > /* Use the original definition for 64-bit arches
> >     or when 64-bit-time by default has *not* been requested */
> > #if __WORDSIZE > 32 || ! defined(__USE_TIME_BITS64)
> >    __time_t tv_sec; /* Seconds. */
> > #else
> >    __time64_t tv_sec;
> > #end
> >    __syscall_slong_t tv_nsec;
> > };  
> 
> Sorry, I'm not following this. Why can't this be the natural 'struct
> timespec { time_t tv_sec; ...; }'? The two forms are equivalent,
> regardless of whether __WORDSIZE > 32 || ! defined(__USE_TIME_BITS64)
> and regardless of what we do about (1), so why use the
> more-complicated and more-confusing definition?

Currently there is __time_t tv_sec, so I've followed the pattern.

I don't mind to change it to time_t tv_sec.

> 
> As Joseph mentioned, there may need to be ifdeffery around tv_nsec
> due to the issue of endian-dependent padding around tv_nsec. 

I thought that we may only be concerned about endiannes on the
"internal" (i.e. struct __timespec64) representation, which would be
passed as the argument to Linux syscalls.

> However,
> there does not need to be ifdeffery around tv_sec. Just use time_t
> there; problem solved.

Ok.

> 
> > 3. Put the internal struct __timespec64 ...
> > into include/time.h and don't introduce separate
> > include/bits/types/struct___timespec64.h
> > 
> > The same idea would be applied to struct timeval  
> 
> Yes.


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
Lukasz Majewski April 10, 2019, 1:05 p.m. UTC | #14
Hi Paul, Joseph,

> >> However, as long as __time64_t and __time_t are not visible to user
> >> code when _TIME_BITS=64, this will address most of my own practical
> >> concerns and should be OK as a compromise.  
> > 
> > To sum up:
> > ----------
> > 
> > 1. __time64_t and __time_t are "exported" (visible
> > in /usr/include/*) always (similar to __off_t and __off64_t).  
> 
> No, the compromise is that these types are "exported" only when
> _TIME_BITS<64. Exporting them when _TIME_BITS==64 is unnecessary and
> confusing. (It's also unnecessary and confusing when _TIME_BITS<64,
> but I'm willing to compromise there because the _TIME_BITS<64 case is
> obsolescent and will eventually go away.)
> 
> > 2. The "exported" struct timespec would look like:
> > 
> > struct timespec
> > {
> > /* Use the original definition for 64-bit arches
> >     or when 64-bit-time by default has *not* been requested */
> > #if __WORDSIZE > 32 || ! defined(__USE_TIME_BITS64)
> >    __time_t tv_sec; /* Seconds. */
> > #else
> >    __time64_t tv_sec;
> > #end
> >    __syscall_slong_t tv_nsec;
> > };  
> 
> Sorry, I'm not following this. Why can't this be the natural 'struct
> timespec { time_t tv_sec; ...; }'? The two forms are equivalent,
> regardless of whether __WORDSIZE > 32 || ! defined(__USE_TIME_BITS64)
> and regardless of what we do about (1), so why use the
> more-complicated and more-confusing definition?

Ok, time_t shall be used for tv_sec.

> 
> As Joseph mentioned, there may need to be ifdeffery around tv_nsec
> due to the issue of endian-dependent padding around tv_nsec.

It is not only the padding. 
The problem is with the size of this struct passed to the kernel as
syscall.

1. __TIMESIZE = 64 -> No problem with kernel ABI
	- sizeof(tv_sec) = 64 bit
	- sizeof(tv_nsec) = 64 bit

	Kernel looks for tv_sec and tv_nsec, each 64 bit

2. __TIMESIZE = 32 (no, Y2038 support) ->  No problem with kernel ABI
	- sizeof(tv_sec) = 32 bit
	- sizeof(tv_nsec) = 32 bit

	Kernel looks for tv_sec and tv_nsec, each 32 bit (those
	syscalls would be removed from the kernel)

3. __TIMESIZE = 32 (Y2038 support) -> ABI problem 
	- sizeof(tv_sec) = 64 bit (time_t)
	- sizeof(tv_nsec) = 32 bit
	- needed padding (tv_pad) 32 bit

	Kernel expects tv_sec and tv_nsec to be both 64 bits.

Exported (installed at /usr/include) struct timespec (to work in above
three cases):

struct timespec
 {
   time_t tv_sec;   /* Seconds.  */
#ifdef __USE_TIME_BITS64
# if BYTE_ORDER == BIG_ENDIAN
   int tv_pad: 32;  /* Padding named for checking/setting */
   __syscall_slong_t tv_nsec; /* Nanoseconds */ 
# else
   __syscall_slong_t tv_nsec; /* Nanoseconds */
   int tv_pad: 32;  /* Padding named for checking/setting */ 
# endif
#else
   __syscall_slong_t tv_nsec;	/* Nanoseconds.  */
#endif
 };


and internal (in ./include/time.h) definition of struct __timespec64:

#if __TIMESIZE == 64
# define __timespec64 timespec
#else
struct __timespec64
{
	__time64_t tv_sec;         /* Seconds */
	__time64_t tv_nsec;         /* Nanoseconds */
};
#endif



> However,
> there does not need to be ifdeffery around tv_sec. Just use time_t
> there; problem solved.
> 
> > 3. Put the internal struct __timespec64 ...
> > into include/time.h and don't introduce separate
> > include/bits/types/struct___timespec64.h
> > 
> > The same idea would be applied to struct timeval  
> 
> Yes.




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
Lukasz Majewski April 11, 2019, 6:39 a.m. UTC | #15
On Wed, 10 Apr 2019 15:05:27 +0200
Lukasz Majewski <lukma@denx.de> wrote:

> Hi Paul, Joseph,
> 
> > >> However, as long as __time64_t and __time_t are not visible to
> > >> user code when _TIME_BITS=64, this will address most of my own
> > >> practical concerns and should be OK as a compromise.    
> > > 
> > > To sum up:
> > > ----------
> > > 
> > > 1. __time64_t and __time_t are "exported" (visible
> > > in /usr/include/*) always (similar to __off_t and __off64_t).    
> > 
> > No, the compromise is that these types are "exported" only when
> > _TIME_BITS<64. Exporting them when _TIME_BITS==64 is unnecessary and
> > confusing. (It's also unnecessary and confusing when _TIME_BITS<64,
> > but I'm willing to compromise there because the _TIME_BITS<64 case
> > is obsolescent and will eventually go away.)
> >   
> > > 2. The "exported" struct timespec would look like:
> > > 
> > > struct timespec
> > > {
> > > /* Use the original definition for 64-bit arches
> > >     or when 64-bit-time by default has *not* been requested */
> > > #if __WORDSIZE > 32 || ! defined(__USE_TIME_BITS64)
> > >    __time_t tv_sec; /* Seconds. */
> > > #else
> > >    __time64_t tv_sec;
> > > #end
> > >    __syscall_slong_t tv_nsec;
> > > };    
> > 
> > Sorry, I'm not following this. Why can't this be the natural 'struct
> > timespec { time_t tv_sec; ...; }'? The two forms are equivalent,
> > regardless of whether __WORDSIZE > 32 || !
> > defined(__USE_TIME_BITS64) and regardless of what we do about (1),
> > so why use the more-complicated and more-confusing definition?  
> 
> Ok, time_t shall be used for tv_sec.
> 
> > 
> > As Joseph mentioned, there may need to be ifdeffery around tv_nsec
> > due to the issue of endian-dependent padding around tv_nsec.  
> 
> It is not only the padding. 
> The problem is with the size of this struct passed to the kernel as
> syscall.
> 
> 1. __TIMESIZE = 64 -> No problem with kernel ABI
> 	- sizeof(tv_sec) = 64 bit
> 	- sizeof(tv_nsec) = 64 bit
> 
> 	Kernel looks for tv_sec and tv_nsec, each 64 bit
> 
> 2. __TIMESIZE = 32 (no, Y2038 support) ->  No problem with kernel ABI
> 	- sizeof(tv_sec) = 32 bit
> 	- sizeof(tv_nsec) = 32 bit
> 
> 	Kernel looks for tv_sec and tv_nsec, each 32 bit (those
> 	syscalls would be removed from the kernel)
> 
> 3. __TIMESIZE = 32 (Y2038 support) -> ABI problem 
> 	- sizeof(tv_sec) = 64 bit (time_t)
> 	- sizeof(tv_nsec) = 32 bit
> 	- needed padding (tv_pad) 32 bit
> 
> 	Kernel expects tv_sec and tv_nsec to be both 64 bits.
> 
> Exported (installed at /usr/include) struct timespec (to work in above
> three cases):
> 
> struct timespec
>  {
>    time_t tv_sec;   /* Seconds.  */
> #ifdef __USE_TIME_BITS64
> # if BYTE_ORDER == BIG_ENDIAN
>    int tv_pad: 32;  /* Padding named for checking/setting */
>    __syscall_slong_t tv_nsec; /* Nanoseconds */ 
> # else
>    __syscall_slong_t tv_nsec; /* Nanoseconds */
>    int tv_pad: 32;  /* Padding named for checking/setting */ 
> # endif
> #else
>    __syscall_slong_t tv_nsec;	/* Nanoseconds.  */
> #endif
>  };
> 
> 
> and internal (in ./include/time.h) definition of struct __timespec64:
> 
> #if __TIMESIZE == 64
> # define __timespec64 timespec
> #else
> struct __timespec64
> {
> 	__time64_t tv_sec;         /* Seconds */
> 	__time64_t tv_nsec;         /* Nanoseconds */
> };
> #endif
> 

I've tested it a bit more and the internal representation also needs
padding to be safely passed to Linux kernel.

Hence, it would look like:

struct __timespec64
{
	__time64_t tv_sec;         /* Seconds */
# if BYTE_ORDER == BIG_ENDIAN
	int tv_pad: 32;            /* Padding named for
	__int32_t tv_nsec;         /* Nanoseconds */
# else
	__int32_t tv_nsec;         /* Nanoseconds */
	int tv_pad: 32;            /* Padding named for
# endif
};
#endif


> 
> 
> > However,
> > there does not need to be ifdeffery around tv_sec. Just use time_t
> > there; problem solved.
> >   
> > > 3. Put the internal struct __timespec64 ...
> > > into include/time.h and don't introduce separate
> > > include/bits/types/struct___timespec64.h
> > > 
> > > The same idea would be applied to struct timeval    
> > 
> > Yes.  
> 
> 
> 
> 
> 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




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 April 17, 2019, 9:42 p.m. UTC | #16
On Wed, 10 Apr 2019, Lukasz Majewski wrote:

> Exported (installed at /usr/include) struct timespec (to work in above
> three cases):
> 
> struct timespec
>  {
>    time_t tv_sec;   /* Seconds.  */
> #ifdef __USE_TIME_BITS64
> # if BYTE_ORDER == BIG_ENDIAN

BYTE_ORDER and BIG_ENDIAN are in the user's namespace.  You have to use 
implementation-namespace macros in such conditionals in installed headers.

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

No.  This *must* be unnamed in the installed header, so that initializers 
in user code { seconds, nanoseconds } continue to work as expected (even 
if not formally required to work by the standards, I think it's clear we 
should keep code with such initializers working).
Lukasz Majewski April 18, 2019, 5:53 a.m. UTC | #17
Hi Joseph,

> On Wed, 10 Apr 2019, Lukasz Majewski wrote:
> 
> > Exported (installed at /usr/include) struct timespec (to work in
> > above three cases):
> > 
> > struct timespec
> >  {
> >    time_t tv_sec;   /* Seconds.  */
> > #ifdef __USE_TIME_BITS64
> > # if BYTE_ORDER == BIG_ENDIAN  
> 
> BYTE_ORDER and BIG_ENDIAN are in the user's namespace.  You have to
> use implementation-namespace macros in such conditionals in installed
> headers.

This patch has been dropped and superseded by new one:
https://patchwork.ozlabs.org/patch/1085395/

In the above patch the struct __timespec64 is only internal for glibc.
The padding is necessary to avoid passing some random data to kernel
(however, Linux kernel is clearing upper 32 bits of tv_nsec anyway).

The struct __timespec64 is not "installed" anymore - as suggested by
Paul it has been changed as:
https://github.com/lmajewski/y2038_glibc/commit/ea9e29e4d8dbde203325a16962e99e5a6891c741#diff-4ddbc47d3262d4f00f3825e4f3627dbb

In that way we would keep exported/installed struct timespec (as it is
now). 

> 
> >    int tv_pad: 32;  /* Padding named for checking/setting */  
> 
> No.  This *must* be unnamed in the installed header, so that
> initializers in user code { seconds, nanoseconds } continue to work
> as expected (even if not formally required to work by the standards,
> I think it's clear we should keep code with such initializers
> working).
> 




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

Patch
diff mbox series

diff --git a/include/time.h b/include/time.h
index ac3163c2a5..d8a3ef35e2 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 <time/bits/types/struct___timespec64.h>
 
 extern __typeof (strftime_l) __strftime_l;
 libc_hidden_proto (__strftime_l)
diff --git a/time/Makefile b/time/Makefile
index 5c6304ece1..dc9ad55678 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___timespec64.h b/time/bits/types/struct___timespec64.h
new file mode 100644
index 0000000000..9946717af6
--- /dev/null
+++ b/time/bits/types/struct___timespec64.h
@@ -0,0 +1,39 @@ 
+/* NB: Include guard matches what <linux/time.h> uses.  */
+/* #ifndef _STRUCT_TIMESPEC
+# error "Never include <bits/types/struct___timespec64.h> directly !"
+#endif */
+
+#ifndef _STRUCT___TIMESPEC64
+#define _STRUCT___TIMESPEC64 1
+
+#include <bits/types.h>
+#include <bits/timesize.h>
+#include <endian.h>
+
+/* The glibc Y2038-proof 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 bitfield, which should always be 0.
+   */
+#if __TIMESIZE==64
+# define __timespec64 timespec
+#else
+#define TIMSPEC64_TV_PAD_DEFINED
+# 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
+
+#endif