Message ID | 20200601140740.16371-1-lukma@denx.de |
---|---|
Headers | show |
Series | y2038: Convert timespec_{sub|add|create} in support to be Y2038 safe | expand |
On Mon, 1 Jun 2020, Lukasz Majewski wrote: > First timespec* functions are renamed to have common "__" prefix for internal > functions. This is a preparatory work for further conversion. Leading "__" is *only* needed when the name is used in contexts where it could conflict with a user identifier. For example, in installed headers or with external linkage. In particular, static inline functions in non-installed headers never need a leading "__". So there is no justification for renaming timespec_compare unless you plan to make it an extern, non-inline function, in which case you should say so explicitly in that patch's commit message. xclock_gettime is inherently unsuitable for use in installed libraries, because it exits (FAIL_EXIT1) on error, which is not suitable for library code. So there is no need to rename that function; any installed library code that uses it has to be fixed not to use it and instead to do appropriate error checks on the result of clock_gettime (returning an error from the caller if appropriate) itself; library code should almost never exit the process on error. Likewise xclock_now, because it calls xclock_gettime, must not be used in installed libraries. These function naming changes are only appropriate for external linkage functions whose semantics are appropriate for use in installed libraries and that are actually used in such libraries or that you intend to be used in such libraries. Please review all those changes to make sure that you don't rename functions for which such library use is not appropriate or not planned.
Hi Joseph, > On Mon, 1 Jun 2020, Lukasz Majewski wrote: > > > First timespec* functions are renamed to have common "__" prefix > > for internal functions. This is a preparatory work for further > > conversion. > > Leading "__" is *only* needed when the name is used in contexts where > it could conflict with a user identifier. For example, in installed > headers or with external linkage. > > In particular, static inline functions in non-installed headers never > need a leading "__". So there is no justification for renaming > timespec_compare unless you plan to make it an extern, non-inline > function, in which case you should say so explicitly in that patch's > commit message. > > xclock_gettime is inherently unsuitable for use in installed > libraries, because it exits (FAIL_EXIT1) on error, which is not > suitable for library code. So there is no need to rename that > function; any installed library code that uses it has to be fixed not > to use it and instead to do appropriate error checks on the result of > clock_gettime (returning an error from the caller if appropriate) > itself; library code should almost never exit the process on error. > Likewise xclock_now, because it calls xclock_gettime, must not be > used in installed libraries. > > These function naming changes are only appropriate for external > linkage functions whose semantics are appropriate for use in > installed libraries and that are actually used in such libraries or > that you intend to be used in such libraries. Please review all > those changes to make sure that you don't rename functions for which > such library use is not appropriate or not planned. > Thanks for very detailed explanation. Considering the above arguments - there is no point in converting timespec_* and xclock_* functions as those are only used internally in glibc - either as helper functions or for writing tests. I will drop patches 02-07. 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
Dear Community, I would like to bring up one relevant Y2038 support issue - to be more specific - the conversion/replacement of struct timespec to struct __timespec64 in glibc internal code (source tree). For example - I would like to convert nptl and pthreads to be Y2038 safe. To do that the timespec_* helpers and some functions, which use futex_time64 syscall, (from 5.1+) need to use struct __timespec64. The problem is with instant replacement of struct timespec with struct __timespec64 in glibc internal code (and tests). To do it I could: - Replace its occurences in relevant directories - like ./nptl or ./sysdeps/pthread - i.e. rename all occurrences in a single directory - Replace them in functions (tests) and use explicit conversion functions - like valid_timespec_to_timespec64() before passing struct __timespec64 arguments (like ones for futex_time64 for nptl). - Replace _all_ occurrences in glibc tree of struct timespec with struct __timespec64 at once with using sed on the glibc tree. The last option seems to be the most appealing as we already use __timespec64 (with its aliasing) for some core system syscalls (like clock_gettime). However, such patch shall be applied just after the release of new stable glibc version (August 2020?) to have time for potential fixes. Which options shall we use? A few more related questions: - Shall tests in ./nptl and ./sysdeps/pthread [*] use struct __timespec64 or struct timespec? From my understanding tests (like ./nptl/tst-*) use exported headers so struct timespec for them is struct __timespec64 anyway for archs with __WORDSIZE == 32 and __TIMESIZE !=64. - Other time related structures needs to be converted as well - like struct itimerspec. [*] - from running scripts/build-many-glibcs.py it looks like only i686-gnu port (HURD) is using code in ./sysdeps/pthread. Will ./sysdeps/pthread be replaced by nptl in some near time in the future (and removed)? 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, 3 Jun 2020, Lukasz Majewski wrote: > To do it I could: > - Replace its occurences in relevant directories - like ./nptl or > ./sysdeps/pthread - i.e. rename all occurrences in a single directory > > - Replace them in functions (tests) and use explicit conversion > functions - like valid_timespec_to_timespec64() before passing struct > __timespec64 arguments (like ones for futex_time64 for nptl). > > - Replace _all_ occurrences in glibc tree of struct timespec with struct > __timespec64 at once with using sed on the glibc tree. Using sed obviously won't work, since external interfaces need different handling from purely internal uses. I think you need to change things bit by bit, in sufficiently small patches for convenient review. Regarding tests, I expect many tests of time-related interfaces should end up being built and run twice on systems that currently use 32-bit time, once to test the interfaces with 32-bit time and once to test the interfaces with 64-bit time. Also, tests can't generally use 64-bit time interfaces from libc until _TIME_BITS=64 support is actually implemented. So I think tests would be one of the last places to change (and similarly installed executables). Whereas any internal use of time in a function in the libraries that does not involve time in its interface can be updated more or less independently of any other such use, provided the relevant internal interfaces using 64-bit time are now available.
Hi Joseph, > On Wed, 3 Jun 2020, Lukasz Majewski wrote: > > > To do it I could: > > - Replace its occurences in relevant directories - like ./nptl or > > ./sysdeps/pthread - i.e. rename all occurrences in a single > > directory > > - Replace them in functions (tests) and use explicit conversion > > functions - like valid_timespec_to_timespec64() before passing > > struct __timespec64 arguments (like ones for futex_time64 for nptl). > > > > - Replace _all_ occurrences in glibc tree of struct timespec with > > struct __timespec64 at once with using sed on the glibc tree. > > Using sed obviously won't work, since external interfaces need > different handling from purely internal uses. I think you need to > change things bit by bit, in sufficiently small patches for > convenient review. Considering the above comment - it seems like it would be best to replace struct timespec with struct __timespec64 in directories - like ./nptl and omit tests from converison. > > Regarding tests, I expect many tests of time-related interfaces > should end up being built and run twice on systems that currently use > 32-bit time, once to test the interfaces with 32-bit time and once to > test the interfaces with 64-bit time. Also, tests can't generally > use 64-bit time interfaces from libc until _TIME_BITS=64 support is > actually implemented. So I think tests would be one of the last > places to change (and similarly installed executables). Whereas any > internal use of time in a function in the libraries that does not > involve time in its interface can be updated more or less > independently of any other such use, provided the relevant internal > interfaces using 64-bit time are now available. > 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
Hello, Lukasz Majewski, le mer. 03 juin 2020 14:53:47 +0200, a ecrit: > [*] - from running scripts/build-many-glibcs.py it looks like only > i686-gnu port (HURD) is using code in ./sysdeps/pthread. ? No, nptl also uses e.g. the C11 threads implementation that is there. > Will ./sysdeps/pthread be replaced by nptl in some near time in the > future (and removed)? Ideally nptl and htl (and fbtl of the freebsd port) would be merged. Apparently nptl is not as linux-specific as I could fear, and now that gnumach has a futex-like interface (gsync) possibly it could just be used, but it won't be just a snap, htl implements at least some cancel support that we need in userland filesystem support (pthread_hurd_cond_timedwait_np), and also the pthread cancel, signal, TLS supports need to be plugged. I tend to make htl interface like nptl with the libc, so that should be feasibly long-term. Samuel
Hi Joseph, > On Wed, 3 Jun 2020, Lukasz Majewski wrote: > > > To do it I could: > > - Replace its occurences in relevant directories - like ./nptl or > > ./sysdeps/pthread - i.e. rename all occurrences in a single > > directory > > - Replace them in functions (tests) and use explicit conversion > > functions - like valid_timespec_to_timespec64() before passing > > struct __timespec64 arguments (like ones for futex_time64 for nptl). > > > > - Replace _all_ occurrences in glibc tree of struct timespec with > > struct __timespec64 at once with using sed on the glibc tree. > > Using sed obviously won't work, since external interfaces need > different handling from purely internal uses. I think you need to > change things bit by bit, in sufficiently small patches for > convenient review. > > Regarding tests, I expect many tests of time-related interfaces > should end up being built and run twice on systems that currently use > 32-bit time, once to test the interfaces with 32-bit time and once to > test the interfaces with 64-bit time. Does it mean that I shall NOT make the struct timespec to struct __timespec64 conversion in glibc tests (e.g. ./sysdeps/pthread/tst-mutex5.c). Would those tests build infrastructure be changed to build them w/ _TIME_BITS=64 support? One thing puzzles me though - it seems like xclock_gettime, xclock_now, timespec_add, timespec_sub are used by glibc tests (#include <support/timespec.h>). Am I correct that those functions are not exported and used solely inside glibc? The problem is that I do need to convert them to support 64 bit time as pthreads use them (sysdeps/pthread/timer_settime.c). This also means that I would need to update tests (otherwise I would experience build/tests errors), which you don't recommend before the _TIME_BITS=64 support is added. How to proceed in this case? Two big parts of Y2038 support are still missing: - One is stat and friends (which may be simplified when we move forward with minimal kernel version supported) - And second are pthreads (nptl). Pthreads are important for user space applications - so they shall be converted sooner than latter IMHO. > Also, tests can't generally > use 64-bit time interfaces from libc until _TIME_BITS=64 support is > actually implemented. Does it mean that tests - like sysdeps/pthread/tst-mutex5.c - will always use exported struct timespec? (aliased to 64 bit struct __timespec64 when needed)? > So I think tests would be one of the last > places to change (and similarly installed executables). > Whereas any > internal use of time in a function in the libraries that does not > involve time in its interface can be updated more or less > independently of any other such use, provided the relevant internal > interfaces using 64-bit time are now available. > As I stated above - e.g. timespec_sub() helper function (from /support) is used by both pthreats and tests. Despite it is internal function - shall I follow the pattern and rewrite it as: __timespec_add64(... struct __timespec64) { .. } #if __TIMESIZE != 64 libc_hidden_def (__timespec_add64) __timespec_add(... struct timespec64) { call __timespec_add64(); } However, the above approach seems to me like an overkill to do this for internally used support function. 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 Jun 24 2020, Lukasz Majewski wrote: > As I stated above - e.g. timespec_sub() helper function (from /support) > is used by both pthreats and tests. There are two definitions of timespec_sub, in support/timespec-sub.c and in sysdeps/pthread/posix-timer.h. The former is for use in tests, the latter is used by libpthread. The functions in support/timespec-*.c should probably be renamed to xtimespec_*. Andreas.
On Wed, 24 Jun 2020, Lukasz Majewski wrote: > > Also, tests can't generally > > use 64-bit time interfaces from libc until _TIME_BITS=64 support is > > actually implemented. > > Does it mean that tests - like sysdeps/pthread/tst-mutex5.c - will > always use exported struct timespec? (aliased to 64 bit struct > __timespec64 when needed)? Yes. Tests normally use public interfaces, not internal ones. Where a test needs to use internal interfaces related to time, such interfaces may end up needing variants for different sizes of time_t (but unless they actually result in functions defined in installed shared libraries and used both from those shared libraries and from other installed libraries or executables, the hidden_def mechanism is not relevant for such interfaces).
Hi Andreas, > On Jun 24 2020, Lukasz Majewski wrote: > > > As I stated above - e.g. timespec_sub() helper function (from > > /support) is used by both pthreats and tests. > > There are two definitions of timespec_sub, in support/timespec-sub.c > and in sysdeps/pthread/posix-timer.h. The former is for use in > tests, the latter is used by libpthread. So if I understood it correctly - the support/* functions are to facilitate writing tests? > > The functions in support/timespec-*.c should probably be renamed to > xtimespec_*. And the 'x' prefix means that those functions are supposed to be used for testing? > > Andreas. > 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, 24 Jun 2020, Lukasz Majewski wrote: > > The functions in support/timespec-*.c should probably be renamed to > > xtimespec_*. > > And the 'x' prefix means that those functions are supposed to be used > for testing? The 'x' prefix is in the style of xmalloc as used in many GNU packages - checking for errors in a safer way than the default libc API. Most of the 'x' functions are thereby unsuitable for use in library code, but may be used in tests and in code that goes into the various executables shipped with glibc, because they exit the program on error and library code should generally not do that. (The 'x' names are also not in the implementation namespace; non-static internal functions used within library code need to have reserved names such as __*.)