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 support for #![no_std] to the wasmtime crate #8533

Merged
merged 25 commits into from
May 4, 2024

Conversation

alexcrichton
Copy link
Member

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.

@alexcrichton alexcrichton requested review from a team as code owners May 3, 2024 03:49
@alexcrichton alexcrichton requested review from fitzgen and removed request for a team May 3, 2024 03:49
@github-actions github-actions bot added wasi Issues pertaining to WASI wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:c-api Issues pertaining to the C API. wasmtime:config Issues related to the configuration of Wasmtime labels May 3, 2024
Copy link

github-actions bot commented May 3, 2024

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.

    Our documentation should be of the following form:

    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.
    
  • If you added a new Config method, or modified an existing one, you
    ensured that this configuration is exercised by the fuzz targets.

    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 (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 for more 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.


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

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

Learn more.

///
/// This function is used to verify that any features enabled for a compiler
/// backend, such as AVX support on x86\_64, are also available on the host.
/// It is undefined behavior to execute an AVX instruction on a host that
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: rust-lang/rustc_codegen_cranelift#951 (comment) See also https://stackoverflow.com/questions/25683690/confusion-about-bsr-and-lzcnt/43443701#43443701

This commit updates Wasmtime's platform support to no longer require an
opt-in `RUSTFLAGS` `--cfg` flag to be specified. With `no_std` becoming
officially supported this should provide a better onboarding experience
where the fallback custom platform is used. This will cause linker
errors if the symbols aren't implemented and searching/googling should
lead back to our docs/repo (eventually, hopefully).
This commit updates the management of TLS to rely on just a single
pointer rather than a pair of a pointer and a `bool`. Additionally
management of the TLS state is pushed into platform-specific modules to
enable different means of managing it, namely the "custom" platform now
has a C function required to implement TLS state for Wasmtime.
The `Duration` type is available in `no_std` but the `Instant` type is
not. The intention is to only support the `threads` proposal if `std` is
active but to assist with this split push the `Duration` further into
Wasmtime to avoid using a type that can't be mentioned in `no_std`.
Move `serde_json` to an optional dependency and gate the guest profiler
entirely on the `profiling` feature.
Have a dedicated trait for consuming `self` in addition to a
`Result`-friendly trait.
Cut down the dependency list if `addr2line` isn't enabled since then
the dependency is not used. While here additionally lift the version
requirement for `addr2line` up to the workspace level.
Pull most types from Wasmtime's `__internal` module as the source of
truth.
No need for synchronization here when mutability is already available in
the necessary contexts.
This commit enables compiling the `runtime`, `gc`, and `component-model`
features of the `wasmtime` crate on targets that do not have `std`. This
tags the crate as `#![no_std]` and then updates everything internally to
import from `core` or `alloc` and adapt for the various idioms. This
ended up requiring some relatively extensive changes, but nothing too
too bad in the grand scheme of things.
@fitzgen
Copy link
Member

fitzgen commented May 3, 2024

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

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ship it!

crates/wasmtime/src/runtime/module.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/runtime/stack.rs Show resolved Hide resolved
crates/wasmtime/src/runtime/store.rs Outdated Show resolved Hide resolved
@@ -308,7 +308,7 @@ pub fn lazy_per_thread_init() {
// This thread local is purely used to register a `Stack` to get deallocated
// when the thread exists. Otherwise this function is only ever called at
// most once per-thread.
thread_local! {
std::thread_local! {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

crates/wasmtime/src/sync_nostd.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/sync_nostd.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/runtime/module.rs Outdated Show resolved Hide resolved
@alexcrichton alexcrichton enabled auto-merge May 4, 2024 21:36
@alexcrichton alexcrichton added this pull request to the merge queue May 4, 2024
Merged via the queue into bytecodealliance:main with commit 81a8916 May 4, 2024
47 checks passed
@alexcrichton alexcrichton deleted the no-std-rebase branch May 4, 2024 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasi Issues pertaining to WASI wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:c-api Issues pertaining to the C API. wasmtime:config Issues related to the configuration of Wasmtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants