From dac3bdb078e2e88e0c9275840d1a1d79aadfea8c Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Sun, 14 Apr 2024 16:40:12 -0500 Subject: [PATCH] c-api: Fix alignment of `wasmtime_val_*` (#8363) * c-api: Fix alignment of `wasmtime_val_*` This commit fixes an issue where `wasmtime_val_raw_t` had an incorrect alignment. In Rust `ValRaw` contains a `u128` which has an alignment of 16 but in C the representation had a smaller alignment meaning that the alignment of the two structures was different. This was seen to cause alignment faults when structure were passed from C++ to Rust, for example. This commit changes the Rust representation of `ValRaw`'s `v128` field to do the same as C which is to use `[u8; 16]`. This avoids the need to raise the alignment in C which appears to be nontrivial. Cranelift is appropriately adjusted to understand that loads/stores from `ValRaw` are no longer aligned. Technically this only applies to the `v128` field but it's not too bad to apply it to the other fields as well. * Try alternate syntax for alignof --- crates/c-api/include/wasmtime/val.h | 11 +++++++++++ crates/c-api/src/val.rs | 5 +++++ crates/cranelift/src/compiler.rs | 8 ++++++-- crates/runtime/src/vmcontext.rs | 27 +++++++++++++++++++-------- 4 files changed, 41 insertions(+), 10 deletions(-) diff --git a/crates/c-api/include/wasmtime/val.h b/crates/c-api/include/wasmtime/val.h index 3721f65138e1..f923b5113639 100644 --- a/crates/c-api/include/wasmtime/val.h +++ b/crates/c-api/include/wasmtime/val.h @@ -7,6 +7,7 @@ #ifndef WASMTIME_VAL_H #define WASMTIME_VAL_H +#include #include #include @@ -306,6 +307,16 @@ typedef union wasmtime_val_raw { void *funcref; } wasmtime_val_raw_t; +// Assert that the shape of this type is as expected since it needs to match +// Rust. +static inline void __wasmtime_val_assertions() { + static_assert(sizeof(wasmtime_valunion_t) == 16, "should be 16-bytes large"); + static_assert(__alignof(wasmtime_valunion_t) == 8, + "should be 8-byte aligned"); + static_assert(sizeof(wasmtime_val_raw_t) == 16, "should be 16 bytes large"); + static_assert(__alignof(wasmtime_val_raw_t) == 8, "should be 8-byte aligned"); +} + /** * \typedef wasmtime_val_t * \brief Convenience alias for #wasmtime_val_t diff --git a/crates/c-api/src/val.rs b/crates/c-api/src/val.rs index b57c75f9995f..adfe61d65197 100644 --- a/crates/c-api/src/val.rs +++ b/crates/c-api/src/val.rs @@ -150,6 +150,11 @@ pub union wasmtime_val_union { pub v128: [u8; 16], } +const _: () = { + assert!(std::mem::size_of::() == 16); + assert!(std::mem::align_of::() == 8); +}; + // The raw pointers are actually optional boxes. unsafe impl Send for wasmtime_val_union where diff --git a/crates/cranelift/src/compiler.rs b/crates/cranelift/src/compiler.rs index b291a2faebd9..dc247b4eba72 100644 --- a/crates/cranelift/src/compiler.rs +++ b/crates/cranelift/src/compiler.rs @@ -963,7 +963,9 @@ impl Compiler { // despite this load/store being unrelated to execution in wasm itself. // For more details on this see the `ValRaw` type in the // `wasmtime-runtime` crate. - let flags = ir::MemFlags::trusted().with_endianness(ir::Endianness::Little); + let flags = ir::MemFlags::new() + .with_notrap() + .with_endianness(ir::Endianness::Little); let value_size = mem::size_of::(); for (i, (val, ty)) in values.iter().copied().zip(types).enumerate() { @@ -1000,7 +1002,9 @@ impl Compiler { // Note that this is little-endian like `store_values_to_array` above, // see notes there for more information. - let flags = MemFlags::trusted().with_endianness(ir::Endianness::Little); + let flags = MemFlags::new() + .with_notrap() + .with_endianness(ir::Endianness::Little); let mut results = Vec::new(); for (i, ty) in types.iter().enumerate() { diff --git a/crates/runtime/src/vmcontext.rs b/crates/runtime/src/vmcontext.rs index fbe63b402ee8..3ae3dab64194 100644 --- a/crates/runtime/src/vmcontext.rs +++ b/crates/runtime/src/vmcontext.rs @@ -991,13 +991,14 @@ pub union ValRaw { /// A WebAssembly `v128` value. /// - /// The payload here is a Rust `u128` which has the same number of bits but - /// note that `v128` in WebAssembly is often considered a vector type such - /// as `i32x4` or `f64x2`. This means that the actual interpretation of the - /// underlying bits is left up to the instructions which consume this value. + /// The payload here is a Rust `[u8; 16]` which has the same number of bits + /// but note that `v128` in WebAssembly is often considered a vector type + /// such as `i32x4` or `f64x2`. This means that the actual interpretation + /// of the underlying bits is left up to the instructions which consume + /// this value. /// /// This value is always stored in a little-endian format. - v128: u128, + v128: [u8; 16], /// A WebAssembly `funcref` value (or one of its subtypes). /// @@ -1032,6 +1033,14 @@ pub union ValRaw { anyref: u32, } +// The `ValRaw` type is matched as `wasmtime_val_raw_t` in the C API so these +// are some simple assertions about the shape of the type which are additionally +// matched in C. +const _: () = { + assert!(std::mem::size_of::() == 16); + assert!(std::mem::align_of::() == 8); +}; + // This type is just a bag-of-bits so it's up to the caller to figure out how // to safely deal with threading concerns and safely access interior bits. unsafe impl Send for ValRaw {} @@ -1055,7 +1064,7 @@ impl std::fmt::Debug for ValRaw { .field("i64", &Hex(self.i64)) .field("f32", &Hex(self.f32)) .field("f64", &Hex(self.f64)) - .field("v128", &Hex(self.v128)) + .field("v128", &Hex(u128::from_le_bytes(self.v128))) .field("funcref", &self.funcref) .field("externref", &Hex(self.externref)) .field("anyref", &Hex(self.anyref)) @@ -1114,7 +1123,9 @@ impl ValRaw { /// Creates a WebAssembly `v128` value #[inline] pub fn v128(i: u128) -> ValRaw { - ValRaw { v128: i.to_le() } + ValRaw { + v128: i.to_le_bytes(), + } } /// Creates a WebAssembly `funcref` value @@ -1180,7 +1191,7 @@ impl ValRaw { /// Gets the WebAssembly `v128` value #[inline] pub fn get_v128(&self) -> u128 { - unsafe { u128::from_le(self.v128) } + unsafe { u128::from_le_bytes(self.v128) } } /// Gets the WebAssembly `funcref` value