wasmtime / PR #8533 Add support for `#![no_std]` to the `... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #8533 Add support for `#![no_std]` to the `...

Wasmtime GitHub notifications bot (May 03 2024 at 03:49):

alexcrichton opened PR #8533 from alexcrichton:no-std-rebase to bytecodealliance:main:

This commit is the final technical piece of #8341 which makes the wasmtime crate itself compatible with #![no_std]. This means that the crate can now be compiled on targets that don't have std when the activated features are a subset of {runtime, gc, component-model}. Support for Wasmtime requires an implementation of wasmtime-platform.h which is something I'd like to also start moving to release artifacts after this as well.

This PR is split into a series of commits with the final one being the "big one" and the others are minor refactorings leading up to it. The major changes made here are:

The "custom" platform of Wasmtime is now enabled by default if nothing else is applicable. This means that there's no opt-in required for no_std support.

Embedders can now define a custom feature detection function, for example "does this host support AVX". This is because the default implementation uses the standard library while that's not available with no_std.

Internal idioms in wasmtime have change to be similar to previous commits for wasmtime-environ and other crates, for example. For example imports come from core and alloc and there's a custom prelude for core + alloc bits and pieces.

Wasmtime now unconditionally uses hashbrown::HashMap for some items such as Linker and GC sets.

The wasmtime crate has a few "custom synchronization primitives" which have real implementations backed by other crates in std mode or "dummy" implementations in no_std mode that effectively assert for no contention. This might require updates to wasmtime-platform.h in the future instead.

Libcall intrinsics for float-related functionality are implemented with the libm crate in no_std mode. In std mode they still use standard library methods.

The main remaining piece I'd like to tackle after this is an update of the no_std documentation for Wasmtime. The page is outdated now and could use some updates. I'll also note that I still have yet to test all these changes beyond what CI is already doing. We don't actually test no_std mode in CI, only that it builds. If anything comes up though I'll work through that in follow-ups.

Wasmtime GitHub notifications bot (May 03 2024 at 03:49):

alexcrichton requested wasmtime-default-reviewers for a review on PR #8533.

Wasmtime GitHub notifications bot (May 03 2024 at 03:49):

alexcrichton requested wasmtime-core-reviewers for a review on PR #8533.

Wasmtime GitHub notifications bot (May 03 2024 at 03:49):

alexcrichton requested fitzgen for a review on PR #8533.

Wasmtime GitHub notifications bot (May 03 2024 at 03:51):

alexcrichton updated PR #8533.

Wasmtime GitHub notifications bot (May 03 2024 at 03:57):

alexcrichton updated PR #8533.

Wasmtime GitHub notifications bot (May 03 2024 at 04:39):

alexcrichton updated PR #8533.

Wasmtime GitHub notifications bot (May 03 2024 at 06:44):

github-actions[bot] commented on PR #8533:

Label Messager: wasmtime:config

It looks like you are changing Wasmtime's configuration options. Make sure to
complete this check list:

[ ] If you added a new Config method, you wrote extensive documentation for
it.

<details>

Our documentation should be of the following form:

```text
Short, simple summary sentence.

More details. These details can be multiple paragraphs. There should be
information about not just the method, but its parameters and results as
well.

Is this method fallible? If so, when can it return an error?

Can this method panic? If so, when does it panic?

Example

Optional example here.
```

</details>

[ ] If you added a new Config method, or modified an existing one, you
ensured that this configuration is exercised by the fuzz targets.

<details>

For example, if you expose a new strategy for allocating the next instance
slot inside the pooling allocator, you should ensure that at least one of our
fuzz targets exercises that new strategy.

Often, all that is required of you is to ensure that there is a knob for this
configuration option in [wasmtime_fuzzing::Config][fuzzing-config] (or one
of its nested structs).

Rarely, this may require authoring a new fuzz target to specifically test this
configuration. See [our docs on fuzzing][fuzzing-docs] for more details.

</details>

[ ] If you are enabling a configuration option by default, make sure that it
has been fuzzed for at least two weeks before turning it on by default.

[fuzzing-config]: https://github.com/bytecodealliance/wasmtime/blob/ca0e8d0a1d8cefc0496dba2f77a670571d8fdcab/crates/fuzzing/src/generators.rs#L182-L194
[fuzzing-docs]: https://docs.wasmtime.dev/contributing-fuzzing.html

<details>

To modify this label's message, edit the <code>.github/label-messager/wasmtime-config.md</code> file.

To add new label messages or remove existing label messages, edit the
<code>.github/label-messager.json</code> configuration file.

Learn more.

</details>

Wasmtime GitHub notifications bot (May 03 2024 at 15:55):

alexcrichton updated PR #8533.

Wasmtime GitHub notifications bot (May 03 2024 at 15:56):

alexcrichton updated PR #8533.

Wasmtime GitHub notifications bot (May 03 2024 at 16:06):

alexcrichton updated PR #8533.

Wasmtime GitHub notifications bot (May 03 2024 at 16:58):

abrown submitted PR review.

Wasmtime GitHub notifications bot (May 03 2024 at 16:58):

abrown created PR review comment:

Undefined behavior... in Wasmtime? I thought we could be pretty sure we'd get a SIGILL or the like?

Wasmtime GitHub notifications bot (May 03 2024 at 17:38):

alexcrichton updated PR #8533.

Wasmtime GitHub notifications bot (May 03 2024 at 17:40):

alexcrichton updated PR #8533.

Wasmtime GitHub notifications bot (May 03 2024 at 17:58):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 03 2024 at 17:58):

alexcrichton created PR review comment:

I'll admit I don't necessarily fully understand this, but my reasoning here is derived from Rust's stance that executing intrinsics your CPU doesn't have support for is undefined behavior. There was a fair amount of discussion about this when first stabilizing SIMD in Rust and I believe the general rationale boils down to:

There's not an ironclad guarantee that unsupported instructions raise a SIGILL for all unsupported instructions. (at least at the time what was considered was all possible extensions to all possible ISAs)

There can be "compiler weirdness" where once a feature is enabled it has subtle changes in other parts like assemblers and other chosen instructions which can cause subtle incompatibilities on systems that don't support the instructions.

Overall I don't think that there's a super simple "this is why it's unsafe" thing I can point to to justify this. I am relatively confident in saying, though, that we're probably not guaranteed a SIGILL for all possible encodings of all instructions we could use on all architectures. Coupling that with this is a niche method most won't use is mostly why I figured it'd be ok to mark this as unsafe

Wasmtime GitHub notifications bot (May 03 2024 at 18:10):

fitzgen commented on PR #8533:

(Taking a look, might be a little while to get through everything here)

Wasmtime GitHub notifications bot (May 03 2024 at 19:38):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (May 03 2024 at 19:38):

bjorn3 created PR review comment:

lzcnt is encoded as rep bsr, which has a redundant prefix. Redundant prefixes tend to be ignored, but are not guaranteed to be. As such lzcnt is interpreted by older cpu's as bsr. This bit me once in cg_clif: https://github.com/rust-lang/rustc_codegen_cranelift/pull/951#issuecomment-663855992 See also https://stackoverflow.com/questions/25683690/confusion-about-bsr-and-lzcnt/43443701#43443701

Wasmtime GitHub notifications bot (May 04 2024 at 16:45):

fitzgen submitted PR review:

ship it!

Wasmtime GitHub notifications bot (May 04 2024 at 16:45):

fitzgen submitted PR review:

ship it!

Wasmtime GitHub notifications bot (May 04 2024 at 16:45):

fitzgen created PR review comment:

conflict

Wasmtime GitHub notifications bot (May 04 2024 at 16:45):

fitzgen created PR review comment:

Why is std okay to use here? Shouldn't this use the wasmtime_tls_{get,set} stuff?

Wasmtime GitHub notifications bot (May 04 2024 at 16:45):

fitzgen created PR review comment:

We could also add hooks to the system interface header to help implement these and push the choice of panicking/aborting or actually doing synchronization out tot he embedder as well down the line. Again, might be worth a comment.

Wasmtime GitHub notifications bot (May 04 2024 at 16:45):

fitzgen created PR review comment:

I guess if we are on unix then we know we have std available?

Wasmtime GitHub notifications bot (May 04 2024 at 16:45):

fitzgen created PR review comment:

Ah okay, you got it in a later commit

Wasmtime GitHub notifications bot (May 04 2024 at 16:45):

fitzgen created PR review comment:

std?

Wasmtime GitHub notifications bot (May 04 2024 at 16:45):

fitzgen created PR review comment:

FWIW, we could add another hook to the system interface header for this, if it is ever requested. Might be worth adding a comment about.

Wasmtime GitHub notifications bot (May 04 2024 at 16:45):

fitzgen created PR review comment:

This isn't going to accidentally bind INITIALIZED as a fresh variable, right? I always forget the details of when that can happen or not... Might be better to use an if/else here since there is just one checked-for case and one default case.

Wasmtime GitHub notifications bot (May 04 2024 at 21:30):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 04 2024 at 21:30):

alexcrichton created PR review comment:

Yeah that was my thinking as well in terms of moving it into the header, good point on leaving a comment though

Wasmtime GitHub notifications bot (May 04 2024 at 21:31):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 04 2024 at 21:31):

alexcrichton created PR review comment:

Nah yeah we're ok to have the match here, I generally rely on the unused variables lint to catch mistakes like this. (I'll go ahead and switch it anyway to an if/else here)

Wasmtime GitHub notifications bot (May 04 2024 at 21:33):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 04 2024 at 21:33):

alexcrichton created PR review comment:

Yeah I think that'd work out too, but my hope is that all std-using platforms can funnel into the first case with eprintln and most no_std platforms probably don't want unwinding anyway

Wasmtime GitHub notifications bot (May 04 2024 at 21:33):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 04 2024 at 21:33):

alexcrichton created PR review comment:

Yeah any modules which are gated on the std feature still import from std and don't need to get switched over. Here this module is only used for async which requires the std feature at this time, so we'll need to change this in the future to an import from core if we want the async feature to work on no_std for example.

Wasmtime GitHub notifications bot (May 04 2024 at 21:34):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 04 2024 at 21:34):

alexcrichton created PR review comment:

Yeah that was my thinking, on platforms that are known to have the standard library we go ahead and just import from the standard library. Things will get a bit messier otherwise because it's not a great experience for Wasmtime to depend on #[repr(C)] symbols silently so I'm hoping to keep it to a minimum.

Wasmtime GitHub notifications bot (May 04 2024 at 21:36):

alexcrichton updated PR #8533.

Wasmtime GitHub notifications bot (May 04 2024 at 21:36):

alexcrichton has enabled auto merge for PR #8533.

Wasmtime GitHub notifications bot (May 04 2024 at 21:39):

alexcrichton updated PR #8533.

Wasmtime GitHub notifications bot (May 04 2024 at 22:25):

alexcrichton merged PR #8533.

Last updated: Nov 22 2024 at 17:03 UTC