From e78caca8144e26d9096bd78e6abdcb6b59a22379 Mon Sep 17 00:00:00 2001 From: Karl Williamson Date: Wed, 3 Apr 2024 17:23:33 -0600 Subject: [PATCH] Revert not using mini_mktime() This fixes GH #22062, which has extensive discussion not repeated in this commit message --- embed.fnc | 2 +- embed.h | 2 +- ext/POSIX/lib/POSIX.pod | 18 ++++--- ext/POSIX/t/time.t | 4 -- locale.c | 105 +++++++++++++--------------------------- 5 files changed, 48 insertions(+), 83 deletions(-) diff --git a/embed.fnc b/embed.fnc index a6ba6e5b61c1..6903959bc861 100644 --- a/embed.fnc +++ b/embed.fnc @@ -3409,7 +3409,7 @@ Adm |bool |sv_streq |NULLOK SV *sv1 \ Adp |bool |sv_streq_flags |NULLOK SV *sv1 \ |NULLOK SV *sv2 \ |const U32 flags -Adp |SV * |sv_strftime_ints \ +EXpx |SV * |sv_strftime_ints \ |NN SV *fmt \ |int sec \ |int min \ diff --git a/embed.h b/embed.h index 967f3098cb73..df70b1c2e083 100644 --- a/embed.h +++ b/embed.h @@ -748,7 +748,6 @@ # define sv_setuv(a,b) Perl_sv_setuv(aTHX_ a,b) # define sv_setuv_mg(a,b) Perl_sv_setuv_mg(aTHX_ a,b) # define sv_streq_flags(a,b,c) Perl_sv_streq_flags(aTHX_ a,b,c) -# define sv_strftime_ints(a,b,c,d,e,f,g,h,i,j) Perl_sv_strftime_ints(aTHX_ a,b,c,d,e,f,g,h,i,j) # define sv_strftime_tm(a,b) Perl_sv_strftime_tm(aTHX_ a,b) # define sv_string_from_errnum(a,b) Perl_sv_string_from_errnum(aTHX_ a,b) # define sv_tainted(a) Perl_sv_tainted(aTHX_ a) @@ -1765,6 +1764,7 @@ # define skipspace_flags(a,b) Perl_skipspace_flags(aTHX_ a,b) # define sv_magicext_mglob(a) Perl_sv_magicext_mglob(aTHX_ a) # define sv_only_taint_gmagic Perl_sv_only_taint_gmagic +# define sv_strftime_ints(a,b,c,d,e,f,g,h,i,j) Perl_sv_strftime_ints(aTHX_ a,b,c,d,e,f,g,h,i,j) # define utf16_to_utf8_base(a,b,c,d,e,f) Perl_utf16_to_utf8_base(aTHX_ a,b,c,d,e,f) # define utf8_to_utf16_base(a,b,c,d,e,f) Perl_utf8_to_utf16_base(aTHX_ a,b,c,d,e,f) # define validate_proto(a,b,c,d) Perl_validate_proto(aTHX_ a,b,c,d) diff --git a/ext/POSIX/lib/POSIX.pod b/ext/POSIX/lib/POSIX.pod index d3720da9b50e..177061505852 100644 --- a/ext/POSIX/lib/POSIX.pod +++ b/ext/POSIX/lib/POSIX.pod @@ -1825,19 +1825,23 @@ Identical to the string form of C<$!>, see L. =item C -Convert date and time information to string. Returns the string. +Convert date and time information to string based on the current +underlying locale of the program (except for any daylight savings time). +Returns the string. Synopsis: strftime(fmt, sec, min, hour, mday, mon, year, wday = -1, yday = -1, isdst = -1) -The month (C), weekday (C), and yearday (C) begin at zero, -I, January is 0, not 1; Sunday is 0, not 1; January 1st is 0, not 1. The -year (C) is given in years since 1900, I, the year 1995 is 95; the +The month (C) begins at zero, +I, January is 0, not 1. The +year (C) is given in years since 1900, I, the year 1995 is 95; the year 2001 is 101. Consult your system's C manpage for details about these and the other arguments. +The C, C, and C parameters are all ignored. + If you want your code to be portable, your format (C) argument should use only the conversion specifiers defined by the ANSI C standard (C99, to play safe). These are C. @@ -1853,9 +1857,11 @@ safest route. The given arguments are made consistent as though by calling C before calling your system's C function, -except that the C value is not affected. +except that the C value is not affected, so that the returned +value will always be as if the locale doesn't have daylight savings +time. -The string for Tuesday, December 12, 1995. +The string for Tuesday, December 12, 1995 in the C locale. $str = POSIX::strftime( "%A, %B %d, %Y", 0, 0, 0, 12, 11, 95, 2 ); diff --git a/ext/POSIX/t/time.t b/ext/POSIX/t/time.t index bbfecd76a402..44cba60654ce 100644 --- a/ext/POSIX/t/time.t +++ b/ext/POSIX/t/time.t @@ -26,9 +26,6 @@ SKIP: { $^O eq "MSWin32" || $^O eq "interix"; tzset(); SKIP: { - TODO: { - local $TODO = "Make sure this test works to expose the problem"; - my @tzname = tzname(); # See extensive discussion in GH #22062. @@ -36,7 +33,6 @@ SKIP: { is(strftime("%Y-%m-%d %H:%M:%S", 0, 30, 2, 10, 2, 124, 0, 0, 0), "2024-03-10 02:30:00", "strftime() doesnt pay attention to dst"); - }; } } diff --git a/locale.c b/locale.c index 49d35c303e6b..6f479d90385c 100644 --- a/locale.c +++ b/locale.c @@ -8058,31 +8058,38 @@ S_maybe_override_codeset(pTHX_ const char * codeset, /* =for apidoc_section $time =for apidoc sv_strftime_tm -=for apidoc_item sv_strftime_ints =for apidoc_item my_strftime -These implement the libc strftime(), but with a different API so that the return -value is a pointer to the formatted result (which MUST be arranged to be FREED -BY THE CALLER). This allows these functions to increase the buffer size as -needed, so that the caller doesn't have to worry about that. +These implement the libc strftime(). On failure, they return NULL, and set C to C. -C and C are preferred, as they transparently -handle the UTF-8ness of the current locale, the input C, and the returned -result. Only if the current C locale is a UTF-8 one (and S> is not in effect) will the result be marked as UTF-8. These differ -only in the form of their inputs. C takes a filled-in -S> parameter. C takes a bunch of integer -parameters that together completely define a given time. +C is preferred, as it transparently handles the UTF-8ness of +the current locale, the input C, and the returned result. Only if the +current C locale is a UTF-8 one (and S> is not in effect) +will the result be marked as UTF-8. + +C takes a pointer to a filled-in S> parameter. It +ignores the values of the C and C fields in it. The other fields +give enough information to accurately calculate these values, and are used for +that purpose. + +The caller assumes ownership of the returned SV with a reference count of 1. C is kept for backwards compatibility. Knowing if its result should be considered UTF-8 or not requires significant extra logic. -Note that C and C effectively are ignored by C -and C, as mini_mktime() overwrites them +The return value is a pointer to the formatted result (which MUST be arranged +to be FREED BY THE CALLER). This allows this function to increase the buffer +size as needed, so that the caller doesn't have to worry about that, unlike +libc C. + +The C, C, and C parameters are ignored by C. +Daylight savings time is never considered to exist, and the values returned for +the other two fields (if C even calls for them) are calculated from the +other parameters, without need for referencing these. -Also note that all three functions are always executed in the underlying +Note that both functions are always executed in the underlying C locale of the program, giving results based on that locale. =cut @@ -8205,74 +8212,30 @@ S_ints_to_tm(pTHX_ struct tm * mytm, mytm->tm_yday = yday; mytm->tm_isdst = isdst; -/* If no mktime() can't call it; and if it doesn't have the extra fields that - * mini_mktime() doesn't handle, no need to call it; use our faster internal - * mini_mktime() instead */ -#if ! defined(HAS_MKTIME) || ( ! defined(HAS_TM_TM_GMTOFF) \ - && ! defined(HAS_TM_TM_ZONE)) - + /* Long-standing behavior is to ignore the effects of locale (in + * particular, daylight savings time) on the input, so we use mini_mktime. + * See GH #22062. */ mini_mktime(mytm); - PERL_UNUSED_ARG(locale); -#else + /* But some of those effect are deemed desirable, so use libc to get the + * values for tm_gmtoff and tm_zone on platforms that have them [perl + * #18238] */ +#if defined(HAS_MKTIME) \ + && (defined(HAS_TM_TM_GMTOFF) || defined(HAS_TM_TM_ZONE)) - /* Otherwise we have to use the (slower) libc mktime(), which does take - * locale into consideration. [perl #18238] */ const char * orig_TIME_locale = toggle_locale_c(LC_TIME, locale); + struct tm mytm2 = *mytm; MKTIME_LOCK; - - (void) mktime(mytm); - + mktime(&mytm2); MKTIME_UNLOCK; restore_toggled_locale_c(LC_TIME, orig_TIME_locale); - /* More is needed if this is the very unlikely case of a leap second - * (sec==60). Various mktime() routines handle these differently: - * a) Some don't know about their existence and normalize them to 0 - * seconds in the next minute; - * b) Others do the same, except if a table lookup indicates there - * actually was an officially declared leap second at the instant the - * input gives, in which case they leave the second at 60, and don't - * adjust the minute. - * c) mini_mktime takes the caller's word for it that this is an actual - * leap second, and leaves it at 60. - * In all cases, if the input was 60 and the result is not, libc did not - * act like mini_mktime, and to preserve long-standing behavior, we call - * mini_mktime for its result. - * - * (As an aside, this means the return of these, if we didn't ignore it - * here, is not consistent across platforms.) */ - if (UNLIKELY(sec == 60 && mytm->tm_sec != 60)) { - - /* But since mini_mktime() doesn't handle these fields, save the - * results we already have for them */ -# if defined(HAS_TM_TM_GMTOFF) || defined(HAS_TM_TM_ZONE) - struct tm mytm2 = *mytm; -# endif - - /* Repeat the entire process with mini_mktime */ - Zero(mytm, 1, struct tm); - mytm->tm_sec = sec; - mytm->tm_min = min; - mytm->tm_hour = hour; - mytm->tm_mday = mday; - mytm->tm_mon = mon; - mytm->tm_year = year; - mytm->tm_wday = wday; - mytm->tm_yday = yday; - mytm->tm_isdst = isdst; - (void) mini_mktime(mytm); - - /* And use the saved libc values for tm_gmtoff and tm_zone */ # ifdef HAS_TM_TM_GMTOFF - mytm->tm_gmtoff = mytm2.tm_gmtoff; + mytm->tm_gmtoff = mytm2.tm_gmtoff; # endif # ifdef HAS_TM_TM_ZONE - mytm->tm_zone = mytm2.tm_zone; + mytm->tm_zone = mytm2.tm_zone; # endif - - } - #endif return;