Skip to content

feat(libc): wire the collate<char> facet vtable - #11166

Merged
oneandonlydean merged 1 commit into
boykopovar:mainfrom
00xDisaster:feat/libc-collate-vtable-wire
Oct 26, 2026
Merged

oneandonlydean merged 1 commit into
boykopovar:mainfrom
00xDisaster:feat/libc-collate-vtable-wire

Conversation

@00xDisaster

@00xDisaster 00xDisaster commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

What

Fill the exported collate<char> vtable with the facet methods that already back g_collateVtable. #9979 added the nine-slot _ZTVSt7collateIcE_nid_postfix data object (NID ruZtIwbCFjk) with the console's 72-byte layout, but every slot was zero, so a title that called compare, transform or hash through the exported vtable faulted. Slots 2 through 8 now hold DestroyCollate, DeleteCollate, RetainFacet, ReleaseFacet, CollateCompare, CollateTransform and CollateHash; slots 0 and 1 stay zero for offset-to-top and typeinfo, as they were. _Getcat still installs g_collateVtable, so both the exported object and facets built by _Getcat call the same implementations.

The exported object is still writable data and supplies no RTTI typeinfo; the debt entry records both. This does not touch time_put or num_put: open #11044 and #11054 implement those different virtuals, and no open PR currently fills this array. #10158 and #10200 add the distinct adjacent messages and numpunct vtable declarations in the same LocaleSupport.cpp block; both are currently CONFLICTING, so this PR merges first and they rebase afterwards, keeping all three declarations. No dependency is required.

Tested

Linux CachyOS 7.3.0-4-cachyos, GCC 16.2.1. Base upstream/main 74ac0e7912a5eaa453fff872f4d4fe803154b08c; head c66f61ff5259e3d05eab0e35c99b948d2b62903b.

  • cmake --build build --target guest_locale_tests --parallel: passed.
  • ctest --test-dir build -R '^guest_locale$' --output-on-failure: 1/1 passed.
  • guest_locale now calls compare, transform, hash, retain and release through &_ZTVSt7collateIcE_nid_postfix[2], checks the compare ordering and FNV hash result, and checks that slots 0 and 1 remain zero while slots 2 through 8 are non-null.
  • tools/check_conventions.py --base upstream/main --head HEAD and git diff --check: clean.
  • GPUs tested: n/a.

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: no

The nine-slot _ZTVSt7collateIcE export added by boykopovar#9979 was still zeroed, so a guest that called through the exported vtable faulted. Fill slots 2 through 8 with the same facet methods that g_collateVtable uses, leave the offset-to-top and typeinfo slots zero, and cover compare, transform, hash, retain and release through the exported object.

@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 exact head c66f61ff5259e3d05eab0e35c99b948d2b62903b.

The exported nine-slot object points at the same collate methods as the existing facet vtable, leaves offset-to-top/typeinfo zero as documented, and the test invokes all populated slots through the exported object. The debt update correctly records the remaining RTTI limitation and the evidence boundary.

Checks: git diff --check upstream/main...HEAD passes and the conventions, eligibility, intake, labels and report checks pass. The author reports guest_locale_tests and guest_locale passing locally. The current hosted relinker and register-context jobs pass; macOS relinker/register-context and write-watch jobs are still queued, while the general Linux/Windows jobs are skipped by the path policy. I did not run a local build in this review.

@github-actions

github-actions Bot commented Oct 25, 2026 •

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 10158 core/libs/prx/libc/src/LocaleSupport.cpp L374
PR 10200 core/libs/prx/libc/src/LocaleSupport.cpp L374
PR 10749 docs/dev/debt/libc.md L190-191

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 6713 docs/dev/debt/libc.md
PR 7081 core/libs/prx/libc/src/LocaleSupport.cpp
docs/dev/debt/libc.md
PR 7386 docs/dev/debt/libc.md
PR 7447 docs/dev/debt/libc.md
PR 7544 docs/dev/debt/libc.md
PR 7648 docs/dev/debt/libc.md
PR 7715 docs/dev/debt/libc.md
PR 8150 docs/dev/debt/libc.md
PR 9371 docs/dev/debt/libc.md
PR 9447 docs/dev/debt/libc.md
PR 9526 core/libs/prx/libc/src/LocaleSupport.cpp
core/libs/tests/GuestLocale.cpp
docs/dev/debt/libc.md
PR 9772 docs/dev/debt/libc.md
PR 9863 docs/dev/debt/libc.md
PR 9929 docs/dev/debt/libc.md
PR 9953 docs/dev/debt/libc.md
PR 9964 docs/dev/debt/libc.md
PR 10002 docs/dev/debt/libc.md
PR 10007 docs/dev/debt/libc.md
PR 10234 docs/dev/debt/libc.md
PR 10315 docs/dev/debt/libc.md
PR 10317 docs/dev/debt/libc.md
PR 10321 docs/dev/debt/libc.md
PR 10325 docs/dev/debt/libc.md
PR 10328 docs/dev/debt/libc.md
PR 10345 docs/dev/debt/libc.md
PR 10349 docs/dev/debt/libc.md
PR 10405 docs/dev/debt/libc.md
PR 10467 docs/dev/debt/libc.md
PR 10506 docs/dev/debt/libc.md
PR 10510 docs/dev/debt/libc.md
PR 10512 docs/dev/debt/libc.md
PR 10529 docs/dev/debt/libc.md
PR 10565 docs/dev/debt/libc.md
PR 11332 docs/dev/debt/libc.md
PR 11341 docs/dev/debt/libc.md

@MarcZX10

Copy link
Copy Markdown
Contributor

Follow-up to my review: the remaining hosted checks have completed. Linux/macOS/Windows relinker, ASan, Linux Debug/Release and macOS Intel Debug/Release register-context, and macOS write-watch jobs pass. Full Linux and Windows product jobs are skipped by the repository path policy.

@MarcZX10

Copy link
Copy Markdown
Contributor

I manually checked the current overlap warnings. #10158 adds _ZTVSt8messagesIcE_nid_postfix and #10200 adds _ZTVSt8numpunctIcE_nid_postfix in the same LocaleSupport.cpp declaration block where this PR implements _ZTVSt7collateIcE_nid_postfix; these are distinct vtables, not duplicate implementations. A three-way merge with each current head reports a content conflict in LocaleSupport.cpp at that insertion point, while the shared GuestLocale.cpp edits merge cleanly. Please coordinate the insertion/rebase resolution before combining them.

@wachi35676

Copy link
Copy Markdown
Contributor

Windows run of head c66f61ff5259 merged onto main 90f918fa7. Windows 11 10.0.26300, WinLibs GCC 15.2.0, Release.

AI-assisted: yes (Claude Code); results above were run on this machine.

@oneandonlydean oneandonlydean self-assigned this Oct 26, 2026
@00xDisaster

Copy link
Copy Markdown
Contributor Author

Merge-order follow-up on the overlap: #10158 and #10200 add the distinct adjacent messages and numpunct vtable declarations in the same LocaleSupport.cpp block, not the collate export. A three-way merge with their current heads conflicts only at that insertion point. Both of those PRs are currently CONFLICTING, while this head is CLEAN and fully reviewed, so I propose this PR merge first and #10158/#10200 rebase afterwards, keeping all three declarations and tests. The dependency line stays none. I did not modify either other branch; if you or mazen320 prefer the opposite order, I will rebase this one instead.

@00xDisaster

Copy link
Copy Markdown
Contributor Author

All checks are now green on c66f61ff52 (18 pass, 2 path-skipped), including the macOS relinker, register-context and write-watch jobs. The merge-order note for #10158/#10200 is in the body and comment above; no code changed since your review. Could you approve this head if you have no further concerns?

@oneandonlydean

Copy link
Copy Markdown
Collaborator

@00xDisaster Thanks for checking the merge with #10158 and #10200. That order works for us: this one first, then those two rebase and keep all three declarations and their tests. The Linux and Windows product jobs were path-skipped on this head, so we'll still run those before it goes in.

@oneandonlydean
oneandonlydean merged commit ed8fd06 into boykopovar:main Oct 26, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants