Skip to content
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

Fix large pointers from WASM #3310

Merged
merged 1 commit into from
Feb 17, 2023
Merged

Conversation

kristoff3r
Copy link
Contributor

Pointers being passed from WASM to JS are interpreted as signed, so large pointers ended up negative. This prevented WASM programs from allocating more than 2GB.

To fix it we make pointer use unsigned (via >>> 0) for all malloc/realloc calls, and for all pointer arguments while generating the shim functions. Ideally there would be an abstraction that guarantees we do it in all cases, but that would require a major refactor.

To test it we use gg-alloc as the global allocator while running tests. This allocator was made specifically for testing this issue, but was never upstreamed. It only allocates memory above 2GiB.

Fixes #2388
Fixes #2957

@kristoff3r kristoff3r force-pushed the ptr-fix branch 2 times, most recently from 9094c36 to 2b0803e Compare February 15, 2023 15:42
@kristoff3r
Copy link
Contributor Author

Hmm, looking at the changes to the reference files it might be simpler to remove pop_ptr and just fix it inside getStringFromWasm instead.

@kristoff3r
Copy link
Contributor Author

Also it looks like the multithreading tests aren't happy about gg-alloc. I tried writing tests for this without using it, but I couldn't figure out how to force allocations above 2GB in the test environment without being extremely slow. If anyone has ideas for that let me know.

@alexcrichton
Copy link
Contributor

Thanks! And yeah I think it's fine to fix without tests for now, allocating 2G in the browser tests is likely going to cause flakiness as well.

Pointers being passed from WASM to JS are interpreted as signed, so
large pointers ended up negative. This prevented WASM programs from
allocating more than 2GB.

To fix it we make all pointers unsigned (via >>> 0) for all malloc/realloc
calls and inside shim functions. Ideally we would have an abstraction
that guarantees we catch all cases, but that would require a major
refactor.

To test it we add gg-alloc as an optional system allocator for
wasm-bindgen-test. It only allocates in the upper 2GB range and was made to
test this exact issue but was never upstreamed.

Fixes rustwasm#2388
Fixes rustwasm#2957
@kristoff3r
Copy link
Contributor Author

I decided to only change stuff in cli-support/src/js/mod.rs, as it seems cleaner (no pointers being fixed multiple times, avoids a half-baked abstraction).

I also made gg-alloc an optional feature for wasm-bindgen-test, and with it the tests now pass, and they fail if I remove the fix.

@alexcrichton alexcrichton merged commit 1aee7e8 into rustwasm:main Feb 17, 2023
@emilk
Copy link

emilk commented Mar 15, 2023

Thanks for fixing this - we run into this issue problem frequently in our memory intensive application.

Any plans for a new release with this critical fix? 🙏

EDIT we've worked around it for now with out own fork, so no stress from our side.

For others that want this: just use cargo install wasm-bindgen-cli --git https://github.com/rerun-io/wasm-bindgen.git --rev 13283975ddf48c2d90758095e235b28d381c5762

See https://github.com/rerun-io/wasm-bindgen/commits/0.2.84-patch

emilk pushed a commit to rerun-io/wasm-bindgen that referenced this pull request Mar 17, 2023
Pointers being passed from WASM to JS are interpreted as signed, so
large pointers ended up negative. This prevented WASM programs from
allocating more than 2GB.

To fix it we make all pointers unsigned (via >>> 0) for all malloc/realloc
calls and inside shim functions. Ideally we would have an abstraction
that guarantees we catch all cases, but that would require a major
refactor.

To test it we add gg-alloc as an optional system allocator for
wasm-bindgen-test. It only allocates in the upper 2GB range and was made to
test this exact issue but was never upstreamed.

Fixes rustwasm#2388
Fixes rustwasm#2957
@Keavon
Copy link

Keavon commented Apr 1, 2023

@kristoff3r @alexcrichton quick question: does this fix double the available memory from 2GB to 4GB, or does it increase the cap beyond 4GB (to what?)?

As a secondary (slightly off-topic) question, what's the release process like? Once wasm-bindgen is given a release, do we also have to wait for wasm-pack to update to the latest wasm-bindgen, or is wasm-pack's version unrelated to wasm-bindgen's version?

Thanks!

@daxpedda
Copy link
Collaborator

daxpedda commented Apr 1, 2023

Only to 4GB. Beyond 4GB requires wasm64 support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants