Make cranelift-interpreter non-generic over value (#6178)

* Make cranelift-interpreter non-generic over value

Fixes #5793

* Review suggestion

Co-authored-by: Jamey Sharp <jamey@minilop.net>

* Fix fuzz target

* Update doc comments

---------

Co-authored-by: Jamey Sharp <jamey@minilop.net>

cranelift/interpreter/src/interpreter.rs

@@ -8,7 +8,7 @@ use crate::frame::Frame;
 use crate::instruction::DfgInstructionContext;
 use crate::state::{InterpreterFunctionRef, MemoryError, State};
 use crate::step::{step, ControlFlow, StepError};
-use crate::value::{Value, ValueError};
+use crate::value::{DataValueExt, ValueError};
 use cranelift_codegen::data_value::DataValue;
 use cranelift_codegen::ir::{
     ArgumentPurpose, Block, Endianness, ExternalName, FuncRef, Function, GlobalValue,
@@ -46,7 +46,7 @@ impl<'a> Interpreter<'a> {
         &mut self,
         func_name: &str,
         arguments: &[DataValue],
-    ) -> Result<ControlFlow<'a, DataValue>, InterpreterError> {
+    ) -> Result<ControlFlow<'a>, InterpreterError> {
         let index = self
             .state
             .functions
@@ -61,7 +61,7 @@ impl<'a> Interpreter<'a> {
         &mut self,
         index: FuncIndex,
         arguments: &[DataValue],
-    ) -> Result<ControlFlow<'a, DataValue>, InterpreterError> {
+    ) -> Result<ControlFlow<'a>, InterpreterError> {
         match self.state.functions.get_by_index(index) {
             None => Err(InterpreterError::UnknownFunctionIndex(index)),
             Some(func) => self.call(func, arguments),
@@ -73,7 +73,7 @@ impl<'a> Interpreter<'a> {
         &mut self,
         function: &'a Function,
         arguments: &[DataValue],
-    ) -> Result<ControlFlow<'a, DataValue>, InterpreterError> {
+    ) -> Result<ControlFlow<'a>, InterpreterError> {
         trace!("Call: {}({:?})", function.name, arguments);
         let first_block = function
             .layout
@@ -91,7 +91,7 @@ impl<'a> Interpreter<'a> {
 
     /// Interpret a [Block] in a [Function]. This drives the interpretation over sequences of
     /// instructions, which may continue in other blocks, until the function returns.
-    fn block(&mut self, block: Block) -> Result<ControlFlow<'a, DataValue>, InterpreterError> {
+    fn block(&mut self, block: Block) -> Result<ControlFlow<'a>, InterpreterError> {
         trace!("Block: {}", block);
         let function = self.state.current_frame_mut().function();
         let layout = &function.layout;
@@ -192,13 +192,13 @@ pub enum InterpreterError {
     FuelExhausted,
 }
 
-pub type LibCallValues<V> = SmallVec<[V; 1]>;
+pub type LibCallValues = SmallVec<[DataValue; 1]>;
-pub type LibCallHandler<V> = fn(LibCall, LibCallValues<V>) -> Result<LibCallValues<V>, TrapCode>;
+pub type LibCallHandler = fn(LibCall, LibCallValues) -> Result<LibCallValues, TrapCode>;
 
 /// Maintains the [Interpreter]'s state, implementing the [State] trait.
 pub struct InterpreterState<'a> {
     pub functions: FunctionStore<'a>,
-    pub libcall_handler: LibCallHandler<DataValue>,
+    pub libcall_handler: LibCallHandler,
     pub frame_stack: Vec<Frame<'a>>,
     /// Number of bytes from the bottom of the stack where the current frame's stack space is
     pub frame_offset: usize,
@@ -232,7 +232,7 @@ impl<'a> InterpreterState<'a> {
     }
 
     /// Registers a libcall handler
-    pub fn with_libcall_handler(mut self, handler: LibCallHandler<DataValue>) -> Self {
+    pub fn with_libcall_handler(mut self, handler: LibCallHandler) -> Self {
         self.libcall_handler = handler;
         self
     }
@@ -254,7 +254,7 @@ impl<'a> InterpreterState<'a> {
     }
 }
 
-impl<'a> State<'a, DataValue> for InterpreterState<'a> {
+impl<'a> State<'a> for InterpreterState<'a> {
     fn get_function(&self, func_ref: FuncRef) -> Option<&'a Function> {
         self.functions
             .get_from_func_ref(func_ref, self.frame_stack.last().unwrap().function())
@@ -263,7 +263,7 @@ impl<'a> State<'a, DataValue> for InterpreterState<'a> {
         self.current_frame().function()
     }
 
-    fn get_libcall_handler(&self) -> LibCallHandler<DataValue> {
+    fn get_libcall_handler(&self) -> LibCallHandler {
         self.libcall_handler
     }
 

cranelift/interpreter/src/state.rs

@@ -22,13 +22,13 @@ use thiserror::Error;
 /// instructions--no heap knowledge required), we can partially implement this trait. See
 /// [ImmutableRegisterState] for an example of this: it only exposes the values referenced by the
 /// SSA references in the current frame and not much else.
-pub trait State<'a, V> {
+pub trait State<'a> {
     /// Retrieve a reference to a [Function].
     fn get_function(&self, func_ref: FuncRef) -> Option<&'a Function>;
     /// Retrieve a reference to the currently executing [Function].
     fn get_current_function(&self) -> &'a Function;
     /// Retrieve the handler callback for a [LibCall](cranelift_codegen::ir::LibCall)
-    fn get_libcall_handler(&self) -> LibCallHandler<V>;
+    fn get_libcall_handler(&self) -> LibCallHandler;
     /// Record that an interpreter has called into a new [Function].
     fn push_frame(&mut self, function: &'a Function);
     /// Record that an interpreter has returned from a called [Function].
@@ -36,14 +36,14 @@ pub trait State<'a, V> {
 
     /// Retrieve a value `V` by its [value reference](cranelift_codegen::ir::Value) from the
     /// virtual register file.
-    fn get_value(&self, name: Value) -> Option<V>;
+    fn get_value(&self, name: Value) -> Option<DataValue>;
     /// Assign a value `V` to its [value reference](cranelift_codegen::ir::Value) in the
     /// virtual register file.
-    fn set_value(&mut self, name: Value, value: V) -> Option<V>;
+    fn set_value(&mut self, name: Value, value: DataValue) -> Option<DataValue>;
     /// Collect a list of values `V` by their  [value references](cranelift_codegen::ir::Value);
     /// this is a convenience method for `get_value`. If no value is found for a value reference,
     /// return an `Err` containing the offending reference.
-    fn collect_values(&self, names: &[Value]) -> Result<SmallVec<[V; 1]>, Value> {
+    fn collect_values(&self, names: &[Value]) -> Result<SmallVec<[DataValue; 1]>, Value> {
         let mut values = SmallVec::with_capacity(names.len());
         for &n in names {
             match self.get_value(n) {
@@ -68,13 +68,13 @@ pub trait State<'a, V> {
         address: Address,
         ty: Type,
         mem_flags: MemFlags,
-    ) -> Result<V, MemoryError>;
+    ) -> Result<DataValue, MemoryError>;
     /// Store a value `V` into memory at the given `address`, checking if it belongs either to the
     /// stack or to one of the heaps; the number of bytes stored corresponds to the specified [Type].
     fn checked_store(
         &mut self,
         address: Address,
-        v: V,
+        v: DataValue,
         mem_flags: MemFlags,
     ) -> Result<(), MemoryError>;
 
@@ -90,12 +90,12 @@ pub trait State<'a, V> {
 
     /// Given a global value, compute the final value for that global value, applying all operations
     /// in intermediate global values.
-    fn resolve_global_value(&self, gv: GlobalValue) -> Result<V, MemoryError>;
+    fn resolve_global_value(&self, gv: GlobalValue) -> Result<DataValue, MemoryError>;
 
     /// Retrieves the current pinned reg value
-    fn get_pinned_reg(&self) -> V;
+    fn get_pinned_reg(&self) -> DataValue;
     /// Sets a value for the pinned reg
-    fn set_pinned_reg(&mut self, v: V);
+    fn set_pinned_reg(&mut self, v: DataValue);
 }
 
 pub enum InterpreterFunctionRef<'a> {
@@ -146,17 +146,14 @@ pub enum MemoryError {
 }
 
 /// This dummy state allows interpretation over an immutable mapping of values in a single frame.
-pub struct ImmutableRegisterState<'a, V>(&'a PrimaryMap<Value, V>);
+pub struct ImmutableRegisterState<'a>(&'a PrimaryMap<Value, DataValue>);
-impl<'a, V> ImmutableRegisterState<'a, V> {
+impl<'a> ImmutableRegisterState<'a> {
-    pub fn new(values: &'a PrimaryMap<Value, V>) -> Self {
+    pub fn new(values: &'a PrimaryMap<Value, DataValue>) -> Self {
         Self(values)
     }
 }
 
-impl<'a, V> State<'a, V> for ImmutableRegisterState<'a, V>
+impl<'a> State<'a> for ImmutableRegisterState<'a> {
-where
-    V: Clone,
-{
     fn get_function(&self, _func_ref: FuncRef) -> Option<&'a Function> {
         None
     }
@@ -165,7 +162,7 @@ where
         unimplemented!()
     }
 
-    fn get_libcall_handler(&self) -> LibCallHandler<V> {
+    fn get_libcall_handler(&self) -> LibCallHandler {
         unimplemented!()
     }
 
@@ -177,11 +174,11 @@ where
         unimplemented!()
     }
 
-    fn get_value(&self, name: Value) -> Option<V> {
+    fn get_value(&self, name: Value) -> Option<DataValue> {
         self.0.get(name).cloned()
     }
 
-    fn set_value(&mut self, _name: Value, _value: V) -> Option<V> {
+    fn set_value(&mut self, _name: Value, _value: DataValue) -> Option<DataValue> {
         None
     }
 
@@ -199,14 +196,14 @@ where
         _addr: Address,
         _ty: Type,
         _mem_flags: MemFlags,
-    ) -> Result<V, MemoryError> {
+    ) -> Result<DataValue, MemoryError> {
         unimplemented!()
     }
 
     fn checked_store(
         &mut self,
         _addr: Address,
-        _v: V,
+        _v: DataValue,
         _mem_flags: MemFlags,
     ) -> Result<(), MemoryError> {
         unimplemented!()
@@ -224,15 +221,15 @@ where
         unimplemented!()
     }
 
-    fn resolve_global_value(&self, _gv: GlobalValue) -> Result<V, MemoryError> {
+    fn resolve_global_value(&self, _gv: GlobalValue) -> Result<DataValue, MemoryError> {
         unimplemented!()
     }
 
-    fn get_pinned_reg(&self) -> V {
+    fn get_pinned_reg(&self) -> DataValue {
         unimplemented!()
     }
 
-    fn set_pinned_reg(&mut self, _v: V) {
+    fn set_pinned_reg(&mut self, _v: DataValue) {
         unimplemented!()
     }
 }

cranelift/interpreter/src/value.rs

@@ -1,7 +1,6 @@
-//! The [Value] trait describes what operations can be performed on interpreter values. The
+//! The [DataValueExt] trait is an extension trait for [DataValue]. It provides a lot of functions
-//! interpreter usually executes using [DataValue]s so an implementation is provided here. The fact
+//! used by the rest of the interpreter.
-//! that [Value] is a trait, however, allows interpretation of Cranelift IR on other kinds of
+
-//! values.
 use core::convert::TryFrom;
 use core::fmt::{self, Display, Formatter};
 use cranelift_codegen::data_value::{DataValue, DataValueCastFailure};
@@ -11,9 +10,8 @@ use thiserror::Error;
 
 pub type ValueResult<T> = Result<T, ValueError>;
 
-pub trait Value: Clone + From<DataValue> {
+pub trait DataValueExt: Sized {
     // Identity.
-    fn ty(&self) -> Type;
     fn int(n: i128, ty: Type) -> ValueResult<Self>;
     fn into_int(self) -> ValueResult<i128>;
     fn float(n: u64, ty: Type) -> ValueResult<Self>;
@@ -34,17 +32,6 @@ pub trait Value: Clone + From<DataValue> {
     fn min(self, other: Self) -> ValueResult<Self>;
 
     // Comparison.
-    fn eq(&self, other: &Self) -> ValueResult<bool>;
-    fn gt(&self, other: &Self) -> ValueResult<bool>;
-    fn ge(&self, other: &Self) -> ValueResult<bool> {
-        Ok(self.eq(other)? || self.gt(other)?)
-    }
-    fn lt(&self, other: &Self) -> ValueResult<bool> {
-        other.gt(self)
-    }
-    fn le(&self, other: &Self) -> ValueResult<bool> {
-        Ok(other.eq(self)? || other.gt(self)?)
-    }
     fn uno(&self, other: &Self) -> ValueResult<bool>;
 
     // Arithmetic.
@@ -240,11 +227,7 @@ macro_rules! bitop {
     };
 }
 
-impl Value for DataValue {
+impl DataValueExt for DataValue {
-    fn ty(&self) -> Type {
-        self.ty()
-    }
-
     fn int(n: i128, ty: Type) -> ValueResult<Self> {
         if ty.is_int() && !ty.is_vector() {
             DataValue::from_integer(n, ty).map_err(|_| ValueError::InvalidValue(ty))
@@ -507,7 +490,7 @@ impl Value for DataValue {
     }
 
     fn max(self, other: Self) -> ValueResult<Self> {
-        if Value::gt(&self, &other)? {
+        if self > other {
             Ok(self)
         } else {
             Ok(other)
@@ -515,21 +498,13 @@ impl Value for DataValue {
     }
 
     fn min(self, other: Self) -> ValueResult<Self> {
-        if Value::lt(&self, &other)? {
+        if self < other {
             Ok(self)
         } else {
             Ok(other)
         }
     }
 
-    fn eq(&self, other: &Self) -> ValueResult<bool> {
-        Ok(self == other)
-    }
-
-    fn gt(&self, other: &Self) -> ValueResult<bool> {
-        Ok(self > other)
-    }
-
     fn uno(&self, other: &Self) -> ValueResult<bool> {
         Ok(self.is_nan()? || other.is_nan()?)
     }
@@ -566,7 +541,7 @@ impl Value for DataValue {
         let denominator = other.clone().into_int()?;
 
         // Check if we are dividing INT_MIN / -1. This causes an integer overflow trap.
-        let min = Value::int(1i128 << (self.ty().bits() - 1), self.ty())?;
+        let min = DataValueExt::int(1i128 << (self.ty().bits() - 1), self.ty())?;
         if self == min && denominator == -1 {
             return Err(ValueError::IntegerOverflow);
         }
@@ -582,7 +557,7 @@ impl Value for DataValue {
         let denominator = other.clone().into_int()?;
 
         // Check if we are dividing INT_MIN / -1. This causes an integer overflow trap.
-        let min = Value::int(1i128 << (self.ty().bits() - 1), self.ty())?;
+        let min = DataValueExt::int(1i128 << (self.ty().bits() - 1), self.ty())?;
         if self == min && denominator == -1 {
             return Err(ValueError::IntegerOverflow);
         }

fuzz/fuzz_targets/cranelift-fuzzgen.rs

@@ -298,7 +298,7 @@ fn build_interpreter(testcase: &TestCase) -> Interpreter {
 
     let state = InterpreterState::default()
         .with_function_store(env)
-        .with_libcall_handler(|libcall: LibCall, args: LibCallValues<DataValue>| {
+        .with_libcall_handler(|libcall: LibCall, args: LibCallValues| {
             use LibCall::*;
             Ok(smallvec![match (libcall, &args[..]) {
                 (CeilF32, [DataValue::F32(a)]) => DataValue::F32(a.ceil()),