* time code progress @ 2013-07-16 19:45 Rich Felker 2013-07-17 9:33 ` Rich Felker 2013-07-22 21:38 ` Rob Landley 0 siblings, 2 replies; 15+ messages in thread From: Rich Felker @ 2013-07-16 19:45 UTC (permalink / raw) To: musl Hi all, I've been working, as promised, on the time code overhaul for the next release of musl. The main issues it's intended to address are: - complete lack of overflow handling in the present code - lack of support for zoneinfo time zones After trying various things, the design I think I've settled on has the following properties: - Internally, all POSIX times (seconds since epoch) are passed around as long long. This ensures that values derived from struct tm, timezone rules, etc. can never overflow, and allows overflow checking to be deferred until it's time to convert the value back into time_t. Without this design, handling the "partial years" at the edge of time_t overflow range is very difficult, as is handling the denormalized struct tm forms mktime is required to support and normalize (for instance, overflowing the year range by using a very large month number). - Instead of converting back and forth to broken-down struct tm for applying timezone rules, the new code simply converts the combination of year and TZ rule string to a POSIX time within the given year at which the transition occurs. This value has type long long so that it cannot overflow even at the boundary years. Determining whether DST is in effect at a given time is simply a range comparison, just like the comparison that will be used for processing zoneinfo-format timezone rules. - For years within the currently-reasonable range/32-bit time_t, the hot path for converting a year to seconds since the epoch involves no division. We can get away with >>2 and %3 since in the range [1901,2099] the leap year rule is simply that multiple-of-4 years are leap years. I would like it be able to have the compiler throw away the larger, slower, general-case code on 32-bit-time_t targets, but unfortunately the need to use long long internally to avoid difficult corner cases is precluding that. - Any representable struct tm will successfully convert to time_t if and only if it represents a value of seconds-since-the-epoch that fits in time_t. Any representable time_t will successfully convert to struct tm if and only if it represents a (normalized) date and time representable in struct tm. The relevant functions will avoid setting errno except on error so that a caller can distinguish between a time_t value of -1 as a valid result and as an error. I'm not yet sure whether the new code will be larger or smaller than the old code. It should be considerably faster, and most importantly, more correct. Rich ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: time code progress 2013-07-16 19:45 time code progress Rich Felker @ 2013-07-17 9:33 ` Rich Felker 2013-07-17 11:39 ` Szabolcs Nagy 2013-07-20 1:11 ` Szabolcs Nagy 2013-07-22 21:38 ` Rob Landley 1 sibling, 2 replies; 15+ messages in thread From: Rich Felker @ 2013-07-17 9:33 UTC (permalink / raw) To: musl On Tue, Jul 16, 2013 at 03:45:53PM -0400, Rich Felker wrote: > Hi all, > > I've been working, as promised, on the time code overhaul for the next > release of musl. The main issues it's intended to address are: Committed. Please let me know what bugs you find. :-) Rich ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: time code progress 2013-07-17 9:33 ` Rich Felker @ 2013-07-17 11:39 ` Szabolcs Nagy 2013-07-17 12:09 ` Jens Gustedt 2013-07-17 13:31 ` Rich Felker 2013-07-20 1:11 ` Szabolcs Nagy 1 sibling, 2 replies; 15+ messages in thread From: Szabolcs Nagy @ 2013-07-17 11:39 UTC (permalink / raw) To: musl * Rich Felker <dalias@aerifal.cx> [2013-07-17 05:33:25 -0400]: > Committed. Please let me know what bugs you find. :-) > gcc found this: diff --git a/src/time/__map_file.c b/src/time/__map_file.c index b6bf272..b322f09 100644 --- a/src/time/__map_file.c +++ b/src/time/__map_file.c @@ -14,7 +14,7 @@ const char unsigned *__map_file(const char *pathname, size_t *size) if (fd < 0) return 0; if (!__syscall(SYS_fstat, fd, &st)) map = __mmap(0, st.st_size, PROT_READ, MAP_SHARED, fd, 0); - __syscall(SYS_close); + __syscall(SYS_close, fd); *size = st.st_size; return map == MAP_FAILED ? 0 : map; } (i think it did not like the syscall arg counting in case of 0 args in #define __SYSCALL_NARGS_X(a,b,c,d,e,f,g,h,n,...) n #define __SYSCALL_NARGS(...) __SYSCALL_NARGS_X(__VA_ARGS__,7,6,5,4,3,2,1,0) ) ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: time code progress 2013-07-17 11:39 ` Szabolcs Nagy @ 2013-07-17 12:09 ` Jens Gustedt 2013-07-17 13:19 ` Szabolcs Nagy 2013-07-17 13:31 ` Rich Felker 1 sibling, 1 reply; 15+ messages in thread From: Jens Gustedt @ 2013-07-17 12:09 UTC (permalink / raw) To: musl [-- Attachment #1: Type: text/plain, Size: 938 bytes --] Hi, Am Mittwoch, den 17.07.2013, 13:39 +0200 schrieb Szabolcs Nagy: > (i think it did not like the syscall arg counting in case of 0 args in > #define __SYSCALL_NARGS_X(a,b,c,d,e,f,g,h,n,...) n > #define __SYSCALL_NARGS(...) __SYSCALL_NARGS_X(__VA_ARGS__,7,6,5,4,3,2,1,0) > ) yes, this is in fact not suitable to test for 0 args. For the preprocessor 0 args basically doesn't exist, there is always one argument, but which is empty. If this is really needed, I could contribute a cooked down version from P99 of an NARGS macro that could deal with such a case. Please let me know. Jens -- :: INRIA Nancy Grand Est :: http://www.loria.fr/~gustedt/ :: :: AlGorille ::::::::::::::: office Nancy : +33 383593090 :: :: ICube :::::::::::::: office Strasbourg : +33 368854536 :: :: ::::::::::::::::::::::::::: gsm France : +33 651400183 :: :: :::::::::::::::::::: gsm international : +49 15737185122 :: [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: time code progress 2013-07-17 12:09 ` Jens Gustedt @ 2013-07-17 13:19 ` Szabolcs Nagy 2013-07-17 13:39 ` Jens Gustedt 0 siblings, 1 reply; 15+ messages in thread From: Szabolcs Nagy @ 2013-07-17 13:19 UTC (permalink / raw) To: musl * Jens Gustedt <jens.gustedt@inria.fr> [2013-07-17 14:09:38 +0200]: > Am Mittwoch, den 17.07.2013, 13:39 +0200 schrieb Szabolcs Nagy: > > (i think it did not like the syscall arg counting in case of 0 args in > > #define __SYSCALL_NARGS_X(a,b,c,d,e,f,g,h,n,...) n > > #define __SYSCALL_NARGS(...) __SYSCALL_NARGS_X(__VA_ARGS__,7,6,5,4,3,2,1,0) > > ) > > yes, this is in fact not suitable to test for 0 args. For the > preprocessor 0 args basically doesn't exist, there is always one > argument, but which is empty. > note that the problem is not that __VA_ARGS__ is empty (it's not, contrary to what i might implied), but that if n becomes 0 (== __VA_ARGS__ expands to one argument), then there is no more arguments in the __SYSCALL_NARGS_X call to substitute for '...', so a simple fix would be #define __SYSCALL_NARGS(...) __SYSCALL_NARGS_X(__VA_ARGS__,7,6,5,4,3,2,1,0, tralala) but i'm not sure if this should be fixed (this is internal code and i think there are no 0 argument syscalls) i just wanted to record how i found the close without fd issue (which shows that some kind of type checking for syscall arguments would help libc hacking.. but that's non-trivial to do) ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: time code progress 2013-07-17 13:19 ` Szabolcs Nagy @ 2013-07-17 13:39 ` Jens Gustedt 2013-07-17 14:36 ` Rich Felker 0 siblings, 1 reply; 15+ messages in thread From: Jens Gustedt @ 2013-07-17 13:39 UTC (permalink / raw) To: musl [-- Attachment #1: Type: text/plain, Size: 1701 bytes --] Am Mittwoch, den 17.07.2013, 15:19 +0200 schrieb Szabolcs Nagy: > note that the problem is not that __VA_ARGS__ is empty > (it's not, contrary to what i might implied), but that > if n becomes 0 (== __VA_ARGS__ expands to one argument), > then there is no more arguments in the __SYSCALL_NARGS_X > call to substitute for '...', so a simple fix would be > > #define __SYSCALL_NARGS(...) __SYSCALL_NARGS_X(__VA_ARGS__,7,6,5,4,3,2,1,0, tralala) right, you always have the name of the syscall as first argument > but i'm not sure if this should be fixed (this is internal > code and i think there are no 0 argument syscalls) > > i just wanted to record how i found the close without fd issue > (which shows that some kind of type checking for syscall > arguments would help libc hacking.. but that's non-trivial > to do) if there are really no 0 argument syscalls #define __SYSCALL_NARGS(...) __SYSCALL_NARGS_X(__VA_ARGS__,7,6,5,4,3,2,1, tralali, tralala) would be an implementation of a first consistency test. But wait my man page of syscall(2) already has an example of a 0 argument one. Other consistency checks would probably a bit more difficult to implement. I could imagine how to check for the number of arguments of particular syscalls. Type checking would be more difficult, and would probably need some maintenance. Jens -- :: INRIA Nancy Grand Est :: http://www.loria.fr/~gustedt/ :: :: AlGorille ::::::::::::::: office Nancy : +33 383593090 :: :: ICube :::::::::::::: office Strasbourg : +33 368854536 :: :: ::::::::::::::::::::::::::: gsm France : +33 651400183 :: :: :::::::::::::::::::: gsm international : +49 15737185122 :: [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: time code progress 2013-07-17 13:39 ` Jens Gustedt @ 2013-07-17 14:36 ` Rich Felker 0 siblings, 0 replies; 15+ messages in thread From: Rich Felker @ 2013-07-17 14:36 UTC (permalink / raw) To: musl On Wed, Jul 17, 2013 at 03:39:35PM +0200, Jens Gustedt wrote: > Am Mittwoch, den 17.07.2013, 15:19 +0200 schrieb Szabolcs Nagy: > > note that the problem is not that __VA_ARGS__ is empty > > (it's not, contrary to what i might implied), but that > > if n becomes 0 (== __VA_ARGS__ expands to one argument), > > then there is no more arguments in the __SYSCALL_NARGS_X > > call to substitute for '...', so a simple fix would be > > > > #define __SYSCALL_NARGS(...) __SYSCALL_NARGS_X(__VA_ARGS__,7,6,5,4,3,2,1,0, tralala) > > right, you always have the name of the syscall as first argument > > > but i'm not sure if this should be fixed (this is internal > > code and i think there are no 0 argument syscalls) > > > > i just wanted to record how i found the close without fd issue > > (which shows that some kind of type checking for syscall > > arguments would help libc hacking.. but that's non-trivial > > to do) > > if there are really no 0 argument syscalls > > #define __SYSCALL_NARGS(...) __SYSCALL_NARGS_X(__VA_ARGS__,7,6,5,4,3,2,1, tralali, tralala) There are, e.g. getpid. So I'm not sure why this issue has never come up before. I'm guessing you have a newer gcc that added a new warning for it; my gcc does not seem to warn. Anyway, I thought of the 0-arg issue back when I came up with these macros, and it was resolved by always having the syscall number. But I failed to realize there would be no arguments for the ... slot of __SYSCALL_NARGS_X. > implement. I could imagine how to check for the number of arguments of > particular syscalls. Type checking would be more difficult, and would > probably need some maintenance. This might be more easily achieved with a grep recipe... Rich ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: time code progress 2013-07-17 11:39 ` Szabolcs Nagy 2013-07-17 12:09 ` Jens Gustedt @ 2013-07-17 13:31 ` Rich Felker 2013-07-17 14:27 ` Szabolcs Nagy 1 sibling, 1 reply; 15+ messages in thread From: Rich Felker @ 2013-07-17 13:31 UTC (permalink / raw) To: musl On Wed, Jul 17, 2013 at 01:39:06PM +0200, Szabolcs Nagy wrote: > * Rich Felker <dalias@aerifal.cx> [2013-07-17 05:33:25 -0400]: > > Committed. Please let me know what bugs you find. :-) > > > > gcc found this: > > diff --git a/src/time/__map_file.c b/src/time/__map_file.c > index b6bf272..b322f09 100644 > --- a/src/time/__map_file.c > +++ b/src/time/__map_file.c > @@ -14,7 +14,7 @@ const char unsigned *__map_file(const char *pathname, size_t *size) > if (fd < 0) return 0; > if (!__syscall(SYS_fstat, fd, &st)) > map = __mmap(0, st.st_size, PROT_READ, MAP_SHARED, fd, 0); > - __syscall(SYS_close); > + __syscall(SYS_close, fd); > *size = st.st_size; > return map == MAP_FAILED ? 0 : map; > } I'm glad to hear this is the worst bug found so far. :-) > (i think it did not like the syscall arg counting in case of 0 args in > #define __SYSCALL_NARGS_X(a,b,c,d,e,f,g,h,n,...) n > #define __SYSCALL_NARGS(...) __SYSCALL_NARGS_X(__VA_ARGS__,7,6,5,4,3,2,1,0) > ) Odd. I think 0-arg syscall is used elsewhere and works, but I agree it looks broken. Rich ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: time code progress 2013-07-17 13:31 ` Rich Felker @ 2013-07-17 14:27 ` Szabolcs Nagy 0 siblings, 0 replies; 15+ messages in thread From: Szabolcs Nagy @ 2013-07-17 14:27 UTC (permalink / raw) To: musl * Rich Felker <dalias@aerifal.cx> [2013-07-17 09:31:10 -0400]: > I'm glad to hear this is the worst bug found so far. :-) > here is another one: posix seems to require EOVERFLOW when the result cannot be represented (i cleaned up my mktime/gmtime test copied from libc-testsuit) diff --git a/src/time/mktime.c b/src/time/mktime.c index e38b461..ef1fb42 100644 --- a/src/time/mktime.c +++ b/src/time/mktime.c @@ -25,6 +25,6 @@ time_t mktime(struct tm *tm) return t; error: - errno = EINVAL; + errno = EOVERFLOW; return -1; } ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: time code progress 2013-07-17 9:33 ` Rich Felker 2013-07-17 11:39 ` Szabolcs Nagy @ 2013-07-20 1:11 ` Szabolcs Nagy 2013-07-20 1:22 ` Rich Felker 1 sibling, 1 reply; 15+ messages in thread From: Szabolcs Nagy @ 2013-07-20 1:11 UTC (permalink / raw) To: musl * Rich Felker <dalias@aerifal.cx> [2013-07-17 05:33:25 -0400]: > Committed. Please let me know what bugs you find. :-) in mktime the overflow check is not strictly ok, time_t is signed so the check relies on signed overflow long long t = __tm_to_secs(tm); // ... if ((time_t)t != t) goto error; i'm not sure how to do the check efficiently without invoking ub, but at least this should be commented time_t seem to be long on all supported platforms so this should work: if (t>LONG_MAX || t<LONG_MIN) goto error; ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: time code progress 2013-07-20 1:11 ` Szabolcs Nagy @ 2013-07-20 1:22 ` Rich Felker 2013-07-20 1:29 ` Szabolcs Nagy 0 siblings, 1 reply; 15+ messages in thread From: Rich Felker @ 2013-07-20 1:22 UTC (permalink / raw) To: musl On Sat, Jul 20, 2013 at 03:11:06AM +0200, Szabolcs Nagy wrote: > * Rich Felker <dalias@aerifal.cx> [2013-07-17 05:33:25 -0400]: > > Committed. Please let me know what bugs you find. :-) > > in mktime the overflow check is not strictly ok, > time_t is signed so the check relies on signed overflow A conversion is not an overflow. If the actual value does not fit, it results in an implementation-defined value (of the destination type) or an implementation-defined signal. I'm happy assuming our implementation does not do the latter, and even that it performs the conversion via modular reduction, but we don't need that assumption here. The fact that, if t does not fit in time_t, then NO value of type time_t can ever be equal to t, is all we need. > time_t seem to be long on all supported platforms so this should work: > > if (t>LONG_MAX || t<LONG_MIN) goto error; And then we would have to fix it when we add x32... Rich ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: time code progress 2013-07-20 1:22 ` Rich Felker @ 2013-07-20 1:29 ` Szabolcs Nagy 2013-07-20 1:35 ` Rich Felker 0 siblings, 1 reply; 15+ messages in thread From: Szabolcs Nagy @ 2013-07-20 1:29 UTC (permalink / raw) To: musl * Rich Felker <dalias@aerifal.cx> [2013-07-19 21:22:26 -0400]: > A conversion is not an overflow. If the actual value does not fit, it > results in an implementation-defined value (of the destination type) you are right sorry for the noise ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: time code progress 2013-07-20 1:29 ` Szabolcs Nagy @ 2013-07-20 1:35 ` Rich Felker 0 siblings, 0 replies; 15+ messages in thread From: Rich Felker @ 2013-07-20 1:35 UTC (permalink / raw) To: musl On Sat, Jul 20, 2013 at 03:29:13AM +0200, Szabolcs Nagy wrote: > * Rich Felker <dalias@aerifal.cx> [2013-07-19 21:22:26 -0400]: > > A conversion is not an overflow. If the actual value does not fit, it > > results in an implementation-defined value (of the destination type) > > you are right > sorry for the noise No problem at all. I was actually very happy to see that somebody is reviewing the time code. :-) Rich ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: time code progress 2013-07-16 19:45 time code progress Rich Felker 2013-07-17 9:33 ` Rich Felker @ 2013-07-22 21:38 ` Rob Landley 2013-07-22 21:46 ` Rich Felker 1 sibling, 1 reply; 15+ messages in thread From: Rob Landley @ 2013-07-22 21:38 UTC (permalink / raw) To: musl; +Cc: musl On 07/16/2013 02:45:53 PM, Rich Felker wrote: > Hi all, > > I've been working, as promised, on the time code overhaul for the next > release of musl. The main issues it's intended to address are: This should be in the wiki somewhere. There should be a design section. Rob ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: time code progress 2013-07-22 21:38 ` Rob Landley @ 2013-07-22 21:46 ` Rich Felker 0 siblings, 0 replies; 15+ messages in thread From: Rich Felker @ 2013-07-22 21:46 UTC (permalink / raw) To: musl On Mon, Jul 22, 2013 at 04:38:38PM -0500, Rob Landley wrote: > On 07/16/2013 02:45:53 PM, Rich Felker wrote: > >Hi all, > > > >I've been working, as promised, on the time code overhaul for the next > >release of musl. The main issues it's intended to address are: > > This should be in the wiki somewhere. There should be a design section. Hmm, I was going to say "this belongs in the documentation", but the wiki would be a great place to stage things that eventually belong in the docs for 1.0. Rich ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2013-07-22 21:46 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-07-16 19:45 time code progress Rich Felker 2013-07-17 9:33 ` Rich Felker 2013-07-17 11:39 ` Szabolcs Nagy 2013-07-17 12:09 ` Jens Gustedt 2013-07-17 13:19 ` Szabolcs Nagy 2013-07-17 13:39 ` Jens Gustedt 2013-07-17 14:36 ` Rich Felker 2013-07-17 13:31 ` Rich Felker 2013-07-17 14:27 ` Szabolcs Nagy 2013-07-20 1:11 ` Szabolcs Nagy 2013-07-20 1:22 ` Rich Felker 2013-07-20 1:29 ` Szabolcs Nagy 2013-07-20 1:35 ` Rich Felker 2013-07-22 21:38 ` Rob Landley 2013-07-22 21:46 ` Rich Felker
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/musl/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).