Skip to content

Fix utf8_to_utf16 and utf16_to_utf8 for s390x#128577

Open
saitama951 wants to merge 3 commits into
dotnet:mainfrom
saitama951:fix_utf8
Open

Fix utf8_to_utf16 and utf16_to_utf8 for s390x#128577
saitama951 wants to merge 3 commits into
dotnet:mainfrom
saitama951:fix_utf8

Conversation

@saitama951
Copy link
Copy Markdown
Contributor

The converters return incorrect string when MINIPAL_TREAT_AS_LITTLE_ENDIAN is set true on s390x

see: #128394 (comment)

The converters return incorrect string when MINIPAL_TREAT_AS_LITTLE_ENDIAN
is set true
@saitama951
Copy link
Copy Markdown
Contributor Author

@Dotnet-s390x build

@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label May 26, 2026
@saitama951
Copy link
Copy Markdown
Contributor Author

cc: @jkotas

@Dotnet-s390x
Copy link
Copy Markdown

Build Queued..

To cancel the current build, please comment:

@Dotnet-s390x cancel

@am11
Copy link
Copy Markdown
Member

am11 commented May 26, 2026

@saitama951, was there any PAL test failing before this change? If not, could we add a case or two there which exercises it (that would fail without this fix)? Possible place of addition:
src/coreclr/pal/tests/palsuite/locale_info/{MultiByteToWideChar,WideCharToMultiByte}/test{1,2,3,4,5}/test{1,2,3,4,5}.cpp

It would help gain some confidence in the future, if someone refactors this code (e.g. with SIMD etc.). We refactored it in 2023 and seeing that we are only finding it out now is a bit worrying. :/

@Dotnet-s390x
Copy link
Copy Markdown

Build Successful
Please check the build logs: http://148.100.85.217:8080/job/dotnet-builds/80/console.

@saitama951
Copy link
Copy Markdown
Contributor Author

saitama951 commented May 26, 2026

@am11 I think the intent of MultiByteToWideChar, WideCharToMultiByte are supposed to be in native endian which worked fine even before this patch, It is only when minipal is forced to convert strings in LE order in big endian system that this failed. I think the eventpipe stuff forces things to write down into a .nettrace in little-endian order.

@am11
Copy link
Copy Markdown
Member

am11 commented May 26, 2026

You can cherry pick this commit main...am11:runtime:chore/utf-bigendian-tests. I don't have access to bigendian machine right now, but if it fails on main branch and passes with PR, it would suffice for minipal API coverage.

@jkotas
Copy link
Copy Markdown
Member

jkotas commented May 26, 2026

You can cherry pick this commit main...am11:runtime:chore/utf-bigendian-tests

I doubt that the Win32 PAL tests work on s390x. We can look into the testing separately.

@am11
Copy link
Copy Markdown
Member

am11 commented May 26, 2026

You can cherry pick this commit main...am11:runtime:chore/utf-bigendian-tests

I doubt that the Win32 PAL tests work on s390x. We can look into the testing separately.

They work just fine #128394 (comment).

@saitama951
Copy link
Copy Markdown
Contributor Author

@am11
Running tests...

......................................................................................Testing for 64 Bit Platforms 
............................................................................................................minipal_convert_utf8_to_utf16 with MINIPAL_TREAT_AS_LITTLE_ENDIAN mismatch at 0: got 0x0041 expected 0x4100


FAILED: locale_info/MultiByteToWideChar/test4/paltest_multibytetowidechar_test4. Exit code: 1

....minipal_get_length_utf16_to_utf8 with MINIPAL_TREAT_AS_LITTLE_ENDIAN returned 97, expected 33


FAILED: locale_info/WideCharToMultiByte/test5/paltest_widechartomultibyte_test5. Exit code: 1

..........................Global Counter Value at the end of the test 0 
..........................

after applying this patch I started to get this failure

..minipal_get_length_utf16_to_utf8 with MINIPAL_TREAT_AS_LITTLE_ENDIAN returned 97, expected 33

I have made endian changes to GetByteCount as well and with that I get

Finished running PAL tests.

PAL Test Results:
  Passed: 305
  Failed: 0

@saitama951
Copy link
Copy Markdown
Contributor Author

@Dotnet-s390x build

@Dotnet-s390x
Copy link
Copy Markdown

Build Queued..

To cancel the current build, please comment:

@Dotnet-s390x cancel

@Dotnet-s390x
Copy link
Copy Markdown

Please check the build logs: http://148.100.85.217:8080/job/dotnet-builds/81/console.

Copy link
Copy Markdown
Member

@am11 am11 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you! 👍

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

Labels

area-PAL-coreclr only for closed issues community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants