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 ABI for raw pointers #3655

Merged
merged 1 commit into from
Oct 13, 2023
Merged

fix ABI for raw pointers #3655

merged 1 commit into from
Oct 13, 2023

Conversation

jrvidal
Copy link
Contributor

@jrvidal jrvidal commented Oct 13, 2023

👋 Hello from Stackblitz

We've noticed very buggy behavior when our Rust code requires a sizable amount of memory (above 2 GiB!). I've traced it back to __wbindgen_malloc returning negative pointers (!!), which screams "i32 overflow" 😬

From what I can gather, changing the WasmDescribe impl. for raw pointers (so they follow u32 ABI conventions instead of i32's) should fix it. I've added a references test to wasm-bindgen-cli, not sure how appropriate/useful is.

Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

Yeah, this definitely looks like a bug to me:

impl<T> IntoWasmAbi for *const T {
type Abi = u32;
#[inline]
fn into_abi(self) -> u32 {
self as u32
}
}
impl<T> FromWasmAbi for *const T {
type Abi = u32;
#[inline]
unsafe fn from_abi(js: u32) -> *const T {
js as *const T
}
}
impl<T> IntoWasmAbi for *mut T {
type Abi = u32;
#[inline]
fn into_abi(self) -> u32 {
self as u32
}
}
impl<T> FromWasmAbi for *mut T {
type Abi = u32;
#[inline]
unsafe fn from_abi(js: u32) -> *mut T {
js as *mut T
}
}

@Liamolucko would you mind confirming this?

@daxpedda daxpedda added the needs review Needs a review by a maintainer. label Oct 13, 2023
Copy link
Collaborator

@Liamolucko Liamolucko left a comment

Choose a reason for hiding this comment

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

Yep, I agree this is a bug. Thanks!

@daxpedda daxpedda removed the needs review Needs a review by a maintainer. label Oct 13, 2023
@daxpedda daxpedda merged commit d2f9315 into rustwasm:main Oct 13, 2023
25 checks passed
daxpedda added a commit that referenced this pull request Oct 13, 2023
@d3lm
Copy link
Contributor

d3lm commented Oct 13, 2023

Awesome! Thanks so much for this quick turn around and for merging this so quickly 🙌 Is there a chance we can get this released?

@daxpedda
Copy link
Collaborator

See #3530.

@jrvidal jrvidal deleted the jrvidal/pointers branch October 15, 2023 15:44
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.

4 participants