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 replacerustc-hash
withhashbrown
andahash
. I'm not a cryptography expert but as far as I can tell,ahash
does have a security improvement overFxHash
in terms of improved DOS resistance. More importantly though,hashbrown
has a feature to useahash
by default, which means that replacingFxHashMap
andFxHashSet
was as easy as a few invocations ofsed
. 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 swappingrustc-hash
back intohashbrown
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 functionrounding_ties_even()
is not available inlibm
, 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 insrc/machinst/vcode.rs
that has some arithmetic with a hard dependency on theunwind
feature, which is unsupported onno_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.
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:
- cfallin: isle
- fitzgen: isle
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
rustc-hash is no_std compatible. You just need to disable the std feature.
afonso360 submitted PR review.
afonso360 created PR review comment:
I looked at the source for
round_ties_even
and they userintf
. 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 forrint{f,}
so we can probably use that.
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 ononce_cell
will need to be reworked and/or have other solutions than "just" enabling this feature.
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 inlibm
alexcrichton created PR review comment:
What I might recommend for this file is to have a
timing.rs
and atiming_disabled.rs
as opposed to a single source file with lots of#[cfg]
, that's worked pretty well for us in the past.
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 usingrustc-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 thesync_nostd.rs
file used by Wasmtime since Cranelift looks like it may have a similar use case.
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;
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.
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.
alexcrichton created PR review comment:
I might echo the desire to remove the
no_std
configuration here entirely perhaps. The generate code can itself containextern crate alloc
which can then be used to importVec
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 usingrustc-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 thesync_nostd.rs
file used by Wasmtime since Cranelift looks like it may have a similar use case.
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 :)
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.
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: Dec 23 2024 at 12:05 UTC