Repository navigation
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
The revised fgetws handling matches the documented disassembly cases: sizes below two return before touching stream/buffer state, and a read error after at least one character returns a terminated buffer while retaining the stream error flag. The tests cover both regressions and preserve the neighboring wide-stream cases. The author clearly labels the source as disassembly rather than console measurement and records the remaining ABI questions in debt. Hosted conventions, eligibility, intake, labels, report, register-context, relinker matrix, ASAN and write-watch checks pass; hosted Linux/Windows product builds are skipped. The Windows focused suite is author-reported. I did not run tests locally.
|
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.
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.
|
There was a problem hiding this comment.
The fgetws changes match the cited bundled-libc disassembly: sizes below two return before touching the buffer, errno or stream, and a read error after stored characters yields a terminated buffer while preserving the stream error. The tests cover those cases; the debt entry distinguishes bundled-binary disassembly from console measurement and retains the unresolved calls/decoding details. Exact-head git diff --check and conventions pass. Hosted conventions, eligibility, intake, labels, report, relinker matrix, ASan, register-context and write-watch pass; hosted Linux/Windows product builds are skipped. The author reports the focused Windows tests. I did not run runtime tests locally. I need to correct my earlier approval against current repository state. This branch is 91 commits behind current main (01b38ec is based on 6ef5a1c), so the code and tests have not been revalidated on the current integration base. Please rebase and rerun the focused standard-stream tests, then update the tested-head/base information. The bundled-binary source is useful evidence for behavior, but does not itself establish a current title stop. The overlap bot flags #9342 on the adjacent docs/dev/debt/libc.md hunk. I checked its patch: it adds the separate gets entry immediately before this PR's fgetws entry; there is no implementation overlap. Please coordinate/rebase the debt edits with that author and declare a dependency only if an actual merge order is needed. Fixes: none (related: #8897) remains accurate because the reported supplementary-character question is not resolved by these two behavior changes.
I’ve noticed that holding this PR because of #5239 isn’t necessary. It’s fine for PRs to keep coming in; they’ll be handled in due course. Please disregard my earlier request to hold or defer it.
|
Thanks @MarcZX10, for re-checking it against current state. You're right that it's 91 commits behind and was validated on the old base. I'm converting it to a draft under the #5239 hold, since no title run shows the need. Before marking it ready again I'll rebase it onto current |
01b38ec to
d15e72d
Compare
MarcZX10
left a comment
There was a problem hiding this comment.
The fgetws changes remain supported by the cited bundled-libc disassembly, the Windows standard-stream tests, and the reported 26-test libc stream/wide suite. I rechecked exact head d15e72dab340aa2daa0e04221e2f7fb55d6efc75 against current main 3ca07d086d5f44fbe3c00370e7f03953ab46ca1b: whitespace and conventions pass, the current-main merge tree is clean, and both changed C++ translation units pass local GCC 12.2 C++20 syntax-only checks with -Wall -Wextra -Werror (suppressing existing unused/missing-field warnings in FormattingWide.cpp). I did not run the runtime suite locally; hosted Linux/Windows product jobs are skipped.
The branch is based on the earlier 8b3a6ba88d8fc74e768f1a7e8e9e36fb69842768 main, so please update to current main and rerun guest_standard_streams plus the focused libc stream/wide tests, then refresh the Tested head. Keep the #9342 debt-file coordination when rebasing; the reported change adds the separate gets entry adjacent to this one. The disassembly is correctly labeled as bundled-binary evidence, not console measurement.
I withdraw the #5239 intake-hold rationale: if the draft status was only because of that old intake direction, it need not stay draft for that reason. This review request is for current-main validation only; readiness remains your call.
d15e72d to
5ada941
Compare
|
@MarcZX10 rebased onto |
|
Back to draft: @MarcZX10 clarified that the #5239 intake guidance is still active (#9688 (comment)). While the open queue is above 700, a PR should name a title shown to need the change on current |
|
I’ve noticed that asking you to hold this PR was unnecessary. It’s fine to keep submitting PRs; maintainers will handle them in due course. |
The fgetws in the libc.prx that titles ship (the same 133 bytes in the copies of PPSA13934, PPSA07923 and PPSA05916, read from disassembly) differs from this one in two cases: - A size below 2 returns null at once, without writing the buffer, errno or the stream (cmp $0x2,%esi; jge, else the null result). Here a size of 1 stored a NUL and returned the buffer, and a size of 0 or less set the stream error and errno 22. - When fgetwc returns -1 after characters were stored, the console null-terminates and returns the buffer; it returns null only when nothing was stored, and never checks ferror. Here a read error after some characters returned null. The rest already matches: each fgetwc result is stored as its low 16 bits (mov %ax,(%rbx)), and the newline test compares all 32 bits. A null buffer or stream, which the console dereferences, keeps failing with errno 22. guest_standard_streams now expects the console results for sizes 1, 0 and -1 and for an invalid byte after a stored character; its fgetwc error indicator check is unchanged. The libc debt entry cites the reading and what it leaves unverified. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
5ada941 to
ea6aa0a
Compare
|
Rebased onto current |
|
Linux run at this PR's exact head Linux 7.2.9 (Arch), Ryzen 9 7950X, g++ (GCC) 16.2.1 20260810, glibc 2.44, Release,
No title was run. No findings. AI-assisted: yes (Claude Code). |
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed updated head ea6aa0a. The focused diff now follows the described bundled libc.prx behavior for sizes below two and for partial reads after fgetwc returns -1, and the tests cover both without broadening the unverified decoding behavior. The refreshed description records 26 focused Windows libc tests passing on three runs, 7 focused Linux tests passing on three runs with a main-vs-PR comparison, conventions passing, and the evidence source accurately labeled as disassembly rather than console measurement. Current hosted checks pass for conventions and the relinker matrix; full Linux/Windows product jobs are skipped by repository policy. No blocking issue found.
What
fgetwsnow follows the console'sfgetwsin two cases where it differed. The console code was read from thelibc.prxthat titles ship insce_module/. I read it from disassembly, not by measuring on a console. The copies of PPSA13934, PPSA07923 and PPSA05916 have the same 133-bytefgetws(F81hKe2k2tg). The addresses below are from PPSA13934's copy (fgetws0x6cab0,fgetwc0x6c7a0). The reading is also posted on #8897 (comment).size < 2errnoor stream flag (cmp $0x2,%esi; jge …, elsexor %r14d,%r14d).size == 1stored a NUL and returned the buffer.size <= 0set the stream error anderrno22.fgetwcreturns -1 after some characters were storedcmp %r14,%rbx; je). It never checksferror.ferrorwas set.Already matching, unchanged:
fgetwcresult is stored as its low 16 bits (mov %ax,(%rbx)).cmp $0xa,%eax).This means the 32-to-16-bit narrowing reported in #8897 is the console's own behaviour, not a loss in this implementation.
Not reproduced, listed in the debt entry:
errno22.0xcof the stream's second flag byte are set.Changed tests: three
guest_standard_streamsassertions encoded the old guesses, and now expect the console results:"A"The
fgetwcerror-indicator check after that invalid byte is unchanged.Tested
BUILD_TESTING=ON.guest_standard_streams,guest_standard_streams_closed_stdoutandguest_standard_streams_exclusive_stdoutpass, as doguest_wide_formatting,guest_wide_strings,guest_wide_conversionandguest_wide_stream_scan.tools/check_conventions.pypasses.mainb786b90 (2026-10-22), head 5ada941:guest_standard_streams,guest_standard_streams_closed_stdout,guest_standard_streams_exclusive_stdoutand the other libc stream, wide-character and formatting tests (26) pass in three runs out of three.check_conventions.pyandgit diff --checkpass. With feat(libc): implement gets line input #9342,libc.mdconflicts only on adjacency: feat(libc): implement gets line input #9342's newgetsentry goes directly above this PR'sfgetwsentry, and both lines are kept unchanged.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