Repository navigation
fix(libc): pick strftime and asctime names from a doubled 32-bit index - #12573
Conversation
The libc.prx that titles ship (PPSA07923, PPSA05916) finds the name of tm_wday or tm_mon at field 2 * value of a table that alternates short and full names, plus one for the full name, computed in 32-bit int. A negative field gives the first short name and a field past the table gives an empty name. The doubling wraps, so values from 2^30 up print "Sun" or "Jan" where main printed nothing, negative values whose doubling turns positive print nothing, and INT_MIN gives the full "Sunday" or "January" for %A and %B. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — looks good
The wrapped 32-bit table-index arithmetic matches the reported title measurements, and the PR adds boundary coverage for the changed strftime paths.
What I ran
- Reviewed PR head
04e27444e0429d6858a2aa4abb1abea136c011d7against current base319621ba2e578bd053b0baba2877fe70520e0737; merge preview is clean (836bea1ee7fc75c9e8e4a886d7adcb6c31b58403). - Environment: Linux x86_64; no local project build or test run.
- Commands:
git diff --check upstream/main...refs/review/pr-12573-current
git merge-tree --write-tree upstream/main refs/review/pr-12573-currentResults
| Check | Result |
|---|---|
| Diff check | Passed |
Merge preview onto current main |
Clean |
| Author-reported title comparison | 20,000 random asctime cases: 0 differences at this head; 15 boundary values across five strftime name forms: 0 differences |
| Hosted Windows test and Ubuntu/Windows relinker, ASan, register-context jobs | Passed |
| Hosted macOS jobs | Queued |
| Hosted product Linux job | Skipped |
| Local build/test | Not run |
Command output from this review
diff_check_exit=0
836bea1ee7fc75c9e8e4a886d7adcb6c31b58403
Negative control
| Assertion | Change used | Result |
|---|---|---|
Wrapped %a, %A, %b, %B, %h values match the title behavior |
Author reports running the new test against the previous Time.cpp |
Failed with the old Sun/Jan result where the new expectation requires Sunday/January for INT_MIN |
I did not run this negative control locally; it is reported in the PR.
Findings
Blocking
None found in the reviewed change.
Non-blocking
- The helper also changes
asctimethrough its sharedtableNamecalls. The PR reports a large native comparison forasctime, but the committed boundary regression cases exercisestrftime; a deterministicCheckAsctimecase for one or two wrapped values would make that changed path easier to guard against future regressions. - The title comparison and negative-control results are author-reported and were not independently reproduced here. MacOS CI remained queued; product Linux was skipped.
Sources
- Current PR arithmetic in
Time.cpp - Current PR call sites, including
asctime - Current PR boundary tests
- PR test evidence and limitations
Not verified
I did not run the project tests locally or reproduce the scratch-loader comparisons. The author reports those results in the PR; macOS checks were queued and hosted product Linux was skipped.
To get this to a good state
No code change is required based on this review. If the head changes, I will review the new diff and rerun the same diff and merge checks.
Correction — asctime source linkThe review's “call sites, including |
rooklz
left a comment
There was a problem hiding this comment.
I reviewed head 04e27444e0 on macOS. The body only covers Windows.
Setup. macOS 26.2, Apple clang, -arch x86_64 under Rosetta, -O1 -fsanitize=address,undefined -fno-sanitize-recover=undefined, the target's -include HostStandardStreams.hpp. Standalone build of Time.cpp, Environment.cpp, Multibyte.cpp with GuestStrftime.cpp.
- Head:
guest_strftimepasses with no sanitizer reports. main'sTime.cppwith the new test fails on exactly the wrapped rows (1073741824,1526330116,2147483647→|||, and the negative rows), matching the body.
Mutants of tableName, each run with and without sanitizers:
| mutant | result |
|---|---|
| 64-bit field, no wrap | killed (INT_MIN row) |
| full flag always 0 | killed |
negative → "" |
killed |
negative → names[0] |
killed |
>= → > past the table |
killed (ASan / segfault) |
>= 2*count - 1 |
killed |
| parity swapped | killed |
* 4u |
killed |
| full flag always 1 | survives, equivalent: for short tables names == shortNames, so an odd field picks the same short name, and +1 on an even 32-bit value never crosses 0 or 2*count |
parity dropped (names[field / 2]) |
survives, equivalent for the same reason (short tables share the pointer; full-table fields are always odd) |
asctime keeps the old index logic |
survives: none of the asctime checks uses a wrapped value |
The body says asctime changes too (632 → 0 on the native comparison), but the test doesn't pin that path. Here's a test-only diff that kills it (head passes, the old logic fails on wrapped high names):
@@ -271,6 +271,14 @@
past.tm_wday = 7;
past.tm_mon = 12;
CheckAsctime("names past the tables", past, " 2 13:05:09 2026\n");
+ GuestTm wrappedHigh = base;
+ wrappedHigh.tm_wday = 1073741824;
+ wrappedHigh.tm_mon = 1073741824;
+ CheckAsctime("wrapped high names", wrappedHigh, "Sun Jan 2 13:05:09 2026\n");
+ GuestTm wrappedLow = base;
+ wrappedLow.tm_wday = -1073741825;
+ wrappedLow.tm_mon = -1073741825;
+ CheckAsctime("wrapped low names", wrappedLow, " 2 13:05:09 2026\n");
GuestTm big = base;I derived these expectations from the rule this PR measured, not from the console. 1073741824 * 2 wraps to 0, which gives the first short name. (uint32)-1073741825 * 2 is 0x7FFFFFFE, which is positive and past the table, so the name is empty. Both match your strftime rows. Please confirm them with your native loader before you take the diff.
Merges. check_conventions passes. git merge-tree is clean against 319621ba2e and against your #12574 (both touch Time.cpp). pr_overlap only lists uncomparable shares (#5021, #9447, #10409).
Approving. The asctime diff is optional hardening.
asctime shares tableName with strftime. Two fixed cases from the libc.prx of PPSA07923 and PPSA05916: a weekday of 2^30 with a month of -1073741825, and the reverse with INT_MIN. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
|
@MarcZX10 thanks. Your non-blocking point is addressed in
AI-assisted: yes (Claude Code). |
|
Linux run at this PR's exact head Arch Linux, kernel 7.2.9, Ryzen 9 7950X, g++ (GCC) 16.2.1 20260810, glibc 2.44, x86-64. Release,
|
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — looks good
The follow-up test-only commit adds the asctime wrap cases requested in review, and the exact-head Linux report plus the existing Windows comparison and independent macOS sanitizer run support the 32-bit table-index behavior; I found no blocker.
What I ran
- Reviewed exact head
fa96aaf92b41dca6a9190dd9c2251555ad331541against currentmain52c09d567d2fa453494dd720fcf53a98149468e4. - Ran
git merge-tree --write-tree,git diff --check, and inspected the implementation and newstrftime/asctimecases. - Checked the author’s Linux report on this exact head and the independent Windows/macOS review reports.
Results
| Check | Result |
|---|---|
| Current-main preview | Clean; tree 540eb089fa3a57013713a19f745c0a05dbd9496d |
| Diff whitespace | Passed; no output |
| Linux focused tests | Author reports guest_strftime passes 3/3 on exact head fa96aaf92b |
| Windows/title comparison | Author reports zero differences for tested strftime boundary values and 20,000 random asctime dates against two title libc.prx versions; four focused test targets pass 3/3 |
| macOS sanitizer report | Independent reviewer reports guest_strftime passes with ASan/UBSan on the earlier implementation head; the current added cases are test-only |
| Hosted checks | Windows, conventions, eligibility, labels, report, relinker matrix, ASan, register-context and write-watch pass; Linux product job skipped |
Raw output
$ git merge-tree --write-tree refs/review/main-20261029b refs/review/pr-12573-20261029
540eb089fa3a57013713a19f745c0a05dbd9496d
exit=0
$ git diff --check refs/review/main-20261029b...refs/review/pr-12573-20261029
(no output)
The test and title-comparison results above are author/reviewer-reported; I did not run them locally.
Negative control
- The author reports that reverting
Time.cppto the previous implementation makes the new wrapped-name regression fail withexpected "Sun|Sunday|Jan|January", received "Sun|Sun|Jan|Jan". I did not rerun this control locally.
Findings
Blocking
- None found.
Non-blocking
- The title comparison covers only the C locale, as the author notes. Current
mainhas advanced since the PR base, but the merge preview is clean and it has not changed the touched implementation/test paths.
Sources
- Implementation at reviewed head
- Regression tests at reviewed head
- Contribution rules at current main
Not verified
- I did not locally build/run the test, reproduce the title-libc comparison, or test non-C locales. The cited behavioral comparison is author-provided.
What
strftime(%a,%A,%b,%h,%B, including%c) andasctimenow pick the weekday and month names for a hugetm_wdayortm_monthe way thelibc.prxthat titles ship does.That library looks a name up at field
2 * valueof a table that alternates short and full names,+ 1for a full name, computed in 32-bitint:mainalready gave the first short name for a negative value and an empty name past the table, so every value from -3 to 14 in the existing test is unchanged. The doubling, though, wraps:main1073741824to2147483647(2^30 up), e.g.%a/%A/%b/%BSun/Sun/Jan/Jan-1073741825,-1884104312Sun/Sun/Jan/JanINT_MIN,%A/%BSunday/JanuarySun/JantableNamenow computes that field:(int32)((uint32)value * 2 + full). Its four callers are unchanged.Tested
Windows 11, WinLibs GCC 15.2.0 UCRT r7, Release,
main319621ba2e.strftimeandasctimeby NID next to this build's, using thelibc.prxof PPSA07923 and of PPSA05916 (same results on both).asctime: 20,000 random dates, a quarter of the fields out of range and an eighth of those any 32-bit value. Differences go from 632 to 0.strftime:%a,%A,%b,%Band%hat 15 chosen values, fromINT_MINthrough the 2^30 boundaries toINT_MAX. Differences go from 27 of 75 to 0.strftimeconversion with in-range dates, plus out-of-range fields from -20 to 59 and buffers of 1 to 40 bytes, already matched and still does, at 3,000 calls each.guest_strftimegains the wrapped values, with expectations taken from the titles' output. With the previousTime.cppit fails (expected "Sun|Sunday|Jan|January", received "Sun|Sun|Jan|Jan").guest_strftime,guest_mktime,guest_localeandguest_crtpass 3/3. Conventions pass.Limitation: only the C locale was compared.
Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)🤖 Generated with Claude Code