From a02a60952839d1eb7a8cbb5e02afa3a6f65148e7 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 24 May 2022 19:14:29 -0500 Subject: [PATCH] Make `ValRaw` fields private (#4186) * Make `ValRaw` fields private Force accessing to go through constructors and accessors to localize the knowledge about little-endian-ness. This is spawned since I made a mistake in #4039 about endianness. * Fix some tests * Component model changes --- benches/call.rs | 2 +- crates/runtime/src/vmcontext.rs | 126 ++++++++++++++++++-- crates/wasmtime/src/component/func/typed.rs | 76 +++++------- crates/wasmtime/src/func.rs | 2 +- crates/wasmtime/src/func/typed.rs | 32 ++--- crates/wasmtime/src/values.rs | 34 +++--- tests/all/call_hook.rs | 8 +- 7 files changed, 187 insertions(+), 93 deletions(-) diff --git a/benches/call.rs b/benches/call.rs index e04aa16bf4..22ba12650a 100644 --- a/benches/call.rs +++ b/benches/call.rs @@ -160,7 +160,7 @@ fn bench_host_to_wasm( let untyped = instance.get_func(&mut *store, name).unwrap(); let params = typed_params.to_vals(); let results = typed_results.to_vals(); - let mut space = vec![ValRaw { i32: 0 }; params.len().max(results.len())]; + let mut space = vec![ValRaw::i32(0); params.len().max(results.len())]; b.iter(|| unsafe { for (i, param) in params.iter().enumerate() { space[i] = param.to_raw(&mut *store); diff --git a/crates/runtime/src/vmcontext.rs b/crates/runtime/src/vmcontext.rs index c670e3d997..22a148a2b0 100644 --- a/crates/runtime/src/vmcontext.rs +++ b/crates/runtime/src/vmcontext.rs @@ -794,7 +794,7 @@ pub union ValRaw { /// or unsigned. The Rust type `i32` is simply chosen for convenience. /// /// This value is always stored in a little-endian format. - pub i32: i32, + i32: i32, /// A WebAssembly `i64` value. /// @@ -803,7 +803,7 @@ pub union ValRaw { /// or unsigned. The Rust type `i64` is simply chosen for convenience. /// /// This value is always stored in a little-endian format. - pub i64: i64, + i64: i64, /// A WebAssembly `f32` value. /// @@ -813,7 +813,7 @@ pub union ValRaw { /// `u32` value is the return value of `f32::to_bits` in Rust. /// /// This value is always stored in a little-endian format. - pub f32: u32, + f32: u32, /// A WebAssembly `f64` value. /// @@ -823,7 +823,7 @@ pub union ValRaw { /// `u64` value is the return value of `f64::to_bits` in Rust. /// /// This value is always stored in a little-endian format. - pub f64: u64, + f64: u64, /// A WebAssembly `v128` value. /// @@ -833,7 +833,7 @@ pub union ValRaw { /// underlying bits is left up to the instructions which consume this value. /// /// This value is always stored in a little-endian format. - pub v128: u128, + v128: u128, /// A WebAssembly `funcref` value. /// @@ -843,7 +843,7 @@ pub union ValRaw { /// carefully calling the correct functions throughout the runtime. /// /// This value is always stored in a little-endian format. - pub funcref: usize, + funcref: usize, /// A WebAssembly `externref` value. /// @@ -853,7 +853,119 @@ pub union ValRaw { /// carefully calling the correct functions throughout the runtime. /// /// This value is always stored in a little-endian format. - pub externref: usize, + externref: usize, +} + +impl ValRaw { + /// Creates a WebAssembly `i32` value + #[inline] + pub fn i32(i: i32) -> ValRaw { + ValRaw { i32: i.to_le() } + } + + /// Creates a WebAssembly `i64` value + #[inline] + pub fn i64(i: i64) -> ValRaw { + ValRaw { i64: i.to_le() } + } + + /// Creates a WebAssembly `i32` value + #[inline] + pub fn u32(i: u32) -> ValRaw { + ValRaw::i32(i as i32) + } + + /// Creates a WebAssembly `i64` value + #[inline] + pub fn u64(i: u64) -> ValRaw { + ValRaw::i64(i as i64) + } + + /// Creates a WebAssembly `f32` value + #[inline] + pub fn f32(i: u32) -> ValRaw { + ValRaw { f32: i.to_le() } + } + + /// Creates a WebAssembly `f64` value + #[inline] + pub fn f64(i: u64) -> ValRaw { + ValRaw { f64: i.to_le() } + } + + /// Creates a WebAssembly `v128` value + #[inline] + pub fn v128(i: u128) -> ValRaw { + ValRaw { v128: i.to_le() } + } + + /// Creates a WebAssembly `funcref` value + #[inline] + pub fn funcref(i: usize) -> ValRaw { + ValRaw { funcref: i.to_le() } + } + + /// Creates a WebAssembly `externref` value + #[inline] + pub fn externref(i: usize) -> ValRaw { + ValRaw { + externref: i.to_le(), + } + } + + /// Gets the WebAssembly `i32` value + #[inline] + pub fn get_i32(&self) -> i32 { + unsafe { i32::from_le(self.i32) } + } + + /// Gets the WebAssembly `i64` value + #[inline] + pub fn get_i64(&self) -> i64 { + unsafe { i64::from_le(self.i64) } + } + + /// Gets the WebAssembly `i32` value + #[inline] + pub fn get_u32(&self) -> u32 { + self.get_i32() as u32 + } + + /// Gets the WebAssembly `i64` value + #[inline] + pub fn get_u64(&self) -> u64 { + self.get_i64() as u64 + } + + /// Gets the WebAssembly `f32` value + #[inline] + pub fn get_f32(&self) -> u32 { + unsafe { u32::from_le(self.f32) } + } + + /// Gets the WebAssembly `f64` value + #[inline] + pub fn get_f64(&self) -> u64 { + unsafe { u64::from_le(self.f64) } + } + + /// Gets the WebAssembly `v128` value + #[inline] + pub fn get_v128(&self) -> u128 { + unsafe { u128::from_le(self.v128) } + } + + /// Gets the WebAssembly `funcref` value + #[inline] + pub fn get_funcref(&self) -> usize { + unsafe { usize::from_le(self.funcref) } + } + + /// Gets the WebAssembly `externref` value + #[inline] + pub fn get_externref(&self) -> usize { + unsafe { usize::from_le(self.externref) } + } } /// Trampoline function pointer type. diff --git a/crates/wasmtime/src/component/func/typed.rs b/crates/wasmtime/src/component/func/typed.rs index cf3964af7f..ddc0fce9ac 100644 --- a/crates/wasmtime/src/component/func/typed.rs +++ b/crates/wasmtime/src/component/func/typed.rs @@ -239,9 +239,7 @@ where // This comment about 64-bit integers is also referred to below with // "WRITEPTR64". let params_and_results = &mut MaybeUninit::new(ParamsAndResults { - params: ValRaw { - i64: (ptr as i64).to_le(), - }, + params: ValRaw::i64(ptr as i64), }); self.call_raw(store, params_and_results) @@ -727,7 +725,7 @@ unsafe impl ComponentValue for () { // Macro to help generate `ComponentValue` implementations for primitive types // such as integers, char, bool, etc. macro_rules! integers { - ($($primitive:ident = $ty:ident in $field:ident $(as $unsigned:ident)?,)*) => ($( + ($($primitive:ident = $ty:ident in $field:ident/$get:ident,)*) => ($( unsafe impl ComponentValue for $primitive { type Lower = ValRaw; type Lift = $primitive; @@ -745,8 +743,7 @@ macro_rules! integers { _func: &Func, dst: &mut MaybeUninit, ) -> Result<()> { - map_maybe_uninit!(dst.$field) - .write((*self $(as $unsigned)? as $field).to_le()); + dst.write(ValRaw::$field(*self as $field)); Ok(()) } @@ -767,17 +764,11 @@ macro_rules! integers { #[inline] fn lift(src: &Self::Lower) -> Result { - // Convert from little-endian and then view the signed storage - // as an optionally-unsigned type. - let field = unsafe { - $field::from_le(src.$field) $(as $unsigned)? - }; - // Perform a lossless cast from our field storage to the // destination type. Note that `try_from` here is load bearing // which rejects conversions like `500u32` to `u8` because // that's out-of-bounds for `u8`. - Ok($primitive::try_from(field)?) + Ok($primitive::try_from(src.$get())?) } } @@ -792,18 +783,18 @@ macro_rules! integers { } integers! { - i8 = S8 in i32, - u8 = U8 in i32 as u32, - i16 = S16 in i32, - u16 = U16 in i32 as u32, - i32 = S32 in i32, - u32 = U32 in i32 as u32, - i64 = S64 in i64, - u64 = U64 in i64 as u64, + i8 = S8 in i32/get_i32, + u8 = U8 in u32/get_u32, + i16 = S16 in i32/get_i32, + u16 = U16 in u32/get_u32, + i32 = S32 in i32/get_i32, + u32 = U32 in u32/get_u32, + i64 = S64 in i64/get_i64, + u64 = U64 in u64/get_u64, } macro_rules! floats { - ($($float:ident/$storage:ident = $ty:ident)*) => ($(const _: () = { + ($($float:ident/$get_float:ident = $ty:ident)*) => ($(const _: () = { /// All floats in-and-out of the canonical ABI always have their NaN /// payloads canonicalized. Conveniently the `NAN` constant in Rust has /// the same representation as canonical NAN, so we can use that for the @@ -834,8 +825,7 @@ macro_rules! floats { _func: &Func, dst: &mut MaybeUninit, ) -> Result<()> { - map_maybe_uninit!(dst.$float) - .write(canonicalize(*self).to_bits().to_le()); + dst.write(ValRaw::$float(canonicalize(*self).to_bits())); Ok(()) } @@ -855,8 +845,7 @@ macro_rules! floats { #[inline] fn lift(src: &Self::Lower) -> Result { - let field = $storage::from_le(unsafe { src.$float }); - Ok(canonicalize($float::from_bits(field))) + Ok(canonicalize($float::from_bits(src.$get_float()))) } } @@ -874,8 +863,8 @@ macro_rules! floats { } floats! { - f32/u32 = Float32 - f64/u64 = Float64 + f32/get_f32 = Float32 + f64/get_f64 = Float64 } unsafe impl ComponentValue for bool { @@ -895,7 +884,7 @@ unsafe impl ComponentValue for bool { _func: &Func, dst: &mut MaybeUninit, ) -> Result<()> { - map_maybe_uninit!(dst.i32).write((*self as i32).to_le()); + dst.write(ValRaw::i32(*self as i32)); Ok(()) } @@ -916,7 +905,7 @@ unsafe impl ComponentValue for bool { #[inline] fn lift(src: &Self::Lower) -> Result { - match i32::from_le(unsafe { src.i32 }) { + match src.get_i32() { 0 => Ok(false), 1 => Ok(true), _ => bail!("invalid boolean value"), @@ -958,7 +947,7 @@ unsafe impl ComponentValue for char { _func: &Func, dst: &mut MaybeUninit, ) -> Result<()> { - map_maybe_uninit!(dst.i32).write((u32::from(*self) as i32).to_le()); + dst.write(ValRaw::u32(u32::from(*self))); Ok(()) } @@ -979,8 +968,7 @@ unsafe impl ComponentValue for char { #[inline] fn lift(src: &Self::Lower) -> Result { - let bits = i32::from_le(unsafe { src.i32 }) as u32; - Ok(char::try_from(bits)?) + Ok(char::try_from(src.get_u32())?) } } @@ -1018,8 +1006,8 @@ unsafe impl ComponentValue for str { let (ptr, len) = lower_string(&mut Memory::new(store.as_context_mut(), func), self)?; // See "WRITEPTR64" above for why this is always storing a 64-bit // integer. - map_maybe_uninit!(dst[0].i64).write((ptr as i64).to_le()); - map_maybe_uninit!(dst[1].i64).write((len as i64).to_le()); + map_maybe_uninit!(dst[0]).write(ValRaw::i64(ptr as i64)); + map_maybe_uninit!(dst[1]).write(ValRaw::i64(len as i64)); Ok(()) } @@ -1155,8 +1143,8 @@ where let (ptr, len) = lower_list(&mut Memory::new(store.as_context_mut(), func), self)?; // See "WRITEPTR64" above for why this is always storing a 64-bit // integer. - map_maybe_uninit!(dst[0].i64).write((ptr as i64).to_le()); - map_maybe_uninit!(dst[1].i64).write((len as i64).to_le()); + map_maybe_uninit!(dst[0]).write(ValRaw::i64(ptr as i64)); + map_maybe_uninit!(dst[1]).write(ValRaw::i64(len as i64)); Ok(()) } @@ -1311,7 +1299,7 @@ where ) -> Result<()> { match self { None => { - map_maybe_uninit!(dst.A1.i32).write(0_i32.to_le()); + map_maybe_uninit!(dst.A1).write(ValRaw::i32(0)); // Note that this is unsafe as we're writing an arbitrary // bit-pattern to an arbitrary type, but part of the unsafe // contract of the `ComponentValue` trait is that we can assign @@ -1323,7 +1311,7 @@ where } } Some(val) => { - map_maybe_uninit!(dst.A1.i32).write(1_i32.to_le()); + map_maybe_uninit!(dst.A1).write(ValRaw::i32(1)); val.lower(store, func, map_maybe_uninit!(dst.A2))?; } } @@ -1354,7 +1342,7 @@ where } fn lift(src: &Self::Lower) -> Result { - Ok(match i32::from_le(unsafe { src.A1.i32 }) { + Ok(match src.A1.get_i32() { 0 => None, 1 => Some(T::lift(&src.A2)?), _ => bail!("invalid option discriminant"), @@ -1442,11 +1430,11 @@ where match self { Ok(e) => { - map_maybe_uninit!(dst.tag.i32).write(0_i32.to_le()); + map_maybe_uninit!(dst.tag).write(ValRaw::i32(0)); e.lower(store, func, map_maybe_uninit!(dst.payload.ok))?; } Err(e) => { - map_maybe_uninit!(dst.tag.i32).write(1_i32.to_le()); + map_maybe_uninit!(dst.tag).write(ValRaw::i32(1)); e.lower(store, func, map_maybe_uninit!(dst.payload.err))?; } } @@ -1492,7 +1480,7 @@ where // being used then both `T` and `E` have zero size. assert!(mem::size_of_val(&src.payload) == 0); - Ok(match i32::from_le(unsafe { src.tag.i32 }) { + Ok(match src.tag.get_i32() { 0 => Ok(unsafe { T::lift(&src.payload.ok)? }), 1 => Err(unsafe { E::lift(&src.payload.err)? }), _ => bail!("invalid expected discriminant"), @@ -1778,7 +1766,7 @@ unsafe impl ComponentReturn for Value { fn lift(store: &StoreOpaque, func: &Func, src: &Self::Lower) -> Result { // FIXME: needs to read an i64 for memory64 - let ptr = u32::from_le(unsafe { src.i32 as u32 }) as usize; + let ptr = src.get_u32() as usize; Value::new(store, func, ptr) } } diff --git a/crates/wasmtime/src/func.rs b/crates/wasmtime/src/func.rs index 3c8463f6a7..1b019c2310 100644 --- a/crates/wasmtime/src/func.rs +++ b/crates/wasmtime/src/func.rs @@ -966,7 +966,7 @@ impl Func { // Store the argument values into `values_vec`. let mut values_vec = store.0.take_wasm_val_raw_storage(); debug_assert!(values_vec.is_empty()); - values_vec.resize_with(values_vec_size, || ValRaw { i32: 0 }); + values_vec.resize_with(values_vec_size, || ValRaw::i32(0)); for (arg, slot) in params.iter().cloned().zip(&mut values_vec) { unsafe { *slot = arg.to_raw(&mut *store); diff --git a/crates/wasmtime/src/func/typed.rs b/crates/wasmtime/src/func/typed.rs index 9cdd3309b2..d16d3e1a42 100644 --- a/crates/wasmtime/src/func/typed.rs +++ b/crates/wasmtime/src/func/typed.rs @@ -231,7 +231,7 @@ pub unsafe trait WasmTy: Send { } macro_rules! integers { - ($($primitive:ident => $ty:ident in $raw:ident)*) => ($( + ($($primitive:ident/$get_primitive:ident => $ty:ident in $raw:ident)*) => ($( unsafe impl WasmTy for $primitive { type Abi = $primitive; #[inline] @@ -248,11 +248,11 @@ macro_rules! integers { } #[inline] unsafe fn abi_from_raw(raw: *mut ValRaw) -> $primitive { - $primitive::from_le((*raw).$raw as $primitive) + (*raw).$get_primitive() } #[inline] unsafe fn abi_into_raw(abi: $primitive, raw: *mut ValRaw) { - (*raw).$raw = abi.to_le() as $raw; + *raw = ValRaw::$primitive(abi); } #[inline] fn into_abi(self, _store: &mut StoreOpaque) -> Self::Abi { @@ -267,14 +267,14 @@ macro_rules! integers { } integers! { - i32 => I32 in i32 - i64 => I64 in i64 - u32 => I32 in i32 - u64 => I64 in i64 + i32/get_i32 => I32 in i32 + i64/get_i64 => I64 in i64 + u32/get_u32 => I32 in i32 + u64/get_u64 => I64 in i64 } macro_rules! floats { - ($($float:ident/$int:ident => $ty:ident)*) => ($( + ($($float:ident/$int:ident/$get_float:ident => $ty:ident)*) => ($( unsafe impl WasmTy for $float { type Abi = $float; #[inline] @@ -291,11 +291,11 @@ macro_rules! floats { } #[inline] unsafe fn abi_from_raw(raw: *mut ValRaw) -> $float { - $float::from_bits($int::from_le((*raw).$float)) + $float::from_bits((*raw).$get_float()) } #[inline] unsafe fn abi_into_raw(abi: $float, raw: *mut ValRaw) { - (*raw).$float = abi.to_bits().to_le(); + *raw = ValRaw::$float(abi.to_bits()); } #[inline] fn into_abi(self, _store: &mut StoreOpaque) -> Self::Abi { @@ -310,8 +310,8 @@ macro_rules! floats { } floats! { - f32/u32 => F32 - f64/u64 => F64 + f32/u32/get_f32 => F32 + f64/u64/get_f64 => F64 } unsafe impl WasmTy for Option { @@ -334,12 +334,12 @@ unsafe impl WasmTy for Option { #[inline] unsafe fn abi_from_raw(raw: *mut ValRaw) -> *mut u8 { - usize::from_le((*raw).externref) as *mut u8 + (*raw).get_externref() as *mut u8 } #[inline] unsafe fn abi_into_raw(abi: *mut u8, raw: *mut ValRaw) { - (*raw).externref = (abi as usize).to_le(); + *raw = ValRaw::externref(abi as usize); } #[inline] @@ -420,12 +420,12 @@ unsafe impl WasmTy for Option { #[inline] unsafe fn abi_from_raw(raw: *mut ValRaw) -> Self::Abi { - usize::from_le((*raw).funcref) as Self::Abi + (*raw).get_funcref() as Self::Abi } #[inline] unsafe fn abi_into_raw(abi: Self::Abi, raw: *mut ValRaw) { - (*raw).funcref = (abi as usize).to_le(); + *raw = ValRaw::funcref(abi as usize); } #[inline] diff --git a/crates/wasmtime/src/values.rs b/crates/wasmtime/src/values.rs index 45d1b10bc1..377c309249 100644 --- a/crates/wasmtime/src/values.rs +++ b/crates/wasmtime/src/values.rs @@ -103,28 +103,24 @@ impl Val { /// [`Func::to_raw`] are unsafe. pub unsafe fn to_raw(&self, store: impl AsContextMut) -> ValRaw { match self { - Val::I32(i) => ValRaw { i32: i.to_le() }, - Val::I64(i) => ValRaw { i64: i.to_le() }, - Val::F32(u) => ValRaw { f32: u.to_le() }, - Val::F64(u) => ValRaw { f64: u.to_le() }, - Val::V128(b) => ValRaw { v128: b.to_le() }, + Val::I32(i) => ValRaw::i32(*i), + Val::I64(i) => ValRaw::i64(*i), + Val::F32(u) => ValRaw::f32(*u), + Val::F64(u) => ValRaw::f64(*u), + Val::V128(b) => ValRaw::v128(*b), Val::ExternRef(e) => { let externref = match e { Some(e) => e.to_raw(store), None => 0, }; - ValRaw { - externref: externref.to_le(), - } + ValRaw::externref(externref) } Val::FuncRef(f) => { let funcref = match f { Some(f) => f.to_raw(store), None => 0, }; - ValRaw { - funcref: funcref.to_le(), - } + ValRaw::funcref(funcref) } } } @@ -138,15 +134,13 @@ impl Val { /// otherwise that `raw` should have the type `ty` specified. pub unsafe fn from_raw(store: impl AsContextMut, raw: ValRaw, ty: ValType) -> Val { match ty { - ValType::I32 => Val::I32(i32::from_le(raw.i32)), - ValType::I64 => Val::I64(i64::from_le(raw.i64)), - ValType::F32 => Val::F32(u32::from_le(raw.f32)), - ValType::F64 => Val::F64(u64::from_le(raw.f64)), - ValType::V128 => Val::V128(u128::from_le(raw.v128)), - ValType::ExternRef => { - Val::ExternRef(ExternRef::from_raw(usize::from_le(raw.externref))) - } - ValType::FuncRef => Val::FuncRef(Func::from_raw(store, usize::from_le(raw.funcref))), + ValType::I32 => Val::I32(raw.get_i32()), + ValType::I64 => Val::I64(raw.get_i64()), + ValType::F32 => Val::F32(raw.get_f32()), + ValType::F64 => Val::F64(raw.get_f64()), + ValType::V128 => Val::V128(raw.get_v128()), + ValType::ExternRef => Val::ExternRef(ExternRef::from_raw(raw.get_externref())), + ValType::FuncRef => Val::FuncRef(Func::from_raw(store, raw.get_funcref())), } } diff --git a/tests/all/call_hook.rs b/tests/all/call_hook.rs index 927e8eaae9..9fed8aa53f 100644 --- a/tests/all/call_hook.rs +++ b/tests/all/call_hook.rs @@ -52,10 +52,10 @@ fn call_wrapped_func() -> Result<(), Error> { |caller: Caller, space| { verify(caller.data()); - assert_eq!((*space.add(0)).i32, 1i32.to_le()); - assert_eq!((*space.add(1)).i64, 2i64.to_le()); - assert_eq!((*space.add(2)).f32, 3.0f32.to_bits().to_le()); - assert_eq!((*space.add(3)).f64, 4.0f64.to_bits().to_le()); + assert_eq!((*space.add(0)).get_i32(), 1i32); + assert_eq!((*space.add(1)).get_i64(), 2i64); + assert_eq!((*space.add(2)).get_f32(), 3.0f32.to_bits()); + assert_eq!((*space.add(3)).get_f64(), 4.0f64.to_bits()); Ok(()) }, )