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

Add wasm64-unknown-unknown support #303

Merged
merged 2 commits into from
Oct 23, 2022
Merged

Add wasm64-unknown-unknown support #303

merged 2 commits into from
Oct 23, 2022

Conversation

josephlr
Copy link
Member

We can build and link just fine, but we cannot actually run the tests as wasm-bindgen-test-runner hasn't yet added support.

Fixes #296

Signed-off-by: Joe Richey joerichey@google.com

@briansmith
Copy link
Contributor

My understanding is there should be a wasm64-wasi target, but I don't know if the Rust toolchain supports it. wasmtime, at least, supports the memory64 proposal; see bytecodealliance/wasmtime#3153.

This raises this question in my mind: shouldgetrandom built for a wasm{32,64}-unknown-unknown target have some way of saying "use the WASI API"? As I guess that might be what wasmtime needs until Rust supports an official wasm64-wasi target.

In wasi.rs we have this:

    match unsafe { random_get(dest.as_mut_ptr() as i32, dest.len() as i32) } {

Both as i32 casts are questionable, but especially the first one seems definitely wrong. (The fact that we need to do pointer-to-integer cast seems wrong in itself).

We also have this:

        err => Err(unsafe { NonZeroU32::new_unchecked(err as u32) }.into()),

I don't think the memory64 proposal changes the size of integer values such as the return value of random_get but it might be worth double-checking.

In js.rs we have this:

                    let sub_buf = buf.subarray(0, chunk.len() as u32);

The Web Crypto API limits the maximum chunk length to a value much smaller than u32::MAX. However, I am not sure it's correct for wasm-bindgen to be limiting arrays to 32-bit sizes and using u32 as the argument type here.

@briansmith
Copy link
Contributor

https://github.com/john-sharratt/cargo-wasix supports Rust programs targeting wasm64-wasi, though I haven't tried it.

.cargo/config Outdated Show resolved Hide resolved
@newpavlov
Copy link
Member

Regarding WASI, I think we should first get official support from the wasi crate.

@josephlr
Copy link
Member Author

https://github.com/john-sharratt/cargo-wasix supports Rust programs targeting wasm64-wasi, though I haven't tried it.

I tried getting it to run, and it doesn't even compile a simple Hello World crate. I think it's just someone's personal project for experimenting with this stuff.

@josephlr
Copy link
Member Author

My understanding is there should be a wasm64-wasi target, but I don't know if the Rust toolchain supports it. wasmtime, at least, supports the memory64 proposal; see bytecodealliance/wasmtime#3153.

Thanks for pointing out the stuff around the WASI target. I opened #306 to make sure we don't compile our current implementation against a 64-bit target. It also fixes some of the sketchy pointer manipulation and unsafe code you pointed out.

This raises this question in my mind: shouldgetrandom built for a wasm{32,64}-unknown-unknown target have some way of saying "use the WASI API"? As I guess that might be what wasmtime needs until Rust supports an official wasm64-wasi target.

I don't think something like that would be useful, and it would definitely confuse things from the current ecosystem where:

  • wasm32-wasi means "I can use WASI"
  • wasm32-unknown-unknown means nothing in particular, but in practice means you're running in some JavaScript environment.

The path forward for 64-bit WASI is:

  • Rust adds a wasm64-wasi target
  • The wasi crate adds support for this target (what @newpavlov mentioned above)
  • wasmtime adds support for using WASI with Memory64

Then, all we need to do is update lib.rs to use our WASI implementation on wasm64-wasi.

@josephlr
Copy link
Member Author

josephlr commented Oct 23, 2022

In js.rs we have this:

                    let sub_buf = buf.subarray(0, chunk.len() as u32);

The Web Crypto API limits the maximum chunk length to a value much smaller than u32::MAX. However, I am not sure it's correct for wasm-bindgen to be limiting arrays to 32-bit sizes and using u32 as the argument type here.

Note that this is technically js_sys not wasm_bindgen, but either way It's certainly an allowed way to do things. I agree that its a little odd, but it won't cause undefined behavior or anything. It just limits you to creating arrays of at most u32::MAX bytes, or operating on the first u32::MAX bytes of an existing ArrayBuffer or TypedArray.

EDIT: While js_sys might want to add support for longer arrays, that doesn't need to involve this crate.

However, I did have to update the Node.js implementation to use chunking, as Node.js's crypto.randomFillSync requires the buffer to have length <= i32::MAX.

@newpavlov adding you to re-review (because of the chunking change).

We can build and link just fine, but we cannot actually run the tests as
`wasm-bindgen-test-runner` hasn't yet added support.

Signed-off-by: Joe Richey <joerichey@google.com>
Signed-off-by: Joe Richey <joerichey@google.com>
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.

Add support for wasm64-unknown-unknown
3 participants