Repository navigation
Conversation
…etdateorder, _Strcoll and _Strxfrm
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed the implementation and tests for the C-locale-only helpers. The fixed-width layouts match the recorded reverse-engineering evidence and are guarded with static assertions; the byte-range comparisons handle embedded NUL and unsigned bytes; transform copies only when the full source fits, as documented in the debt entry. The Ctype table reuses the existing _Getpctype table, and the test covers its negative sentinel and all 256 byte entries. Current conventions, intake, eligibility, relinker matrix, register-context, write-watch, and report checks pass. Full Linux/Windows builds were skipped, and I did not run a local build in this session.
|
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.
|
MarcZX10
left a comment
There was a problem hiding this comment.
The implementation review and reported evidence are comprehensive: the fixed-width structs are guarded, byte comparisons include high-bit and embedded-NUL cases, and the full-copy-only _Strxfrm behavior matches the documented binary evidence. The checked head is based on a6dcea927, 47 upstream commits behind current main (110a18ba01). Please update the branch and rerun conventions, overlap/debt checks, and the focused C locale helper suite against the updated tree. The overlap report also has incomplete comparisons for shared CMakeLists.txt and libc debt files; confirm the additions remain additive with current work during that recheck. Existing hosted checks pass as listed; full Linux/Windows jobs were skipped, and I did not run local tests.
harisibrar1
left a comment
There was a problem hiding this comment.
Checked against main 830ccd1, the Dinkumware C-locale _Strcoll/_Strxfrm/_Ctypevec/_Collvec definitions, the SysV x86-64 return classification, and the NID calculator:
- All six NIDs match, and none of the six is on main yet. The head merges onto 830ccd1 with no conflicts in
CMakeLists.txtordocs/dev/debt/libc.md, so the rebase request should be easy to satisfy. - The struct sizes give the return paths described: the 16-byte
Ctypevec/Collveccome back in rax:rdx, and the 40-byteCvtvecthrough the hidden pointer._Strcoll/_Strxfrmmatch the Dinkumware C-locale behaviour. - Nit: in
time_base::dateorder(no_order, dmy, mdy, ymd, ydm), 2 ismdy, the usual C-locale order, soDateOrderMdywould be a clearer name thanDateOrderNone(CLocaleHelpers.cpp:30). - Nit: the trailing-bytes check in
CheckCvt(GuestCLocaleHelpers.cpp:159-164) copiessizeof(Cvtvec)from a local intoraw, so bytes 40..47 can't change and the check always passes. It could be dropped, or the test could call through a pointer to a function returning a larger struct.
Reviewed with AI assistance (Claude Code); the checks above were run locally on my machine.
|
Windows run of 52a3e3b merged onto Built |
…prx/libc/CMakeLists.txt)
…oykopovar#9466 in core/libs/CMakeLists.txt and libc/CMakeLists.txt: the kept-both-sides merge dropped the closing PATH line and endif() of the test block each PR's block was inserted after
|
Windows check of head
AI-assisted: yes (Claude Code); results above were run on this machine. |
jotitadev
left a comment
There was a problem hiding this comment.
Merge-skew check: head 52a3e3b1a merged onto current main abed03546 (clean merge, git merge --no-commit). This head's CI ran on the October 21 base. Linux x86_64, i7-8700, Ubuntu 26.04, GCC 15.2, Release, BUILD_TESTING=ON, full build, then the full non-GPU suite (ctest -E "^agc|gpu|vulkan|video_out" -j6). main alone: 1363/1363 pass. With this PR there are two failures, each 3/3. Both come from main changes that landed after the earlier runs in this thread.
1. guest_c_locale_helpers aborts ("C locale helper check failed at line 138"). On current main, _Getpctype()['\t'] is 0x4c0, while the test's Classify expects 0xc0 (line 138, and the literal at line 142). #10387 (d2e18d5, merged 2026-10-26 01:28 UTC) made the shared classification table mark tab with 0x400, as the console's libc does (isblank). _Getctype returns that same table, so the change is only in the test's expectation:
if (value >= '\t' && value <= '\r') mask |= 0x40;
+ if (value == '\t') mask |= 0x400;
@@
- Require(vec.table[' '] == 0x04 && vec.table['\t'] == 0xc0 && vec.table['\n'] == 0xc0);
+ Require(vec.table[' '] == 0x04 && vec.table['\t'] == 0x4c0 && vec.table['\n'] == 0xc0);2. public_headers fails (its 64 public_headers_compile_* dependents don't run):
_Getctype_nid_postfix / _Getcoll_nid_postfix / _Getcvt_nid_postfix / _Getdateorder_nid_postfix / _Strcoll_nid_postfix / _Strxfrm_nid_postfix:
expected exactly one header declaration; found 1: core/libs/tests/GuestCLocaleHelpers.cpp:28..33
_Getpctype_nid_postfix: expected exactly one header declaration; found 3: core/libs/prx/libc/include/Api.hpp:496, core/libs/prx/libc/src/CLocaleHelpers.cpp:36, core/libs/tests/GuestCLocaleHelpers.cpp:27
#10706 (merged 2026-10-24) added this audit. Each export needs one declaration, in a header reachable from libc.hpp. The fix I checked:
- Move the
GuestCLocale::Ctypevec/Collvec/Cvtvecstructs, with theirstatic_asserts, fromCLocaleHelpers.cppintocore/libs/prx/libc/include/Api.hpp. - Declare the six exports in
Api.hppright after_Getpctype_nid_postfix. - Include
Api.hppinCLocaleHelpers.cppand drop its local_Getpctypedeclaration. - In the test, include
Api.hpp, drop the local structs andextern "C"block, and useusing GuestCLocale::Ctypevec;(and the same forCollvecandCvtvec).
With both changes on top of the merge, the full non-GPU suite passes 1364/1364, including guest_c_locale_helpers, guest_locale, guest_character, public_headers and all public_headers_compile_*. I can post the full diff if that helps.
The head still merges cleanly onto the newer main 92bfca720. Since abed03546, nothing there has touched the audit script, libc/include/Api.hpp or the tab bit.
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
|
A titles-side cross-check of two of the helpers, against the head Setup: a scratch loader runs a decrypted
So the I didn't build this PR; this checks the console side only. AI-assisted: yes (Claude Code). |
|
@AlxcBTW two things on this one, it merges cleanly otherwise:
|
|
Static review plus an independent Linux run (CachyOS, kernel 7.2.10, GCC 16.2.1, Ninja, Release, What I checked (static)
Focused test run
Not an AGC/shader/VideoOut change — no paired Vulkan-validation run required. The debt entry records exactly what remains unverified (whether |
What
Adds six Dinkumware C locale helpers that libc did not export yet, as real
APS5_VABIexports in a new file,core/libs/prx/libc/src/CLocaleHelpers.cpp(added to the libc source list; no other locale file is touched):_GetctypegwAf8VMkXS0{_Getpctype(), 0}(table pointer, delete flag) as a 16-byte struct in rax:rdx; the table is the existing classification table, not a copy_Getcoll15TBQ3jmXvo_GetcvtHHXmPak83kQ1at +0,1at +4, zeros from +8_GetdateorderLMZCS1i3KTU_StrcollZyciM6GWVO8(first1, last1, first2, last2, collvec): compares the two ranges bytewise as unsigned bytes; returns the difference of the first differing bytes, otherwise 0 for equal lengths and -1 or 1 when one range is a prefix of the other_StrxfrmEvPpDIUyQPI(dest, destEnd, src, srcEnd, collvec): copies the source range when it fits[dest, destEnd)and returns its length; a range that does not fit is not copied at allC locale only; the collation argument is ignored.
The behaviour was read from the
libc.prxbundled with PPSA16064 (disassembly; nothing from it is in the repository), which differs from what the names suggest in four places, so the implementation follows the binary:_Getctypereturns its result in registers (rax:rdx), not through a hidden pointer;_Getcvtis the one that uses the hidden pointer._Getdateorderreturns 2, not 0._Strcollreturns the raw byte difference of the first mismatch ("a"against"c"gives -2), not a normalised -1, and ranges are compared by length asint._Strxfrmwrites nothing when the source is longer than the destination range (it does not copy a truncated prefix); it still returns the source length._Getcollis a bareretin that binary, so the register pair it returns is whatever the caller left; this returns zeros, which is deterministic and what a caller that ignores the C locale collation vector sees. The_Getcvtblock is written exactly as far as the console writes it (40 bytes); whether the struct is larger is not known. Both are listed in the libc technical debt (including that the_Getcvtsize is unconfirmed) (docs/dev/debt/libc.md). None of the six was listed there before, so no entry is removed.Tested
ci/test/libc-c-locale-helpers-r(this branch's head52a3e3b1a3plus the CI scaffolding commit),ctest -R '^(guest_c_locale_helpers|guest_locale|guest_rune_locale|guest_character|guest_strings|guest_wide_strings)$': 6 of 6 passed, including the newguest_c_locale_helpersand the existing locale, character and string tests. The Windows job was not run; the test is registered with the samePATHenvironment modification as its neighbours.core/libs/tests/GuestCLocaleHelpers.cppcalls the exports directly with hand-computed vectors:_Strcoll: equal ranges give 0, empty against empty 0,"a"/"b"negative,"b"/"a"positive,"a"/"c"exactly -2, prefixes give -1 and 1,0x80/0x7fand0xff/0x01compare as unsigned bytes (1, 254), embedded NUL bytes are compared, a mismatch in the 17th byte after 16 equal ones is found._Strxfrm: destination larger (copied, bytes after it untouched, returns 5), destination exactly the length (copied), destination smaller or zero-length (nothing written, still returns 5), empty source returns 0, binary bytes00 80 ff 61are copied unchanged._Getctype: struct size and field offsets bystatic_assert, table pointer equal to_Getpctype(), delete flag 0,table[-1]0, and for all 0..255 the entry equals the hand-written classification of that byte (digit, upper, lower, space, punct, hex and control bits; 128..255 are 0)._Getcollzeros,_Getcvt(size and offset bystatic_assert, page 1, maximum 1, rest zero, no write past 40 bytes),_Getdateorder2.python3 tools/check_conventions.py --base origin/mainpasses. Based on0c6a5bd593.tools/pr_overlap.pyfinds no pull request implementing these functions (a search of open PR titles and bodies for each name also finds none); it lists textual overlaps incore/libs/prx/libc/CMakeLists.txtanddocs/dev/debt/libc.mdwith several open libc PRs (for example feat(libc): export the num_get and time_get locale objects as data #8880, feat(libc): cover the codecvt, numpunct, num_put and time_put facets, their ids and std::clog #9038, fix(libc): convert multibyte text as UTF-8 like the console's C locale #8855 edit other locale files and the same two shared files), which are line-level additions in separate places.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)app.exefromlibc.prx); no Game bring-up discussion yet. Needed before thestd::__sce_v2locale facets.