WASM support #100
WASM support #100
Conversation
bzip2-sys/wasm_shim.rs
Outdated
| #[no_mangle] | ||
| pub extern "C" fn rust_bzip2_wasm_shim_malloc(size: usize) -> *mut c_void { | ||
| unsafe { | ||
| let layout = Layout::from_size_align_unchecked(size, 1); |
There was a problem hiding this comment.
I think this will need to change, notably the alignment of 1. The default malloc alignment is at least the alignment of usize and I think it's actually twice that, although it'd be good to check what malloc guarantees.
bzip2-sys/wasm_shim.rs
Outdated
| // layout is not actually used | ||
| let layout = Layout::from_size_align_unchecked(1, 1); |
There was a problem hiding this comment.
This unfortunately is not something I'm personally willing to rely on. The comment that this isn't used is only applicable with the dlmalloc system allocator which can be overridden and may not be true in the future.
I think that the malloc/free/calloc shims here will need to have a header/footer with the size to reconstruct the Layout here.
There was a problem hiding this comment.
Do you think that plays a role for WASM though? Can you even use dlmalloc with WASM? I kinda copied this part - would have to dig deeper into the dealloc implementation.
bzip2-sys/wasm_shim.rs
Outdated
| pub extern "C" fn rust_bzip2_wasm_shim_calloc(nmemb: usize, size: usize) -> *mut c_void { | ||
| unsafe { | ||
| let layout = Layout::from_size_align_unchecked(size * nmemb, 1); | ||
| alloc(layout).cast() |
There was a problem hiding this comment.
I don't think this is correct because I believe the memory needs to be zeroed to respect the contract of the calloc function.
bzip2-sys/build.rs
Outdated
| // wasm32-unknown-emscripten should not be included here. | ||
| // See: https://github.com/gyscos/zstd-rs/pull/209 | ||
| let need_wasm_shim = env::var("TARGET").map_or(false, |target| { | ||
| target == "wasm32-unknown-unknown" || target == "wasm32-wasi" |
There was a problem hiding this comment.
WASI shouldn't need a shim I believe since that should use the wasi-sdk C compiler which has all the support necessary.
I believe this PR should only affect wasm32-unknown-unknown
bzip2-sys/bzlib.h
Outdated
There was a problem hiding this comment.
Out of curiosity, where did this file come from?
There was a problem hiding this comment.
This came from #92 and in turn from the zstd crate (https://github.com/gyscos/zstd-rs/tree/main/zstd-safe/zstd-sys/wasm-shim)
There was a problem hiding this comment.
I removed this file since I don't think we need it. This came from the other open PR.
bzip2-sys/lib.rs
Outdated
| #[cfg(target_arch = "wasm32")] | ||
| mod wasm_shim; |
There was a problem hiding this comment.
Could this be behind something like cfg(wasm_shim) which is printed out by the build script?
bzip2-sys/Cargo.toml
Outdated
| @@ -28,3 +28,5 @@ cc = "1.0" | |||
| [features] | |||
| # Enable this feature if you want to have a statically linked bzip2 | |||
| static = [] | |||
| std = [] # Use std types instead of libc in bindgen | |||
There was a problem hiding this comment.
I think std and libc are the same, so this may not be necessary?
bzip2-sys/wasm-shim/stdlib.h
Outdated
There was a problem hiding this comment.
Could the wasm-shim directory have a README.md explaining what it is, how it's used, and why it's necessary?
wolfv
force-pushed
the
feature/wasm-support
branch
from
e5653a5
to
635bae0
Compare
|
Would love a re-review & potential release. We're trying to test this in CI and it's a bit tricky to test against a branch :) |
This compiles and works quite nicely with
--target wasm32-unknown-unknown.