-
Notifications
You must be signed in to change notification settings - Fork 602
Fix 5.42 regression with strftime() and daylight savings #23921
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
khwilliamson
wants to merge
10
commits into
Perl:blead
Choose a base branch
from
khwilliamson:strftime
base: blead
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+241
−111
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The next commits that fix some bugs showed these were not properly getting initialized.
On some systems this was unused. Now that we have C99, we can move the declaration and some #ifdef's and not declare it unless it is going to be used.
tl;dr: Fixes GH Perl#23878 I botched this in Perl 5.42. These conditional compilation statements were just plain wrong, causing code to be skipped that should have been compiled. It only affected the few hours of the year when daylight savings time is removed, so that the hour value is repeated. We didn't have a good test for that. gory details: libc uses 'struct tm' to hold information about a given instant in time, containing fields for things like the year, month, hour, etc. The libc function mktime() is used to normalize the structure, adjusting, say, an input Nov 31 to be Dec 01. One of the fields in the structure, 'is_dst', indicates if daylight savings is in effect, or whether that fact is unknown. If unknown, mktime() is supposed to calculate the answer and to change 'is_dst' accordingly. Some implementations appear to always do this calculation even when the input value says the result is known. Others appear to honor it. Some libc implementations have extra fields in 'struct tm'. Perl has a stripped down version of mktime(), called mini_mktime(), written by Larry Wall a long time ago. I don't know why. This crippled version ignores locale and daylight time. It also doesn't know about the extra fields in 'struct tm' that some implementations have. Nor can it be extended to know about those fields, as they are dependent on timezone and daylight time, which it deliberately doesn't consider. The botched #ifdef's were supposed to compensate for both the extra fields in the struct and that some libc implementations always recalculate 'is_dst'. On systems with these fields, the botched #if's caused only mini_mktime() to be called. This meant that these extra fields didn't get populated, and daylight time is never considered to be in effect. And 'is_dst' does not get changed from the input. On systems without these fields, the regular libc mktime() would be called appropriately. The bottom line is that for the portion of the year when daylight savings is not in effect, that portion worked properly. The two extra fields would not be populated, so if some code were to read them, it would only get the proper values by chance. We got no reports of this. I attribute that to the fact that the use of these is not portable, so code wouldn't tend to use them. There are portable ways to access the information they contain. Tests were failing for the portions of the year when daylight savings is in effect; see GH Perl#22351. The code looked correct just reading it (not seeing the flaw in the #ifdef's), so I assumed that it was an issue in the libc implementations and instituted a workaround. (I can't now think of a platform where there hasn't been a problem with a libc with something regarding locales, so that was a reasonable assumption.) Among other things (fixed in the next commit), that workaround overrode the 'is_dst' field after the call to mini_mktime(), so that the value actually passed to libc strftime() indicated that daylight is in effect. What happens next depends on the libc strftime() implementation. It could conceivably itself call mktime() which might choose to override is_dst to be the correct value, and everything would always work. The more likely possibility is that it just takes the values in the struct as-is. Remember that those values on systems with the extra fields were calculated as if daylight savings wasn't in effect, but now we're telling strftime() to use those values as if it were in effect. This is a discrepancy. I'd have to trace through some libc implementations to understand why this discrepancy seems to not matter except at the transition time. But the bottom line is this commit removes that discrepancy, and causes mktime() to be called appropriately on systems where it wasn't, so strftime() should now function properly.
Because of the bug fixed in the previous commit, this function was changed in 5.42 to have a work around, which is no longer needed.
Because of the bug fixed two commits ago, this function was changed in 5.42 to have a work around, which is no longer needed.
Due to the differences in various systems' implementations, I think it is a good idea to more fully document the vagaries I have discovered, and how perl resolves them.
159c36b to
61bab2c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
tl;dr:
Fixes #23878
I botched this in Perl 5.42. These conditional compilation statements in locale.c were just plain wrong, causing code to be skipped that should have been compiled. It only affected the few hours of the year when daylight savings time is removed, so that the hour value is repeated. We didn't have a good test for that.
gory details:
libc uses 'struct tm' to hold information about a given instant in time, containing fields for things like the year, month, hour, etc. The libc function mktime() is used to normalize the structure, adjusting, say, an input Nov 31 to be Dec 01.
One of the fields in the structure, 'is_dst', indicates if daylight savings is in effect, or whether that fact is unknown. If unknown, mktime() is supposed to calculate the answer and to change 'is_dst' accordingly. Some implementations appear to always do this calculation even when the input value says the result is known. Others appear to honor it.
Some libc implementations have extra fields in 'struct tm'.
Perl has a stripped down version of mktime(), called mini_mktime(), written by Larry Wall a long time ago. I don't know why. This crippled version ignores locale and daylight time. It also doesn't know about the extra fields in 'struct tm' that some implementations have. Nor can it be extended to know about those fields, as they are dependent on timezone and daylight time, which it deliberately doesn't consider.
The botched #ifdef's were supposed to compensate for both the extra fields in the struct and that some libc implementations always recalculate 'is_dst'.
On systems with these fields, the botched #if's caused only mini_mktime() to be called. This meant that these extra fields didn't get populated, and daylight time is never considered to be in effect. And 'is_dst' does not get changed from the input.
On systems without these fields, the regular libc mktime() would be called appropriately.
The bottom line is that for the portion of the year when daylight savings is not in effect, things mostly worked properly. The two extra fields would not be populated, so if some code were to read them, it would only get the proper values by chance. We got no failure reports of this. I attribute that to the fact that the use of these is not portable, so code wouldn't tend to use them. There are portable ways to access the information they contain.
Tests were failing for the portions of the year when daylight savings is in effect; see GH #22351. The code looked correct just reading it (not seeing the flaw in the #ifdef's), so I assumed that it was an issue in the libc implementations and instituted a workaround. (I can't now think of a platform where there hasn't been a problem with a libc with something regarding locales, so that was a reasonable assumption.)
Among other things that workaround overrode the 'is_dst' field after the call to mini_mktime(), so that the value actually passed to libc strftime() indicated that daylight is in effect.
What happens next depends on the libc strftime() implementation. It could conceivably itself call mktime() which might choose to override is_dst to be the correct value, and everything would always work. The more likely possibility is that it just takes the values in the struct as-is. Remember that those values on systems with the extra fields were calculated as if daylight savings wasn't in effect, but now we're telling strftime() to use those values as if it were in effect. This is a discrepancy. I'd have to trace through some libc implementations to understand why this discrepancy seems to not matter except at the transition time.
But the bottom line is this p.r removes that discrepancy, and causes mktime() to be called appropriately on systems where it wasn't, so strftime() should now function properly. And the workarounds are also removed.
This regression fix should go into a maintenance release.