| Message ID | 20170907224219.12483-1-albert.aribaud@3adev.fr |
|---|---|
| Headers | show |
| Series | Make GLIBC Y2038-proof | expand |
General observations on this patch series: * Nothing should use a GLIBC_Y2038 version. Use GLIBC_2.27 and update as necessary during rebases if this doesn't get into 2.27. * Nothing should go in architecture-specific files such as sysdeps/unix/sysv/linux/arm/Versions. For OS-independent interfaces use files such as time/Versions - wherever the existing versions of those interfaces for existing time_t are. * How such a patch series was tested needs to be described in the summary of the series. For a series like this, full testsuite runs on multiple 32-bit and 64-bit architectures as well as use of build-many-glibcs.py to do the compilation tests for (almost) all configurations is important (or if a series is a preliminary version with limited architecture support, that should be stated explicitly, but the full testsuite should still pass on at least one 32-bit and one 64-bit architecture). * I'd expect lots of extra tests using _TIME_BITS=64 to be added in such a patch series, to make sure that every new ABI is covered by the testsuite. * Documentation of _TIME_BITS is clearly needed. * You need to make sure that new ABIs are not added / used on platforms where time_t is already 64-bit. * All new files need a descriptive first line before the copyright notice. * All copyright ranges in new files should end with 2017. * No "Contributed by" in new files.
On Fri, 8 Sep 2017 00:41:27 +0200, "Albert ARIBAUD (3ADEV)" <albert.aribaud@3adev.fr> wrote : > This series implements Y2038-proof types, implementation, internal > functions, and APIs. For more information, see documentation at > https://sourceware.org/glibc/wiki/Y2038ProofnessDesign > > Each patch in this series except the last add one type or function, > with a few exception where several functions are implemented as a whole. > > The last patch enables the _TIME_BITS mechanism. Once this patch is > applied, API header files will will use 64-bit time if _TIME_BITS is > defined and equal to 64 prior to their inclusion, and conversively, > will use 32-bit time if _TIME_BITS is different from 64 or does not > exist. Additional info: this RFC patch series only works on ARM, but it should be extended to all 32-bit architectures later. Cordialement, Albert ARIBAUD 3ADEV
On Thu, Sep 7, 2017 at 7:21 PM, Joseph Myers <joseph@codesourcery.com> wrote: > General observations on this patch series: I concur with all of Joseph's observations and would add: > * You need to make sure that new ABIs are not added / used on platforms > where time_t is already 64-bit. Elaborating on this, when time_t is already 64 bits, defining _TIME_BITS=64 should have no effect on the generated code, and defining _TIME_BITS=32 should cause a compile-time error. * We are not going to land these patches until the kernel side is finalized. All of the places where you have "// TODO: implement after the kernel does" must be replaced with an actual implementation. Likewise, we need a complete implementation for all supported architectures before landing. You're not on the hook to implement this feature for the Hurd, but you should coordinate with Samuel Thibault so that at least you do not make glibc-for-Hurd more broken than it already is. (But I would encourage you to create a named branch in glibc git for this project if you haven't already, to make it easier for others to experiment with it.) * I understand that you are trying to make the transition as seamless as possible with _TIME_BITS and so on, but I would seriously question whether it is appropriate to preserve super-legacy APIs such as 'stime' and 'utime' in the extended mode. (I'd put the cutoff at 'gettimeofday', which is still heavily used even though 'clock_gettime' officially supersedes it.) * The POSIX (and ISO C now, argh) requirement for tv_nsec to be 'long' is, as discussed at great length earlier, incorrect and should be ignored. It should instead be a new typedef 'nsec_t' available in <sys/types.h>, matching the kernel's choice of 32 or 64 bits for this field (all _t names are reserved for future extension, so the typedef doesn't need to be _GNU_SOURCE-only). This "willful violation" (as the HTML5 folks put it) must, of course, be documented. * We do not use Signed-off-by: and we actively want you to _not_ include it in your patch submissions. (We have the FSF-mandated actual copyright assignments instead.) * The patch series is split too finely. I recommend breaking it up by type instead of by function -- time64_t + all the functions that use only time64_t; struct timeval64 + all the functions that use only time64_t and/or struct timeval64; and so on. zw
On Fri, 8 Sep 2017, Zack Weinberg wrote: > * I understand that you are trying to make the transition as seamless > as possible with _TIME_BITS and so on, but I would seriously question > whether it is appropriate to preserve super-legacy APIs such as > 'stime' and 'utime' in the extended mode. (I'd put the cutoff at > 'gettimeofday', which is still heavily used even though > 'clock_gettime' officially supersedes it.) I don't think utime is super-legacy; it's in the 2008 edition of POSIX. Now, stime is not part of any supported standard (it was withdrawn in XPG3) - such old BSD/SVID interfaces that are now in __USE_MISC but no other standard are definitely worth considering for obsoletion (removing declarations / documentation, making into compat symbols not available for new ports / static linking) if there are clear replacements for any current uses / not much current use. And such obsoletion for nonstandard time-related interfaces may well be a useful preliminary to reduce the number of interfaces needing _TIME_BITS=64 versions. (Many such interfaces are of course not time-related but just as deserving of consideration for obsoletion; in general obsoletion of one such interface is independent of obsoletion of any others.) > * The POSIX (and ISO C now, argh) requirement for tv_nsec to be 'long' > is, as discussed at great length earlier, incorrect and should be > ignored. It should instead be a new typedef 'nsec_t' available in And as discussed, I disagree and don't think we should invent any such typedef or deviate from this requirement, given that long is fully able to represent all valid nanoseconds values, in the absence of clear evidence that POSIX and ISO C agree with removing that requirement (e.g. an Austin Group issue tagged issue8 with accepted changes for the next revision of POSIX, combined with acceptance for C2X in WG14 minutes of the issue raised by the Austin Group liaison - or a WG14 change accepted for C2X with agreement on the Austin Group side following the change being passed in the other direction). When the matter was raised with the Austin Group in May 2014, no-one contradicted Rich Felker's comment (which I agree with) that the x32 issue is just a Linux kernel bug which needs to be fixed. (I think this use of long is *less* of an issue than e.g. printf returning int, since you can legitimately print more characters than fit in int.) > <sys/types.h>, matching the kernel's choice of 32 or 64 bits for this > field (all _t names are reserved for future extension, so the typedef > doesn't need to be _GNU_SOURCE-only). This "willful violation" (as the They are not reserved in ISO C, only in POSIX.
On 09/08/2017 09:43 AM, Joseph Myers wrote: > (I think this use of long is*less* of an issue than e.g. printf > returning int, since you can legitimately print more characters than fit > in int.) printf is obviously more-important since it's used all over the place and is more likely to run into the arbitrary int limit. That being said, in practice GNU Coreutils, GNU Emacs, and other application code that I help maintain long ago stopped assuming the POSIX requirement that tv_nsec must be of type 'long', precisely because we want to be portable to x32. So from my point of view this ship already sailed and the POSIX requirement is no longer helpful; on the contrary, it is misleading programmers who are trying to write portable software. If the issue is raised again at the Austin Group level, please let us know. I at least would like to put in my two cents.
On Fri, Sep 8, 2017 at 12:43 PM, Joseph Myers <joseph@codesourcery.com> wrote: > On Fri, 8 Sep 2017, Zack Weinberg wrote: >> * I understand that you are trying to make the transition as seamless >> as possible with _TIME_BITS and so on, but I would seriously question >> whether it is appropriate to preserve super-legacy APIs such as >> 'stime' and 'utime' in the extended mode. (I'd put the cutoff at >> 'gettimeofday', which is still heavily used even though >> 'clock_gettime' officially supersedes it.) > > I don't think utime is super-legacy; it's in the 2008 edition of POSIX. Hmm, OK. > Now, stime is not part of any supported standard (it was withdrawn in > XPG3) - such old BSD/SVID interfaces that are now in __USE_MISC but no > other standard are definitely worth considering for obsoletion (removing > declarations / documentation, making into compat symbols not available for > new ports / static linking) if there are clear replacements for any > current uses / not much current use. That's a good point. What I should have said is "before we do this, we should inspect all affected interfaces and see whether any of them are obsolete to the point where we should demote them to compat symbols, at which point they don't need time64 versions". stime is the only one I _know_ falls into that category: withdrawn in XPG3 + completely superseded by newer APIs + niche usage (there are very few programs that need to _set_ the system time, after all) (unfortunately, a lot of the hits on codesearch.debian.net are unrelated use of the same name, to the point where I can't actually tell how many real uses there are, but I would bet any program that still uses it does so only as a fallback-for-legacy-OSes from clock_settime and friends). >> * The POSIX (and ISO C now, argh) requirement for tv_nsec to be 'long' >> is, as discussed at great length earlier, incorrect and should be >> ignored. It should instead be a new typedef 'nsec_t' available in > > And as discussed, I disagree and don't think we should invent any such > typedef or deviate from this requirement, given that long is fully able to > represent all valid nanoseconds values Neither you, nor Rich, nor anyone else on the cited Austin Group discussion has addressed the actual issue, which is that this is a datum embedded in structures passed across the kernel/user boundary by reference, it is impossible to enumerate all such structures, therefore its type _must_ be a typedef so that it can be made to match the kernel's expectations, whatever those might be. (And I don't particularly see why people seem to think this is an x32-specific issue. It potentially affects *any* 32-bit ABI on a 64-bit kernel.) >> <sys/types.h>, matching the kernel's choice of 32 or 64 bits for this >> field (all _t names are reserved for future extension, so the typedef >> doesn't need to be _GNU_SOURCE-only). This "willful violation" (as the > > They are not reserved in ISO C, only in POSIX. <sys/types.h> doesn't exist in ISO C as far as I know, so how could it be? But any program that uses it opts into POSIX's specification for that header, even if compiled with -std=c89 and no _foo_SOURCE definitions. zw
Hi Joseph, On Thu, 7 Sep 2017 23:21:16 +0000, Joseph Myers <joseph@codesourcery.com> wrote : > General observations on this patch series: > > * Nothing should use a GLIBC_Y2038 version. Use GLIBC_2.27 and update as > necessary during rebases if this doesn't get into 2.27. Understood, will do. > * Nothing should go in architecture-specific files such as > sysdeps/unix/sysv/linux/arm/Versions. For OS-independent interfaces > use files such as time/Versions - wherever the existing versions of > those interfaces for existing time_t are. Will do. > * How such a patch series was tested needs to be described in the summary > of the series. For a series like this, full testsuite runs on multiple > 32-bit and 64-bit architectures as well as use of build-many-glibcs.py to > do the compilation tests for (almost) all configurations is important (or > if a series is a preliminary version with limited architecture support, > that should be stated explicitly, but the full testsuite should still pass > on at least one 32-bit and one 64-bit architecture). Will do. Right now, the patches are tested using an ad hoc system (as stated in the cover letter). I'll run the GLIBC test suite and add results to the cover letter in the next patcxh series iteration. > * I'd expect lots of extra tests using _TIME_BITS=64 to be added in such a > patch series, to make sure that every new ABI is covered by the testsuite. Indeed. > * Documentation of _TIME_BITS is clearly needed. Where should this documentation go? > * You need to make sure that new ABIs are not added / used on platforms > where time_t is already 64-bit. Will do. > * All new files need a descriptive first line before the copyright notice. Understood. > * All copyright ranges in new files should end with 2017. Will fix. > * No "Contributed by" in new files. Will fix. Thanks for your feedback! Cordialement, Albert ARIBAUD 3ADEV
On Fri, 8 Sep 2017, Zack Weinberg wrote: > > And as discussed, I disagree and don't think we should invent any such > > typedef or deviate from this requirement, given that long is fully able to > > represent all valid nanoseconds values > > Neither you, nor Rich, nor anyone else on the cited Austin Group > discussion has addressed the actual issue, which is that this is a > datum embedded in structures passed across the kernel/user boundary by > reference, it is impossible to enumerate all such structures, > therefore its type _must_ be a typedef so that it can be made to match > the kernel's expectations, whatever those might be. I don't see this as in any way an issue for having a definition of struct timespec that conforms to ISO C and POSIX requirements. Code passing data to the kernel knows the structures being used locally and needs to convert it to the kernel's layout, which might be e.g. struct __kernel_timespec64. I don't see that as different in essence from e.g. converting a local-time broken down date/hours/minutes/seconds/nanoseconds value before passing it to the kernel, if that's the form the particular application stores time in. It just so happens that, as an optimization, we can arrange the layout so that we both are POSIX-compatible and do not require any layout conversions between timestamps coming *from* the kernel and struct timespec. > (And I don't particularly see why people seem to think this is an > x32-specific issue. It potentially affects *any* 32-bit ABI on a > 64-bit kernel.) The kernel does the conversion just fine for a pure 32-bit struct timespec - as it does for the entire compat interface - and could do them perfectly well for a structure with 64-bit seconds and 32-bit (+ padding) nanoseconds. It just so happens not to for the very specific case of x32. > >> <sys/types.h>, matching the kernel's choice of 32 or 64 bits for this > >> field (all _t names are reserved for future extension, so the typedef > >> doesn't need to be _GNU_SOURCE-only). This "willful violation" (as the > > > > They are not reserved in ISO C, only in POSIX. > > <sys/types.h> doesn't exist in ISO C as far as I know, so how could it > be? But any program that uses it opts into POSIX's specification for My point is that you can't expose nsec_t, even as a typedef for long, in <time.h> for C11.
On Fri, 8 Sep 2017, Albert ARIBAUD wrote: > > * Documentation of _TIME_BITS is clearly needed. > > Where should this documentation go? In manual/creature.texi, alongside the documentation of _FILE_OFFSET_BITS. (It occurred to me that patches 50-52 don't seem to have reached the mailing list, so I don't know what was in them.)
Hi Zack, On Fri, 8 Sep 2017 12:19:08 -0400, Zack Weinberg <zackw@panix.com> wrote : > On Thu, Sep 7, 2017 at 7:21 PM, Joseph Myers <joseph@codesourcery.com> wrote: > > General observations on this patch series: > > I concur with all of Joseph's observations and would add: > > > * You need to make sure that new ABIs are not added / used on platforms > > where time_t is already 64-bit. > > Elaborating on this, when time_t is already 64 bits, defining > _TIME_BITS=64 should have no effect on the generated code, and > defining _TIME_BITS=32 should cause a compile-time error. Will add. > * We are not going to land these patches until the kernel side is > finalized. All of the places where you have "// TODO: implement after > the kernel does" must be replaced with an actual implementation. > Likewise, we need a complete implementation for all supported > architectures before landing. You're not on the hook to implement > this feature for the Hurd, but you should coordinate with Samuel > Thibault so that at least you do not make glibc-for-Hurd more broken > than it already is. Regarding whether the patches should land only once the kernel is Y0238-safe: these patches are intended to run even on a current and thus completely Y2038-unsafe kernel, in which case they use 64-bit-time on user side (to make applications compiled with _TIME_BITS==64 happy at least until Y2038 happens) and 32-bit time on kernel side. So I don't see why these patches should wait for a Y2038-safe kernel per se. Re the TODOs: note that right now, all these TODOs are coded in such a way that the implementation "falls through" to using 32-bit syscalls or code, just like it would if running on an older kernel. > (But I would encourage you to create a named branch in glibc git for > this project if you haven't already, to make it easier for others to > experiment with it.) There is such a branch, mentioned in the implementation part of the https://sourceware.org/glibc/wiki/Y2038ProofnessDesign document. > * I understand that you are trying to make the transition as seamless > as possible with _TIME_BITS and so on, but I would seriously question > whether it is appropriate to preserve super-legacy APIs such as > 'stime' and 'utime' in the extended mode. (I'd put the cutoff at > 'gettimeofday', which is still heavily used even though > 'clock_gettime' officially supersedes it.) I'm all for not adding 64-bit-time versions of APIs which are obsolete. The patch series is organized to facilitate removing or adding types and functions. As to which APIs should be made obsolete, there appears to be room for debate. I will add a section in the design document listing obsolete(d) APIs (there are a few which are tagged as such, but not in a section of their own yet) and as consensus emerges, I will update the patch series and document. > * The POSIX (and ISO C now, argh) requirement for tv_nsec to be 'long' > is, as discussed at great length earlier, incorrect and should be > ignored. It should instead be a new typedef 'nsec_t' available in > <sys/types.h>, matching the kernel's choice of 32 or 64 bits for this > field (all _t names are reserved for future extension, so the typedef > doesn't need to be _GNU_SOURCE-only). This "willful violation" (as the > HTML5 folks put it) must, of course, be documented. (seems to be a topic for debate) > * We do not use Signed-off-by: and we actively want you to _not_ > include it in your patch submissions. (We have the FSF-mandated > actual copyright assignments instead.) Will fix. > * The patch series is split too finely. I recommend breaking it up by > type instead of by function -- time64_t + all the functions that use > only time64_t; struct timeval64 + all the functions that use only > time64_t and/or struct timeval64; and so on. As indicated above, I broke it up by type and function (except when dependencies made it really impractical) to easy adding or removing (and reviewing) individual functions. 50-odd patches are manageable IMO, but maybe some people may be put off by it... What I'll do is keep two branches: this fine-grained one, and a coarser-grained one where all changes related to one type are grouped. Best of both worlds. > zw Thanks for your review! Cordialement, Albert ARIBAUD 3ADEV
On Fri, 8 Sep 2017, Albert ARIBAUD wrote: > Regarding whether the patches should land only once the kernel is > Y0238-safe: these patches are intended to run even on a current and thus > completely Y2038-unsafe kernel, in which case they use 64-bit-time on > user side (to make applications compiled with _TIME_BITS==64 happy at > least until Y2038 happens) and 32-bit time on kernel side. So I don't > see why these patches should wait for a Y2038-safe kernel per se. Where the kernel layouts of structures are suitable for use by glibc, we'd like to avoid translation layers. E.g., we need to know what the kernel's struct stat64 variant for 64-bit time_t will look like to ensure no translation layer is needed. (If the answer is that statx is the interface that should be used for 64-bit times in struct stat so the kernel doesn't need to define such a stat64 variant, then the translation layer is needed anyway and we should maybe use the asm-generic 64-bit struct stat variant of the layout on all 32-bit platforms.)
On Fri, 8 Sep 2017 17:59:00 +0000, Joseph Myers <joseph@codesourcery.com> wrote : > On Fri, 8 Sep 2017, Albert ARIBAUD wrote: > > > Regarding whether the patches should land only once the kernel is > > Y0238-safe: these patches are intended to run even on a current and thus > > completely Y2038-unsafe kernel, in which case they use 64-bit-time on > > user side (to make applications compiled with _TIME_BITS==64 happy at > > least until Y2038 happens) and 32-bit time on kernel side. So I don't > > see why these patches should wait for a Y2038-safe kernel per se. > > Where the kernel layouts of structures are suitable for use by glibc, we'd > like to avoid translation layers. E.g., we need to know what the kernel's > struct stat64 variant for 64-bit time_t will look like to ensure no > translation layer is needed. (If the answer is that statx is the > interface that should be used for 64-bit times in struct stat so the > kernel doesn't need to define such a stat64 variant, then the translation > layer is needed anyway and we should maybe use the asm-generic 64-bit > struct stat variant of the layout on all 32-bit platforms.) Understood -- but as soon as 64-bit-time types are frozen on the kernel side, we could freeze the corresponding GLIBC API types (hopefully without a translation layer needed) and then if GLIBC gets out there before the kernel does, it won't be a problem since in any case the 64-bit-time implementation must fall back to 32-bit syscalls if the 64-bit syscalls return an ENOSYS. Cordialement, Albert ARIBAUD 3ADEV
On Fri, Sep 8, 2017 at 1:24 PM, Joseph Myers <joseph@codesourcery.com> wrote: > On Fri, 8 Sep 2017, Zack Weinberg wrote: > >> Neither you, nor Rich, nor anyone else on the cited Austin Group >> discussion has addressed the actual issue, which is that this is a >> datum embedded in structures passed across the kernel/user boundary by >> reference, it is impossible to enumerate all such structures, >> therefore its type _must_ be a typedef so that it can be made to match >> the kernel's expectations, whatever those might be. > > I don't see this as in any way an issue for having a definition of struct > timespec that conforms to ISO C and POSIX requirements. Code passing data > to the kernel knows the structures being used locally and needs to convert > it to the kernel's layout, which might be e.g. struct __kernel_timespec64. This only works if all such structures can be enumerated and forced through a compatibility layer. History indicates that it is not, in fact, possible to enumerate all such structures (the problem case I know about is device-specific ioctl operations, which tend to be created by driver authors who don't realize the need for ABI compatibility shims until it's too late). >> (And I don't particularly see why people seem to think this is an >> x32-specific issue. It potentially affects *any* 32-bit ABI on a >> 64-bit kernel.) > > The kernel does the conversion just fine for a pure 32-bit struct timespec > - as it does for the entire compat interface For the reason given above, I confidently predict that there is at least one failure to do this conversion correctly in every released kernel ever. >> <sys/types.h> doesn't exist in ISO C as far as I know, so how could it >> be? But any program that uses it opts into POSIX's specification for > > My point is that you can't expose nsec_t, even as a typedef for long, in > <time.h> for C11. I suppose it could be __nsec_t in time.h, but I would prefer to include "and time.h defines nsec_t" in the scope of the willful violation of C11. zw
On Fri, Sep 8, 2017 at 2:16 PM, Albert ARIBAUD <albert.aribaud@3adev.fr> wrote: > On Fri, 8 Sep 2017 17:59:00 +0000, Joseph Myers > <joseph@codesourcery.com> wrote : > >> On Fri, 8 Sep 2017, Albert ARIBAUD wrote: >> >> > Regarding whether the patches should land only once the kernel is >> > Y0238-safe: these patches are intended to run even on a current and thus >> > completely Y2038-unsafe kernel, in which case they use 64-bit-time on >> > user side (to make applications compiled with _TIME_BITS==64 happy at >> > least until Y2038 happens) and 32-bit time on kernel side. So I don't >> > see why these patches should wait for a Y2038-safe kernel per se. >> >> Where the kernel layouts of structures are suitable for use by glibc, we'd >> like to avoid translation layers. E.g., we need to know what the kernel's >> struct stat64 variant for 64-bit time_t will look like to ensure no >> translation layer is needed. (If the answer is that statx is the >> interface that should be used for 64-bit times in struct stat so the >> kernel doesn't need to define such a stat64 variant, then the translation >> layer is needed anyway and we should maybe use the asm-generic 64-bit >> struct stat variant of the layout on all 32-bit platforms.) > > Understood -- but as soon as 64-bit-time types are frozen on the kernel > side, we could freeze the corresponding GLIBC API types (hopefully > without a translation layer needed) and then if GLIBC gets out there > before the kernel does, it won't be a problem since in any case the > 64-bit-time implementation must fall back to 32-bit syscalls if the > 64-bit syscalls return an ENOSYS. I think that's right. Syscalls we can add support for later, types are much more troublesome. zw
Hi Joseph, On Fri, 8 Sep 2017 17:26:26 +0000, Joseph Myers <joseph@codesourcery.com> wrote : > On Fri, 8 Sep 2017, Albert ARIBAUD wrote: > > > > * Documentation of _TIME_BITS is clearly needed. > > > > Where should this documentation go? > > In manual/creature.texi, alongside the documentation of _FILE_OFFSET_BITS. Thanks, will do in 01/52 of v2. > (It occurred to me that patches 50-52 don't seem to have reached the > mailing list, so I don't know what was in them.) You're correct. I did not get any NDR so I cannot tell what happened... I have sent the rest (threaded properly after 49/52, although missing "References:" headers to 0/48 and previous). Cordialement, Albert ARIBAUD 3ADEV