wasm-tools icon indicating copy to clipboard operation
wasm-tools copied to clipboard

Add no-std support to wasmparser

Open ethindp opened this issue 4 years ago • 7 comments

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

ethindp avatar Oct 16 '21 08:10 ethindp

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.

alexcrichton avatar Oct 18 '21 15:10 alexcrichton

@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.

ethindp avatar Oct 19 '21 01:10 ethindp

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.

alexcrichton avatar Oct 19 '21 15:10 alexcrichton

@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.

ethindp avatar Oct 19 '21 21:10 ethindp

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.

alexcrichton avatar Oct 20 '21 14:10 alexcrichton

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 avatar Jan 09 '22 10:01 Robbepop

@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 avatar Jan 22 '22 16:01 Robbepop

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.

Robbepop avatar Sep 12 '22 14:09 Robbepop

Sounds good.

alexcrichton avatar Sep 12 '22 15:09 alexcrichton