| Message ID | 20190327085210.22019-2-lukma@denx.de |
|---|---|
| State | New |
| Headers | show |
| Series | y2038: clock_settime rework to be Y2038 safe on 32bit systems | expand |
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.)
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
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.
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
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.
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.
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).
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.
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).
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.
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
>> 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.
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
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
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
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).
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
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