Message ID | 20200601140740.16371-1-lukma@denx.de |
---|---|
Headers |
Return-Path: <libc-alpha-bounces@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 458A03870911; Mon, 1 Jun 2020 14:08:01 +0000 (GMT) X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mail-out.m-online.net (mail-out.m-online.net [212.18.0.9]) by sourceware.org (Postfix) with ESMTPS id 694053851C17 for <libc-alpha@sourceware.org>; Mon, 1 Jun 2020 14:07:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 694053851C17 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=denx.de Authentication-Results: sourceware.org; spf=none smtp.mailfrom=lukma@denx.de Received: from frontend01.mail.m-online.net (unknown [192.168.8.182]) by mail-out.m-online.net (Postfix) with ESMTP id 49bH7b1P1Pz1qsbQ; Mon, 1 Jun 2020 16:07:54 +0200 (CEST) Received: from localhost (dynscan1.mnet-online.de [192.168.6.70]) by mail.m-online.net (Postfix) with ESMTP id 49bH7Z5Bpgz1qwwd; Mon, 1 Jun 2020 16:07:54 +0200 (CEST) X-Virus-Scanned: amavisd-new at mnet-online.de Received: from mail.mnet-online.de ([192.168.8.182]) by localhost (dynscan1.mail.m-online.net [192.168.6.70]) (amavisd-new, port 10024) with ESMTP id 6rWuOkSpSEX9; Mon, 1 Jun 2020 16:07:52 +0200 (CEST) X-Auth-Info: 9H+kTdc8k0+pxJzSkkH8DS/wQsg/sdRMLu8RAVhSbDA= Received: from localhost.localdomain (85-222-111-42.dynamic.chello.pl [85.222.111.42]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.mnet-online.de (Postfix) with ESMTPSA; Mon, 1 Jun 2020 16:07:52 +0200 (CEST) From: Lukasz Majewski <lukma@denx.de> To: Joseph Myers <joseph@codesourcery.com>, Paul Eggert <eggert@cs.ucla.edu>, Adhemerval Zanella <adhemerval.zanella@linaro.org> Subject: [RFC 00/12] [RFC] y2038: Convert timespec_{sub|add|create} in support to be Y2038 safe Date: Mon, 1 Jun 2020 16:07:28 +0200 Message-Id: <20200601140740.16371-1-lukma@denx.de> X-Mailer: git-send-email 2.20.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=no autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list <libc-alpha.sourceware.org> List-Unsubscribe: <http://sourceware.org/mailman/options/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-request@sourceware.org?subject=help> List-Subscribe: <http://sourceware.org/mailman/listinfo/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=subscribe> Cc: Florian Weimer <fweimer@redhat.com>, GNU C Library <libc-alpha@sourceware.org>, Andreas Schwab <schwab@suse.de>, Alistair Francis <alistair.francis@wdc.com> Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" <libc-alpha-bounces@sourceware.org> |
Series |
y2038: Convert timespec_{sub|add|create} in support to be Y2038 safe
|
|
Message
Lukasz Majewski
June 1, 2020, 2:07 p.m. UTC
This patch set is the first attempt to convert functions utilized by nptl and pthreads to support 64 bit time. First timespec* functions are renamed to have common "__" prefix for internal functions. This is a preparatory work for further conversion. One question with this patch set is how shall we tackle the conversion of time related functions (like e.g. ./support/timespec_add) used internally in glibc as helper functions for internal code or tests. I could follow the approach used for glibc syscalls (like clock_settime) to have two versions depending on __TIMESIZE and __WORDSIZE, but this seems to be an overkill for internally used functions. Patches with "Convert:" prefix replace struct timespec with struct __timespec64 in timespec_* functions. This is necessary to have in-glibc proper support for 64 bit time. The problem is with instant replacement of struct timespec with struct __timespec64 in glibc internal code (and tests). For replacing timesize with __timesize64 I could: - Replace its occurences in relevant directories - like ./nptl or ./sysdeps/pthreads - Replace them in functions (tests) and use explicit conversion functions - like valid_timespec_to_timespec64() - Replace _all_ occurences of struct timespec with struct __timespec64 at once with using sed on glibc tree. The last option seems to be the most appealing as we already use __timespec64 (with its aliassing) for some core system syscalls (like clock_gettime). However, such patch shall be applied just after the release of new stable version to have time for potential fixes. Lukasz Majewski (12): doc: Fix wording and formattine in ./support/README y2038: Rename timespec_compare to __timespec_compare y2038: Rename make_timespec to __make_timespec y2038: Rename xclock_gettime to __xclock_gettime y2038: Rename xclock_now to __xclock_now y2038: Rename timespec_sub to __timespec_sub y2038: Rename timespec_add to __timespec_add y2038: Convert __make_timespec to be Y2038 safe y2038: Convert __xclock_gettime to be Y2038 safe y2038: Convert __xclock_now to be Y2038 safe y2038: Convert timespec* files in ./support to be Y2038 safe y2038: Convert timespec_* from posix-timer.h to be Y2038 safe nptl/tst-eintr2.c | 4 ++-- nptl/tst-eintr5.c | 4 ++-- nptl/tst-rwlock6.c | 14 +++++++------- nptl/tst-rwlock7.c | 10 +++++----- nptl/tst-rwlock9.c | 10 +++++----- nptl/tst-sem17.c | 2 +- nptl/tst-sem5.c | 4 ++-- support/README | 4 ++-- support/timespec-add.c | 12 ++++++------ support/timespec-sub.c | 18 +++++++++--------- support/timespec.c | 12 ++++++------ support/timespec.h | 27 ++++++++++++++------------- support/xclock_gettime.c | 5 ++--- support/xtime.h | 9 +++++---- sysdeps/mach/clock_nanosleep.c | 4 ++-- sysdeps/pthread/posix-timer.h | 11 ++++++----- sysdeps/pthread/timer_gettime.c | 4 ++-- sysdeps/pthread/timer_routines.c | 14 +++++++------- sysdeps/pthread/timer_settime.c | 4 ++-- sysdeps/pthread/tst-cond11.c | 2 +- sysdeps/pthread/tst-cond26.c | 2 +- sysdeps/pthread/tst-cond27.c | 4 ++-- sysdeps/pthread/tst-join14.c | 2 +- sysdeps/pthread/tst-join3.c | 8 ++++---- sysdeps/pthread/tst-join5.c | 12 ++++++------ sysdeps/pthread/tst-mutex5.c | 12 ++++++------ sysdeps/pthread/tst-mutex9.c | 4 ++-- sysdeps/pthread/tst-rwlock14.c | 2 +- 28 files changed, 111 insertions(+), 109 deletions(-)
Comments
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 __*.)