Skip to content

fix(libc): use guest locale character conversions - #1614

Closed
gdrosa wants to merge 1 commit into
boykopovar:mainfrom
gdrosa:fix/libc-guest-locale-conversions
Closed

gdrosa wants to merge 1 commit into
boykopovar:mainfrom
gdrosa:fix/libc-guest-locale-conversions

Conversation

@gdrosa

@gdrosa gdrosa commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

What

_Mbtowcx and _Wctombx currently use host CRT conversions despite the guest's single-byte C locale.

On a UTF-8 host, U+00E9 is encoded as C3 A9 instead of E9, while valid guest bytes may be rejected or combined into multibyte sequences. Invalid wide characters can also expose the Windows host EILSEQ value (42) instead of the guest value (86).

Changes

  • Delegate conversions to the existing guest mbrtowc_nid_postfix and wcrtomb_nid_postfix implementations using a 16-bit temporary.
  • Preserve existing argument validation and exception paths.
  • Extend guest_locale regression coverage to include all 256 input bytes, independent decoding of C3 A9, errno preservation, output canaries, rejected wide units, and both C and available UTF-8 host locales.

Only LocaleSupport.cpp and GuestLocale.cpp are modified: 6 insertions and 4 deletions in production code, plus regression tests.

This fix is independent of #1507.

Tested

Initial verification baseline: 4d03ecf4dee14407b8f388883ceaedee944c8fbe
Initial tested contribution: 6f4939f25abae6f451a67c626f353bc17b7837d5

Windows x64 — GCC 15.2.0 (WinLibs UCRT POSIX SEH r7), Release

  • Built the complete libc component and all test executables; 34/34 CTests passed, with zero skips.
  • Exercised both host C and UTF-8 regression paths.
  • Real-DLL PoC: 9 failures on the original main baseline (09a17e963aa075e1576abc4bcb3f2e5c3219e4e7), zero with the fix. Verified the loaded DLL paths.
  • Independent real-DLL harness: 660,017 assertions passed, covering all 65,536 guest wide units and 256 input bytes across both host locales, buffer capacities 1/2/SIZE_MAX, null/zero arguments, errno, opaque state, output canaries, and protected page boundaries.

Ubuntu 24.04 x86-64 — GCC 13.3.0, CMake 4.4.4, Ninja 1.13.0, Release

  • Built libc and all 34 component test executables; 37/37 libc CTests passed across three consecutive runs, with zero skips.
  • Exercised both host C and UTF-8 regression paths.
  • Built the complete project and all patched guest libraries.
  • Full repository suite: 289 tests passed, zero failures, with 177 GPU execution tests skipped because the runner has no usable Vulkan device. Verified with --repeat until-fail:3.
  • Native Linux verification run checks out and verifies the exact contribution commit.

Additional checks

  • Production translation unit passed -Wtype-limits -Werror=type-limits.
  • Repository conventions, PR template checks, and git diff --check passed.

CI note

The original Linux verification recorded a stale guest_formatting_tests entry in the test-registration checker. That issue was fixed upstream by #1603. This branch is now rebased onto main at 7995d6578d289310c25b8e104119c8bbfabb14f9; the refreshed contribution is aa83621aa00b4d85a833e7697c8f8997d5911f32. No checker or build-configuration changes are included in this PR.

On the refreshed branch, the Windows libc build and all 34 component CTests pass again; the real-DLL harness passes all 660,017 assertions. The test-registration check passes on both Windows and Linux.

The refreshed Linux run verifies this exact contribution commit and passes every workflow step, with no continue-on-error. It uses CMake/Ninja in a virtualenv, GCC 13.3.0, Release and SPIRV-Tools enabled; it performs no system-package installation. Results:

  • Complete libc build and 37/37 libc CTests passed, repeated three times, with zero skips; host C and UTF-8 regression paths both pass.
  • Complete project build and all patched guest libraries passed.
  • Full repository suite repeated three times: 289 passed, 177 skipped, zero failures out of 466 tests. All skipped cases are GPU execution tests reporting no usable Vulkan device on the runner.
  • Test-registration, production type-warning and local conventions/template checks passed.

Upstream pull-request workflows still await maintainer approval (action_required, no jobs); the independent fork results above do not populate the PR's checks.

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 in TechnicalDebt
  • No notes, investigation .md files or images added to the repository
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • AI-assisted: yes

@frvnzz frvnzz left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed aa83621. The fix is correct, and the regression test fails on main for the reason the PR states.

Production change

  • _Mbtowcx and _Wctombx now go through the guest mbrtowc and wcrtomb in Multibyte.cpp, so they map bytes 0–255 one-to-one, as mbrtowc, wcsrtombs_s and the rest of the guest C locale already do. The local extern "C" declarations match the definitions in Multibyte.cpp, and both are in the same libc target.
  • On main, the host's C locale decides the result. glibc's C locale is 7-bit: gconv_simple.c sends any byte above 0x7f to STANDARD_FROM_LOOP_ERR_HANDLER (glibc bug 19932). So on Linux, _Wctombx(0xE9) and _Mbtowcx("\xE9") throw where the guest converts. The new 0–255 loop catches exactly that.

Tests

  • They check errno: it is unchanged after a successful conversion and 86 after an invalid unit. They also check that no output bytes are written on failure, and that C3 A9 is decoded as two units. That covers both the Windows EILSEQ (42) problem and the UTF-8 problem from the description.
  • setlocale runs after CheckGuestCalls has joined its threads, so the global locale switch can't race with them. The saved locale is restored afterwards.

Verified independently

  • The branch is 175 commits behind main but merges cleanly. It also merges cleanly together with the other open PRs touching LocaleSupport.cpp (#1982, #1993).
  • The linked fork run 37814520846 checks out aa83621 exactly, and guest_locale passes in all three repetitions.

Nits, optional

  • *dst = static_cast<std::uint16_t>(converted);: converted is already std::uint16_t, so the cast can go.
  • With the guest mbrtowc, _Mbtowcx can no longer return -1, -2 or more than INT_MAX once count == 0 and null src are rejected. Keeping the throws as guards for a future multibyte locale is fine. Mentioning it only in case you prefer to drop the dead branches.
  • On a host without C.UTF-8 or .UTF8, the UTF-8 half is skipped with only a log line, so ctest still reports a pass. That's acceptable for CI images that have the locale; just noting the coverage depends on it.

LGTM.

Review written with AI assistance; the merges and the CI log check above were run for this review.

@xXPBSXx

xXPBSXx commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

Tested on Windows 10 (19045), MinGW-w64 GCC 15.2.0 (WinLibs r7), Release: this PR merged onto main c080fc2d5 (no conflict).

  • guest_locale passes with the PR (0.09 s).
  • With the PR's change to LocaleSupport.cpp taken back to main, the extended test fails (Guest locale check failed, an unhandled exception), so the new checks fail without the change.

Not tested: Linux, a console, a title run.

AI-assisted (Claude Code).

@oneandonlydean

Copy link
Copy Markdown
Collaborator

Closing: no title is known to reach this. With the queue at 700+ open PRs, please hold off on new PRs that don't name a title needing the change until it's down. If a title stops on this on current main, reopen with the title ID and the log line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants