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 havestd
when the activated features are a subset of{runtime, gc, component-model}
. Support for Wasmtime requires an implementation ofwasmtime-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 forwasmtime-environ
and other crates, for example. For example imports come fromcore
andalloc
and there's a custom prelude for core + alloc bits and pieces.- Wasmtime now unconditionally uses
hashbrown::HashMap
for some items such asLinker
and GC sets.- The
wasmtime
crate has a few "custom synchronization primitives" which have real implementations backed by other crates instd
mode or "dummy" implementations inno_std
mode that effectively assert for no contention. This might require updates towasmtime-platform.h
in the future instead.- Libcall intrinsics for float-related functionality are implemented with the
libm
crate inno_std
mode. Instd
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 testno_std
mode in CI, only that it builds. If anything comes up though I'll work through that in follow-ups.
alexcrichton requested wasmtime-default-reviewers for a review on PR #8533.
alexcrichton requested wasmtime-core-reviewers for a review on PR #8533.
alexcrichton requested fitzgen for a review on PR #8533.
alexcrichton updated PR #8533.
alexcrichton updated PR #8533.
alexcrichton updated PR #8533.
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 nestedstruct
s).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.</details>
alexcrichton updated PR #8533.
alexcrichton updated PR #8533.
alexcrichton updated PR #8533.
abrown submitted PR review.
abrown created PR review comment:
Undefined behavior... in Wasmtime? I thought we could be pretty sure we'd get a SIGILL or the like?
alexcrichton updated PR #8533.
alexcrichton updated PR #8533.
alexcrichton submitted PR review.
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
fitzgen commented on PR #8533:
(Taking a look, might be a little while to get through everything here)
bjorn3 submitted PR review.
bjorn3 created PR review comment:
lzcnt
is encoded asrep bsr
, which has a redundant prefix. Redundant prefixes tend to be ignored, but are not guaranteed to be. As suchlzcnt
is interpreted by older cpu's asbsr
. 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
fitzgen submitted PR review:
ship it!
fitzgen submitted PR review:
ship it!
fitzgen created PR review comment:
conflict
fitzgen created PR review comment:
Why is
std
okay to use here? Shouldn't this use thewasmtime_tls_{get,set}
stuff?
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.
fitzgen created PR review comment:
I guess if we are on
unix
then we know we havestd
available?
fitzgen created PR review comment:
Ah okay, you got it in a later commit
fitzgen created PR review comment:
std
?
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.
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 anif
/else
here since there is just one checked-for case and one default case.
alexcrichton submitted PR review.
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
alexcrichton submitted PR review.
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 anif
/else
here)
alexcrichton submitted PR review.
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
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Yeah any modules which are gated on the
std
feature still import fromstd
and don't need to get switched over. Here this module is only used forasync
which requires thestd
feature at this time, so we'll need to change this in the future to an import fromcore
if we want theasync
feature to work on no_std for example.
alexcrichton submitted PR review.
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.
alexcrichton updated PR #8533.
alexcrichton has enabled auto merge for PR #8533.
alexcrichton updated PR #8533.
alexcrichton merged PR #8533.
Last updated: Dec 23 2024 at 12:05 UTC