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 no-std support to wasmparser #364

Closed
wants to merge 3 commits into from
Closed

Conversation

ethindp
Copy link

@ethindp ethindp commented Oct 16, 2021

Signed-off-by: Ethin Probst ethindp@protonmail.com

Signed-off-by: Ethin Probst <ethindp@protonmail.com>
@alexcrichton
Copy link
Member

Thanks for the PR, but this library doesn't support #![no_std] at this time for the same reasons as Wasmtime, so to add something like this would warrant discussion and agreement for Wasmtime first.

@ethindp
Copy link
Author

ethindp commented Oct 19, 2021

@alexcrichton Why would whether this supports no_std or not be dependent on what wasmtime supports? All this does is parse wasm -- it doesn't actually execute or JIT it, and it (by default) includes the standard library. The change was a trivial one. Merging this functionality won't affect wasmtime in any way, and the same patterns that've been used thus far can continue to be used in this library since I doubt you'll need threading or anything like that if your just parsing Wasm binaries and letting the end-developer handle the structures generated from them. If wasmparser -- which is what changed -- won't ever depend on things that you'll only find in a C library, which appears to be the case since everything that the library does is portable, I'm quite confused on the refusal to merge this PR.

@alexcrichton
Copy link
Member

Well in some sense I'm one of the primary maintainers of this crate and I'm also the one who wrote up the policy for wasmtime of not supporting #![no_std]. In another sense one of this crate's main purposes is to serve Wasmtime and if no_std makes development on Wasmtime more difficult then that applies to this crate as well and we don't want that.

If you'd like to discuss the no_std policy I'd recommend opening a specific issue on Wasmtime than continuing here in a PR on a different repo.

@ethindp
Copy link
Author

ethindp commented Oct 19, 2021

@alexcrichton That's the thing: this PR doesn't actually do that. It will not make wasmtime development harder or anything. So long as you don't go importing (for example) std::thread::* in wasmparser (which you shouldn't because you shouldn't need any networking/threading/process handling functions in a Wasm parser) you shouldn't need to do anything. Even std::sync::* will work fine (including mutexes/spinlocks/reader-writer locks). Wasmtime shouldn't need to do anything on its end to use this PR.

@alexcrichton
Copy link
Member

Sorry all I can recommend is reiterating my above point, this isn't the place to discuss the bytecodealliance position on #![no_std], but rather that should happen as an issue on Wasmtime or an RFC even.

@Robbepop
Copy link
Contributor

Robbepop commented Jan 9, 2022

For me a no_std wasmparser would also be nice since I require Wasm compilation support without WASI.
So I gave it a try today myself and have to conclude that wasmparser being a crate in a workspace makes this task more difficult since other workspace members fiddle with Cargo features of wasmparser itself.
You can inspect this by adding --verbose to cargo build commands.
This PR also has some issues, for example the use std::error::Error is invalid in no_std contexts which is undiscovered since this PR does not fix the aforementioned Cargo issues with feature resolution in workspaces.

tldr; std feature is always included in wasmparser builds due to the workspace.

To work around this you either have to convert the entire workspace to support no_std or move wasmparser into its own crate or at least exclude it from the workspace.
At a minimum it is enough to support no_std for wasmparser-dump crate since it is directly depended on (dev-dependency) by wasmparser itself. However, I am not sure a no_std version of wasmparser-dump is actually useful.

@Robbepop
Copy link
Contributor

@ethindp FYI I published a no_std compatible wasmparser crate here: https://crates.io/crates/wasmparser-nostd

It also switches out Hash{Map,Set} with BTree{Map,Set} internally so that the implementation is not attackable in no_std environment. Enable no_std by disabling default features, e.g. cargo build --no-default-features.

@Robbepop
Copy link
Contributor

I think we can close this PR.

  • It is outdated.
  • We already have an open issue that informs people about no_std support in wasmparser et. al.
  • The issue contains a link to the actual wasmparser-nostd fork that I maintain since a while.

@alexcrichton
Copy link
Member

Sounds good.

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.

None yet

3 participants