Repository navigation
fix(libc): read swscanf scanset ranges and string lengths as titles do - #12420
Conversation
|
I independently built the focused scanner targets from head
All three passed under The scanset loop consumes One coordination point: the overlap report flags the shared |
|
@MarcZX10 I made Depends on match the author’s existing proposal in Overlap: #9087, #9863 and #10729 first, then this debt-entry edit rebased after them. Those authors have already been asked; their agreement is still pending, so this is a proposed order rather than settled coordination. The shared line is documentation only; the scanner remains ready for review. Your focused Linux results remain explicitly QEMU x86-64 execution, separate from the author’s Windows-host title-library measurements. No native Linux, console or title comparison is inferred from your run. AI-assisted: yes. |
rooklz
left a comment
There was a problem hiding this comment.
Reviewed head 96363b0687 on macOS, since the description says Linux and macOS weren't tested. Setup: macOS 26 (Apple silicon), Apple clang 21.0.0, libc++, C++20, built for x86_64 and run under Rosetta. arm64 can't build ScanningWide.cpp at all because of the sizeof(long double) == 16 static asserts.
Setup. guest_wide_conversion isn't in the relinker-only lane and the full libc target needs submodules, so I compiled ScanningWide.cpp alone (head, then main 750cf6cf48's copy) against a small harness. The harness stubs only the 7 imports the object needs: UTF-8 wcrtomb, ASCII iswspace, and stream functions that abort if called. It runs the 21 rows from the new test table plus 7 more: %js, %zs, %Ls, %ts, %[]] on ab, %[]-] on ]-a, and %y on empty input. Each row checks the return value and the stored bytes against an untouched ! buffer.
-
Head, ASan+UBSan: all 21 PR rows and the 6 extra length/scanset rows pass. No sanitizer reports.
-
main'sScanningWide.cpp: 23 of the 28 rows fail. 12 throwscanset range is implementation-defined, 7 throwinvalid string length modifier(h,hh,ll,j,z,L,t) and 4 throwunsupported conversion(%y×3 and%hls, reported as conversion 108 =l). The 5 rows with no range (%[-a],%[a-],%[^]-],%[]],%[]-]) already pass. So the change is exactly the three throws the description lists. -
Mutants on
InSet/Conversion, all killed by the PR's own 21 rows:member + 2 <→member + 1 <, so a trailing-starts a range: only the%[a-]row fails (my extra%[]-]row fails too).member += 3→member += 1, so ends are re-read and-stops being a member again after a range: 5 rows fail.- range test
&&→||: 7 rows fail. - no range at the first member: 9 rows fail.
- reversed range swapped to match
[min, max]:%[z-a]and%[a--]fail. - unknown conversion
Matching→Input: the 3 unknown-conversion rows fail.
So the table pins the range parsing tightly.
-
git merge-treeagainstupstream/main750cf6cf48is clean.check_conventions --base upstream/mainwith this title and body reports 0 errors. Its one warning,debt-lengthonlibc.md:201at 3265 characters, was already over the limit onmain(3083).
One non-blocking question. With no input left, an unknown conversion returns 0 on this branch: swscanf(u"", u"%y", …) gives 0, and so does u" " with u" %y". %d and %s on the same empty input return EOF (-1). That matches what %S/%C already do on main, and the debt entry says for those that "whether input ending before them is an input failure instead is unverified". If your 66 pairs didn't include an empty-input unknown conversion, it may be worth adding "unknown conversions" to that unverified clause, since the new sentence says they "follow the libc.prx of PPSA07923 and PPSA05916" without that caveat. If they did include it, a row for it in the table would lock it in.
AI-assisted: yes (Claude Code); everything above was run on this machine.
|
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.
|
|
The range handling holds up against the cases added —
|
|
A second copy of the range rule was not updated with the one in
|
|
@ebosido444-ai thanks, both comments were useful. I measured each point on titles' own First comment
Second comment
Checks:
AI-assisted: yes (Claude Code). |
The swscanf of titles' libc.prx (PPSA07923 and PPSA05916 agree) accepts what this scanner threw for: - a scanset range X-Y matches the units from X to Y; a reversed range matches nothing, its ends included; a dash first, last or right after a range is a member, and a ] that opens the set can start a range; - %s, %c and %[ with any length but l (h, hh, ll, j, z, L) read narrow; - an unknown conversion such as %y is a matching failure. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The swscanf of titles' libc.prx (PPSA07923 and PPSA05916 agree) reads %Ld and %Lx as an int, ignores any length on %p (%lp, %hp), and ends the scan at an unterminated scanset as a matching failure, even at the end of the input. Here all three threw. The range test in InSet also no longer forms a pointer past the end of a set shorter than two members. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
1bb1f97 to
a51d64d
Compare
|
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.
Review — changes requested for overlap coordination
I reviewed the scanner changes on head a51d64dbb74f749fcd422aedbc73800c4f84c644. The scanset walker handles the reported left-to-right range/member cases, and the text/unknown-conversion paths match the Windows title-library measurements described in the PR. I found no new code defect in the changed logic. The author reports 66 differential cases with zero mismatches, three passing runs of the focused Windows suites, and a negative control. I did not run the harness locally.
I cannot clear the overlap/readiness blocker yet. The proposed order (#9087, #9863 and #10729 first, then this PR) is recorded as pending, and the #9863 author has not confirmed the #9863-first sequence. Please obtain confirmation from the overlapping authors and keep the Depends on list aligned with those agreements. Current main also changed the shared docs/dev/debt/libc.md after this branch’s base; the merge preview is clean, but the debt hunk needs reconciliation against that current version.
After the order and debt entry are settled, the existing scanner test results remain relevant if production/test files are unchanged; rerun conventions/overlap checks on the integrated head. Hosted policy and build checks currently pass, with Linux product CI skipped.
|
Added the requested provenance caveat in Only one debt sentence changes. Production, tests, public headers and submodule pointers are unchanged. Conventions against current main and whitespace pass (the existing long-entry warning remains), and the current-main merge preview is clean; no new runtime, Windows or SDK measurement is claimed. The range-rule change in the narrow scanner’s Patch and source-preservation receipt, SHA-256 AI-assisted: yes (Codex). |
|
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,
This head adds only a debt-file sentence; this is a runtime run at the head itself. |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, changes requested
The exact-head Linux test report is useful, but the PR still records the overlapping authors’ landing-order agreement as pending.
What I ran
- Exact head:
3961a1f069963e5cf67d70124ae8c9c0576b51d1; PR base recorded ase4da19aff06e25ae16b90f460dff22e8ac1eb786; currentmain:fd9b3f62d43d6753d28a31a7c285e1f51e9c1b42. - Environment: Linux x86_64; GitHub CLI/API inspection only. I did not build or run the tests locally.
- Commands:
gh api repos/boykopovar/AnyPS5/pulls/12420 --jq '{head:.head.sha,base:.base.sha,mergeable:.mergeable,state:.state}'
gh api 'repos/boykopovar/AnyPS5/compare/e4da19aff06e25ae16b90f460dff22e8ac1eb786...fd9b3f62d43d6753d28a31a7c285e1f51e9c1b42' --jq '[.files[] | select(.filename=="core/libs/prx/libc/src/ScanningWide.cpp" or .filename=="core/libs/tests/GuestWideConversion.cpp" or .filename=="docs/dev/debt/libc.md") | {filename,status,additions,deletions}]'
gh pr checks 12420 --repo boykopovar/AnyPS5Results
| Check | Result |
|---|---|
| Current-main file comparison | No changes to the three PR paths between the recorded base and current main; GitHub reports the PR mergeable. |
| Latest code change | Author says the new commit changes only one debt sentence; production and test files are unchanged. |
| Focused test | Author reports exact-head Linux guest_wide_conversion passed 3/3. I did not run it. |
| Hosted checks | Eligibility, conventions, labels, report, Windows, relinker, ASan, and register-context checks pass; Linux product tests are skipped. |
| Landing-order agreement | The PR body still says confirmations from the overlapping authors are pending. |
Negative control
| Assertion | Change to code under test | Did the new test fail? |
|---|---|---|
| The scanner tests reject incorrect parsing behavior | No mutation run in this recheck; the latest change is debt text only. | Not exercised by me. |
Findings
Blocking
The checklist says Depends on: #9087, #9863, #10729 but also says the authors’ agreement is pending. The current contribution rule says to agree with authors of overlapping PRs which lands first and declare that order with Depends on. Please obtain the confirmations and update the checklist to the agreed order before requesting approval.
Non-blocking
The author’s exact-head Linux test report is useful, and the latest change is documentation-only. I found no new scanner-code issue in this head. I did not independently run the test or the convention checks.
Sources
- Contribution rule at current
mainfd9b3f62d43d6753d28a31a7c285e1f51e9c1b42 - PR #12420 at exact head
3961a1f069963e5cf67d70124ae8c9c0576b51d1 - Author’s exact-head Linux test report
Not verified
No local build, focused test, convention check, or negative-control mutation. Linux product CI is skipped by the workflow.
To get this to a good state
Obtain explicit author agreement on the order for the overlapping PRs and update the checklist accordingly. I will recheck the same merge, overlap, convention, and focused-test state on the updated head.
|
@MarcZX10 I checked the author threads again. #9087’s author explicitly agreed that it lands first and this PR follows. The requests to #9863 and #10729 remain unanswered. The checklist now separates the confirmed order from those pending confirmations; no agreement is inferred. @UnhealthyKraken @dwk001 could you confirm the proposed order, your debt-entry edit first and #12420 rebased afterward? The proposal preserves both sentences in the shared scanner debt entry and adds no implementation dependency. The scanner code and prior measured results are unchanged by this metadata clarification. AI-assisted: yes (Codex). |
|
Re-checked: the only change since the approved head is the author's docs commit, which adds one |
What
swscanfandswscanf_sthrew for three things that titles' ownlibc.prx(PPSA07923 and PPSA05916 agree on every row) reads without trouble. A title that hit any of them stopped.Scanset ranges. Members are read left to right.
X-Yis a range when a-follows a member and isn't the last character before]:bca-%[a-c]bcaza%[z-a]-ab/a-b%[-a]/%[a-]-a/a-: a first or last-is a memberb/a-c%[a-a-c]a-c: after a range,-is a member again+%[*--]*to-b]%[]-c]]that opens the set can start a rangeéa%[a-é]dcb/-]a%[^a-c]/%[^]-]d/ 0Lengths on
%s,%cand%[.h,hh,ll,j,zandLread narrow, like no length. Onlylreads wide. Here every length butlthrew.Unknown conversions.
%y,%5yand%hls(hfollowed by the conversionl) are matching failures. Here they threw.Unknown integer and floating lengths,
%nwith suppression or width, and narrow input thatwcrtombcan't convert still throw; those weren't measured.Tested
Windows 11 Enterprise 10.0.22621, Ryzen 9 5950X, WinLibs GCC 15.2.0 UCRT r7, Ninja, Release,
BUILD_TESTING=ON, onmain750cf6cf48.libc.prx: I loaded PPSA07923's and PPSA05916'slibc.prxnatively and called theirswscanfby NID (HNnWdT43ues) side by side with this build. Over 66 input/format pairs, comparing the return value,errno, the bytes or units stored and%n:mainthrows on the first range,%hsor unknown conversion; the other rows already matched;guest_wide_conversion(21 new rows; the two "scanset range throws" checks now expect the measured result),guest_wide_stream_scanandguest_stdio_variadicpass in each of 3 runs.main'sScanningWide.cpp,guest_wide_conversionfails.Not tested on Linux or macOS. The scanner is the same code on every platform.
Overlap:
tools/pr_overlap.pyreports #9087, #9863 and #10729, all editing the same longswscanfdebt entry (libc.mdline 201). This PR changes only its "It throws for ..." sentence. #10324 touchesScanningWide.cppin unrelated stream-orientation lines. I've asked on #9087, #9863 and #10729 to let theirs land first; I'll rebase after each.Empty-input provenance clarification — 2026-10-29
Added the requested provenance caveat in
3961a1f069963e5cf67d70124ae8c9c0576b51d1: unknown conversions at end of input (for example%yon empty input) were not established by the published measurements, and their matching-failure result is explicitly unverified. This does not treat the separately measured empty unterminated-scanset rows as evidence for%y.Only one debt sentence changes. Production, tests, public headers and submodule pointers are unchanged. Conventions against current main and whitespace pass (the existing long-entry warning remains), and the current-main merge preview is clean; no new runtime, Windows or SDK measurement is claimed. The range-rule change in the narrow scanner’s
InWideScansetis separately implemented by #12468, rather than claimed as part of this PR. Pending external merge-order confirmations remain pending.Patch and source-preservation receipt, SHA-256
c08b29d044ad929bf7d681059a6a330959ab81edf8784502dcc369f3148b49f9.AI-assisted: yes (Codex).
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