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 basic wasm32: C types and malloc/realloc/free. #1092

Closed
wants to merge 1 commit into from

Conversation

rodrigorc
Copy link

Currently trying to compile "zip" modules from crates.io (without bzip2) for wasm32-unknown-unknown fails because of libc usage, even though all the code is native rust.
It fails because even it is full Rust code, it uses types from libc, such as c_int and c_void, for compatibility with other C bindings.
I see no reason why wasm32 should not have the C types defined, just like the Switch.

Another issue is that the deflate library uses malloc/free from libc. These can be easily emulated with the global allocator, so we can emulate them here.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@alexcrichton
Copy link
Member

Thanks for the PR!

This crate, however, is a small shim around the underlying system's exported functionality and doesn't itself implement polyfills or shims like this. A polyfill like this is left to higher-level crates which wrap libc, but libc itself won't provide these sorts of abstractions.

The typedefs also don't have a clear definition in C as there isn't a widely-agreed-upon definition of the C ABI yet (aka what sizes each of the types in C are). There's an early effort to define a hopefully standard sysroot implementation for wasm (with reference types and such), and we'll be sure to want to follow that closely for type definitions!

@rodrigorc
Copy link
Author

rodrigorc commented Oct 13, 2018 via email

@alexcrichton
Copy link
Member

Yes in this case you'll need to patch the crate for wasm to not use libc types/methods. We don't heavily review all platforms added to libc, so they're not all held to as strict of a standard as the main tier 1 targets.

@rodrigorc
Copy link
Author

Unfortunately, zip depends on flate2 that depends on miniz_oxide_c_api. And as its name implies, te latter has C types all over the public API, as it is a drop-in replacement of C miniz. The malloc/free requirement can be solved with a few cfg(target) but the public API cannot be easily changed.

So until flate2 supports a C-less environment, I'm afraid I'm stuck with my libc hack.

@alexcrichton
Copy link
Member

It's true that this is unfortunate! Note, however, that flate2 has an issue for wasm support which was fixed in Frommi/miniz_oxide#35 and is waiting for an new version of the crate to be published. That's doing the "proper" fix here which is to wrap libc locally, and not pushing the logic into this crate.

@rodrigorc
Copy link
Author

Ah, that's great, thanks for the link. I've just tried that and now miniz_oxide compiles fine, but flate2 fails.
I'm now trying to patch flate2, but that's not so easy, because the libc stub in miniz_oxide is private, but the C types are in the public API (is that even allowed?).

What would be the proper solution for this? Making miniz_oxide::libc public seems like giving it a responsibility that it does not deserve. Maybe re-exporting the used libc types, and make the clients use these instead? But it looks like flate2 uses C types in a lot of places, not only in the miniz API...

Or maybe creating a new crate wasm32_libc_stub? But then, this crate would just have the same definitions as this PR, and its users will have to use it or libc conditionally, every time:

#[cfg(not(all(target_arch = "wasm32", not(target_os = "emscripten"))))]
extern crate libc;
#[cfg(all(target_arch = "wasm32", not(target_os = "emscripten")))]
extern crate wasm32_lib_stub as libc;

IMHO, we would better having at least the C types in libc.

@alexcrichton
Copy link
Member

@rodrigorc it's possible yes for a wasm32_libc_stub crate to be maintained, but it wouldn't be done here. The libc crate here is for the official definition for each target, matching compilers like clang and gcc. Since wasm32-unknown-unknown is picking up steam in Clang (officially shipping on the next version) we could provide definitions of c_int and such in this crate, but things like malloc would still be left to client crates.

@rodrigorc
Copy link
Author

@alexcrichton I understand malloc/free are not appropriate here, but miniz_oxide has already patched that, so I'm happy without them.

About the other types, however, I've noticed that c_void is actually in core::ffi so already defined in wasm32 today, only that libc does not re-export it.

And curiously many of the other C types are defined in std::os::raw, but they are redefined, not re-exported, in libc. I wonder why is that, historical accident?

Then, should we wait for clang-wasm to arrive in order to add the C types to this crate?

@alexcrichton
Copy link
Member

Yeah let's wait for clang-wasm to ship before adding the types to this crate I think, and std::os::raw vs libc is indeed a historical accident effectively.

@rodrigorc
Copy link
Author

Ok, I'll close this PR and keep using my clib fork until clang-wasm is deployed (cargo makes it so easy).

By any chance, do you have any link about the clang-wasm development?

Thanks for you attention!

@rodrigorc rodrigorc closed this Oct 16, 2018
@alexcrichton
Copy link
Member

Ah unfortunately I'm not sure how to stay in tune with wasm development in clang, but I do know that it's merged as a mainstream target so the next release (Clang 8) will have wasm support by default

nmattia added a commit to nmattia/lodepng-rust that referenced this pull request Nov 12, 2021
This adds a wasm32 implementation for `free`, `malloc` and `realloc`.
Originally written by @rodrigorc in
rust-lang/libc#1092 and adapted.
nmattia added a commit to nmattia/lodepng-rust that referenced this pull request Nov 23, 2021
This adds a wasm32 implementation for `free`, `malloc` and `realloc`.
Originally written by @rodrigorc in
rust-lang/libc#1092 and adapted.

NOTE: This is only enabled for `wasm32-unknown-unknown` ([`target_arch =
"wasm32"`](https://doc.rust-lang.org/reference/conditional-compilation.html#target_arch), [`target_os =
"unknown"`](https://doc.rust-lang.org/reference/conditional-compilation.html#target_os)), because other wasm targets (like `wasm32-unknown-emscripten`) may provide a `libc` implementation.
nmattia added a commit to nmattia/lodepng-rust that referenced this pull request Nov 23, 2021
This adds a wasm32 implementation for `free`, `malloc` and `realloc`.
Originally written by @rodrigorc in
rust-lang/libc#1092 and adapted.

NOTE: This is only enabled for `wasm32-unknown-unknown` ([`target_arch =
"wasm32"`](https://doc.rust-lang.org/reference/conditional-compilation.html#target_arch), [`target_os =
"unknown"`](https://doc.rust-lang.org/reference/conditional-compilation.html#target_os)), because other wasm targets (like `wasm32-unknown-emscripten`) may provide a `libc` implementation.
nmattia added a commit to nmattia/lodepng-rust that referenced this pull request Nov 29, 2021
This adds a wasm32 implementation for `free`, `malloc` and `realloc`.
Originally written by @rodrigorc in
rust-lang/libc#1092 and adapted.

NOTE: This is only enabled for `wasm32-unknown-unknown` ([`target_arch =
"wasm32"`](https://doc.rust-lang.org/reference/conditional-compilation.html#target_arch), [`target_os =
"unknown"`](https://doc.rust-lang.org/reference/conditional-compilation.html#target_os)), because other wasm targets (like `wasm32-unknown-emscripten`) may provide a `libc` implementation.
kornelski pushed a commit to kornelski/lodepng-rust that referenced this pull request Nov 30, 2021
This adds a wasm32 implementation for `free`, `malloc` and `realloc`.
Originally written by @rodrigorc in
rust-lang/libc#1092 and adapted.

NOTE: This is only enabled for `wasm32-unknown-unknown` ([`target_arch =
"wasm32"`](https://doc.rust-lang.org/reference/conditional-compilation.html#target_arch), [`target_os =
"unknown"`](https://doc.rust-lang.org/reference/conditional-compilation.html#target_os)), because other wasm targets (like `wasm32-unknown-emscripten`) may provide a `libc` implementation.
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.

3 participants