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

Value of f32.is_sign_positive() differs from native code #3139

Closed
dtolnay opened this issue Oct 25, 2023 · 7 comments
Closed

Value of f32.is_sign_positive() differs from native code #3139

dtolnay opened this issue Oct 25, 2023 · 7 comments

Comments

@dtolnay
Copy link
Member

dtolnay commented Oct 25, 2023

In the most recently nightly, I started getting a failing test with cargo miri test in https://github.com/dtolnay/basic-toml. The crate does not contain unsafe code so I wonder whether this could be a Miri bug.

The failing test is here, specifically this assertion. It deserializes a f32 from TOML +nan and asserts that .is_sign_positive() returns true. Under cargo test, the f32's bit pattern is 0x7FC0_0000 which is +NaN. Under cargo miri test the bit pattern is 0xFFC0_0000 which is -NaN and the test fails.

https://github.com/dtolnay/basic-toml/actions/runs/6634770330/job/18024693503

I made some attempts at a minimal repro along the lines of the following, but was not able to reproduce a -NaN this way, yet.

use serde::Deserialize as _;

fn main() {
    println!("{}", f64::NAN.is_sign_positive());
    println!("{}", f32::NAN.is_sign_positive());
    println!("{}", (f64::NAN as f32).is_sign_positive());
    println!("{}", (unsafe { std::ptr::read_volatile(&f64::NAN) } as f32).is_sign_positive());
    println!("{}", f32::deserialize(serde::de::value::F64Deserializer::<serde::de::value::Error>::new(f64::NAN)).unwrap().is_sign_positive());
}

Next I tried bisecting Miri commits by doing ./miri toolchain && ./miri install in the miri repo followed by cargo +miri miri test --test float against that commit of the basic-toml repo linked above. The bisect landed on #3136 which has no relevant code change in miri. Based on the rust-version change in that PR, I checked git diff 9e3f784eb2c..62fae2305e5 (rust-lang/rust@9e3f784...62fae23#files_bucket) which also had nothing that looked relevant.

I'll keep working on a minimal repro but filing this now in case you have a ready way to debug this kind of thing, or spot something obvious in #3136 or the rustc diff that I overlooked.

rustc 1.75.0-nightly (df871fbf0 2023-10-24)
binary: rustc
commit-hash: df871fbf053de3a855398964cd05fadbe91cf4fd
commit-date: 2023-10-24
host: x86_64-unknown-linux-gnu
release: 1.75.0-nightly
LLVM version: 17.0.3
@RalfJung
Copy link
Member

This is most likely caused by rust-lang/rust#116551. Miri is now implementing the NaN semantics proposed in rust-lang/rfcs#3514, which aligns Miri with codegen. Rust does not guarantee anything about the sign of NaNs, and that can already be observed in real code, so Miri always returning the same sign was a bug that we fixed.

@Muon
Copy link

Muon commented Oct 25, 2023

Although we make no promises about the sign of NaNs produced by arithmetic expressions, deserializing a +NaN should produce a +NaN, and the sign should be preserved under copies.

@dtolnay
Copy link
Member Author

dtolnay commented Oct 25, 2023

Thanks for the links! I had not seen the RFC or PR.

@Muon I think there ends up being a f64 as f32 involved somewhere inside serde, because the toml crate always deserializes +nan as f64::NAN, and it's later cast to f32 by f32's Deserialize impl. This is defined as producing non-deterministic sign in the RFC.

I will need to update serde to go through something like this:

fn f64_as_f32(f: f64) -> f32 {
    if /* ??? */ && f.is_nan() {
        if f.is_sign_positive() {
            f32::NAN
        } else {
            -f32::NAN
        }
    } else {
        f as f32
    }
}

where ??? is hopefully something more sophisticated than cfg!(miri), but I don't know. Maybe cfg!(any(miri, not(any(target_arch = "x86_64", ...)))) if there are particular platforms where we can be sure f64 as f32 preserves sign.

@Muon
Copy link

Muon commented Oct 25, 2023

@dtolnay IEEE 754 doesn't guarantee that format conversions preserve NaN sign, and neither does LLVM. Also, f32::NAN might be negative. If you need a positive NaN, I recommend using copysign() instead.

@dtolnay
Copy link
Member Author

dtolnay commented Oct 25, 2023

Good call. Something like this then:

fn f64_as_f32(f: f64) -> f32 {
    (f as f32).copysign(if f.is_sign_positive() { 1.0 } else { -1.0 })
}

@dtolnay dtolnay closed this as completed Oct 25, 2023
@RalfJung
Copy link
Member

Yes that kind of cast would reliably preserve the sign. Other parts of the NaN (signaling, payload) are still non-deterministic though (subject to the guarantees in the RFC).

@dtolnay
Copy link
Member Author

dtolnay commented Oct 26, 2023

3 new clippy lints to come out of this:

Thanks @epage. And thanks @RalfJung for implementing rust-lang/rust#116551, the educational value has been large.

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