locale.c: use type-agnostic code for storing tm_zone #21949
Conversation
On some platforms, struct tm has a tm_zone member, but its type is not consistent: - Linux: const char *tm_zone; - FreeBSD (& probably other BSDs): char *tm_zone; In order to save/restore tm_zone, we can't just use a "const char *" or "char *" variable because different parts of this code would always be a const violation on one platform or the other. Workaround: Use the tm_zone member of a full struct tm, which has the right type no matter the platform. Fixes Perl#21948.
|
@mauke, could you create a |
|
@jkeenan Done (I think). |
| /* But since mini_mktime() doesn't handle these fields, save the | ||
| * results we already have for them */ | ||
| # ifdef HAS_TM_TM_GMTOFF | ||
| long int gmtoff = mytm->tm_gmtoff; |
There was a problem hiding this comment.
Is this general enough that we don't have to use your struct tm method for this field too? When writing this, I was surprised that this was a long, since I can't imagine it ever exceeding 14 * 60 * 60 (for double daylight time 12 timezones away from gmt).
There was a problem hiding this comment.
14 * 60 * 60 = 50400, which exceeds the range of a 16-bit signed integer type (which is all C requires from int). To get a type that is guaranteed to be at least 32 bits wide, you need long int (or int_least32_t, but that's only C99, I believe).
I removed the gmtoff local variable both for symmetry with zone and because I'd already "paid" for an entire struct tm on the stack, so why not make use of its fields?
There was a problem hiding this comment.
somehow I misread this until I downloaded it and looked on my box. You already had done it the way I was suggesting.
|
On 2/8/24 11:18, mauke wrote:
@jkeenan <https://github.com/jkeenan> Done (I think).
Thanks. Smoking now on the machine from which I filed ticket.
|
|
This is yours, I think? https://perl5.test-smoke.org/report/5049091 |
Yes, it is from my machine. However, I really wish that you hadn't been so quick to merge this branch into blead. The overwhelming majority of our *BSD smoke-test reports are coming from Carlos, including the ones where compilation was done with g++ or clang++ and where I spotted these build-time failures in the first place. My impression is that most of our non-GH smokers run on a 24- or 48-hour cycle, so I generally try to give a |
|
I blame this one on:
Also:
But I'll try to keep it mind for next time. |
|
I disagree that we should have waited. My assertion is that getting blead back to being unbroken is higher priority than waiting for smoke reports that the fix works everywhere. blead breakages are a potential issue in perpetuity for future bisections, condemning our future selves and our successors to have to do extra quite unexpected, unwelcome research should a bisect land in just the wrong place. This effort dwarfs the consequences of the current fix being incomplete. In this case, looking at the fix, and the analysis of the problem, it was quite unlikely that it would work for just one and not the other boxes. And then there is the immediate problem. I have smokes that I started today that were ruined by this problem, and had to be restarted. (I realize this is ironic, since I was the one who created the problem.) And I refreshed blead and restarted them; now they are ruined again by #21957 not being yet merged. To get anything like timely results, I will have to apply that fix to my smokes and restart them. The bottom line is that minimizing the period that blead is broken is a big deal. |
4 participants
On some platforms, struct tm has a tm_zone member, but its type is not consistent:
In order to save/restore tm_zone, we can't just use a "const char *" or "char *" variable because different parts of this code would always be a const violation on one platform or the other.
Workaround: Use the tm_zone member of a full struct tm, which has the right type no matter the platform.
Fixes #21948.