From 548edc8ba13c904389ecee00b8841c516f7d56c3 Mon Sep 17 00:00:00 2001 From: Robin Freyler Date: Thu, 11 Jan 2024 17:24:56 +0100 Subject: [PATCH] Fix `{f32,f64}.cmp` codegen bug (#881) * add regression tests found via fuzzing * adjust existing tests to correct codegen * fix {f32,f64}.cmp codegen bug --- .../engine/translator/tests/op/cmp/f32_ge.rs | 11 +++- .../engine/translator/tests/op/cmp/f32_le.rs | 11 +++- .../engine/translator/tests/op/cmp/f64_ge.rs | 11 +++- .../engine/translator/tests/op/cmp/f64_le.rs | 11 +++- .../tests/regression/fuzz_12_f32.wat | 20 ++++++ .../tests/regression/fuzz_12_f64.wat | 20 ++++++ .../engine/translator/tests/regression/mod.rs | 64 +++++++++++++++++++ crates/wasmi/src/engine/translator/visit.rs | 36 ++--------- 8 files changed, 140 insertions(+), 44 deletions(-) create mode 100644 crates/wasmi/src/engine/translator/tests/regression/fuzz_12_f32.wat create mode 100644 crates/wasmi/src/engine/translator/tests/regression/fuzz_12_f64.wat diff --git a/crates/wasmi/src/engine/translator/tests/op/cmp/f32_ge.rs b/crates/wasmi/src/engine/translator/tests/op/cmp/f32_ge.rs index 17e812eb55..48e0476f59 100644 --- a/crates/wasmi/src/engine/translator/tests/op/cmp/f32_ge.rs +++ b/crates/wasmi/src/engine/translator/tests/op/cmp/f32_ge.rs @@ -5,9 +5,14 @@ const WASM_OP: WasmOp = WasmOp::cmp(WasmType::F32, "ge"); #[test] #[cfg_attr(miri, ignore)] fn same_reg() { - let expected = [Instruction::ReturnImm32 { - value: AnyConst32::from(true), - }]; + let expected = [ + Instruction::f32_ge( + Register::from_i16(1), + Register::from_i16(0), + Register::from_i16(0), + ), + Instruction::return_reg(1), + ]; test_binary_same_reg(WASM_OP, expected) } diff --git a/crates/wasmi/src/engine/translator/tests/op/cmp/f32_le.rs b/crates/wasmi/src/engine/translator/tests/op/cmp/f32_le.rs index 146244cace..93b931c21d 100644 --- a/crates/wasmi/src/engine/translator/tests/op/cmp/f32_le.rs +++ b/crates/wasmi/src/engine/translator/tests/op/cmp/f32_le.rs @@ -5,9 +5,14 @@ const WASM_OP: WasmOp = WasmOp::cmp(WasmType::F32, "le"); #[test] #[cfg_attr(miri, ignore)] fn same_reg() { - let expected = [Instruction::ReturnImm32 { - value: AnyConst32::from(true), - }]; + let expected = [ + Instruction::f32_le( + Register::from_i16(1), + Register::from_i16(0), + Register::from_i16(0), + ), + Instruction::return_reg(1), + ]; test_binary_same_reg(WASM_OP, expected) } diff --git a/crates/wasmi/src/engine/translator/tests/op/cmp/f64_ge.rs b/crates/wasmi/src/engine/translator/tests/op/cmp/f64_ge.rs index a0d54ab02c..c656b6da69 100644 --- a/crates/wasmi/src/engine/translator/tests/op/cmp/f64_ge.rs +++ b/crates/wasmi/src/engine/translator/tests/op/cmp/f64_ge.rs @@ -5,9 +5,14 @@ const WASM_OP: WasmOp = WasmOp::cmp(WasmType::F64, "ge"); #[test] #[cfg_attr(miri, ignore)] fn same_reg() { - let expected = [Instruction::ReturnImm32 { - value: AnyConst32::from(true), - }]; + let expected = [ + Instruction::f64_ge( + Register::from_i16(1), + Register::from_i16(0), + Register::from_i16(0), + ), + Instruction::return_reg(1), + ]; test_binary_same_reg(WASM_OP, expected) } diff --git a/crates/wasmi/src/engine/translator/tests/op/cmp/f64_le.rs b/crates/wasmi/src/engine/translator/tests/op/cmp/f64_le.rs index a61e0acba9..399bbfe00f 100644 --- a/crates/wasmi/src/engine/translator/tests/op/cmp/f64_le.rs +++ b/crates/wasmi/src/engine/translator/tests/op/cmp/f64_le.rs @@ -5,9 +5,14 @@ const WASM_OP: WasmOp = WasmOp::cmp(WasmType::F64, "le"); #[test] #[cfg_attr(miri, ignore)] fn same_reg() { - let expected = [Instruction::ReturnImm32 { - value: AnyConst32::from(true), - }]; + let expected = [ + Instruction::f64_le( + Register::from_i16(1), + Register::from_i16(0), + Register::from_i16(0), + ), + Instruction::return_reg(1), + ]; test_binary_same_reg(WASM_OP, expected) } diff --git a/crates/wasmi/src/engine/translator/tests/regression/fuzz_12_f32.wat b/crates/wasmi/src/engine/translator/tests/regression/fuzz_12_f32.wat new file mode 100644 index 0000000000..c6fe9dd69f --- /dev/null +++ b/crates/wasmi/src/engine/translator/tests/regression/fuzz_12_f32.wat @@ -0,0 +1,20 @@ +(module + (func (param f32) + f32.const -nan:0x7fffff (;=NaN;) + local.tee 0 + local.get 0 + local.get 0 + f32.le + br_if 0 + unreachable + ) + (func (param f32) + f32.const -nan:0x7fffff (;=NaN;) + local.tee 0 + local.get 0 + local.get 0 + f32.ge + br_if 0 + unreachable + ) +) \ No newline at end of file diff --git a/crates/wasmi/src/engine/translator/tests/regression/fuzz_12_f64.wat b/crates/wasmi/src/engine/translator/tests/regression/fuzz_12_f64.wat new file mode 100644 index 0000000000..b4a7bc0bc3 --- /dev/null +++ b/crates/wasmi/src/engine/translator/tests/regression/fuzz_12_f64.wat @@ -0,0 +1,20 @@ +(module + (func (param f64) + f64.const -nan:0xfffffffffffff (;=NaN;) + local.tee 0 + local.get 0 + local.get 0 + f64.le + br_if 0 + unreachable + ) + (func (param f64) + f64.const -nan:0xfffffffffffff (;=NaN;) + local.tee 0 + local.get 0 + local.get 0 + f64.ge + br_if 0 + unreachable + ) +) \ No newline at end of file diff --git a/crates/wasmi/src/engine/translator/tests/regression/mod.rs b/crates/wasmi/src/engine/translator/tests/regression/mod.rs index bb973b4dde..f32d1fca80 100644 --- a/crates/wasmi/src/engine/translator/tests/regression/mod.rs +++ b/crates/wasmi/src/engine/translator/tests/regression/mod.rs @@ -231,3 +231,67 @@ fn fuzz_regression_11() { ]) .run() } + +#[test] +#[cfg_attr(miri, ignore)] +fn fuzz_regression_12_f32() { + let wat = include_str!("fuzz_12_f32.wat"); + let wasm = wat2wasm(wat); + TranslationTest::new(wasm) + .expect_func(ExpectedFunc::new([ + Instruction::copy_imm32(Register::from_i16(0), u32::MAX), + Instruction::f32_le( + Register::from_i16(1), + Register::from_i16(0), + Register::from_i16(0), + ), + Instruction::return_nez(1), + Instruction::Trap(TrapCode::UnreachableCodeReached), + ])) + .expect_func(ExpectedFunc::new([ + Instruction::copy_imm32(Register::from_i16(0), u32::MAX), + Instruction::f32_ge( + Register::from_i16(1), + Register::from_i16(0), + Register::from_i16(0), + ), + Instruction::return_nez(1), + Instruction::Trap(TrapCode::UnreachableCodeReached), + ])) + .run() +} + +#[test] +#[cfg_attr(miri, ignore)] +fn fuzz_regression_12_f64() { + let wat = include_str!("fuzz_12_f64.wat"); + let wasm = wat2wasm(wat); + TranslationTest::new(wasm) + .expect_func( + ExpectedFunc::new([ + Instruction::copy(0, -1), + Instruction::f64_le( + Register::from_i16(1), + Register::from_i16(0), + Register::from_i16(0), + ), + Instruction::return_nez(1), + Instruction::Trap(TrapCode::UnreachableCodeReached), + ]) + .consts([u64::MAX]), + ) + .expect_func( + ExpectedFunc::new([ + Instruction::copy(0, -1), + Instruction::f64_ge( + Register::from_i16(1), + Register::from_i16(0), + Register::from_i16(0), + ), + Instruction::return_nez(1), + Instruction::Trap(TrapCode::UnreachableCodeReached), + ]) + .consts([u64::MAX]), + ) + .run() +} diff --git a/crates/wasmi/src/engine/translator/visit.rs b/crates/wasmi/src/engine/translator/visit.rs index 57ca668228..57fb77809c 100644 --- a/crates/wasmi/src/engine/translator/visit.rs +++ b/crates/wasmi/src/engine/translator/visit.rs @@ -1950,14 +1950,7 @@ impl<'a> VisitOperator<'a> for FuncTranslator { self.translate_fbinary( Instruction::f32_le, TypedValue::f32_le, - |this, lhs: Register, rhs: Register| { - if lhs == rhs { - // Optimization: `x < x` is always `true` - this.alloc.stack.push_const(true); - return Ok(true); - } - Ok(false) - }, + Self::no_custom_opt, |this, _lhs: Register, rhs: f32| { if rhs.is_nan() { // Optimization: `x <= NAN` is always `false` @@ -1981,14 +1974,7 @@ impl<'a> VisitOperator<'a> for FuncTranslator { self.translate_fbinary( Instruction::f32_ge, TypedValue::f32_ge, - |this, lhs: Register, rhs: Register| { - if lhs == rhs { - // Optimization: `x >= x` is always `true` - this.alloc.stack.push_const(true); - return Ok(true); - } - Ok(false) - }, + Self::no_custom_opt, |this, _lhs: Register, rhs: f32| { if rhs.is_nan() { // Optimization: `x >= NAN` is always `false` @@ -2126,14 +2112,7 @@ impl<'a> VisitOperator<'a> for FuncTranslator { self.translate_fbinary( Instruction::f64_le, TypedValue::f64_le, - |this, lhs: Register, rhs: Register| { - if lhs == rhs { - // Optimization: `x < x` is always `true` - this.alloc.stack.push_const(true); - return Ok(true); - } - Ok(false) - }, + Self::no_custom_opt, |this, _lhs: Register, rhs: f64| { if rhs.is_nan() { // Optimization: `x <= NAN` is always `false` @@ -2157,14 +2136,7 @@ impl<'a> VisitOperator<'a> for FuncTranslator { self.translate_fbinary( Instruction::f64_ge, TypedValue::f64_ge, - |this, lhs: Register, rhs: Register| { - if lhs == rhs { - // Optimization: `x >= x` is always `true` - this.alloc.stack.push_const(true); - return Ok(true); - } - Ok(false) - }, + Self::no_custom_opt, |this, _lhs: Register, rhs: f64| { if rhs.is_nan() { // Optimization: `x >= NAN` is always `false`