Repository navigation
Conversation
…c.prx does The libc.prx that titles ship (PPSA07923, PPSA05916) never matches a literal format byte from 0x80 up, not even against the same input byte: the scan stops there with the count so far, before the end-of-input check, so an empty input gives 0 rather than EOF. The sscanf_s scanner matched such bytes literally. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
The parser change matches the title measurements and handles high-bit bytes without signed-char comparison, but the test hunk conflicts with open #12507 and the agreed landing order is not recorded.
What I ran
- Reviewed PR head
95a98f1ef42a24e9f48a2a50be4d19a0d8cfb592againstmainat319621ba2e578bd053b0baba2877fe70520e0737. git diff --check upstream/main...refs/review/pr-12558— passed.git merge-tree --write-tree upstream/main refs/review/pr-12558— clean (6241cf3b2330a0b7109ba7ead32336e357744b2f).git merge-tree --write-tree refs/review/pr-12507 refs/review/pr-12558— content conflict incore/libs/tests/Formatting.cpp; the implementation file merges cleanly.- Inspected current checks: ASan, Ubuntu register-context, conventions, eligibility, intake, labels, and report pass. Windows, relinker platform jobs, macOS register-context, and write-watch jobs are pending; the Linux product job is skipped.
Findings
Blocking
- The test insertion conflicts with open #12507 in
core/libs/tests/Formatting.cpp. #12507 is already approved and changes adjacent scanner tests. CONTRIBUTING.md says overlapping PR authors should agree which lands first and declare the order withDepends on. The current description acknowledges the textual conflict but the checklist saysDepends on: none; I found no explicit agreement with #12507's author. Please coordinate and record the order, then resolve/rebase the test block accordingly. This is an integration requirement; I found no separate defect in the one-line parser change.
Non-blocking
- The PR supplies title-side comparisons, a main control, and focused Windows tests. Those results are author-reported; I did not reproduce them. The edge cases cover high-bit bytes, empty input, prior conversions, and a 0x7f control.
Not verified
- I did not build or run
guest_formattinglocally. - Windows and several platform-specific CI jobs are still pending; the Linux product workflow is skipped.
To get this to a good state
- Record the agreed order with #12507 and resolve the overlapping test insertion. Then rerun the relevant scanner tests and wait for the required CI checks.
|
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,
|
|
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
left a comment
There was a problem hiding this comment.
Re-checked — changes still requested
The test overlap with #12507 merges cleanly, but a direct merge preview confirms a source conflict with #9087, whose Windows sscanf routing this PR says it will follow; the order is not agreed or declared in the checklist.
What I ran
- PR head:
95a98f1ef42a24e9f48a2a50be4d19a0d8cfb592; currentmain:319621ba2e578bd053b0baba2877fe70520e0737. git diff --check upstream/main...refs/review/pr-12558-current— passed (no output).git merge-tree --write-tree upstream/main refs/review/pr-12558-current— clean, tree6241cf3b2330a0b7109ba7ead32336e357744b2f.git merge-tree --write-tree refs/review/pr-9087-current refs/review/pr-12558-current— conflict incore/libs/prx/libc/src/Formatting.cpp.git merge-tree --write-tree refs/review/pr-12507-current refs/review/pr-12558-current— clean, treed32ad0ee9f37770e92fd6fcfd697376df5607f96.- I read the exact diffs, latest description, and author-reported test results; I did not run this PR's tests locally.
Results
| Check | Result |
|---|---|
| Diff check | Passed |
Merge preview onto current main |
Clean |
| Pairwise preview with #9087 | Content conflict in Formatting.cpp |
| Pairwise preview with #12507 | Clean; the earlier test-insertion concern is not a blocker |
| Hosted Ubuntu/Windows relinker and ASan | Passed |
| Hosted Ubuntu register-context | Passed |
| Hosted Windows job | Passed |
| Hosted macOS relinker/register-context/write-watch | Queued at last check |
| Hosted Linux product tests | Skipped |
| Focused formatting tests | Author reports 3/3 on Linux and 3/3 on Windows; not independently rerun here |
Raw merge and diff-check output
$ git merge-tree --write-tree upstream/main refs/review/pr-12558-current
6241cf3b2330a0b7109ba7ead32336e357744b2f
$ git merge-tree --write-tree refs/review/pr-12507-current refs/review/pr-12558-current
d32ad0ee9f37770e92fd6fcfd697376df5607f96
$ git merge-tree --write-tree refs/review/pr-9087-current refs/review/pr-12558-current
Auto-merging core/libs/prx/libc/src/Formatting.cpp
CONFLICT (content): Merge conflict in core/libs/prx/libc/src/Formatting.cpp
$ git diff --check upstream/main...refs/review/pr-12558-current
(no output; exit 0)
Negative control
Not independently run. The PR description reports that guest_formatting fails with main's Formatting.cpp while the PR test remains unchanged.
Findings
Blocking
- The PR overlap with #9087 produces a content conflict in
Formatting.cpp. The description says the Windows plain-sscanfpath will be routed throughScanSourceafter #9087, while the checklist currently saysDepends on: none. Please agree on the landing order with #9087's author and record the prerequisite/order in the checklist. If #9087 must land first for the planned Windows routing, list#9087as a dependency and retain the focused regression when rebasing.
Non-blocking
- The exact pairwise merge with #12507 is clean, so I am withdrawing the earlier concern that the two test insertions require a merge-order blocker. The author-provided Linux/Windows tests and title comparisons are useful evidence, but I have not reproduced them locally.
Sources
- #12558 parser change at the reviewed head
- #12558 description and current dependency checklist
- #9087 conflicting parser change
- #12507 tests used for pairwise merge verification
- Contribution rule for overlap coordination and declared dependencies
Not verified
I did not independently run guest_formatting, the Windows title comparison, or the reported random-scan integration. macOS CI was still queued; the Linux product workflow is skipped.
To get this to a good state
Coordinate and record the order with #9087, preserving the test and behavior when the conflicting hunks are integrated. I will recheck the exact updated head and rerun the focused formatting test.
|
@MarcZX10 the order with #9087 is now agreed and recorded in the checklist: orezraey confirmed that #9087 lands first and this PR rebases after it. The checklist now lists The resolution keeps #9087's mismatch line unchanged and puts this PR's AI-assisted: yes (Claude Code). |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — approved
The author has resolved the outstanding overlap concern: #9087’s author confirmed the order, this PR now declares Depends on: #9087, and the reported Windows integration run tested both heads together. I reviewed the current diff and verified the PR remains clean against current main; the dependency still prevents merging until #9087 is handled.
What I ran
- Refreshed exact PR head, review history and hosted CI.
- Reviewed the current PR diff and the updated overlap checklist.
- Compared current-main and #9087 pairwise merge previews; ran
git diff --check. - Did not build or run the tests locally.
Results
| Check | Result |
|---|---|
| Current-main merge preview | Clean |
| Pairwise merge with #9087 | Clean |
git diff --check |
Pass |
| Hosted Windows, conventions, eligibility, labels, relinker, ASan, register-context and write-watch | Passed |
| Hosted Linux product job | Skipped |
| Author-reported Windows integration tests | Five focused tests passed 3/3 on #9087 + this exact head |
| Author-reported Linux test | guest_formatting passed 3/3 on this exact head |
Unedited local check output
$ git merge-base upstream/main refs/review/upstream-12558
319621ba2e578bd053b0baba2877fe70520e0737
$ git rev-list --left-right --count upstream/main...refs/review/upstream-12558
270 1
$ git merge-tree --write-tree upstream/main refs/review/upstream-12558
6302eb58966c66dbe3c0ea9a72f8af4ca0cbc5da
$ git merge-tree --write-tree refs/review/upstream-9087 refs/review/upstream-12558
72e0c284c9e6eca16e75c9e1b3beb0455d6a0b27
$ git diff --check upstream/main...refs/review/upstream-12558
(no output; exit status 0)
$ git diff --name-status 319621ba2e578bd053b0baba2877fe70520e0737..upstream/main -- formatting/stdio paths touched by this PR
(no output)
Negative control
Not run independently in this re-check. The author’s focused integration report compares both PRs together; the earlier base/control evidence remains author-reported.
Findings
Blocking
None found in the current diff. The declared prerequisite #9087 remains open, so this approval does not indicate that the PR is ready to merge ahead of its dependency.
Non-blocking
The branch is 270 commits behind current main, but the intervening upstream commits do not touch this PR’s formatting source or test paths, and the current-main merge preview is clean.
Sources
- Current PR head
95a98f1ef42a24e9f48a2a50be4d19a0d8cfb592 - Current upstream
mainedf6804cf52c62eacafc592dd6142cb4efe5bd05 - Author’s order and integrated Windows test report
- #9087 author’s order confirmation
- Repository overlap-order rule
Not verified
I did not independently reproduce the Windows or Linux test reports.
|
A Windows run with GCC 16: Windows 11 Pro 10.0.22631, i9-9900K, MSYS2 UCRT64 GCC 16.1.0, Release, this PR (
AI-assisted: yes (Claude Code) |
…high-literal # Conflicts: # core/libs/tests/Formatting.cpp
|
Refreshed onto Windows, WinLibs GCC 15.2 UCRT, Release:
AI-assisted: yes. |
❌ AMD GPU test: failedPR head
Failing testsThis PR changes test files, so each failing test was also built and run on main with only the PR's test changes applied. ✅ there means the failure comes from the PR's other changes; ❌ means the test fails on main too, so this run does not show the PR caused it.
agc_graphics_profile_draw: Timeout (also failed when re-run alone), assertion and diagnosticsUnstable on main (5, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (10, not counted against this PR)Validation IDs already present on main (4 IDs in 805 tests)Skipped (71)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
Linux check of exact head
AI-assisted: yes |
What
sscanf_s,vsscanf_sand the stream_sforms now treat a literal format byte from 0x80 up the way thelibc.prxthat titles ship does: it never matches. The scan stops there with the count so far, even when the input holds the same byte. That stop comes before the end-of-input check, so an empty input gives 0, notEOF.mainmatched such bytes literally. Most likely the titles decode format literals through the C-localembtowc, which rejects bytes from 0x80 up.The "without this PR" column is a local build whose
ScanSourceliteral branch is unchanged frommain; the control in Tested checksmain's ownFormatting.cpp. Measured by calling thesscanf(1Pk0qZQGeWo) andsscanf_s(24m4Z4bUaoY) of PPSA07923 natively; plainsscanfbehaves the same.80\x80%n%nnot written%n= 1c3 a9\xc3\xa9%n%nnot written%n= 235 80%d\x80%n%nnot written%n= 261 62 5d 80%[ab]]\x80%n%nnot written%n= 4\x80%nEOF7f\x7f%n%n= 1)It's a single check in
ScanSource's literal branch. Plainsscanfon Windows still usesWindowsScanning.hpp; it picks this up when it's routed throughScanSource, which I'll do after #9087.This is the last difference a local integration run found. That run combined this PR with #12507, #12530, #12532, #12428, #12468 and that routing, tested against the
libc.prxof PPSA07923 and PPSA05916. Without this PR, 2 of 100,000 random scansets still differed, both with a high byte after the set. With it, that run gives 0 of 100,000 in both titles, forsscanfandsscanf_s.Overlap: the source change doesn't touch the other PRs' lines. The new tests are inserted after the same line of
core/libs/tests/Formatting.cppas #12507's, so the two conflict textually there. The conflict is trivial (both blocks are kept), and whichever lands second takes it.Tested
Windows 11 Enterprise 10.0.22621, Ryzen 9 5950X, WinLibs GCC 15.2.0 UCRT r7, Ninja, Release,
BUILD_TESTING=ON, head95a98f1ef4onmain319621ba2e.\xff,\xe9, a leading space,%d \x80) give 0 differences from the titles'sscanf_swith this build.guest_formattinggains six of these cases throughvsscanf_s. Withmain'sFormatting.cpp, it fails.guest_formatting,guest_stdio_variadic,guest_scan_count,guest_wide_stream_scan,guest_wide_formatting,guest_wide_conversion,guest_strings,file_streams,guest_safe_errorsand theguest_standard_streams*tests pass 3/3. Conventions pass.Independent Windows validation reported by bunbeans2-arch for topic
95a98f1ef42a24e9f48a2a50be4d19a0d8cfb592merged into main409fe64199: Windows 11 Pro 22631, i9-9900K, MSYS2 UCRT64 GCC 16.1, Release.guest_formattingpasses 5/5; retaining this PR's tests while restoringFormatting.cppto main fails 5/5; plain main with its own source/tests passes 3/3. This is the contributor's run, not a new local Windows execution.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)ScanSource: this PR's>= 0x80line goes just before fix(libc): follow snprintf_s and sscanf_s constraints as measured on a PS5 #9087's mismatch line, which stays unchanged. With that resolution the five scanning tests pass 3/3 on Windows.🤖 Generated with Claude Code