Skip to content

Commit

Permalink
c-api: Fix alignment of wasmtime_val_* (bytecodealliance#8363)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
alexcrichton authored Apr 14, 2024
1 parent ca5f1bb commit dac3bdb
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 10 deletions.
11 changes: 11 additions & 0 deletions crates/c-api/include/wasmtime/val.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#ifndef WASMTIME_VAL_H
#define WASMTIME_VAL_H

#include <stdalign.h>
#include <wasm.h>
#include <wasmtime/extern.h>

Expand Down Expand Up @@ -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
Expand Down
5 changes: 5 additions & 0 deletions crates/c-api/src/val.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,11 @@ pub union wasmtime_val_union {
pub v128: [u8; 16],
}

const _: () = {
assert!(std::mem::size_of::<wasmtime_val_union>() == 16);
assert!(std::mem::align_of::<wasmtime_val_union>() == 8);
};

// The raw pointers are actually optional boxes.
unsafe impl Send for wasmtime_val_union
where
Expand Down
8 changes: 6 additions & 2 deletions crates/cranelift/src/compiler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::<u128>();
for (i, (val, ty)) in values.iter().copied().zip(types).enumerate() {
Expand Down Expand Up @@ -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() {
Expand Down
27 changes: 19 additions & 8 deletions crates/runtime/src/vmcontext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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).
///
Expand Down Expand Up @@ -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::<ValRaw>() == 16);
assert!(std::mem::align_of::<ValRaw>() == 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 {}
Expand All @@ -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))
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit dac3bdb

Please sign in to comment.