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

target-feature=+soft-float breaks things #374

Open
m-ou-se opened this issue Aug 18, 2020 · 3 comments
Open

target-feature=+soft-float breaks things #374

m-ou-se opened this issue Aug 18, 2020 · 3 comments

Comments

@m-ou-se
Copy link
Member

m-ou-se commented Aug 18, 2020

A crate compiled on x86_64 with -Ctarget-feature=+soft-float will use floating point conversions from compiler-builtins. However, some of the floating point conversions in compiler-builtins are implemented using floating point instructions. This breaks when enabling the +soft-float target-feature. Should +soft-float be supported? Or should this have given an error? Right now, it silently gives wrong results:

fn main() {
    println!("1234 = {:e}", a(1234));
}

#[inline(never)]
fn a(x: u64) -> f64 {
    x as f64
}
$ rustc main.rs -Ctarget-feature=+soft-float && ./main 
1234 = 4.66101421257866e-310

https://godbolt.org/z/9zvecP

@nagisa
Copy link
Member

nagisa commented Aug 18, 2020

Should +soft-float be supported? Or should this have given an error?

Could go either way. On one hand the SSE hardware is guaranteed to exist on x86_64, on the other hand it could be disabled (such as is the case during very early boot). Doubt that's an useful enough use-case to support and spend time on though.

@m-ou-se
Copy link
Member Author

m-ou-se commented Aug 18, 2020

I suppose +soft-float would be used when compiling kernel code that shouldn't touch floating point registers. Not sure of all the other use cases though.

In any case, silently producing wrong results is very bad™. Not sure how to turn this into an error though, or even what part would be responsible for producing that error.

Looks like the compiler-builtins and core (etc.) should be compiled with the same target-features as the crate using it. Even with pure soft implementations, the return value still goes through a floating point register. +soft-float expects floats to be returned in a regular register (effectively changing the ABI). The only reason most conversions seem to work on x86_64, is because both compiler-builtins and core use xmm0 of the -soft-float ABI, which is left untouched by the +soft-float crate. So, interacting with the value directly from a +soft-float crate breaks, even when formatting it unmodified (using core) 'works':

fn main() {
	assert_eq!(a(1234), 1234.0);
}

#[inline(never)]
fn a(x: u32) -> f32 {
	x as f32
}
$ rustc main.rs -Ctarget-feature=+soft-float && ./main 
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `1234.0`,
 right: `1234.0`', main.rs:2:5

So this is probably a rust issue instead of a compiler-builtins issue.

@josephlr
Copy link
Contributor

josephlr commented Oct 2, 2020

So this is probably a rust issue instead of a compiler-builtins issue.

Agreed. Interestingly, just copying in the compiler-builtins Rust implementation into the same file seems to have everything work (https://godbolt.org/z/Tqxd87), so it's probably just the fact that libcore is built with -soft-float. I don't think we build a fallback C implementation for __floatundidf, but I might be wrong about that.

It would be interesting to see if this still happens when doing RUSTFLAGS="-C target-feature=+soft-float" cargo build -Zbuild-std. Unfortunately, libcore for the x86_64-unknown-linux-gnu target seems to require -soft-float (otherwise you get LLVM errors).

The solution is to probably just ban +soft-float if you're using a precompiled libcore.

I suppose +soft-float would be used when compiling kernel code that shouldn't touch floating point registers. Not sure of all the other use cases though.

That's the only use I've seen on x86_64, but virtually all Rust OS kernels set this flag, including https://github.com/cloud-hypervisor/rust-hypervisor-firmware. So I agree that +soft-float is a use case we should care about.

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

No branches or pull requests

3 participants