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
wasmi-labs · Jan 11, 2024 · 548edc8 · 548edc8
1 parent ab32d75
commit 548edc8
Show file tree

Hide file tree

Showing 8 changed files with 140 additions and 44 deletions.
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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
+  )
+)
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
@@ -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
+  )
+)
diff --git 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
@@ -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`