cranelift-codegen: no_std support #9007

marceline-cramer · 2024-07-24T22:00:02Z

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.

- switch out all instances of rustc-hash with hashbrown - refactor timing module to support disabling on no_std - ratchet codegen crate no_std support in no_std CI checks - use core_math for libm-based arithmetic traits on no_std - enable hard std feature dep for souper-harvest feature

github-actions · 2024-07-25T01:04:09Z

Subscribe to Label Action

cc @cfallin, @fitzgen

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:

cfallin: isle
fitzgen: isle

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

Learn more.

bjorn3 · 2024-07-25T07:58:20Z

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

afonso360 · 2024-07-25T09:02:14Z

cranelift/codegen/src/ir/immediates.rs

+        // TODO: find a suitable alternative to std round_ties_even() in no_std
+        Self::with_float(self.as_f64().round())


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.

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

alexcrichton

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.

alexcrichton · 2024-07-25T14:33:19Z

Cargo.toml

 capstone = "0.12.0"
-once_cell = { version = "1.12.0", default-features = false }
+once_cell = { version = "1.12.0", default-features = false, features = ["critical-section"] }


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.

alexcrichton · 2024-07-25T14:35:41Z

cranelift/codegen/src/ir/immediates.rs

+        // TODO: find a suitable alternative to std round_ties_even() in no_std
+        Self::with_float(self.as_f64().round())


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

alexcrichton · 2024-07-25T14:36:59Z

cranelift/codegen/src/isa/riscv64/inst/args.rs

@@ -57,7 +60,7 @@ macro_rules! newtype_of_reg {
        // NB: We cannot implement `DerefMut` because that would let people do
        // nasty stuff like `*my_xreg.deref_mut() = some_freg`, breaking the
        // invariants that `XReg` provides.
-        impl std::ops::Deref for $newtype_reg {
+        impl core::ops::Deref for $newtype_reg {


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.

alexcrichton · 2024-07-25T14:38:39Z

cranelift/codegen/src/lib.rs

 #[cfg(not(feature = "std"))]
+#[macro_use]
+extern crate core as std;


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;

alexcrichton · 2024-07-25T14:40:15Z

cranelift/codegen/src/machinst/vcode.rs

+                    let caller_sp_to_cfa_offset =
+                        crate::isa::unwind::systemv::caller_sp_to_cfa_offset();


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.

alexcrichton · 2024-07-25T14:41:21Z

cranelift/codegen/src/timing.rs

+#[cfg(feature = "timing")]
+use core::cell::RefCell;


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.

alexcrichton · 2024-07-25T14:42:04Z

cranelift/isle/isle/src/codegen.rs

+        if options.no_std {
+            writeln!(code, "use alloc::vec::Vec;").unwrap();
+            writeln!(code, "use core::{{marker::PhantomData, ops}};").unwrap();
+        } else {
+            writeln!(code, "use std::{{marker::PhantomData, ops}};").unwrap();
+        }


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

tschneidereit · 2024-09-07T12:56:52Z

@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 :)

marceline-cramer · 2024-09-13T05:20:24Z

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

tschneidereit · 2024-09-13T12:34:37Z

@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 :)

marceline-cramer added 4 commits July 24, 2024 15:08

cranelift-isle: add option to enable no_std building

58a4308

Tweak workspace dependencies to support codegen no_std

0931de3

cranelift: correct use of hashbrown in codegen API

2ad7e55

github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:aarch64 Issues related to AArch64 backend. isle Related to the ISLE domain-specific language labels Jul 25, 2024

afonso360 reviewed Jul 25, 2024

View reviewed changes

alexcrichton reviewed Jul 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cranelift-codegen: no_std support #9007

cranelift-codegen: no_std support #9007

marceline-cramer commented Jul 24, 2024

github-actions bot commented Jul 25, 2024

bjorn3 commented Jul 25, 2024

afonso360 Jul 25, 2024

alexcrichton Jul 25, 2024

alexcrichton left a comment

alexcrichton Jul 25, 2024

alexcrichton Jul 25, 2024

alexcrichton Jul 25, 2024

alexcrichton Jul 25, 2024

alexcrichton Jul 25, 2024

alexcrichton Jul 25, 2024

alexcrichton Jul 25, 2024

tschneidereit commented Sep 7, 2024

marceline-cramer commented Sep 13, 2024

tschneidereit commented Sep 13, 2024

		// TODO: find a suitable alternative to std round_ties_even() in no_std
		Self::with_float(self.as_f64().round())

		let caller_sp_to_cfa_offset =
		crate::isa::unwind::systemv::caller_sp_to_cfa_offset();

cranelift-codegen: no_std support #9007

Are you sure you want to change the base?

cranelift-codegen: no_std support #9007

Conversation

marceline-cramer commented Jul 24, 2024

github-actions bot commented Jul 25, 2024

Subscribe to Label Action

bjorn3 commented Jul 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tschneidereit commented Sep 7, 2024

marceline-cramer commented Sep 13, 2024

tschneidereit commented Sep 13, 2024