Skip to content

feat(libSceIcu): implement converter stop and skip callbacks - #12692

Merged
adriadam10 merged 3 commits into
boykopovar:mainfrom
Ninot1Quyi:feat/icu-converter-callbacks
Oct 30, 2026
Merged

adriadam10 merged 3 commits into
boykopovar:mainfrom
Ninot1Quyi:feat/icu-converter-callbacks

Conversation

@Ninot1Quyi

Copy link
Copy Markdown
Contributor

What

Replace four throwing inventory stubs with typed implementations of UCNV_FROM_U_CALLBACK_STOP, UCNV_FROM_U_CALLBACK_SKIP, UCNV_TO_U_CALLBACK_STOP, and UCNV_TO_U_CALLBACK_SKIP.

STOP preserves the caller's error except for FROM_U's unassigned default-ignorable case. SKIP implements null-context and "i" policies, including default-ignorable precedence, and leaves lifecycle notifications untouched. Converter argument layouts remain opaque because these callbacks do not access them. Headers expose each export once; add a focused runtime fixture and update the remaining-stub debt count from 107 to 103.

Signatures and behavior follow ICU 78.1 ucnv_err.h and ucnv_err.cpp. Names/NIDs are in the pinned PS5 3.20 inventory. This is an independent implementation; no ICU source is vendored or linked.

The debt entry records absent console verification, the ICU 65.1/78.1 difference at U+180F, and guards for undefined upstream inputs. Unaccessed pointers remain unread. Converter execution and other callbacks remain unimplemented.

Overlap: the repository checker scanned all 1,410 open PRs, followed by manual inspection of all 16 ICU candidates. None implements these four functions/NIDs. Mechanical conflicts remain with #10967 (test registration), #12320/#12629/#12630 (debt inventory/count), and #12451 (adjacent debt entry). Preserve both registrations/entries and recount remaining stubs when combining them. #11088/#11115/#11118/#11122/#11132/#11136 already conflict with main and could not be compared automatically; their implementations were checked manually and are distinct. No implementation dependency is required.

Tested

Ubuntu 24.04 x86_64, GCC 13.3, Release, BUILD_TESTING=ON; base 3a2b91981651569163768854a1fcdc7193b6667b.

  • Full project CMake configuration succeeded; built the real libSceIcu, libc/libkernel dependencies, nid_patcher and ICU test targets. Focused CTest: 69/69 passed (four ICU runtime tests, public-header audit, 64 public-header compile shards). The final source-order adjustment was followed by another build and 5/5 runtime/audit checks. The entire repository test suite was not run.
  • Fixture checks all callback reasons in the upstream enum's representable 0–7 domain, context policies, default-ignorable boundaries, status/errno preservation, ignored pointers, and invalid inputs.
  • A temporary oracle containing the pinned official ICU callback bodies and macro passed 11,146,420 differential comparisons, including the full Unicode range. Oracle files are not part of the patch.
  • Complete ICU provider plus the callback fixture passed UBSan with no report.
  • Patched the real libc, libkernel and libSceIcu outputs using the CMake-prescribed nid_patcher arguments. Calls through all four inventory NIDs throw NotImplemented on main; the new exports pass behavioral checks. dladdr confirmed all three PRXs loaded from the isolated patched directory.
  • python3 tools/check_conventions.py --base origin/main, git diff --check, and tools/pr_overlap.py --branch codex/icu-next passed (overlap findings described above).

No game/homebrew, console hardware, Windows or macOS execution was performed.

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

@github-actions

github-actions Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

Progress report (bb04a4f → 207ac4b)

📈 System libraries: 46.52% (+0.03%, +4 implemented, -4 removed functions)

✅ 4 implemented
Library Name
libSceIcu UCNV_FROM_U_CALLBACK_SKIP_nid_postfix
libSceIcu UCNV_FROM_U_CALLBACK_STOP_nid_postfix
libSceIcu UCNV_TO_U_CALLBACK_SKIP_nid_postfix
libSceIcu UCNV_TO_U_CALLBACK_STOP_nid_postfix
🗑️ 4 removed
Library Name
libSceIcu sceIcuUnknown_BZY5mpL0Syk_nid_no_patch
libSceIcu sceIcuUnknown_JKRf_2yrIaE_nid_no_patch
libSceIcu sceIcuUnknown_WLoOrEfsob4_nid_no_patch
libSceIcu sceIcuUnknown_uKBkvN84hbw_nid_no_patch

Changed libraries (implemented exports)

Library Before After Change
libSceIcu 17/120 (14.17%) 21/120 (17.50%) +3.33 pp

@adriadam10 adriadam10 self-assigned this Oct 29, 2026

@rooklz rooklz 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.

I tested this on macOS, which the body didn't cover. Head is 63529b3.

Standalone build. I built ConverterCallbacks.cpp with the PR's GuestIcuConverterCallbacks.cpp under ASan and UBSan on macOS 26 arm64 (Apple clang, -fno-sanitize-recover=undefined). The only shim was NotImplemented_nid_no_patch. It passes in 0.2 s with no sanitizer reports.

Differential against an ICU binary. I compared the four callbacks against Homebrew ICU 78.3's libicuuc (UCNV_*_CALLBACK_{STOP,SKIP}_78, linked as a binary, not read as source). The inputs were:

  • contexts: null, "i", "x", "", "I", "ii", "s", "c"
  • reasons 0–7
  • 9 initial error values (including 0, negatives and warnings)
  • every code point from -2 to 0x110001 for the FROM_U pair

That is 1,283,462,784 calls with 0 differences (3.4 s). 78.3 agrees with the PR's U+180F range.

Mutants. I ran 31 mutants of ConverterCallbacks.cpp:

  • one edge for each of the 17 default-ignorable ranges
  • reason bounds (> 6, dropped < 0)
  • the <= 2 gate on both SKIPs, moved to <= 1 and <= 3
  • reason == 0 widened to <= 1 in FROM STOP and in the FROM SKIP ignorable check
  • the 'i' byte changed to 'I', and the 'i' reason gate dropped
  • the null-context clause dropped
  • TO STOP writing the error
  • the null-error guard dropped

The PR test kills all 31. 29 of them also change results against ICU 78.3. The other two (null-error guard, dropped < 0) only affect inputs ICU leaves undefined, and the debt entry already documents them.

Other checks.

  • check_conventions --base 3a2b919816: clean.
  • git merge-tree against upstream/main 3a2b919: clean, and pairwise with #10967: clean.
  • With #12451 it conflicts in the debt file, as the body says.

No changes requested.

@github-actions

github-actions Bot commented Oct 29, 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 10967 core/libs/prx/libSceIcu/CMakeLists.txt L19-20
PR 12629 docs/dev/debt/libSceIcu.md L19

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 12320 core/libs/prx/libSceIcu/CMakeLists.txt
core/libs/prx/libSceIcu/Export.cpp
core/libs/prx/libSceIcu/libSceIcu.hpp
docs/dev/debt/libSceIcu.md
PR 12738 core/libs/prx/libSceIcu/CMakeLists.txt
core/libs/prx/libSceIcu/libSceIcu.hpp
docs/dev/debt/libSceIcu.md

@oneandonlydean

Copy link
Copy Markdown
Collaborator

@Ninot1Quyi reviewed at 63529b3 against ICU 78.1 ucnv_err.cpp: the same 17-range default-ignorable table and the same STOP / SKIP / null-context / "i" / reason logic; the extra checks (reason outside 0..7, null error pointer) throw and are documented. The NIDs recompute to exactly the four removed stubs, and the debt count 107 -> 103 is right. The code is an independent re-expression; only the Unicode default-ignorable ranges (property data) coincide, so no ICU notice is needed. The remaining-stub count will conflict mechanically with #12320 #12629 #12630 #12451. No requests.

@adriadam10 we'll merge this once exact-head CI is green, unless you'd rather merge it yourself.

@JJenkx

JJenkx commented Oct 29, 2026

Copy link
Copy Markdown
Contributor

Windows run of this head (63529b3c6d), merged onto main 52c09d567d (clean merge, local only). Windows 11 Enterprise 10.0.22621, Ryzen 9 5950X, WinLibs GCC 15.2.0 UCRT r7, Ninja, Release, BUILD_TESTING=ON, no Vulkan driver visible.

  • This PR: guest_icu_converter_callbacks passes 3 of 3 (ctest --repeat until-fail:3).
  • Negative control: not possible. With this PR's source files (core/libs/prx/libSceIcu/ConverterCallbacks.cpp, core/libs/prx/libSceIcu/ConverterCallbacks.hpp, core/libs/prx/libSceIcu/Export.cpp, core/libs/prx/libSceIcu/libSceIcu.hpp) put back to main's version, and the ones it adds removed, the build fails against the rest of the PR, so there is no source-only control.

AI-assisted: yes (Claude Code).

@github-actions github-actions Bot added build docs prx needs rebase Conflicts with main, rebase needed labels Oct 29, 2026
@github-actions github-actions Bot added needs rebase Conflicts with main, rebase needed and removed needs rebase Conflicts with main, rebase needed labels Oct 29, 2026
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 29, 2026
@adriadam10
adriadam10 merged commit 91a6995 into boykopovar:main Oct 30, 2026
10 checks passed
@adriadam10

Copy link
Copy Markdown
Collaborator

Checked the four callbacks against ICU 78.1 ucnv_err.cpp (default-ignorable table, STOP/SKIP, null/"i" context, reason range), the removed stubs against the new NIDs, the debt count, and the fixture's edge rows; matches the macOS, Windows and Linux runs above; merged.

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.

5 participants