i686 floating point behavior does not agree with unit tests in debug mode

[ecstatic-morse]

Compiling and running the following code with -C opt-level=0 for 32-bit Linux on a recent nightly (2020-06-07) results in a failing assertion.

fn main() {
    let masked_nan2 = f32::NAN.to_bits() ^ 0x0055_5555;
    assert_eq!(f32::from_bits(masked_nan2).to_bits(), masked_nan2);
}

rustc +nightly --target=i686-unknown-linux-gnu test.rs && ./test

Output:

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `2144687445`,
 right: `2140493141`', test.rs:3:5

However, this code is taken directly from a unit test for libstd added in #46012, so presumably it should succeed.

rust/src/libstd/f32.rs

Lines 1508 to 1514 in 5949391

	let masked_nan1 = f32::NAN.to_bits() ^ 0x002A_AAAA;
	let masked_nan2 = f32::NAN.to_bits() ^ 0x0055_5555;
	assert!(f32::from_bits(masked_nan1).is_nan());
	assert!(f32::from_bits(masked_nan2).is_nan());
	assert_eq!(f32::from_bits(masked_nan1).to_bits(), masked_nan1);
	assert_eq!(f32::from_bits(masked_nan2).to_bits(), masked_nan2);

[RalfJung]

This is probably caused by x87 instructions... I think we have an issue about that somewhere already, not sure where though.
Cc @hanna-kruppe

[ecstatic-morse]

Indeed. After the mask, the value in masked_nan2 is the following. On x86, this value represents a signalling NaN.

0 11111111 00101010101010101010101
    exp           significand

The call to f32::from_bits puts the same value onto the top of the x87 FPU stack with an FLD instruction before returning. After this is called, the #IA flag (invalid arithmetic operand exception) is set in the x87 status register, and the value on top of the stack is an 80-bit quiet NaN (0x7fffc000010000000000). According to the reference manual, this occurs only if the operand to FLD is not also a double extended-precision float.

When the return value is popped off the FPU stack in main and stored in memory with an FSTP instruction, the signalling/quiet bit (the most-significant bit of the significand) is set. On x86, this represents a quiet NaN.

0 11111111 10101010101010101010101
    exp           significand

[hanna-kruppe]

Independently of x87, the intent behind the test is on shaky ground. LLVM optimizations do not generally preserve NaN bit strings, see e.g. #55131.

[ecstatic-morse]

Seems like we should update the docs for {f32,f64}::from_bits then? Currently, they claim

This is currently identical to transmute::<u32, f32>(v) on all platforms

and

this implementation favors preserving the exact bits

[hanna-kruppe]

The first part isn't wrong, as far as I can tell. It literally is implemented as transmute, it's just that handling any float values in general does not always "preserves the exact bits".

[ecstatic-morse]

fn main() {
    let snan: u32 = 0xff << 23 | 1;

    let round_trip = unsafe {
        let float: f32 = std::mem::transmute(snan);
        std::mem::transmute(float)
    };

    assert_eq!(snan, round_trip);
    // assert_eq!(f32::from_bits(snan).to_bits(), snan);
}

@hanna-kruppe This succeeds in debug mode unless the last line is uncommented, so the docs are misleading at the very least.

[hanna-kruppe]

I believe that difference is due to the transmute getting const-eval'd, the LLVM IR just stores the same u32 bit pattern to the stack several times. While this does end up making a difference today:

1. Several changes to rustc could make the difference disappear again (enabling MIR inlining, not running const-prop in debug builds, forbidding const-prop from evaluating tranmsutes, etc.)
2. Conversely, all sorts of harmless changes to the version that uses transmute directly can break it. For example, anything that stops constant propagation. Or introducing your own function boundaries.

It's not that the functions in std do something different from transmute, it's that -- contrary to all usual intuition -- function boundaries and IR optimizations matter for this behavior. This whole situation is an incredibly mess and headache, and picking on this one sentence from the docs is not going to help clarify it. If we want to give users proper guidelines about this, we need to go back to the drawing board and figure out what the rules actually are and should be.

[ecstatic-morse]

I believe that difference is due to the transmute getting const-eval'd

If you mean MIR const propagated, it does not. Not sure about LLVM const propagation, but adding black_box and wrapping the round trip in a function call did not change behavior. I believe the fact that std::mem::transmute is direct re-export of the intrinsic accounts for the difference.

I'm not saying that this area isn't a mess, but the docs from_bits don't acknowledge this at all, and are trivially wrong in the case I've shown above. I'm going to update them to reflect this.

[ecstatic-morse]

@Mark-Simulacrum Besides the unresolved problems around NaN payloads, this raises another issue: Should we be running unit tests for the standard library in debug mode on CI?

As far as I know, we don't distribute a non-optimized libstd, but maybe we should be building one and running its tests as part of the nopt CI jobs?

[Mark-Simulacrum]

I am fairly certain (hopeful, I guess) that most of our builders do have debug assertions enabled... however, it sounds like what you actually want here is a debug libstd. I don't know how feasible that is from a performance perspective (I imagine running those tests is considerably slower?).

We do actually have a i686-gnu-nopt builder, and I think some other nopt builders, (https://github.com/rust-lang/rust/blob/master/src/ci/docker/i686-gnu-nopt/Dockerfile#L23). That's currently configured to not enable optimizations for tests, but that only affects compiletest-run tests. For tests like this which are heavily dependent on such, we can move them to run-pass tests to get benefits of debug testing. Note that debug assertions are currently disabled on that builder due to CI time concerns.

[Mark-Simulacrum]

Basically, as with all such questions, it comes down to someone sitting down and timing how long a debug build of std/std tests + running those tests would take, then timing some current task (e.g. release std tests) and scaling that to CI time. If that's sufficiently small in CI timescales, we can add it, if not we'll have to not do so.