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

NaNs are quieted on Emscripten #136197

Open
purplesyringa opened this issue Jan 28, 2025 · 2 comments
Open

NaNs are quieted on Emscripten #136197

purplesyringa opened this issue Jan 28, 2025 · 2 comments
Labels
A-ABI Area: Concerning the application binary interface (ABI) A-floating-point Area: Floating point numbers and arithmetic A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. I-miscompile Issue: Correct Rust code lowers to incorrect machine code O-emscripten Target: 50% off wasm32-unknown-musl. the savings come out of stdio.h, but hey, you get SDL! T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@purplesyringa
Copy link
Contributor

tests/ui/abi/numbers-arithmetic/return-float.rs currently fails under wasm32-unknown-emscripten:

---- [ui] tests/ui/abi/numbers-arithmetic/return-float.rs stdout ----

error: test run failed!
status: exit status: 101
command: cd "/home/purplesyringa/rust/build/x86_64-unknown-linux-gnu/test/ui/abi/numbers-arithmetic/return-float" && RUSTC="/home/purplesyringa/rust/build/x86_64-unknown-linux-gnu/stage1/bin/rustc" RUST_TEST_THREADS="8" "node" "/home/purplesyringa/rust/build/x86_64-unknown-linux-gnu/test/ui/abi/numbers-arithmetic/return-float/a.js"
stdout: none
--- stderr -------------------------------

thread 'main' panicked at /home/purplesyringa/rust/tests/ui/abi/numbers-arithmetic/return-float.rs:25:13:
assertion `left == right` failed
  left: 2144687445
 right: 2140493141
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
------------------------------------------

This reproduces on 66d6064 on Node v23.6.0. WASI is fine, and I think it uses Node during tests too, so probably not a Node bug.

@rustbot label +A-ABI +A-floating-point +I-miscompile +O-emscripten +T-compiler +E-needs-investigation

@purplesyringa purplesyringa added the C-bug Category: This is a bug. label Jan 28, 2025
@rustbot rustbot added needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. A-ABI Area: Concerning the application binary interface (ABI) A-floating-point Area: Floating point numbers and arithmetic E-needs-investigation Call for partcipation: This issues needs some investigation to determine current status I-miscompile Issue: Correct Rust code lowers to incorrect machine code O-emscripten Target: 50% off wasm32-unknown-musl. the savings come out of stdio.h, but hey, you get SDL! T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 28, 2025
@workingjubilee workingjubilee changed the title NaNs are quitened on Emscripten NaNs are quieted on Emscripten Jan 28, 2025
@jieyouxu jieyouxu removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jan 29, 2025
@beetrees
Copy link
Contributor

This appears to be caused by catching unwinds:

struct Droppable;

impl Drop for Droppable {
    fn drop(&mut self) {}
}

const NOT_CANNONICAL_F32: f32 = f32::from_bits(f32::NAN.to_bits() | 1);
const NOT_CANNONICAL_F64: f64 = f64::from_bits(f64::NAN.to_bits() | 1);

// The issue will only occur when compiled with panic=unwind.
fn main() {
    // Commenting out the line below will make the issue disappear.
    let _x = Droppable;
    // This affects both function arguments and function return values.
    // These should both print 7fc00001 but will instead print 7fc00000.
	print_float_f32(NOT_CANNONICAL_F32);
    print_hex_f32(unsafe { std::mem::transmute::<f32, u32>(get_float_f32()) });
    // These should both print 7ff8000000000001 but will instead print 7ff8000000000000.
	print_float_f64(NOT_CANNONICAL_F64);
    print_hex_f64(unsafe { std::mem::transmute::<f64, u64>(get_float_f64()) });
}

#[inline(never)]
fn get_float_f32() -> f32 {
    NOT_CANNONICAL_F32
}

#[inline(never)]
fn get_float_f64() -> f64 {
    NOT_CANNONICAL_F64
}

#[inline(never)]
fn print_float_f32(x: f32) {
	print_hex_f32(unsafe { std::mem::transmute::<f32, u32>(x) })
}

#[inline(never)]
fn print_float_f64(x: f64) {
	print_hex_f64(unsafe { std::mem::transmute::<f64, u64>(x) })
}

#[inline(never)]
fn print_hex_f32(x: u32) {
	println!("{:x}", x);
}

#[inline(never)]
fn print_hex_f64(x: u64) {
	println!("{:x}", x);
}

When Emscripten wants to catch an unwind, it trampolines the function call via one of the invoke_* JS functions. JavaScript does not distinguish between NaNs with different payloads, and it appears that JavaScript engines canonicalize all NaN values to a positive quiet NaN with the payload (excluding quiet bit) that is all 0s (I'm guessing this is done because otherwise they would interfere with NaN-boxing). Emscripten needs to be changed to trampoline calls in a way that doesn't change the function arguments or return values (I think this will require changes both to LLVM and to emcc).

@rustbot label -E-needs-investigation +A-LLVM

@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. and removed E-needs-investigation Call for partcipation: This issues needs some investigation to determine current status labels Jan 29, 2025
@purplesyringa
Copy link
Contributor Author

Good to know, thanks. This must be emscripten-core/emscripten#22743. The consensus seems to be we should "just" switch to new EH ABI, which fixes this bug.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Jan 29, 2025
…jieyouxu

Fix a couple Emscripten tests

This fixes a couple Emscripten tests where the correct fix is more or less obvious. A couple UI tests are still broken with this PR:

- `tests/ui/abi/numbers-arithmetic/return-float.rs` (rust-lang#136197)
- `tests/ui/no_std/no-std-unwind-binary.rs` (haven't debugged yet)
- `tests/ui/test-attrs/test-passed.rs` (haven't debugged this either)

`@rustbot` label +T-compiler +O-emscripten
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 30, 2025
…jieyouxu

Fix a couple Emscripten tests

This fixes a couple Emscripten tests where the correct fix is more or less obvious. A couple UI tests are still broken with this PR:

- `tests/ui/abi/numbers-arithmetic/return-float.rs` (rust-lang#136197)
- `tests/ui/no_std/no-std-unwind-binary.rs` (haven't debugged yet)
- `tests/ui/test-attrs/test-passed.rs` (haven't debugged this either)

``@rustbot`` label +T-compiler +O-emscripten
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 30, 2025
…jieyouxu

Fix a couple Emscripten tests

This fixes a couple Emscripten tests where the correct fix is more or less obvious. A couple UI tests are still broken with this PR:

- `tests/ui/abi/numbers-arithmetic/return-float.rs` (rust-lang#136197)
- `tests/ui/no_std/no-std-unwind-binary.rs` (haven't debugged yet)
- `tests/ui/test-attrs/test-passed.rs` (haven't debugged this either)

```@rustbot``` label +T-compiler +O-emscripten
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 30, 2025
…jieyouxu

Fix a couple Emscripten tests

This fixes a couple Emscripten tests where the correct fix is more or less obvious. A couple UI tests are still broken with this PR:

- `tests/ui/abi/numbers-arithmetic/return-float.rs` (rust-lang#136197)
- `tests/ui/no_std/no-std-unwind-binary.rs` (haven't debugged yet)
- `tests/ui/test-attrs/test-passed.rs` (haven't debugged this either)

````@rustbot```` label +T-compiler +O-emscripten
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 30, 2025
…jieyouxu

Fix a couple Emscripten tests

This fixes a couple Emscripten tests where the correct fix is more or less obvious. A couple UI tests are still broken with this PR:

- `tests/ui/abi/numbers-arithmetic/return-float.rs` (rust-lang#136197)
- `tests/ui/no_std/no-std-unwind-binary.rs` (haven't debugged yet)
- `tests/ui/test-attrs/test-passed.rs` (haven't debugged this either)

`````@rustbot````` label +T-compiler +O-emscripten
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jan 30, 2025
Rollup merge of rust-lang#136199 - purplesyringa:emscripten-tests, r=jieyouxu

Fix a couple Emscripten tests

This fixes a couple Emscripten tests where the correct fix is more or less obvious. A couple UI tests are still broken with this PR:

- `tests/ui/abi/numbers-arithmetic/return-float.rs` (rust-lang#136197)
- `tests/ui/no_std/no-std-unwind-binary.rs` (haven't debugged yet)
- `tests/ui/test-attrs/test-passed.rs` (haven't debugged this either)

`````@rustbot````` label +T-compiler +O-emscripten
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ABI Area: Concerning the application binary interface (ABI) A-floating-point Area: Floating point numbers and arithmetic A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. I-miscompile Issue: Correct Rust code lowers to incorrect machine code O-emscripten Target: 50% off wasm32-unknown-musl. the savings come out of stdio.h, but hey, you get SDL! T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants