Message ID | CAKmqyKP=kmeJjcsDepetvsVrph3xJ7G8yQdNFNyX-wL2ZMtqmA@mail.gmail.com |
---|---|
State | Superseded |
Headers |
Received: (qmail 57047 invoked by alias); 16 Sep 2019 22:49:51 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 57038 invoked by uid 89); 16 Sep 2019 22:49:50 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-17.7 required=5.0 tests=AWL, BAYES_00, FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy=timer X-HELO: mail-lj1-f194.google.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=5JBCKqrBAxCK19ZA3prOn7Do+b3JkOGJZ1MOYrW4yuI=; b=GmkJfQWcoVZnmCmKj12xbahR/2mBjvV3hGMVfjzuw4ZeBmZA7vMDJ3kjnvy7pxK6xC CF/ODV/68CmM1Tx965KqAiCI4T0Zy3QkaqaG+T8PzVsQvSu+VAb1x4oNHRF1H2Iab+DJ KGQePXB9qfPLpqcnDlW1ulGJroebx45tsOYMX/CTrENylzCis/YVFxLt/x1Cx9Mjjw+F TkCUJJdjbbgHjOqyJnv8eYj2y8L7ifrEuA2Wfiq3W2WdhdXMxr8R8QOG79ZcEHRV4Iis rLyeLCThhZRL7QhLctlZQhCrC9vi0wEh5Kbb/NcrBQpAUh8quayRJwAPPu9nDpPHgsHH rjaw== MIME-Version: 1.0 References: <20190906145911.30207-1-lukma@denx.de> <CAKmqyKMdsVZg_ZS7fLTt23dy2vnZc1z85b_+8heiK-8UiqMx+g@mail.gmail.com> <alpine.DEB.2.21.1909062122220.30243@digraph.polyomino.org.uk> In-Reply-To: <alpine.DEB.2.21.1909062122220.30243@digraph.polyomino.org.uk> From: Alistair Francis <alistair23@gmail.com> Date: Mon, 16 Sep 2019 15:45:16 -0700 Message-ID: <CAKmqyKP=kmeJjcsDepetvsVrph3xJ7G8yQdNFNyX-wL2ZMtqmA@mail.gmail.com> Subject: Re: [PATCH v7 0/3] y2038: Linux: Introduce __clock_settime64 function To: Joseph Myers <joseph@codesourcery.com> Cc: Lukasz Majewski <lukma@denx.de>, Zack Weinberg <zackw@panix.com>, Arnd Bergmann <arnd@arndb.de>, Alistair Francis <alistair.francis@wdc.com>, GNU C Library <libc-alpha@sourceware.org>, Adhemerval Zanella <adhemerval.zanella@linaro.org>, Florian Weimer <fweimer@redhat.com>, "Carlos O'Donell" <carlos@redhat.com>, Stepan Golosunov <stepan@golosunov.pp.ru> Content-Type: text/plain; charset="UTF-8" |
Commit Message
Alistair Francis
Sept. 16, 2019, 10:45 p.m. UTC
On Fri, Sep 6, 2019 at 2:28 PM Joseph Myers <joseph@codesourcery.com> wrote: > > On Fri, 6 Sep 2019, Alistair Francis wrote: > > > Which I can fix with this diff: > > This diff is not the right way to fix this build failure. > > One of the design principles in the Y2038 support is that is __TIMESIZE == > 64, the time functions *aren't* trivial wrappers of time64 functions; > rather, the time64 function definitions are remapped (via macros) so they > define the function name with no "64". For each case where there is a > pair of functions (for different time_t types) in the __TIMESIZE == 32 > case, there should just be the one function when __TIMESIZE == 64. > > This ensures that the Y2038 changes don't add any overhead at all in the > glibc binaries on existing platforms with __TIMESIZE == 64. > > You should look at exactly what the types in question are, that are being > reported as conflicting in your build (probably by looking at preprocessed > source). __timespec64 and timespec are supposed to be the same type (via > #define) when __TIMESIZE == 64, to avoid such incompatibilities. Looking at the place where the__timespec64 is defined they aren't the same for __TIMESIZE == 64 on a 32-bit system. The code is below: #if __WORDSIZE == 64 \ || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64) # define __timespec64 timespec #else /* The glibc Y2038-proof struct __timespec64 structure for a time value. To keep things Posix-ish, we keep the nanoseconds field a 32-bit signed long, but since the Linux field is a 64-bit signed int, we pad our tv_nsec with a 32-bit int. As a general rule the Linux kernel is ignoring upper 32 bits of tv_nsec field. */ struct __timespec64 { __time64_t tv_sec; /* Seconds */ # if BYTE_ORDER == BIG_ENDIAN __int32_t tv_pad; /* Padding */ __int32_t tv_nsec; /* Nanoseconds */ # else __int32_t tv_nsec; /* Nanoseconds */ __int32_t tv_pad; /* Padding */ # endif }; #endif So looking at that with __TIMESIZE == 64 but __WORDSIZE == 32 (as on RV32) we will specify a __timespec64 struct that is different to the timespec struct. Would the correct fix be to add __TIMESIZE == 64 to the #if mentioned above? This diff fixes the build for me: /* The glibc Y2038-proof struct __timespec64 structure for a time value. Alistair > > -- > Joseph S. Myers > joseph@codesourcery.com
Comments
On Mon, 16 Sep 2019, Alistair Francis wrote:
> Would the correct fix be to add __TIMESIZE == 64 to the #if mentioned above?
Those other conditions imply __TIMESIZE == 64. So replacing the existing
conditions by __TIMESIZE == 64 might be correct, but not adding it while
leaving the existing (then redundant) conditions there as well.
I think you should replace the conditional by __TIMESIZE == 64 (if that
works), which I think should be safe for the reasons explained below.
There is a possibility that we end up needing two different internal names
instead of the present one, but I don't think that should block changing
the conditional at this point to make further progress unless that causes
problems with the existing patches on existing configurations.
The design for defining 64-bit-time functions with *64 names and then
remapping those names to the public ones via #define if __TIMESIZE == 64
implies that the type used in declaring / defining *64 functions needs to
be mapped to struct timespec if __TIMESIZE == 64.
But if you need to set padding explicitly to zero when passing 64-bit
timespecs into the kernel (the case of kernels 5.1.0 through 5.1.4, on
32-bit architectures for which those kernel versions had a corresponding
64-bit architecture with compat syscall support), you then need a type
with named padding (whereas the public struct timespec needs to have an
unnamed bit-field for the padding to stay compatible with existing sources
with { tv_sec, tv_nsec } initializers).
So this links into what things should look like at the API level inside
glibc for dealing with zeroing padding, if we do that at all. However,
unless we fix x32 to use "long int" for tv_nsec we don't need to deal with
zeroing padding for any existing system with __TIMESIZE == 64 (and I don't
think it's likely to be relevant for future 32-bit ports that use
__TIMESIZE == 64 either, because the kernel issue is only for compat
syscalls for 32-bit binaries under 64-bit kernels, which I don't think are
relevant to any 32-bit architectures supported in 5.1 kernels but not yet
in glibc).
Hi Alistair, Joseph, > On Fri, Sep 6, 2019 at 2:28 PM Joseph Myers <joseph@codesourcery.com> > wrote: > > > > On Fri, 6 Sep 2019, Alistair Francis wrote: > > > > > Which I can fix with this diff: > > > > This diff is not the right way to fix this build failure. > > > > One of the design principles in the Y2038 support is that is > > __TIMESIZE == 64, the time functions *aren't* trivial wrappers of > > time64 functions; rather, the time64 function definitions are > > remapped (via macros) so they define the function name with no > > "64". For each case where there is a pair of functions (for > > different time_t types) in the __TIMESIZE == 32 case, there should > > just be the one function when __TIMESIZE == 64. > > > > This ensures that the Y2038 changes don't add any overhead at all > > in the glibc binaries on existing platforms with __TIMESIZE == 64. > > > > You should look at exactly what the types in question are, that are > > being reported as conflicting in your build (probably by looking at > > preprocessed source). __timespec64 and timespec are supposed to be > > the same type (via #define) when __TIMESIZE == 64, to avoid such > > incompatibilities. > > Looking at the place where the__timespec64 is defined they aren't the > same for __TIMESIZE == 64 on a 32-bit system. > > The code is below: > > #if __WORDSIZE == 64 \ > || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64) ^^^^^^^^^^^^^ - [1] > # define __timespec64 timespec > #else > /* The glibc Y2038-proof struct __timespec64 structure for a time > value. To keep things Posix-ish, we keep the nanoseconds field a > 32-bit signed long, but since the Linux field is a 64-bit signed int, > we pad our tv_nsec with a 32-bit int. > > As a general rule the Linux kernel is ignoring upper 32 bits of > tv_nsec field. */ > struct __timespec64 ^^^^^^^^^^ - [3] > { > __time64_t tv_sec; /* Seconds */ > # if BYTE_ORDER == BIG_ENDIAN > __int32_t tv_pad; /* Padding */ > __int32_t tv_nsec; /* Nanoseconds */ > # else > __int32_t tv_nsec; /* Nanoseconds */ > __int32_t tv_pad; /* Padding */ > # endif > }; > #endif > > So looking at that with __TIMESIZE == 64 but __WORDSIZE == 32 (as on > RV32) we will specify a __timespec64 struct that is different to the > timespec struct. > > Would the correct fix be to add __TIMESIZE == 64 to the #if mentioned > above? I've replaced the condition [1] with plain #if __TIMESIZE == 64 [2], but then there was a concern that: Replace __TIMESIZE with __WORDSIZE (as architectures with __TIMESIZE==64 will need to use this struct with 32 bit tv_nsec field). Alistair - I guess that the size of RISC-V's struct timespec tv_nsec is 4? (as it shall be long tv_nsec; [4]) ? Then if you replace the condition [1] with #if __TIMESIZE == 64 you would have: struct timespec { __time_t tv_sec; /* Seconds. */ __syscall_slong_t tv_nsec; /* Nanoseconds. */ } And then, You would have tv_sec (8 bytes) and tv_nsec (4 bytes) [*]. Kernel's clock_settime64 expects to receive two 8 bytes values. In the best case you would leak glibc data to the kenel. In the worst case kernel will modify this data, which may be some other struct's field. The condition [1] prevents from this for machines with __WORDSIZE == 32 (excluding x32). Joseph is the above concern valid ? > > This diff fixes the build for me: > > diff --git a/include/time.h b/include/time.h > index 5f7529f10e9..ff5f18ec56c 100644 > --- a/include/time.h > +++ b/include/time.h > @@ -51,7 +51,8 @@ extern void __tz_compute (__time64_t timer, struct > tm *tm, int use_localtime) > __THROW attribute_hidden; > > #if __WORDSIZE == 64 \ > - || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64) > + || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64) || \ > + __TIMESIZE == 64 > # define __timespec64 timespec > #else > /* The glibc Y2038-proof struct __timespec64 structure for a time > value. > > Alistair > > > > > -- > > Joseph S. Myers > > joseph@codesourcery.com Note: [2] - https://patchwork.ozlabs.org/patch/1092580/ [4] - https://linux.die.net/man/3/clock_gettime [*] - what is the output of sizeof(__syscall_slong_t) on RISCV 32 bit? 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 Tue, 17 Sep 2019, Lukasz Majewski wrote: > Then if you replace the condition [1] with #if __TIMESIZE == 64 you > would have: > > struct timespec > { > __time_t tv_sec; /* Seconds. */ > __syscall_slong_t tv_nsec; /* Nanoseconds. */ > } The *public* struct timespec (defined in time/bits/types/struct_timespec.h) should be changed for ports that define __TIMESIZE == 64 while __SYSCALL_WORDSIZE == 32. That is, if __TIMESIZE == 64, and if __SYSCALL_WORDSIZE (if defined) is 32 or __WORDSIZE (if __SYSCALL_WORDSIZE is not defined), then struct timespec needs endian-dependent padding (defined as an *unnamed* 32-bit bit-field, so that it gets ignored for initializers). (This is the same padding as would be needed in the case where __TIMESIZE == 32 but _TIME_BITS=64 is defined, but _TIME_BITS=64 support for headers comes later.) RV32 has got away without that change to struct timespec because it's little-endian, and as long as __time_t is 8-byte-aligned implicit padding works as well as explicit in the little-endian case. If BE, or if 8-byte __time_t is only 4-byte-aligned in structs (and so the struct ends up as 12-byte without explicit padding), there would be problems. I think it's cleanest to make the padding explicit even in the cases where in fact implicit padding would give the same layout. RV32 does not need any support for clearing the padding before passing struct timespec to the kernel, because that's only relevant for compat syscalls in Linux 5.1.0 to 5.1.4 and the RISC-V kernel doesn't yet have compat syscall support for running RV32 binaries under RV64 kernels.
Hi Joseph, Alistair, > On Tue, 17 Sep 2019, Lukasz Majewski wrote: > > > Then if you replace the condition [1] with #if __TIMESIZE == 64 you > > would have: > > > > struct timespec > > { > > __time_t tv_sec; /* Seconds. */ > > __syscall_slong_t tv_nsec; /* Nanoseconds. */ > > } > > The *public* struct timespec (defined in > time/bits/types/struct_timespec.h) should be changed for ports that > define __TIMESIZE == 64 while __SYSCALL_WORDSIZE == 32. > > That is, if __TIMESIZE == 64, and if __SYSCALL_WORDSIZE (if defined) > is 32 or __WORDSIZE (if __SYSCALL_WORDSIZE is not defined), then > struct timespec needs endian-dependent padding (defined as an > *unnamed* 32-bit bit-field, so that it gets ignored for > initializers). Ok, I will prepare proper patch with adjusting the *public* struct timespec. > (This is the same padding as would be needed in the > case where __TIMESIZE == 32 but _TIME_BITS=64 is defined, but > _TIME_BITS=64 support for headers comes later.) As for example here [1]. Just for (my) confirmation: - New 32 bits glibc ports (like RISC-V 32) will get __TIMESIZE == 64 (__WORDSIZE == 32) and no need to define the -D_TIME_BITS=64 during the compilation. They will just get 64 bit time API support from the outset. - Already supported 32 bits architectures (like armv7-a with __WORDSIZE == 32) will keep __TIMESIZE == 32 and require -D_TIME_BITS=64 for compilation. After glibc sets the minimal supported kernel version to 5.1 and all conversions for syscalls to support 64 bit time API are done the __TIMESIZE will be set to 64 and -D_TIME_BITS=64 will not be required anymore for compilation. Until the above switch happens we will redirect time calls - like in [2]. > > RV32 has got away without that change to struct timespec because it's > little-endian, and as long as __time_t is 8-byte-aligned implicit > padding works as well as explicit in the little-endian case. Ok. > If BE, > or if 8-byte __time_t is only 4-byte-aligned in structs (and so the > struct ends up as 12-byte without explicit padding), there would be > problems. Yes. > I think it's cleanest to make the padding explicit even in > the cases where in fact implicit padding would give the same layout. Ok. So then we shall keep the condition: #if __WORDSIZE == 64 \ || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64) # define __timespec64 timespec #else // struct __timespec64 with endian dependent, explicit padding and // __time64_t tv_sec; #fi And RV32 shall use the explicitly defined struct __timespec64 (from #else) as presented in [3] ? > > RV32 does not need any support for clearing the padding before > passing struct timespec to the kernel, because that's only relevant > for compat syscalls in Linux 5.1.0 to 5.1.4 and the RISC-V kernel > doesn't yet have compat syscall support for running RV32 binaries > under RV64 kernels. Ok. Thanks for clarification. > Note: [1] - https://github.com/lmajewski/y2038_glibc/commit/c6740c05ea9b224a552847d10402f98da8376994#diff-4ddbc47d3262d4f00f3825e4f3627dbbR10 [2] - https://github.com/lmajewski/y2038_glibc/commit/c6740c05ea9b224a552847d10402f98da8376994#diff-07934c1fe09f0e6357413e138856c786R225 [3] - https://github.com/lmajewski/y2038_glibc/commit/926634e657fa5a927152bf7eb06a62e8468f75ae#diff-5b9f1c6457e0e10079f657f283c19861R53 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 Tue, 17 Sep 2019, Lukasz Majewski wrote: > - New 32 bits glibc ports (like RISC-V 32) will get __TIMESIZE == > 64 (__WORDSIZE == 32) and no need to define the -D_TIME_BITS=64 > during the compilation. They will just get 64 bit time API support > from the outset. Yes, at least if such ports wish to use 64-bit time; I don't think we've really discussed if we want to *require* 64-bit time for future ports (e.g. the next revised resubmissions of the ARC and NDS32 ports). Certainly the work required right now for ARC or NDS32 to use 64-bit time would be significantly more than the work for RV32 (because they also support older kernel versions without the 64-bit-time syscalls, so all the Y2038 work for fallback at runtime to older syscalls becomes relevant), unless they decide on 5.1 or later as minimum kernel version. > - Already supported 32 bits architectures (like armv7-a with __WORDSIZE > == 32) will keep __TIMESIZE == 32 and require -D_TIME_BITS=64 for > compilation. Yes. > After glibc sets the minimal supported kernel version to 5.1 and all > conversions for syscalls to support 64 bit time API are done the > __TIMESIZE will be set to 64 and -D_TIME_BITS=64 will not be required > anymore for compilation. No. __TIMESIZE means the size of time_t in the unsuffixed ABIs in glibc, not the _TIME_BITS-dependent size of time_t in the current compilation. We hope in future to make _TIME_BITS=64 the default and only API supported for new compilations (which is independent of what the minimum kernel version is), but __TIMESIZE would still be 32, because the unsuffixed ABIs would remain compatible with existing binaries using 32-bit time. > Ok. So then we shall keep the condition: > > #if __WORDSIZE == 64 \ > || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64) > # define __timespec64 timespec > #else No. __timespec64 should be defined to timespec whenever __TIMESIZE == 64. The timespec to which it is defined, in the public header, would gain padding. The condition #if __WORDSIZE == 64 \ || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64) is correct as a condition for struct timespec (in the public header) *not* to have padding.
diff --git a/include/time.h b/include/time.h index 5f7529f10e9..ff5f18ec56c 100644 --- a/include/time.h +++ b/include/time.h @@ -51,7 +51,8 @@ extern void __tz_compute (__time64_t timer, struct tm *tm, int use_localtime) __THROW attribute_hidden; #if __WORDSIZE == 64 \ - || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64) + || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64) || \ + __TIMESIZE == 64 # define __timespec64 timespec #else