Stream: git-wasmtime

Topic: wasmtime / PR #9007 cranelift-codegen: no_std support

view this post on Zulip Wasmtime GitHub notifications bot (Jul 24 2024 at 22:00):

marceline-cramer opened PR #9007 from marceline-cramer:cranelift-codegen-no-std to bytecodealliance:main:

Closes #1158, previous context and discussion can be found there. This is a major milestone towards allowing Cranelift to compile code while in embedded or kernel environments.

I do still have a handful of pinch points to work through before this is ready to merge.

First of all, the most major overhaul I've made to the codegen crate is to replace rustc-hash with hashbrown and ahash. I'm not a cryptography expert but as far as I can tell, ahash does have a security improvement over FxHash in terms of improved DOS resistance. More importantly though, hashbrown has a feature to use ahash by default, which means that replacing FxHashMap and FxHashSet was as easy as a few invocations of sed. However, I believe that this switch in hashing function has caused some of the codegen expected output tests in CI to fail, and the semantics of the codegen have been slightly changed. I could experiment with swapping rustc-hash back into hashbrown as its hasher implementation locally but I wanted to know if preserving these semantics was important before I undertook another major type substitution throughout the code. Please take a look at the output of the failing CI for more info.

Second of all, on no_std, floating-point arithmetic is not available. To resolve this, I brought in the core_math crate, which implements floating-point arithmetic traits for core primitives using libm's implementation of softfloat. However, the function rounding_ties_even() is not available in libm, and I'm not sure how to go about replacing it. Input would be appreciated. I do know that if the failing codegen output unit tests I discussed are updated to pass with the new output, other tests catch the change in arithmetic semantics and fail.

Finally, there's this else block in src/machinst/vcode.rs that has some arithmetic with a hard dependency on the unwind feature, which is unsupported on no_std. I'm not sure what to do with that.

Thank you so much for the help with all of this, I appreciate your time immensely. I'm really happy to answer any questions and update my code as needed.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 01:04):

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

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "isle"

Thus the following users have been cc'd because of the following labels:

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 07:58):

bjorn3 commented on PR #9007:

rustc-hash is no_std compatible. You just need to disable the std feature.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 09:02):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 09:02):

afonso360 created PR review comment:

I looked at the source for round_ties_even and they use rintf. It looks like this is due to portability between platforms, but it looks like it gives the same results if we don't change rounding modes. libm has support for rint{f,} so we can probably use that.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 14:44):

alexcrichton created PR review comment:

We'll want to find alternative solutions to this since personally I'd consider the critical-section feature as inappropriate for our use cases. That enables spin locks which just keep spinning which are not suitable anywhere outside of a kernel-with-interrupts-disabled context, so the dependencies on once_cell will need to be reworked and/or have other solutions than "just" enabling this feature.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 14:44):

alexcrichton created PR review comment:

We'll want to be careful though such that where std is enabled we want the same implementation cranelift has today which is using presumably optimal routines in the standard library. Only when that feature is disabled should it fall back to the possibly-slower routines based in libm

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 14:44):

alexcrichton created PR review comment:

What I might recommend for this file is to have a timing.rs and a timing_disabled.rs as opposed to a single source file with lots of #[cfg], that's worked pretty well for us in the past.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 14:44):

alexcrichton submitted PR review:

Thanks again for your work on this! I've left some additional comments to those above below. At a high level I might recommend splitting out the s/std/core/ rename to a separate PR along with what's needed to handle hash maps (agreed would be best to keep using rustc-hash).

The once-cell bits are going to require some careful thought since we can't enable spin locks for the entire workspace. One idea would be to shuffle around the sync_nostd.rs file used by Wasmtime since Cranelift looks like it may have a similar use case.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 14:44):

alexcrichton created PR review comment:

Oh this shouldn't be done in my opinion, it can create a lot of confusion by renaming core crates to each other.

I'd recommend following the pattern of Wasmtime crates which is to do:

#[cfg(feature = "std")]
#[macro_use]
extern crate std;

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 14:44):

alexcrichton created PR review comment:

This PR might take a moment to figure out some of the other dependencies and such, so if you'd like I think it would be reasonable to split out the s/std/core/ rename to a separate PR.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 14:44):

alexcrichton created PR review comment:

The definition of this function is "return 0" so I think it's fine to lift this function out of unwind and move it somewhere else.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 14:44):

alexcrichton created PR review comment:

I might echo the desire to remove the no_std configuration here entirely perhaps. The generate code can itself contain extern crate alloc which can then be used to import Vec

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2024 at 14:44):

alexcrichton submitted PR review:

Thanks again for your work on this! I've left some additional comments to those above below. At a high level I might recommend splitting out the s/std/core/ rename to a separate PR along with what's needed to handle hash maps (agreed would be best to keep using rustc-hash).

The once-cell bits are going to require some careful thought since we can't enable spin locks for the entire workspace. One idea would be to shuffle around the sync_nostd.rs file used by Wasmtime since Cranelift looks like it may have a similar use case.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 07 2024 at 12:56):

tschneidereit commented on PR #9007:

@marceline-cramer is there anything we can to do help you get the review comments addressed? Getting this in would be excellent, and you've already put in the major part of the work, after all :)

view this post on Zulip Wasmtime GitHub notifications bot (Sep 13 2024 at 05:20):

marceline-cramer commented on PR #9007:

@marceline-cramer is there anything we can to do help you get the review comments addressed? Getting this in would be excellent, and you've already put in the major part of the work, after all :)

Apologies for putting this off for so long. I've been capital-B Busy the last couple months. I'll try to provide an answer to that question over the weekend when I can refresh my memory on what precisely is happening here and process the comments.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 13 2024 at 12:34):

tschneidereit commented on PR #9007:

@marceline-cramer no worries at all—life happens to all of us! I really meant the question in the way I worded it: as an offer to help get things unblocked :)

Last updated: Nov 22 2024 at 16:03 UTC