Repository: bytecodealliance/wasmtime (rust)

Issue:
Panic compiling modules with NaN constants
On `main`:

```
$ cargo run compile foo.wat
    Finished dev [unoptimized + debuginfo] target(s) in 0.13s
     Running `target/debug/wasmtime compile foo.wat`
thread '<unnamed>' panicked at 'explicit panic', cranelift/codegen/src/scoped_hash_map.rs:125:30
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
```

with this input:

```wasm
(module
  (func
    i32.const 0
    if  ;; label = @0
      unreachable
    end
    f32.const nan
    drop
  )
)
```

I believe this was introduced in https://github.com/bytecodealliance/wasmtime/pull/4849 (cc @afonso360  and @jameysharp )

Pull request:
Make DataValue, not Ieee32/64, respect IEEE754
This fixes #4857 by partially reverting #4849.

It turns out that `Ieee32` and `Ieee64` need bitwise equality semantics so they can be used as hash-table keys.

Moving the IEEE754 semantics up a layer to `DataValue` makes sense in conjunction with #4855, where we introduced a `DataValue::bitwise_eq` alternative implementation of equality for those cases where users of `DataValue` still want the bitwise equality semantics.

cc: @afonso360

Patch:
diff --git a/cranelift/codegen/src/data_value.rs b/cranelift/codegen/src/data_value.rs
index affc5b711..86f1882c3 100644
--- a/cranelift/codegen/src/data_value.rs
+++ b/cranelift/codegen/src/data_value.rs
@@ -10,7 +10,7 @@ use core::fmt::{self, Display, Formatter};
 ///
 /// [Value]: crate::ir::Value
 #[allow(missing_docs)]
-#[derive(Clone, Debug, PartialEq, PartialOrd)]
+#[derive(Clone, Debug, PartialOrd)]
 pub enum DataValue {
     B(bool),
     I8(i8),
@@ -29,6 +29,44 @@ pub enum DataValue {
     V64([u8; 8]),
 }
 
+impl PartialEq for DataValue {
+    fn eq(&self, other: &Self) -> bool {
+        use DataValue::*;
+        match (self, other) {
+            (B(l), B(r)) => l == r,
+            (B(_), _) => false,
+            (I8(l), I8(r)) => l == r,
+            (I8(_), _) => false,
+            (I16(l), I16(r)) => l == r,
+            (I16(_), _) => false,
+            (I32(l), I32(r)) => l == r,
+            (I32(_), _) => false,
+            (I64(l), I64(r)) => l == r,
+            (I64(_), _) => false,
+            (I128(l), I128(r)) => l == r,
+            (I128(_), _) => false,
+            (U8(l), U8(r)) => l == r,
+            (U8(_), _) => false,
+            (U16(l), U16(r)) => l == r,
+            (U16(_), _) => false,
+            (U32(l), U32(r)) => l == r,
+            (U32(_), _) => false,
+            (U64(l), U64(r)) => l == r,
+            (U64(_), _) => false,
+            (U128(l), U128(r)) => l == r,
+            (U128(_), _) => false,
+            (F32(l), F32(r)) => l.as_f32() == r.as_f32(),
+            (F32(_), _) => false,
+            (F64(l), F64(r)) => l.as_f64() == r.as_f64(),
+            (F64(_), _) => false,
+            (V128(l), V128(r)) => l == r,
+            (V128(_), _) => false,
+            (V64(l), V64(r)) => l == r,
+            (V64(_), _) => false,
+        }
+    }
+}
+
 impl DataValue {
     /// Try to cast an immediate integer (a wrapped `i64` on most Cranelift instructions) to the
     /// given Cranelift [Type].
diff --git a/cranelift/codegen/src/ir/immediates.rs b/cranelift/codegen/src/ir/immediates.rs
index 445e3f9ae..a98a9596f 100644
--- a/cranelift/codegen/src/ir/immediates.rs
+++ b/cranelift/codegen/src/ir/immediates.rs
@@ -475,8 +475,11 @@ impl FromStr for Offset32 {
 /// We specifically avoid using a f32 here since some architectures may silently alter floats.
 /// See: https://github.com/bytecodealliance/wasmtime/pull/2251#discussion_r498508646
 ///
+/// The [PartialEq] and [Hash] implementations are over the underlying bit pattern, but
+/// [PartialOrd] respects IEEE754 semantics.
+///
 /// All bit patterns are allowed.
-#[derive(Copy, Clone, Debug, Eq, Hash)]
+#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)]
 #[cfg_attr(feature = "enable-serde", derive(Serialize, Deserialize))]
 #[repr(C)]
 pub struct Ieee32(u32);
@@ -487,8 +490,11 @@ pub struct Ieee32(u32);
 /// We specifically avoid using a f64 here since some architectures may silently alter floats.
 /// See: https://github.com/bytecodealliance/wasmtime/pull/2251#discussion_r498508646
 ///
+/// The [PartialEq] and [Hash] implementations are over the underlying bit pattern, but
+/// [PartialOrd] respects IEEE754 semantics.
+///
 /// All bit patterns are allowed.
-#[derive(Copy, Clone, Debug, Eq, Hash)]
+#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)]
 #[cfg_attr(feature = "enable-serde", derive(Serialize, Deserialize))]
 #[repr(C)]
 pub struct Ieee64(u64);
@@ -845,12 +851,6 @@ impl PartialOrd for Ieee32 {
     }
 }
 
-impl PartialEq<Ieee32> for Ieee32 {
-    fn eq(&self, other: &Ieee32) -> bool {
-        self.as_f32().eq(&other.as_f32())
-    }
-}
-
 impl Display for Ieee32 {
     fn fmt(&self, f: &mut Formatter) -> fmt::Result {
         let bits: u32 = self.0;
@@ -1037,12 +1037,6 @@ impl PartialOrd for Ieee64 {
     }
 }
 
-impl PartialEq<Ieee64> for Ieee64 {
-    fn eq(&self, other: &Ieee64) -> bool {
-        self.as_f64().eq(&other.as_f64())
-    }
-}
-
 impl Display for Ieee64 {
     fn fmt(&self, f: &mut Formatter) -> fmt::Result {
         let bits: u64 = self.0;
diff --git a/cranelift/codegen/src/ir/instructions.rs b/cranelift/codegen/src/ir/instructions.rs
index 02a4d48e8..2f8452504 100644
--- a/cranelift/codegen/src/ir/instructions.rs
+++ b/cranelift/codegen/src/ir/instructions.rs
@@ -17,7 +17,6 @@ use core::str::FromStr;
 use serde::{Deserialize, Serialize};
 
 use crate::bitset::BitSet;
-use crate::data_value::DataValue;
 use crate::entity;
 use crate::ir::{
     self,
@@ -289,39 +288,6 @@ impl InstructionData {
         }
     }
 
-    /// Return the value of an immediate if the instruction has one or `None` otherwise. Only
-    /// immediate values are considered, not global values, constant handles, condition codes, etc.
-    pub fn imm_value(&self) -> Option<DataValue> {
-        match self {
-            &InstructionData::UnaryBool { imm, .. } => Some(DataValue::from(imm)),
-            // 8-bit.
-            &InstructionData::BinaryImm8 { imm, .. }
-            | &InstructionData::TernaryImm8 { imm, .. } => Some(DataValue::from(imm as i8)), // Note the switch from unsigned to signed.
-            // 32-bit
-            &InstructionData::UnaryIeee32 { imm, .. } => Some(DataValue::from(imm)),
-            &InstructionData::HeapAddr { imm, .. } => {
-                let imm: u32 = imm.into();
-                Some(DataValue::from(imm as i32)) // Note the switch from unsigned to signed.
-            }
-            &InstructionData::Load { offset, .. }
-            | &InstructionData::Store { offset, .. }
-            | &InstructionData::StackLoad { offset, .. }
-            | &InstructionData::StackStore { offset, .. }
-            | &InstructionData::TableAddr { offset, .. } => Some(DataValue::from(offset)),
-            // 64-bit.
-            &InstructionData::UnaryImm { imm, .. }
-            | &InstructionData::BinaryImm64 { imm, .. }
-            | &InstructionData::IntCompareImm { imm, .. } => Some(DataValue::from(imm.bits())),
-            &InstructionData::UnaryIeee64 { imm, .. } => Some(DataValue::from(imm)),
-            // 128-bit; though these immediates are present logically in the IR they are not
-            // included in the `InstructionData` for memory-size reasons. This case, returning
-            // `None`, is left here to alert users of this method that they should retrieve the
-            // value using the `DataFlowGraph`.
-            &InstructionData::Shuffle { imm: _, .. } => None,
-            _ => None,
-        }
-    }
-
     /// If this is a trapping instruction, get its trap code. Otherwise, return
     /// `None`.
     pub fn trap_code(&self) -> Option<TrapCode> {
diff --git a/cranelift/codegen/src/machinst/isle.rs b/cranelift/codegen/src/machinst/isle.rs
index fdc0bbf1d..cf73d0b43 100644
--- a/cranelift/codegen/src/machinst/isle.rs
+++ b/cranelift/codegen/src/machinst/isle.rs
@@ -7,7 +7,6 @@ use std::cell::Cell;
 use target_lexicon::Triple;
 
 pub use super::MachLabel;
-pub use crate::data_value::DataValue;
 pub use crate::ir::{
     dynamic_to_fixed, ArgumentExtension, Constant, DynamicStackSlot, ExternalName, FuncRef,
     GlobalValue, Immediate, SigRef, StackSlot,
diff --git a/cranelift/interpreter/src/step.rs b/cranelift/interpreter/src/step.rs
index 16347734f..064224611 100644
--- a/cranelift/interpreter/src/step.rs
+++ b/cranelift/interpreter/src/step.rs
@@ -114,7 +114,28 @@ where
                     length => panic!("unexpected Shuffle mask length {}", length),
                 }
             }
-            _ => inst.imm_value().unwrap(),
+            InstructionData::UnaryBool { imm, .. } => DataValue::from(imm),
+            // 8-bit.
+            InstructionData::BinaryImm8 { imm, .. } | InstructionData::TernaryImm8 { imm, .. } => {
+                DataValue::from(imm as i8) // Note the switch from unsigned to signed.
+            }
+            // 32-bit
+            InstructionData::UnaryIeee32 { imm, .. } => DataValue::from(imm),
+            InstructionData::HeapAddr { imm, .. } => {
+                let imm: u32 = imm.into();
+                DataValue::from(imm as i32) // Note the switch from unsigned to signed.
+            }
+            InstructionData::Load { offset, .. }
+            | InstructionData::Store { offset, .. }
+            | InstructionData::StackLoad { offset, .. }
+            | InstructionData::StackStore { offset, .. }
+            | InstructionData::TableAddr { offset, .. } => DataValue::from(offset),
+            // 64-bit.
+            InstructionData::UnaryImm { imm, .. }
+            | InstructionData::BinaryImm64 { imm, .. }
+            | InstructionData::IntCompareImm { imm, .. } => DataValue::from(imm.bits()),
+            InstructionData::UnaryIeee64 { imm, .. } => DataValue::from(imm),
+            _ => unreachable!(),
         })
     };
 
diff --git a/cranelift/interpreter/src/value.rs b/cranelift/interpreter/src/value.rs
index 0e0f738e4..2dccca472 100644
--- a/cranelift/interpreter/src/value.rs
+++ b/cranelift/interpreter/src/value.rs
@@ -208,14 +208,6 @@ macro_rules! binary_match {
         }
     };
 }
-macro_rules! comparison_match {
-    ( $op:path[$arg1:expr, $arg2:expr]; [ $( $data_value_ty:ident ),* ] ) => {
-        match ($arg1, $arg2) {
-            $( (DataValue::$data_value_ty(a), DataValue::$data_value_ty(b)) => { Ok($op(a, b)) } )*
-            _ => unimplemented!("comparison: {:?}, {:?}", $arg1, $arg2)
-        }
-    };
-}
 
 impl Value for DataValue {
     fn ty(&self) -> Type {
@@ -475,11 +467,11 @@ impl Value for DataValue {
     }
 
     fn eq(&self, other: &Self) -> ValueResult<bool> {
-        comparison_match!(PartialEq::eq[&self, &other]; [I8, I16, I32, I64, I128, U8, U16, U32, U64, U128, F32, F64])
+        Ok(self == other)
     }
 
     fn gt(&self, other: &Self) -> ValueResult<bool> {
-        comparison_match!(PartialOrd::gt[&self, &other]; [I8, I16, I32, I64, I128, U8, U16, U32, U64, U128, F32, F64])
+        Ok(self > other)
     }
 
     fn uno(&self, other: &Self) -> ValueResult<bool> {


Test patch:
diff --git a/tests/misc_testsuite/issue4857.wast b/tests/misc_testsuite/issue4857.wast
new file mode 100644
index 000000000..c233b9125
--- /dev/null
+++ b/tests/misc_testsuite/issue4857.wast
@@ -0,0 +1,10 @@
+(module
+  (func
+    i32.const 0
+    if
+      unreachable
+    end
+    f32.const nan
+    drop
+  )
+)