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 environments with the FPU disabled #25

Merged
merged 3 commits into from
Jun 13, 2024

Conversation

lylythechosenone
Copy link
Contributor

Currently, the crate causes an exception on systems where the FPU is off/nonexistent, because of this code segment that saves floating-point registers. This PR removes that floating-point saving on -softfloat targets.

src/lib.rs Outdated
@@ -2,6 +2,7 @@
#![feature(c_unwind)]
#![feature(naked_functions)]
#![feature(non_exhaustive_omitted_patterns_lint)]
#![feature(cfg_target_abi)]
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a way to achieve this without additional nightly features? I'd like to avoid adding extra ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it might be doable with target_feature, but I'm not sure. Will do some testing and find out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Never mind, the counterparts in target_feature are still unstable. The other option would be a cargo feature, but that feels weird. Your choice I guess.

Copy link
Contributor

@taiki-e taiki-e Feb 28, 2024

Choose a reason for hiding this comment

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

cfg_target_abi has been stabilized in the recent nightly (rust-lang/rust#119590), so we can now remove this #![feature]

Register(64..=95) => &mut self.fp[(reg.0 - 64) as usize],
_ => unimplemented!(),
}
}
}

#[naked]
#[cfg(target_abi = "softfloat")]
pub extern "C-unwind" fn save_context(f: extern "C" fn(&mut Context, *mut ()), ptr: *mut ()) {
// No need to save caller-saved registers here.
Copy link
Owner

Choose a reason for hiding this comment

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

Could you extract the asm using macro to avoid duplication? See riscv for example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sure, will do.

Copy link
Owner

Choose a reason for hiding this comment

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

Please take another look of riscv code to avoid code duplication

@lylythechosenone
Copy link
Contributor Author

As I said in my comments, I fixed these as much as I could. I need your input on the first one.

@lylythechosenone
Copy link
Contributor Author

Not sure how I closed this.

Anyways, it's finally done. Sorry for the long wait. Hopefully this is what you envisioned.

unsafe {
#[cfg(target_feature = "fp-armv8")]
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK there is no such a target feature in aarch64 (at least on the Rust side):

$ rustc +nightly --print cfg --target aarch64-unknown-linux-gnu | grep target_feature
target_feature="neon"
$ rustc +nightly --print cfg --target aarch64-unknown-none | grep target_feature
target_feature="neon"
$ rustc +nightly --print cfg --target aarch64-unknown-none-softfloat | grep target_feature

https://github.com/rust-lang/rust/blob/b5b13568fb5da4ac988bde370008d6134d3dfe6c/compiler/rustc_target/src/target_features.rs#L87

arm has it though: https://github.com/rust-lang/rust/blob/b5b13568fb5da4ac988bde370008d6134d3dfe6c/compiler/rustc_target/src/target_features.rs#L62

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, weird—the error when I tried to use unwinding mentioned fp-armv8 explicitly. I'll fix that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be more clear: when I tried to use the current unwinding master branch on aarch64-unknown-none-softfloat, I got a compile error saying that the registers d[n] require target feature fp-armv8. This is likely a diagnostic issue, and I'll see if I can reproduce it to send that over to rustc.

`fp-armv8` is an arm feature, not an aarch64 feature.
@nbdd0121 nbdd0121 merged commit ac07eba into nbdd0121:trunk Jun 13, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants