Repository navigation
feat(libc): implement collate<wchar_t>::_Getcat - #5662
Conversation
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the PR. One blocking point:
core/libs/prx/libc/src/LocaleSupport.cpp:278: the wide facet is published with the narrowg_collateVtable, so anycollate<wchar_t>compare/transform/hashruns the narrow code over 16-bit characters read as bytes. That returns plausible but wrong results instead of throwing (e.g. little-endian byte order gives a different ordering for characters above 0xFF,hashdiffers from a wide hash, anddo_transformthrows on the zero high byte of every ASCII wide character). Exact-or-throw applies (CONTRIBUTING): either add a wide vtable with 16-bitdo_compare/do_transform/do_hash, or make the wide vtable entries throw until they are implemented, and document that in the debt entry.core/libs/tests/GuestCollate.cpp:101-108: the wide test only compares narrow ASCII strings through the narrow vtable, so it pins the behaviour above. Once the wide vtable exists, add rows withwchar_t-width data (a character >0xFF, an embedded high byte).
Welcome to push again with that addressed.
1867b58 to
661b167
Compare
|
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.
|
|
Addressed with the real wide vtable (option 1), pushed as a second commit.
The new test rows pin exactly your cases: a character above 0xFF (ordering now differs from byte-wise), an embedded high byte, and wide hash/transform. They fail on the previous head and pass with the fix (Windows + WSL, plus 9 locale neighbours on both). The debt entry keeps the derived-not-measured honesty, including the hash-widening judgement call and the byte-sized wide-string limitation. |
|
Windows run of head
Titles: catalog homebrew, each relinked with that side's own relinker and libs, 60 s in a window (
XPSemu stops the same way on both sides. Its own log ( That is the geometry-VGPR stop of #5604, not a The second commit addresses both points of @adriadam10's review: the wide facet now has its own vtable (the test checks it differs from the narrow one), and there are rows with characters above 0xFF, an embedded high byte, and wide hash and transform. One thing I would check before merging. Minor points:
Verdict: on Windows the build, the tests (including the negative control) and the non-GPU suite pass, and the three titles behave as on AI-assisted (Claude Code). |
adriadam10
left a comment
There was a problem hiding this comment.
Checked the new 16-bit g_wideCollateVtable (LocaleSupport.cpp:160-220): compare by value then length, transform sized in bytes with a 16-bit terminator, hash one round per character. The wide rows in guest_collate (characters above 0xFF, embedded high byte) fail on main without the change and pass here; local build + ctest 105/105 green, and the Windows run by @screamm covers MinGW. Merging.
What
collate<wchar_t>::_Getcatis implemented, so the guest wide collation facet is built instead of throwing.The implementation is the narrow one, copied from
_ZNSt7collateIcE7_GetcatEPPKNSt6locale5facetEPKS1_that sits next to it in the same file: allocate aGuestLocale::CollateFacet(0x20 bytes) through the guest'soperator new, leave the two collation-table words at offsets 0x10 and 0x18 null, publish the base facet with a zero reference count, and throwstd::invalid_argument("collate::_Getcat: only the C locale is supported")for a null locale, a null implementation, a null name or any name other than"C". Both entry points returnsize_tand answer 1, which is the facet id the C locale's single category uses.GuestLocalemodels one facet layout and oneCollateVtablefor both element types, so the wide facet carries the narrow vtable and acollate<wchar_t>comparison, transform or hash runs the narrowdo_compare,do_transformanddo_hashover the guest's 16-bit characters read as bytes. The console'slibc.prxexports both_Getcat(NIDs2wz4rthdiy8andBSVJqITGCyI) over the same facet, but its wide vtable is not reproduced here; the debt entry records this, the console's null collation-table pointers, and that no module of the PPSA59322, PPSA76677 or PPSA97358 dumps in this tree imports either symbol, so nothing here has been exercised by a title.std::_Atomic_assert(_ZSt14_Atomic_assertPKcS0_, NIDu2tMGOLaqnE) is not part of this PR. The search for its definition came back empty: GCC'slibstdc++has no_Atomic_assertin any release from 4.5.4 to 16.2 or in its ChangeLog from 2007 to 2022, and neither does LLVM'slibc++fromrelease/3.2.xto current. It stays theNotImplemented_nid_no_patchstub rather than gaining an invented message shape.Tested
guest_collategrowsCheckWideGetcat, which mirrors the narrow case: a nullfacetargument returns 1 without allocating, an existing facet is left alone, a non-C name, a null name and a null implementation all throw without allocating, the C locale allocates exactly one 0x20-byte facet whose vtable is the narrow one and whose two collation words are null, compare and hash answer through that vtable, an invalid character range throws, and retain/release/destroy/deleteObject behave as for the narrow facet. On unmodifiedmain(0aa4cb1) with only this test file applied the test fails withstd::runtime_error: _ZNSt7collateIwE7_GetcatEPPKNSt6locale5facetEPKS1__nid_postfix not implemented; with the change it passes.guest_collatepasses 3 runs, and the neighbour suites pass 10/10 (guest_locale,guest_rune_locale,guest_strftime,guest_time_conversion,guest_wcsrtombs,guest_wcsrtombs_s,guest_wide_conversion,guest_generic_category,guest_character). The exception ABI suites pass 22/22;guest_exception_handlerreports "Not Run" on this branch and on the base alike because the focused Windows runs built only the named targets, not its test binary; it is not a regression.guest_collatepasses 1/1 and the same ten neighbour suites pass 10/10 twice.python tools/check_conventions.py --base refs/remotes/upstream/mainpasses;python tools/check_test_registration.py build-g15reports 786 test executables with none unregistered.Checklist
main; no other open PR implements the same functions.mdfiles or images added to the repository