| Message ID | 53894e78c91ed5e037077f7249b85cefe635ae1f.1565398513.git.alistair.francis@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | RISC-V glibc port for the 32-bit | expand |
Alistair Francis wrote: > - int tz_minuteswest; /* Minutes west of GMT. */ > - int tz_dsttime; /* Nonzero if DST is ever in effect. */ > + int tz_minuteswest_dep; /* Minutes west of GMT. */ > + int tz_dsttime_dep; /* Nonzero if DST is ever in effect. */ These two members should be declared with __attribute_deprecated__. And once we do that, do we still need to change their names? I don't recall any other instance of such name-changing. If we do change their names, I suggest a more-obvious suffix than "_dep" (which could stand for a lot of things); "_deprecated" would be clearer.
On Fri, Aug 9, 2019 at 6:20 PM Paul Eggert <eggert@cs.ucla.edu> wrote: > > Alistair Francis wrote: > > - int tz_minuteswest; /* Minutes west of GMT. */ > > - int tz_dsttime; /* Nonzero if DST is ever in effect. */ > > + int tz_minuteswest_dep; /* Minutes west of GMT. */ > > + int tz_dsttime_dep; /* Nonzero if DST is ever in effect. */ > These two members should be declared with __attribute_deprecated__. And once we > do that, do we still need to change their names? I don't recall any other > instance of such name-changing. The name changing is coming from a suggestion here [1]. I do find it a little aggressive though as I do see some build failures, specifically for systemd. > > If we do change their names, I suggest a more-obvious suffix than "_dep" (which > could stand for a lot of things); "_deprecated" would be clearer. I'll change the name if we still decided to stick with the name change. 1: https://sourceware.org/ml/libc-alpha/2019-07/msg00614.html Alistair
* Alistair Francis: > On Fri, Aug 9, 2019 at 6:20 PM Paul Eggert <eggert@cs.ucla.edu> wrote: >> >> Alistair Francis wrote: >> > - int tz_minuteswest; /* Minutes west of GMT. */ >> > - int tz_dsttime; /* Nonzero if DST is ever in effect. */ >> > + int tz_minuteswest_dep; /* Minutes west of GMT. */ >> > + int tz_dsttime_dep; /* Nonzero if DST is ever in effect. */ >> These two members should be declared with __attribute_deprecated__. And once we >> do that, do we still need to change their names? I don't recall any other >> instance of such name-changing. > > The name changing is coming from a suggestion here [1]. I do find it a > little aggressive though as I do see some build failures, specifically > for systemd. Good point. systemd does this: tz.tz_minuteswest = -minutesdelta; tz.tz_dsttime = 0; /* DST_NONE */ /* * If the RTC does not run in UTC but in local time, the very first * call to settimeofday() will set the kernel's timezone and will warp the * system clock, so that it runs in UTC instead of the local time we * have read from the RTC. */ if (settimeofday(tv_null, &tz) < 0) return negative_errno(); It seems to me that struct timezone is not actually deprecated on the kernel side when used with settimeofday. This would be something which has to happen before we can change its definition. Thanks, Florian
On Sun, Aug 11, 2019 at 6:52 AM Florian Weimer <fweimer@redhat.com> wrote: > > * Alistair Francis: > > > On Fri, Aug 9, 2019 at 6:20 PM Paul Eggert <eggert@cs.ucla.edu> wrote: > >> > >> Alistair Francis wrote: > >> > - int tz_minuteswest; /* Minutes west of GMT. */ > >> > - int tz_dsttime; /* Nonzero if DST is ever in effect. */ > >> > + int tz_minuteswest_dep; /* Minutes west of GMT. */ > >> > + int tz_dsttime_dep; /* Nonzero if DST is ever in effect. */ > >> These two members should be declared with __attribute_deprecated__. And once we > >> do that, do we still need to change their names? I don't recall any other > >> instance of such name-changing. > > > > The name changing is coming from a suggestion here [1]. I do find it a > > little aggressive though as I do see some build failures, specifically > > for systemd. > > Good point. systemd does this: > > tz.tz_minuteswest = -minutesdelta; > tz.tz_dsttime = 0; /* DST_NONE */ > > /* > * If the RTC does not run in UTC but in local time, the very first > * call to settimeofday() will set the kernel's timezone and will warp the > * system clock, so that it runs in UTC instead of the local time we > * have read from the RTC. > */ > if (settimeofday(tv_null, &tz) < 0) > return negative_errno(); > > It seems to me that struct timezone is not actually deprecated on the > kernel side when used with settimeofday. This would be something which > has to happen before we can change its definition. Ok, so in this case for 32-bit y2038 safe platforms (without gettimeofday and settimeofday) we still seem fine to ignore the struct timezone. It doesn't look like we can rename the members yet though. Can we mark them as deprecated in the glibc source? That will just generate warnings instead of errors right? Alistair > > Thanks, > Florian
On Sun, Aug 11, 2019 at 03:52:45PM +0200, Florian Weimer wrote: > * Alistair Francis: > > > On Fri, Aug 9, 2019 at 6:20 PM Paul Eggert <eggert@cs.ucla.edu> wrote: > >> > >> Alistair Francis wrote: > >> > - int tz_minuteswest; /* Minutes west of GMT. */ > >> > - int tz_dsttime; /* Nonzero if DST is ever in effect. */ > >> > + int tz_minuteswest_dep; /* Minutes west of GMT. */ > >> > + int tz_dsttime_dep; /* Nonzero if DST is ever in effect. */ > >> These two members should be declared with __attribute_deprecated__. And once we > >> do that, do we still need to change their names? I don't recall any other > >> instance of such name-changing. > > > > The name changing is coming from a suggestion here [1]. I do find it a > > little aggressive though as I do see some build failures, specifically > > for systemd. > > Good point. systemd does this: > > tz.tz_minuteswest = -minutesdelta; > tz.tz_dsttime = 0; /* DST_NONE */ > > /* > * If the RTC does not run in UTC but in local time, the very first > * call to settimeofday() will set the kernel's timezone and will warp the > * system clock, so that it runs in UTC instead of the local time we > * have read from the RTC. > */ > if (settimeofday(tv_null, &tz) < 0) > return negative_errno(); > > It seems to me that struct timezone is not actually deprecated on the > kernel side when used with settimeofday. This would be something which > has to happen before we can change its definition. The only effect I'm aware of it ever having had on the kernel side was actively harmful: applying the offset to the timestamps of all files on fatfs mounts. This has all kinds of consistency problems with DST, timezone changes, etc. and is lossy/destructive of data. Back in 2005 I recall writing a script to do a best-case repair on digital camera timestamps (correctly recorded on the fatfs in UTC) that Linux mangled via moving them to hdd under several different local timezones. systemd should not be doing this at all. If anyone wants this feature it should be a mount option tzoff=..., not a global that's secretly set to match your system tz. Rich
* Rich Felker: > On Sun, Aug 11, 2019 at 03:52:45PM +0200, Florian Weimer wrote: >> * Alistair Francis: >> >> > On Fri, Aug 9, 2019 at 6:20 PM Paul Eggert <eggert@cs.ucla.edu> wrote: >> >> >> >> Alistair Francis wrote: >> >> > - int tz_minuteswest; /* Minutes west of GMT. */ >> >> > - int tz_dsttime; /* Nonzero if DST is ever in effect. */ >> >> > + int tz_minuteswest_dep; /* Minutes west of GMT. */ >> >> > + int tz_dsttime_dep; /* Nonzero if DST is ever in effect. */ >> >> These two members should be declared with __attribute_deprecated__. And once we >> >> do that, do we still need to change their names? I don't recall any other >> >> instance of such name-changing. >> > >> > The name changing is coming from a suggestion here [1]. I do find it a >> > little aggressive though as I do see some build failures, specifically >> > for systemd. >> >> Good point. systemd does this: >> >> tz.tz_minuteswest = -minutesdelta; >> tz.tz_dsttime = 0; /* DST_NONE */ >> >> /* >> * If the RTC does not run in UTC but in local time, the very first >> * call to settimeofday() will set the kernel's timezone and will warp the >> * system clock, so that it runs in UTC instead of the local time we >> * have read from the RTC. >> */ >> if (settimeofday(tv_null, &tz) < 0) >> return negative_errno(); >> >> It seems to me that struct timezone is not actually deprecated on the >> kernel side when used with settimeofday. This would be something which >> has to happen before we can change its definition. > > The only effect I'm aware of it ever having had on the kernel side was > actively harmful: applying the offset to the timestamps of all files > on fatfs mounts. This has all kinds of consistency problems with DST, > timezone changes, etc. and is lossy/destructive of data. Back in 2005 > I recall writing a script to do a best-case repair on digital camera > timestamps (correctly recorded on the fatfs in UTC) that Linux mangled > via moving them to hdd under several different local timezones. Fair enough. I filed: <https://github.com/systemd/systemd/issues/13305> Let's see what happens. > systemd should not be doing this at all. If anyone wants this feature > it should be a mount option tzoff=..., not a global that's secretly > set to match your system tz. It looks like time_offset already exists. Thanks, Florian
On Fri, Aug 9, 2019 at 9:04 PM Alistair Francis <alistair.francis@wdc.com> wrote: > > Append the struct timezone members with '_dep'. This indicates that > these members are deprecated and will cause build failures on code that > is currently using the members. I had been working on a more thorough version of this patch, one that removed these fields altogether (replacing them with `char __preserve_historic_size[2*sizeof(int)]`) and changed _all_ of the code that touches them, within glibc, to either ignore the fields or zero them out. This patch tripped over an Alpha-only ABI headache (Alpha has compatibility versions of both gettimeofday and settimeofday) and then I ran out of time to work on it. In view of the reported behavior of systemd, though, I now think neither your approach nor mine is exactly what we should do. We should do something along these lines for the next release: - We should _not_ rename the fields of struct timezone, but we should mark them __attribute_deprecated__ so that code still using them triggers compile warnings. - We should do the same for the timezone fields of struct timeb. - Our implementation of gettimeofday should always pass NULL for struct timezone to the kernel, and write zeroes to any struct timezone argument that is supplied. (This will transitively apply to ftime.) - We should leave settimeofday alone until the kernel has an alternative means of doing the "RTC clock warp" thing that systemd is trying to do. Do you have time to work that patch up? You can get my patch-in-progress from https://sourceware.org/git/?p=glibc.git;a=log;h=refs/heads/zack/gtod-no-timezone (please grab at least the changes to manual/time.texi). If you don't have time, I will try to find time to work on it this week. zw
On Mon, 12 Aug 2019, Zack Weinberg wrote: > - Our implementation of gettimeofday should always pass NULL for > struct timezone to the kernel, and write zeroes to any struct timezone > argument that is supplied. (This will transitively apply to ftime.) Given that the kernel timezone is in fact meaningful (for the kernel's interpretation of data shared with other OSes, such as the RTC clock and some filesystem timestamps, that is kept in local time - just not for the purpose for which the timezone settings in gettimeofday / settimeofday were originally intended), I think gettimeofday should continue to read that information from the kernel when the kernel provides it (i.e., the uses of the kernel timezone mean that other syscalls do *not* supersede the gettimeofday syscall for this purpose and so access to that information from that syscall should continue to be provided).
On Mon, Aug 12, 2019 at 4:20 PM Joseph Myers <joseph@codesourcery.com> wrote: > > On Mon, 12 Aug 2019, Zack Weinberg wrote: > > > - Our implementation of gettimeofday should always pass NULL for > > struct timezone to the kernel, and write zeroes to any struct timezone > > argument that is supplied. (This will transitively apply to ftime.) > > Given that the kernel timezone is in fact meaningful (for the kernel's > interpretation of data shared with other OSes, such as the RTC clock and > some filesystem timestamps, that is kept in local time - just not for the > purpose for which the timezone settings in gettimeofday / settimeofday > were originally intended), I think gettimeofday should continue to read > that information from the kernel when the kernel provides it Insisting on this would mean that we'd have to go back to the kernel people to request a new API _before_ we could proceed with the time64 transition. I don't like that idea. Also, unlike settimeofday where we know there is at least one real, intentional user, I am betting that all or nearly all existing uses of gettimeofday with a non-NULL timezone argument are actually bugs. Instead, what if we add Linux-specific set_hw_timezone(const struct timezone *) and get_hw_timezone(struct timezone *) (sys/timex.h seems like an appropriate place for the prototypes) that will call settimeofday/gettimeofday now, or whatever new interface the kernel people invent to replace this function, in the future. We can then proceed to make both gettimeofday and settimeofday behave-as-if they always calls clock_[gs]ettime(CLOCK_REALTIME) under the hood, and this issue is decoupled from the time64 transition. zw
On Mon, 12 Aug 2019, Zack Weinberg wrote: > On Mon, Aug 12, 2019 at 4:20 PM Joseph Myers <joseph@codesourcery.com> wrote: > > > > On Mon, 12 Aug 2019, Zack Weinberg wrote: > > > > > - Our implementation of gettimeofday should always pass NULL for > > > struct timezone to the kernel, and write zeroes to any struct timezone > > > argument that is supplied. (This will transitively apply to ftime.) > > > > Given that the kernel timezone is in fact meaningful (for the kernel's > > interpretation of data shared with other OSes, such as the RTC clock and > > some filesystem timestamps, that is kept in local time - just not for the > > purpose for which the timezone settings in gettimeofday / settimeofday > > were originally intended), I think gettimeofday should continue to read > > that information from the kernel when the kernel provides it > > Insisting on this would mean that we'd have to go back to the kernel > people to request a new API _before_ we could proceed with the time64 No, but it might mean that these two functions are exceptions to the general rule of functions for 32-bit time being thin wrappers round their 64-bit counterparts. (For the existing ABIs I think keeping interfacing with the kernel timezone is also a matter of ABI compatibility for these functions, even if the new _TIME_BITS=64 version of gettimeofday just writes zeroes to the timezone.)
On Mon, Aug 12, 2019 at 8:42 AM Zack Weinberg <zackw@panix.com> wrote: > > On Fri, Aug 9, 2019 at 9:04 PM Alistair Francis > <alistair.francis@wdc.com> wrote: > > > > Append the struct timezone members with '_dep'. This indicates that > > these members are deprecated and will cause build failures on code that > > is currently using the members. > > I had been working on a more thorough version of this patch, one that > removed these fields altogether (replacing them with `char > __preserve_historic_size[2*sizeof(int)]`) and changed _all_ of the > code that touches them, within glibc, to either ignore the fields or > zero them out. This patch tripped over an Alpha-only ABI headache > (Alpha has compatibility versions of both gettimeofday and > settimeofday) and then I ran out of time to work on it. > > In view of the reported behavior of systemd, though, I now think > neither your approach nor mine is exactly what we should do. We > should do something along these lines for the next release: > > - We should _not_ rename the fields of struct timezone, but we should > mark them __attribute_deprecated__ so that code still using them > triggers compile warnings. This sounds good to me > - We should do the same for the timezone fields of struct timeb. > - Our implementation of gettimeofday should always pass NULL for > struct timezone to the kernel, and write zeroes to any struct timezone > argument that is supplied. (This will transitively apply to ftime.) I don't think this is the right idea here and reading further down the list it seems like not everyone is on board. > - We should leave settimeofday alone until the kernel has an > alternative means of doing the "RTC clock warp" thing that systemd is > trying to do. Agreed, at least for systems that have a settimeofday syscall. > > Do you have time to work that patch up? You can get my > patch-in-progress from > https://sourceware.org/git/?p=glibc.git;a=log;h=refs/heads/zack/gtod-no-timezone > (please grab at least the changes to manual/time.texi). If you don't > have time, I will try to find time to work on it this week. I do have time and am happy to do it, the problem is that WDC still doesn't have a FSF contributor agreement so from my understanding the patch couldn't be applied. Alistair > > zw
On Mon, Aug 12, 2019 at 2:26 PM Joseph Myers <joseph@codesourcery.com> wrote: > > On Mon, 12 Aug 2019, Zack Weinberg wrote: > > > On Mon, Aug 12, 2019 at 4:20 PM Joseph Myers <joseph@codesourcery.com> wrote: > > > > > > On Mon, 12 Aug 2019, Zack Weinberg wrote: > > > > > > > - Our implementation of gettimeofday should always pass NULL for > > > > struct timezone to the kernel, and write zeroes to any struct timezone > > > > argument that is supplied. (This will transitively apply to ftime.) > > > > > > Given that the kernel timezone is in fact meaningful (for the kernel's > > > interpretation of data shared with other OSes, such as the RTC clock and > > > some filesystem timestamps, that is kept in local time - just not for the > > > purpose for which the timezone settings in gettimeofday / settimeofday > > > were originally intended), I think gettimeofday should continue to read > > > that information from the kernel when the kernel provides it > > > > Insisting on this would mean that we'd have to go back to the kernel > > people to request a new API _before_ we could proceed with the time64 > > No, but it might mean that these two functions are exceptions to the > general rule of functions for 32-bit time being thin wrappers round their > 64-bit counterparts. (For the existing ABIs I think keeping interfacing > with the kernel timezone is also a matter of ABI compatibility for these > functions, even if the new _TIME_BITS=64 version of gettimeofday just > writes zeroes to the timezone.) This is what I was picturing as well, with the individual members marked as deprecated. Alistair > > -- > Joseph S. Myers > joseph@codesourcery.com
On Mon, Aug 12, 2019 at 5:26 PM Joseph Myers <joseph@codesourcery.com> wrote: > On Mon, 12 Aug 2019, Zack Weinberg wrote: > > On Mon, Aug 12, 2019 at 4:20 PM Joseph Myers <joseph@codesourcery.com> wrote: > > > On Mon, 12 Aug 2019, Zack Weinberg wrote: > > > > > > > - Our implementation of gettimeofday should always pass NULL for > > > > struct timezone to the kernel, and write zeroes to any struct timezone > > > > argument that is supplied. (This will transitively apply to ftime.) > > > > > > Given that the kernel timezone is in fact meaningful (for the kernel's > > > interpretation of data shared with other OSes, such as the RTC clock and > > > some filesystem timestamps, that is kept in local time - just not for the > > > purpose for which the timezone settings in gettimeofday / settimeofday > > > were originally intended), I think gettimeofday should continue to read > > > that information from the kernel when the kernel provides it > > > > Insisting on this would mean that we'd have to go back to the kernel > > people to request a new API _before_ we could proceed with the time64 > > (For the existing ABIs I think keeping interfacing > with the kernel timezone is also a matter of ABI compatibility for these > functions, even if the new _TIME_BITS=64 version of gettimeofday just > writes zeroes to the timezone.) I really can't agree with that. At all. The kernel time zone is a trip hazard for the overwhelming majority of applications. We would probably be _fixing bugs_ (well, rendering them harmless) with the above proposed change to gettimeofday. Also, I think having gettimeofday behave differently between different architectures is a bad idea, and having gettimeofday behave differently depending on _TIME_BITS is an even worse idea. And we know we _can't_ have gettimeofday mimic the old behavior when the only available get-time syscall is clock_gettime, so... zw
Joseph Myers wrote: > it might mean that these two functions are exceptions to the > general rule of functions for 32-bit time being thin wrappers round their > 64-bit counterparts. If I'm understanding you correctly, you're suggesting that the existing-ABI get/settimeofday calls behave as before (albeit with __attribute_deprecated__ warnings at compile-time), whereas the new-ABI gettimeofday clears *TZ if TZ is nonnull and the new-ABI settimeofday ignores TZ. That way, the one or two ancient apps that rely on this stuff will still work if compiled in 32-bit time_t mode. This sounds like a good way to go forward. It might also make sense for the new-ABI gettimeofday to simply ignore its TZ argument, as that would be easier to document and explain (plus a bit faster for the typical gettimeofday case).
On Mon, Aug 12, 2019 at 8:53 PM Paul Eggert <eggert@cs.ucla.edu> wrote: > Joseph Myers wrote: > > it might mean that these two functions are exceptions to the > > general rule of functions for 32-bit time being thin wrappers round their > > 64-bit counterparts. > > If I'm understanding you correctly, you're suggesting that the existing-ABI > get/settimeofday calls behave as before (albeit with __attribute_deprecated__ > warnings at compile-time), whereas the new-ABI gettimeofday clears *TZ if TZ is > nonnull and the new-ABI settimeofday ignores TZ. This is also what I understand Joseph to be suggesting. > That way, the one or two > ancient apps that rely on this stuff will still work if compiled in 32-bit > time_t mode. This sounds like a good way to go forward. However, I don't agree with this assessment of it I think it would be confusing, and potentially a source of bugs that only affect less popular architectures, if new-ABI and old-ABI gettimeofday don't treat the TZ argument the same. And I suspect that any "ancient apps" are actually mishandling time zones and we'd be doing them a favor by making them think they're operating in UTC from now on. (systemd's use of the timezone argument to settimeofday is a separate issue.) > It might also make sense for the new-ABI gettimeofday to simply ignore its TZ > argument, as that would be easier to document and explain (plus a bit faster for > the typical gettimeofday case). That's not safe; callers that pass a non-NULL TZ argument to gettimeofday may well rely on it writing _something_ there. If it doesn't, they'll be operating on stack garbage.
diff --git a/sysdeps/unix/bsd/ftime.c b/sysdeps/unix/bsd/ftime.c index 3a1c6e9b01c..bc4dfdab945 100644 --- a/sysdeps/unix/bsd/ftime.c +++ b/sysdeps/unix/bsd/ftime.c @@ -34,7 +34,7 @@ ftime (struct timeb *timebuf) ++timebuf->time; timebuf->millitm = 0; } - timebuf->timezone = tz.tz_minuteswest; - timebuf->dstflag = tz.tz_dsttime; + timebuf->timezone = tz.tz_minuteswest_dep; + timebuf->dstflag = tz.tz_dsttime_dep; return 0; } diff --git a/time/sys/time.h b/time/sys/time.h index 5dbc7fc627f..a35ccb7a58b 100644 --- a/time/sys/time.h +++ b/time/sys/time.h @@ -51,8 +51,8 @@ __BEGIN_DECLS This is obsolete and should never be used. */ struct timezone { - int tz_minuteswest; /* Minutes west of GMT. */ - int tz_dsttime; /* Nonzero if DST is ever in effect. */ + int tz_minuteswest_dep; /* Minutes west of GMT. */ + int tz_dsttime_dep; /* Nonzero if DST is ever in effect. */ }; typedef struct timezone *__restrict __timezone_ptr_t;
Append the struct timezone members with '_dep'. This indicates that these members are deprecated and will cause build failures on code that is currently using the members. The struct timezone *tz variable contaions information on the current timezone, in this structure: struct timezone { int tz_minuteswest; /* minutes west of Greenwich */ int tz_dsttime; /* type of DST correction */ }; The members are being renamed to create compilation failures for anyone who is using them. This is being done for the following reasons. On 32-bit systems with __ARCH_WANT_TIME32_SYSCALLS not defined there is no way way to get the struct timezone via a syscall. AFAIK there are no plans to add suppor to a future kernel. The Linux documentation says that "The use of the timezone structure is obsolete; the tz argument should normally be specified as NULL." Most callers of gettimeofday() don't use the timezone data, see example code from Debian below. If __ASSUME_TIME64_SYSCALLS and __NR_clock_gettime64 are not defined then struct timezone *tz will be set as usual. Example code from Debian: struct timeval my_gettime(void) { struct timezone tz_ignored; struct timeval tv; gettimeofday(&tv, &tz_ignored); return tv; } Signed-off-by: Alistair Francis <alistair.francis@wdc.com> --- sysdeps/unix/bsd/ftime.c | 4 ++-- time/sys/time.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)