Skip to content

Fix UTF16ToString in TEXTDECODER=2 and MAXIMUM_MEMORY>=2GB #24335

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 16, 2025

Conversation

RReverser
Copy link
Collaborator

I accidentally noticed that Embind UTF-16 support was producing incorrect results with MAXIMUM_MEMORY over 2GB.

Turns out, the TextDecoder implementation of UTF16ToString itself was broken in this mode due to bitwise ops, so I've added new tests and fixed this bug, slightly simplifying code in the process.

@RReverser
Copy link
Collaborator Author

What's up with Windows CI? Seems to be failing in download stage on several PRs.

@RReverser RReverser requested a review from sbc100 May 15, 2025 15:46
@sbc100
Copy link
Collaborator

sbc100 commented May 15, 2025

What's up with Windows CI? Seems to be failing in download stage on several PRs.

Looks like infra flake with the chocolaty package manager. I think its fairly rare. About as common as npm infra flake I would say.

@RReverser RReverser force-pushed the fix-utf16-textdecoder branch 2 times, most recently from 01cfa62 to 8a66970 Compare May 15, 2025 21:11
@RReverser RReverser enabled auto-merge (squash) May 15, 2025 21:11
@RReverser RReverser force-pushed the fix-utf16-textdecoder branch from 8a66970 to 60b4ebb Compare May 16, 2025 00:08
@RReverser
Copy link
Collaborator Author

FAIL [0.002s]: test_async_hello_v8 (test_core.instance) looks like a broken test on main branch.

@sbc100
Copy link
Collaborator

sbc100 commented May 16, 2025

Yup, test_async_hello_v8 is unrelated. Sorry about that.

@RReverser RReverser force-pushed the fix-utf16-textdecoder branch from 60b4ebb to b7d209b Compare May 16, 2025 20:38
RReverser added 4 commits May 16, 2025 22:15
I accidentally noticed that Embind UTF-16 support was producing incorrect results with MAXIMUM_MEMORY over 2GB.

Turns out, the `TextDecoder` implementation of `UTF16ToString` itself was broken in this mode due to bitwise ops, so I've added new tests and fixed this bug, slightly simplifying code in the process.
@RReverser RReverser force-pushed the fix-utf16-textdecoder branch from b7d209b to 527651e Compare May 16, 2025 21:15
@RReverser RReverser merged commit 8f1c0e9 into emscripten-core:main May 16, 2025
29 checks passed
@RReverser RReverser deleted the fix-utf16-textdecoder branch May 16, 2025 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants