Skip to content

Add no-std support to wasmparser#364

Closed
ethindp wants to merge 3 commits into
bytecodealliance:mainfrom
ethindp:main
Closed

Add no-std support to wasmparser#364
ethindp wants to merge 3 commits into
bytecodealliance:mainfrom
ethindp:main

Conversation

@ethindp

@ethindp ethindp commented Oct 16, 2021

Copy link
Copy Markdown

Signed-off-by: Ethin Probst [email protected]

@alexcrichton

Copy link
Copy Markdown
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

ethindp commented Oct 19, 2021

Copy link
Copy Markdown
Author

@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
Copy Markdown
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

ethindp commented Oct 19, 2021

Copy link
Copy Markdown
Author

@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
Copy Markdown
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

Robbepop commented Jan 9, 2022

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

@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
Copy Markdown
Collaborator

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
Copy Markdown
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.

3 participants