Repository: bytecodealliance/wasmtime
Pull request #5893 (2023-02-28 15:13:45)

cranelift: Add big and little endian memory accesses to interpreter
This PR aims to add specific big-endian and little-endian memory accesses to the interpreter when performing a load or store operation, as discussed over at #5881.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

- [ ] This has been discussed in issue #..., or if not, please tell us why
  here.
- [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.
- [ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the [code of
conduct](https://github.com/bytecodealliance/wasmtime/blob/master/CODE_OF_CONDUCT.md).
-->


afonso360: This looks great! Thanks!

Patch:
diff --git a/cranelift/codegen/src/data_value.rs b/cranelift/codegen/src/data_value.rs
index 2da55fc9c..6cba82c75 100644
--- a/cranelift/codegen/src/data_value.rs
+++ b/cranelift/codegen/src/data_value.rs
@@ -101,12 +101,55 @@ impl DataValue {
         }
     }
 
-    /// Write a [DataValue] to a slice.
+    fn swap_bytes(self) -> Self {
+        match self {
+            DataValue::I8(i) => DataValue::I8(i.swap_bytes()),
+            DataValue::I16(i) => DataValue::I16(i.swap_bytes()),
+            DataValue::I32(i) => DataValue::I32(i.swap_bytes()),
+            DataValue::I64(i) => DataValue::I64(i.swap_bytes()),
+            DataValue::I128(i) => DataValue::I128(i.swap_bytes()),
+            DataValue::U8(i) => DataValue::U8(i.swap_bytes()),
+            DataValue::U16(i) => DataValue::U16(i.swap_bytes()),
+            DataValue::U32(i) => DataValue::U32(i.swap_bytes()),
+            DataValue::U64(i) => DataValue::U64(i.swap_bytes()),
+            DataValue::U128(i) => DataValue::U128(i.swap_bytes()),
+            DataValue::F32(f) => DataValue::F32(Ieee32::with_bits(f.bits().swap_bytes())),
+            DataValue::F64(f) => DataValue::F64(Ieee64::with_bits(f.bits().swap_bytes())),
+            DataValue::V128(mut v) => {
+                v.reverse();
+                DataValue::V128(v)
+            }
+            DataValue::V64(mut v) => {
+                v.reverse();
+                DataValue::V64(v)
+            }
+        }
+    }
+
+    /// Converts `self` to big endian from target's endianness.
+    pub fn to_be(self) -> Self {
+        if cfg!(target_endian = "big") {
+            self
+        } else {
+            self.swap_bytes()
+        }
+    }
+
+    /// Converts `self` to little endian from target's endianness.
+    pub fn to_le(self) -> Self {
+        if cfg!(target_endian = "little") {
+            self
+        } else {
+            self.swap_bytes()
+        }
+    }
+
+    /// Write a [DataValue] to a slice in native-endian byte order.
     ///
     /// # Panics:
     ///
     /// Panics if the slice does not have enough space to accommodate the [DataValue]
-    pub fn write_to_slice(&self, dst: &mut [u8]) {
+    pub fn write_to_slice_ne(&self, dst: &mut [u8]) {
         match self {
             DataValue::I8(i) => dst[..1].copy_from_slice(&i.to_ne_bytes()[..]),
             DataValue::I16(i) => dst[..2].copy_from_slice(&i.to_ne_bytes()[..]),
@@ -121,12 +164,30 @@ impl DataValue {
         };
     }
 
-    /// Read a [DataValue] from a slice using a given [Type].
+    /// Write a [DataValue] to a slice in big-endian byte order.
+    ///
+    /// # Panics:
+    ///
+    /// Panics if the slice does not have enough space to accommodate the [DataValue]
+    pub fn write_to_slice_be(&self, dst: &mut [u8]) {
+        self.clone().to_be().write_to_slice_ne(dst);
+    }
+
+    /// Write a [DataValue] to a slice in little-endian byte order.
+    ///
+    /// # Panics:
+    ///
+    /// Panics if the slice does not have enough space to accommodate the [DataValue]
+    pub fn write_to_slice_le(&self, dst: &mut [u8]) {
+        self.clone().to_le().write_to_slice_ne(dst);
+    }
+
+    /// Read a [DataValue] from a slice using a given [Type] with native-endian byte order.
     ///
     /// # Panics:
     ///
     /// Panics if the slice does not have enough space to accommodate the [DataValue]
-    pub fn read_from_slice(src: &[u8], ty: Type) -> Self {
+    pub fn read_from_slice_ne(src: &[u8], ty: Type) -> Self {
         match ty {
             types::I8 => DataValue::I8(i8::from_ne_bytes(src[..1].try_into().unwrap())),
             types::I16 => DataValue::I16(i16::from_ne_bytes(src[..2].try_into().unwrap())),
@@ -152,15 +213,33 @@ impl DataValue {
         }
     }
 
-    /// Write a [DataValue] to a memory location.
+    /// Read a [DataValue] from a slice using a given [Type] in big-endian byte order.
+    ///
+    /// # Panics:
+    ///
+    /// Panics if the slice does not have enough space to accommodate the [DataValue]
+    pub fn read_from_slice_be(src: &[u8], ty: Type) -> Self {
+        DataValue::read_from_slice_ne(src, ty).to_be()
+    }
+
+    /// Read a [DataValue] from a slice using a given [Type] in little-endian byte order.
+    ///
+    /// # Panics:
+    ///
+    /// Panics if the slice does not have enough space to accommodate the [DataValue]
+    pub fn read_from_slice_le(src: &[u8], ty: Type) -> Self {
+        DataValue::read_from_slice_ne(src, ty).to_le()
+    }
+
+    /// Write a [DataValue] to a memory location in native-endian byte order.
     pub unsafe fn write_value_to(&self, p: *mut u128) {
         let size = self.ty().bytes() as usize;
-        self.write_to_slice(std::slice::from_raw_parts_mut(p as *mut u8, size));
+        self.write_to_slice_ne(std::slice::from_raw_parts_mut(p as *mut u8, size));
     }
 
-    /// Read a [DataValue] from a memory location using a given [Type].
+    /// Read a [DataValue] from a memory location using a given [Type] in native-endian byte order.
     pub unsafe fn read_value_from(p: *const u128, ty: Type) -> Self {
-        DataValue::read_from_slice(
+        DataValue::read_from_slice_ne(
             std::slice::from_raw_parts(p as *const u8, ty.bytes() as usize),
             ty,
         )
diff --git a/cranelift/codegen/src/ir/instructions.rs b/cranelift/codegen/src/ir/instructions.rs
index f28cc45d2..40507db01 100644
--- a/cranelift/codegen/src/ir/instructions.rs
+++ b/cranelift/codegen/src/ir/instructions.rs
@@ -403,7 +403,9 @@ impl InstructionData {
             &InstructionData::Load { flags, .. }
             | &InstructionData::LoadNoOffset { flags, .. }
             | &InstructionData::Store { flags, .. }
-            | &InstructionData::StoreNoOffset { flags, .. } => Some(flags),
+            | &InstructionData::StoreNoOffset { flags, .. }
+            | &InstructionData::AtomicCas { flags, .. }
+            | &InstructionData::AtomicRmw { flags, .. } => Some(flags),
             _ => None,
         }
     }
diff --git a/cranelift/fuzzgen/src/lib.rs b/cranelift/fuzzgen/src/lib.rs
index 04d2815ba..1a4c912ec 100644
--- a/cranelift/fuzzgen/src/lib.rs
+++ b/cranelift/fuzzgen/src/lib.rs
@@ -145,7 +145,7 @@ impl fmt::Debug for TestCase {
             let returns = &self.main().signature.returns;
             let placeholder_output = returns
                 .iter()
-                .map(|param| DataValue::read_from_slice(&[0; 16][..], param.value_type))
+                .map(|param| DataValue::read_from_slice_ne(&[0; 16][..], param.value_type))
                 .map(|val| format!("{}", val))
                 .collect::<Vec<_>>()
                 .join(", ");
diff --git a/cranelift/interpreter/src/interpreter.rs b/cranelift/interpreter/src/interpreter.rs
index 290901dc4..aa44f8e3b 100644
--- a/cranelift/interpreter/src/interpreter.rs
+++ b/cranelift/interpreter/src/interpreter.rs
@@ -11,8 +11,8 @@ use crate::step::{step, ControlFlow, StepError};
 use crate::value::{Value, ValueError};
 use cranelift_codegen::data_value::DataValue;
 use cranelift_codegen::ir::{
-    ArgumentPurpose, Block, ExternalName, FuncRef, Function, GlobalValue, GlobalValueData, LibCall,
-    StackSlot, TrapCode, Type, Value as ValueRef,
+    ArgumentPurpose, Block, Endianness, ExternalName, FuncRef, Function, GlobalValue,
+    GlobalValueData, LibCall, MemFlags, StackSlot, TrapCode, Type, Value as ValueRef,
 };
 use log::trace;
 use smallvec::SmallVec;
@@ -192,10 +192,16 @@ pub struct InterpreterState<'a> {
     pub frame_offset: usize,
     pub stack: Vec<u8>,
     pub pinned_reg: DataValue,
+    pub native_endianness: Endianness,
 }
 
 impl Default for InterpreterState<'_> {
     fn default() -> Self {
+        let native_endianness = if cfg!(target_endian = "little") {
+            Endianness::Little
+        } else {
+            Endianness::Big
+        };
         Self {
             functions: FunctionStore::default(),
             libcall_handler: |_, _| Err(TrapCode::UnreachableCodeReached),
@@ -203,6 +209,7 @@ impl Default for InterpreterState<'_> {
             frame_offset: 0,
             stack: Vec::with_capacity(1024),
             pinned_reg: DataValue::U64(0),
+            native_endianness,
         }
     }
 }
@@ -308,7 +315,12 @@ impl<'a> State<'a, DataValue> for InterpreterState<'a> {
         Address::from_parts(size, AddressRegion::Stack, 0, final_offset)
     }
 
-    fn checked_load(&self, addr: Address, ty: Type) -> Result<DataValue, MemoryError> {
+    fn checked_load(
+        &self,
+        addr: Address,
+        ty: Type,
+        mem_flags: MemFlags,
+    ) -> Result<DataValue, MemoryError> {
         let load_size = ty.bytes() as usize;
         let addr_start = addr.offset as usize;
         let addr_end = addr_start + load_size;
@@ -324,10 +336,18 @@ impl<'a> State<'a, DataValue> for InterpreterState<'a> {
             _ => unimplemented!(),
         };
 
-        Ok(DataValue::read_from_slice(src, ty))
+        Ok(match mem_flags.endianness(self.native_endianness) {
+            Endianness::Big => DataValue::read_from_slice_be(src, ty),
+            Endianness::Little => DataValue::read_from_slice_le(src, ty),
+        })
     }
 
-    fn checked_store(&mut self, addr: Address, v: DataValue) -> Result<(), MemoryError> {
+    fn checked_store(
+        &mut self,
+        addr: Address,
+        v: DataValue,
+        mem_flags: MemFlags,
+    ) -> Result<(), MemoryError> {
         let store_size = v.ty().bytes() as usize;
         let addr_start = addr.offset as usize;
         let addr_end = addr_start + store_size;
@@ -343,7 +363,10 @@ impl<'a> State<'a, DataValue> for InterpreterState<'a> {
             _ => unimplemented!(),
         };
 
-        Ok(v.write_to_slice(dst))
+        Ok(match mem_flags.endianness(self.native_endianness) {
+            Endianness::Big => v.write_to_slice_be(dst),
+            Endianness::Little => v.write_to_slice_le(dst),
+        })
     }
 
     fn function_address(
@@ -493,9 +516,10 @@ impl<'a> State<'a, DataValue> for InterpreterState<'a> {
                     global_type,
                 }) => {
                     let mut addr = Address::try_from(current_val)?;
+                    let mem_flags = MemFlags::trusted();
                     // We can forego bounds checking here since its performed in `checked_load`
                     addr.offset += offset as u64;
-                    current_val = self.checked_load(addr, global_type)?;
+                    current_val = self.checked_load(addr, global_type, mem_flags)?;
                 }
 
                 // We are done resolving this, return the current value
diff --git a/cranelift/interpreter/src/state.rs b/cranelift/interpreter/src/state.rs
index b98f0c552..c797bea7e 100644
--- a/cranelift/interpreter/src/state.rs
+++ b/cranelift/interpreter/src/state.rs
@@ -4,7 +4,8 @@ use crate::address::{Address, AddressSize};
 use crate::interpreter::LibCallHandler;
 use cranelift_codegen::data_value::DataValue;
 use cranelift_codegen::ir::{
-    ExternalName, FuncRef, Function, GlobalValue, LibCall, Signature, StackSlot, Type, Value,
+    ExternalName, FuncRef, Function, GlobalValue, LibCall, MemFlags, Signature, StackSlot, Type,
+    Value,
 };
 use cranelift_codegen::isa::CallConv;
 use cranelift_entity::PrimaryMap;
@@ -62,10 +63,20 @@ pub trait State<'a, V> {
     ) -> Result<Address, MemoryError>;
     /// Retrieve a value `V` from memory at the given `address`, checking if it belongs either to the
     /// stack or to one of the heaps; the number of bytes loaded corresponds to the specified [Type].
-    fn checked_load(&self, address: Address, ty: Type) -> Result<V, MemoryError>;
+    fn checked_load(
+        &self,
+        address: Address,
+        ty: Type,
+        mem_flags: MemFlags,
+    ) -> Result<V, 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) -> Result<(), MemoryError>;
+    fn checked_store(
+        &mut self,
+        address: Address,
+        v: V,
+        mem_flags: MemFlags,
+    ) -> Result<(), MemoryError>;
 
     /// Compute the address of a function given its name.
     fn function_address(
@@ -182,11 +193,21 @@ where
         unimplemented!()
     }
 
-    fn checked_load(&self, _addr: Address, _ty: Type) -> Result<V, MemoryError> {
+    fn checked_load(
+        &self,
+        _addr: Address,
+        _ty: Type,
+        _mem_flags: MemFlags,
+    ) -> Result<V, MemoryError> {
         unimplemented!()
     }
 
-    fn checked_store(&mut self, _addr: Address, _v: V) -> Result<(), MemoryError> {
+    fn checked_store(
+        &mut self,
+        _addr: Address,
+        _v: V,
+        _mem_flags: MemFlags,
+    ) -> Result<(), MemoryError> {
         unimplemented!()
     }
 
diff --git a/cranelift/interpreter/src/step.rs b/cranelift/interpreter/src/step.rs
index 0eddf9c35..d0362e93d 100644
--- a/cranelift/interpreter/src/step.rs
+++ b/cranelift/interpreter/src/step.rs
@@ -8,7 +8,7 @@ use cranelift_codegen::data_value::DataValue;
 use cranelift_codegen::ir::condcodes::{FloatCC, IntCC};
 use cranelift_codegen::ir::{
     types, AbiParam, AtomicRmwOp, Block, BlockCall, ExternalName, FuncRef, Function,
-    InstructionData, Opcode, TrapCode, Type, Value as ValueRef,
+    InstructionData, MemFlags, Opcode, TrapCode, Type, Value as ValueRef,
 };
 use log::trace;
 use smallvec::{smallvec, SmallVec};
@@ -482,8 +482,10 @@ where
             };
 
             let addr_value = calculate_addr(types::I64, imm(), args()?)?;
+            let mem_flags = inst.memflags().expect("instruction to have memory flags");
             let loaded = assign_or_memtrap(
-                Address::try_from(addr_value).and_then(|addr| state.checked_load(addr, load_ty)),
+                Address::try_from(addr_value)
+                    .and_then(|addr| state.checked_load(addr, load_ty, mem_flags)),
             );
 
             match (loaded, kind) {
@@ -505,33 +507,37 @@ where
             };
 
             let addr_value = calculate_addr(types::I64, imm(), args_range(1..)?)?;
+            let mem_flags = inst.memflags().expect("instruction to have memory flags");
             let reduced = if let Some(c) = kind {
                 arg(0)?.convert(c)?
             } else {
                 arg(0)?
             };
             continue_or_memtrap(
-                Address::try_from(addr_value).and_then(|addr| state.checked_store(addr, reduced)),
+                Address::try_from(addr_value)
+                    .and_then(|addr| state.checked_store(addr, reduced, mem_flags)),
             )
         }
         Opcode::StackLoad => {
             let load_ty = inst_context.controlling_type().unwrap();
             let slot = inst.stack_slot().unwrap();
             let offset = sum(imm(), args()?)? as u64;
+            let mem_flags = MemFlags::trusted();
             assign_or_memtrap({
                 state
                     .stack_address(AddressSize::_64, slot, offset)
-                    .and_then(|addr| state.checked_load(addr, load_ty))
+                    .and_then(|addr| state.checked_load(addr, load_ty, mem_flags))
             })
         }
         Opcode::StackStore => {
             let arg = arg(0)?;
             let slot = inst.stack_slot().unwrap();
             let offset = sum(imm(), args_range(1..)?)? as u64;
+            let mem_flags = MemFlags::trusted();
             continue_or_memtrap({
                 state
                     .stack_address(AddressSize::_64, slot, offset)
-                    .and_then(|addr| state.checked_store(addr, arg))
+                    .and_then(|addr| state.checked_store(addr, arg, mem_flags))
             })
         }
         Opcode::StackAddr => {
@@ -1238,7 +1244,9 @@ where
             let op = inst.atomic_rmw_op().unwrap();
             let val = arg(1)?;
             let addr = arg(0)?.into_int()? as u64;
-            let loaded = Address::try_from(addr).and_then(|addr| state.checked_load(addr, ctrl_ty));
+            let mem_flags = inst.memflags().expect("instruction to have memory flags");
+            let loaded = Address::try_from(addr)
+                .and_then(|addr| state.checked_load(addr, ctrl_ty, mem_flags));
             let prev_val = match loaded {
                 Ok(v) => v,
                 Err(e) => return Ok(ControlFlow::Trap(CraneliftTrap::User(memerror_to_trap(e)))),
@@ -1265,13 +1273,15 @@ where
                 )
                 .and_then(|v| Value::convert(v, ValueConversionKind::ToSigned)),
             }?;
-            let stored =
-                Address::try_from(addr).and_then(|addr| state.checked_store(addr, replace));
+            let stored = Address::try_from(addr)
+                .and_then(|addr| state.checked_store(addr, replace, mem_flags));
             assign_or_memtrap(stored.map(|_| prev_val_to_assign))
         }
         Opcode::AtomicCas => {
             let addr = arg(0)?.into_int()? as u64;
-            let loaded = Address::try_from(addr).and_then(|addr| state.checked_load(addr, ctrl_ty));
+            let mem_flags = inst.memflags().expect("instruction to have memory flags");
+            let loaded = Address::try_from(addr)
+                .and_then(|addr| state.checked_load(addr, ctrl_ty, mem_flags));
             let loaded_val = match loaded {
                 Ok(v) => v,
                 Err(e) => return Ok(ControlFlow::Trap(CraneliftTrap::User(memerror_to_trap(e)))),
@@ -1280,7 +1290,7 @@ where
             let val_to_assign = if Value::eq(&loaded_val, &expected_val)? {
                 let val_to_store = arg(2)?;
                 Address::try_from(addr)
-                    .and_then(|addr| state.checked_store(addr, val_to_store))
+                    .and_then(|addr| state.checked_store(addr, val_to_store, mem_flags))
                     .map(|_| loaded_val)
             } else {
                 Ok(loaded_val)
@@ -1290,17 +1300,20 @@ where
         Opcode::AtomicLoad => {
             let load_ty = inst_context.controlling_type().unwrap();
             let addr = arg(0)?.into_int()? as u64;
+            let mem_flags = inst.memflags().expect("instruction to have memory flags");
             // We are doing a regular load here, this isn't actually thread safe.
             assign_or_memtrap(
-                Address::try_from(addr).and_then(|addr| state.checked_load(addr, load_ty)),
+                Address::try_from(addr)
+                    .and_then(|addr| state.checked_load(addr, load_ty, mem_flags)),
             )
         }
         Opcode::AtomicStore => {
             let val = arg(0)?;
             let addr = arg(1)?.into_int()? as u64;
+            let mem_flags = inst.memflags().expect("instruction to have memory flags");
             // We are doing a regular store here, this isn't actually thread safe.
             continue_or_memtrap(
-                Address::try_from(addr).and_then(|addr| state.checked_store(addr, val)),
+                Address::try_from(addr).and_then(|addr| state.checked_store(addr, val, mem_flags)),
             )
         }
         Opcode::Fence => {


Test patch:
diff --git a/cranelift/filetests/filetests/runtests/atomic-cas-subword-big.clif b/cranelift/filetests/filetests/runtests/atomic-cas-subword-big.clif
index 4fb63be0f..9be9323dd 100644
--- a/cranelift/filetests/filetests/runtests/atomic-cas-subword-big.clif
+++ b/cranelift/filetests/filetests/runtests/atomic-cas-subword-big.clif
@@ -1,3 +1,4 @@
+test interpret
 test run
 target s390x
 
diff --git a/cranelift/filetests/filetests/runtests/atomic-cas-subword-little.clif b/cranelift/filetests/filetests/runtests/atomic-cas-subword-little.clif
index ae5871737..f7565d63d 100644
--- a/cranelift/filetests/filetests/runtests/atomic-cas-subword-little.clif
+++ b/cranelift/filetests/filetests/runtests/atomic-cas-subword-little.clif
@@ -1,3 +1,4 @@
+test interpret
 test run
 target s390x
 target aarch64
diff --git a/cranelift/filetests/filetests/runtests/atomic-rmw-subword-big.clif b/cranelift/filetests/filetests/runtests/atomic-rmw-subword-big.clif
index dacbcb708..f0f52c12a 100644
--- a/cranelift/filetests/filetests/runtests/atomic-rmw-subword-big.clif
+++ b/cranelift/filetests/filetests/runtests/atomic-rmw-subword-big.clif
@@ -1,3 +1,4 @@
+test interpret
 test run
 target s390x
 target s390x has_mie2
@@ -431,10 +432,10 @@ block0(v0: i32, v1: i64, v2: i16):
     v6 = load.i32 big v3
     return v6
 }
-; run: %atomic_rmw_xchg_little_i16(0x12345678, 0, 0x1111) == 0x11115678
-; run: %atomic_rmw_xchg_little_i16(0x12345678, 0, 0xffff) == 0xffff5678
-; run: %atomic_rmw_xchg_little_i16(0x12345678, 2, 0x1111) == 0x12341111
-; run: %atomic_rmw_xchg_little_i16(0x12345678, 2, 0xffff) == 0x1234ffff
+; run: %atomic_rmw_xchg_big_i16(0x12345678, 0, 0x1111) == 0x11115678
+; run: %atomic_rmw_xchg_big_i16(0x12345678, 0, 0xffff) == 0xffff5678
+; run: %atomic_rmw_xchg_big_i16(0x12345678, 2, 0x1111) == 0x12341111
+; run: %atomic_rmw_xchg_big_i16(0x12345678, 2, 0xffff) == 0x1234ffff
 
 
 function %atomic_rmw_xchg_big_i8(i32, i64, i8) -> i32 {
diff --git a/cranelift/filetests/filetests/runtests/atomic-rmw-subword-little.clif b/cranelift/filetests/filetests/runtests/atomic-rmw-subword-little.clif
index a6d16c7a6..2cb9beba7 100644
--- a/cranelift/filetests/filetests/runtests/atomic-rmw-subword-little.clif
+++ b/cranelift/filetests/filetests/runtests/atomic-rmw-subword-little.clif
@@ -1,3 +1,4 @@
+test interpret
 test run
 target s390x
 target s390x has_mie2