diff --git a/crates/component-macro/src/lib.rs b/crates/component-macro/src/lib.rs index afef164778..9c801a270f 100644 --- a/crates/component-macro/src/lib.rs +++ b/crates/component-macro/src/lib.rs @@ -618,7 +618,7 @@ impl Expander for LowerExpander { if ty.is_some() { pattern = quote!(Self::#ident(value)); - lower = quote!(value.lower(store, options, #internal::map_maybe_uninit!(dst.payload.#ident))); + lower = quote!(value.lower(store, options, dst)); store = quote!(value.store( memory, offset + ::PAYLOAD_OFFSET32, @@ -630,8 +630,14 @@ impl Expander for LowerExpander { } lowers.extend(quote!(#pattern => { - #internal::map_maybe_uninit!(dst.tag).write(wasmtime::ValRaw::i32(#index_u32 as i32)); - #lower + #internal::map_maybe_uninit!(dst.tag).write(wasmtime::ValRaw::u32(#index_u32)); + unsafe { + #internal::lower_payload( + #internal::map_maybe_uninit!(dst.payload), + |payload| #internal::map_maybe_uninit!(payload.#ident), + |dst| #lower, + ) + } })); stores.extend(quote!(#pattern => { @@ -652,13 +658,6 @@ impl Expander for LowerExpander { options: &#internal::Options, dst: &mut std::mem::MaybeUninit, ) -> #internal::anyhow::Result<()> { - // See comment in as Lower>::lower for why we zero out the payload here - unsafe { - #internal::map_maybe_uninit!(dst.payload) - .as_mut_ptr() - .write_bytes(0u8, 1); - } - match self { #lowers } @@ -791,13 +790,10 @@ impl Expander for ComponentTypeExpander { } VariantStyle::Enum => quote!(#name,), }); + lower_payload_case_declarations.extend(quote!(#ident: [wasmtime::ValRaw; 0],)); } } - if lower_payload_case_declarations.is_empty() { - lower_payload_case_declarations.extend(quote!(_dummy: ())); - } - let typecheck = match style { VariantStyle::Variant => quote!(typecheck_variant), VariantStyle::Union => quote!(typecheck_union), diff --git a/crates/environ/src/fact/trampoline.rs b/crates/environ/src/fact/trampoline.rs index bedc8d5781..a058ebeb98 100644 --- a/crates/environ/src/fact/trampoline.rs +++ b/crates/environ/src/fact/trampoline.rs @@ -2485,10 +2485,13 @@ impl Compiler<'_, '_> { | (ValType::F64, ValType::F64) => {} (ValType::I32, ValType::F32) => self.instruction(F32ReinterpretI32), - (ValType::I64, ValType::I32) => self.instruction(I32WrapI64), + (ValType::I64, ValType::I32) => { + self.assert_i64_upper_bits_not_set(idx); + self.instruction(I32WrapI64); + } (ValType::I64, ValType::F64) => self.instruction(F64ReinterpretI64), - (ValType::F64, ValType::F32) => self.instruction(F32DemoteF64), (ValType::I64, ValType::F32) => { + self.assert_i64_upper_bits_not_set(idx); self.instruction(I32WrapI64); self.instruction(F32ReinterpretI32); } @@ -2501,6 +2504,7 @@ impl Compiler<'_, '_> { | (ValType::F32, ValType::F64) | (ValType::F64, ValType::I32) | (ValType::F64, ValType::I64) + | (ValType::F64, ValType::F32) // not used in the component model | (ValType::ExternRef, _) @@ -2514,6 +2518,19 @@ impl Compiler<'_, '_> { } } + fn assert_i64_upper_bits_not_set(&mut self, local: u32) { + if !self.module.debug { + return; + } + self.instruction(LocalGet(local)); + self.instruction(I64Const(32)); + self.instruction(I64ShrU); + self.instruction(I32WrapI64); + self.instruction(If(BlockType::Empty)); + self.trap(Trap::AssertFailed("upper bits are unexpectedly set")); + self.instruction(End); + } + /// Converts the top value on the WebAssembly stack which has type /// `src_ty` to `dst_tys[0]`. /// @@ -2531,7 +2548,6 @@ impl Compiler<'_, '_> { (ValType::F32, ValType::I32) => self.instruction(I32ReinterpretF32), (ValType::I32, ValType::I64) => self.instruction(I64ExtendI32U), (ValType::F64, ValType::I64) => self.instruction(I64ReinterpretF64), - (ValType::F32, ValType::F64) => self.instruction(F64PromoteF32), (ValType::F32, ValType::I64) => { self.instruction(I32ReinterpretF32); self.instruction(I64ExtendI32U); @@ -2545,6 +2561,7 @@ impl Compiler<'_, '_> { | (ValType::F64, ValType::F32) | (ValType::I32, ValType::F64) | (ValType::I64, ValType::F64) + | (ValType::F32, ValType::F64) // not used in the component model | (ValType::ExternRef, _) diff --git a/crates/fuzzing/src/generators/component_types.rs b/crates/fuzzing/src/generators/component_types.rs index bd3c64755c..1e78645a9d 100644 --- a/crates/fuzzing/src/generators/component_types.rs +++ b/crates/fuzzing/src/generators/component_types.rs @@ -141,6 +141,7 @@ macro_rules! define_static_api_test { let mut config = Config::new(); config.wasm_component_model(true); + config.debug_adapter_modules(input.arbitrary()?); let engine = Engine::new(&config).unwrap(); let wat = declarations.make_component(); let wat = wat.as_bytes(); diff --git a/crates/fuzzing/src/oracles.rs b/crates/fuzzing/src/oracles.rs index bdaca94ae5..0bf6617495 100644 --- a/crates/fuzzing/src/oracles.rs +++ b/crates/fuzzing/src/oracles.rs @@ -1087,7 +1087,9 @@ pub fn dynamic_component_api_target(input: &mut arbitrary::Unstructured) -> arbi let case = input.arbitrary::()?; - let engine = component_test_util::engine(); + let mut config = component_test_util::config(); + config.debug_adapter_modules(input.arbitrary()?); + let engine = Engine::new(&config).unwrap(); let mut store = Store::new(&engine, (Box::new([]) as Box<[Val]>, None)); let wat = case.declarations().make_component(); let wat = wat.as_bytes(); diff --git a/crates/runtime/src/vmcontext.rs b/crates/runtime/src/vmcontext.rs index c2ee49a9b7..ae15342ea4 100644 --- a/crates/runtime/src/vmcontext.rs +++ b/crates/runtime/src/vmcontext.rs @@ -983,7 +983,13 @@ impl ValRaw { /// Creates a WebAssembly `i32` value #[inline] pub fn i32(i: i32) -> ValRaw { - ValRaw { i32: i.to_le() } + // Note that this is intentionally not setting the `i32` field, instead + // setting the `i64` field with a zero-extended version of `i`. For more + // information on this see the comments on `Lower for Result` in the + // `wasmtime` crate. Otherwise though all `ValRaw` constructors are + // otherwise constrained to guarantee that the initial 64-bits are + // always initialized. + ValRaw::u64((i as u32).into()) } /// Creates a WebAssembly `i64` value @@ -995,7 +1001,9 @@ impl ValRaw { /// Creates a WebAssembly `i32` value #[inline] pub fn u32(i: u32) -> ValRaw { - ValRaw::i32(i as i32) + // See comments in `ValRaw::i32` for why this is setting the upper + // 32-bits as well. + ValRaw::u64(i.into()) } /// Creates a WebAssembly `i64` value @@ -1007,7 +1015,9 @@ impl ValRaw { /// Creates a WebAssembly `f32` value #[inline] pub fn f32(i: u32) -> ValRaw { - ValRaw { f32: i.to_le() } + // See comments in `ValRaw::i32` for why this is setting the upper + // 32-bits as well. + ValRaw::u64(i.into()) } /// Creates a WebAssembly `f64` value diff --git a/crates/wasmtime/src/component/func.rs b/crates/wasmtime/src/component/func.rs index fceab79a96..ce00716377 100644 --- a/crates/wasmtime/src/component/func.rs +++ b/crates/wasmtime/src/component/func.rs @@ -1,4 +1,5 @@ use crate::component::instance::{Instance, InstanceData}; +use crate::component::storage::storage_as_slice; use crate::component::types::Type; use crate::component::values::Val; use crate::store::{StoreOpaque, Stored}; @@ -317,8 +318,6 @@ impl Func { if param_abi.flat_count(MAX_FLAT_PARAMS).is_none() { self.store_args(store, &options, ¶m_abi, ¶ms, args, dst) } else { - dst.write([ValRaw::u64(0); MAX_FLAT_PARAMS]); - let dst = &mut unsafe { mem::transmute::<_, &mut [MaybeUninit; MAX_FLAT_PARAMS]>(dst) } @@ -443,7 +442,7 @@ impl Func { // later get used in post-return. flags.set_needs_post_return(true); let val = lift(store.0, &options, ret)?; - let ret_slice = cast_storage(ret); + let ret_slice = storage_as_slice(ret); let data = &mut store.0[self.0]; assert!(data.post_return_arg.is_none()); match ret_slice.len() { @@ -453,16 +452,6 @@ impl Func { } return Ok(val); } - - unsafe fn cast_storage(storage: &T) -> &[ValRaw] { - assert!(std::mem::size_of_val(storage) % std::mem::size_of::() == 0); - assert!(std::mem::align_of_val(storage) == std::mem::align_of::()); - - std::slice::from_raw_parts( - (storage as *const T).cast(), - mem::size_of_val(storage) / mem::size_of::(), - ) - } } /// Invokes the `post-return` canonical ABI option, if specified, after a diff --git a/crates/wasmtime/src/component/func/host.rs b/crates/wasmtime/src/component/func/host.rs index 1d6b5d5c20..b7301e2698 100644 --- a/crates/wasmtime/src/component/func/host.rs +++ b/crates/wasmtime/src/component/func/host.rs @@ -1,4 +1,5 @@ use crate::component::func::{Memory, MemoryMut, Options}; +use crate::component::storage::slice_to_storage_mut; use crate::component::{ComponentParams, ComponentType, Lift, Lower, Type, Val}; use crate::{AsContextMut, StoreContextMut, ValRaw}; use anyhow::{anyhow, bail, Context, Result}; @@ -197,7 +198,7 @@ where // There's a 2x2 matrix of whether parameters and results are stored on the // stack or on the heap. Each of the 4 branches here have a different // representation of the storage of arguments/returns which is represented - // by the type parameter that we pass to `cast_storage`. + // by the type parameter that we pass to `slice_to_storage_mut`. // // Also note that while four branches are listed here only one is taken for // any particular `Params` and `Return` combination. This should be @@ -206,13 +207,15 @@ where // branch, but today is not that day. if Params::flatten_count() <= MAX_FLAT_PARAMS { if Return::flatten_count() <= MAX_FLAT_RESULTS { - let storage = cast_storage::>(storage); + let storage = + slice_to_storage_mut::>(storage); let params = Params::lift(cx.0, &options, &storage.assume_init_ref().args)?; let ret = closure(cx.as_context_mut(), params)?; flags.set_may_leave(false); ret.lower(&mut cx, &options, map_maybe_uninit!(storage.ret))?; } else { - let storage = cast_storage::>(storage).assume_init_ref(); + let storage = + slice_to_storage_mut::>(storage).assume_init_ref(); let params = Params::lift(cx.0, &options, &storage.args)?; let ret = closure(cx.as_context_mut(), params)?; let mut memory = MemoryMut::new(cx.as_context_mut(), &options); @@ -223,7 +226,7 @@ where } else { let memory = Memory::new(cx.0, &options); if Return::flatten_count() <= MAX_FLAT_RESULTS { - let storage = cast_storage::>(storage); + let storage = slice_to_storage_mut::>(storage); let ptr = validate_inbounds::(memory.as_slice(), &storage.assume_init_ref().args)?; let params = Params::load(&memory, &memory.as_slice()[ptr..][..Params::SIZE32])?; @@ -231,7 +234,7 @@ where flags.set_may_leave(false); ret.lower(&mut cx, &options, map_maybe_uninit!(storage.ret))?; } else { - let storage = cast_storage::>(storage).assume_init_ref(); + let storage = slice_to_storage_mut::>(storage).assume_init_ref(); let ptr = validate_inbounds::(memory.as_slice(), &storage.args)?; let params = Params::load(&memory, &memory.as_slice()[ptr..][..Params::SIZE32])?; let ret = closure(cx.as_context_mut(), params)?; @@ -263,22 +266,6 @@ fn validate_inbounds(memory: &[u8], ptr: &ValRaw) -> Result(storage: &mut [ValRaw]) -> &mut MaybeUninit { - // Assertions that LLVM can easily optimize away but are sanity checks here - assert!(std::mem::size_of::() % std::mem::size_of::() == 0); - assert!(std::mem::align_of::() == std::mem::align_of::()); - assert!(std::mem::align_of_val(storage) == std::mem::align_of::()); - - // This is an actual runtime assertion which if performance calls for we may - // need to relax to a debug assertion. This notably tries to ensure that we - // stay within the bounds of the number of actual values given rather than - // reading past the end of an array. This shouldn't actually trip unless - // there's a bug in Wasmtime though. - assert!(std::mem::size_of_val(storage) >= std::mem::size_of::()); - - &mut *storage.as_mut_ptr().cast() -} - unsafe fn handle_result(func: impl FnOnce() -> Result<()>) { match panic::catch_unwind(AssertUnwindSafe(func)) { Ok(Ok(())) => {} diff --git a/crates/wasmtime/src/component/func/typed.rs b/crates/wasmtime/src/component/func/typed.rs index 6a84627834..ae7019e881 100644 --- a/crates/wasmtime/src/component/func/typed.rs +++ b/crates/wasmtime/src/component/func/typed.rs @@ -1,4 +1,5 @@ use crate::component::func::{Func, Memory, MemoryMut, Options}; +use crate::component::storage::{storage_as_slice, storage_as_slice_mut}; use crate::store::StoreOpaque; use crate::{AsContext, AsContextMut, StoreContext, StoreContextMut, ValRaw}; use anyhow::{anyhow, bail, Context, Result}; @@ -1681,6 +1682,34 @@ where } } +/// Lowers the payload of a variant into the storage for the entire payload, +/// handling writing zeros at the end of the representation if this payload is +/// smaller than the entire flat representation. +/// +/// * `payload` - the flat storage space for the entire payload of the variant +/// * `typed_payload` - projection from the payload storage space to the +/// individaul storage space for this variant. +/// * `lower` - lowering operation used to initialize the `typed_payload` return +/// value. +/// +/// For more information on this se the comments in the `Lower for Result` +/// implementation below. +pub unsafe fn lower_payload( + payload: &mut MaybeUninit

, + typed_payload: impl FnOnce(&mut MaybeUninit

) -> &mut MaybeUninit, + lower: impl FnOnce(&mut MaybeUninit) -> Result<()>, +) -> Result<()> { + let typed = typed_payload(payload); + lower(typed)?; + + let typed_len = storage_as_slice(typed).len(); + let payload = storage_as_slice_mut(payload); + for slot in payload[typed_len..].iter_mut() { + *slot = ValRaw::u64(0); + } + Ok(()) +} + unsafe impl ComponentVariant for Result where T: ComponentType, @@ -1700,35 +1729,89 @@ where options: &Options, dst: &mut MaybeUninit, ) -> Result<()> { - // Start out by zeroing out the payload. This will ensure that if either - // arm doesn't initialize some values then everything is still - // deterministically set. + // This implementation of `Lower::lower`, if you're reading these from + // the top of this file, is the first location that the "join" logic of + // the component model's canonical ABI encountered. The rough problem is + // that let's say we have a component model type of the form: // - // Additionally, this initialization of zero means that the specific - // types written by each `lower` call below on each arm still has the - // correct value even when "joined" with the other arm. + // (result u64 (error (tuple f32 u16))) // - // Finally note that this is required by the canonical ABI to some - // degree where if the `Ok` arm initializes fewer values than the `Err` - // arm then all the remaining values must be initialized to zero, and - // that's what this does. - unsafe { - map_maybe_uninit!(dst.payload) - .as_mut_ptr() - .write_bytes(0u8, 1); - } - + // The flat representation of this is actually pretty tricky. Currently + // it is: + // + // i32 i64 i32 + // + // The first `i32` is the discriminant for the `result`, and the payload + // is represented by `i64 i32`. The "ok" variant will only use the `i64` + // and the "err" variant will use both `i64` and `i32`. + // + // In the "ok" variant the first issue is encountered. The size of one + // variant may not match the size of the other variants. All variants + // start at the "front" but when lowering a type we need to be sure to + // initialize the later variants (lest we leak random host memory into + // the guest module). Due to how the `Lower` type is represented as a + // `union` of all the variants what ends up happening here is that + // internally within the `lower_payload` after the typed payload is + // lowered the remaining bits of the payload that weren't initialized + // are all set to zero. This will guarantee that we'll write to all the + // slots for each variant. + // + // The "err" variant encounters the second issue, however, which is that + // the flat representation for each type may differ between payloads. In + // the "ok" arm an `i64` is written, but the `lower` implementation for + // the "err" arm will write an `f32` and then an `i32`. For this + // implementation of `lower` to be valid the `f32` needs to get inflated + // to an `i64` with zero-padding in the upper bits. What may be + // surprising, however, is that none of this is handled in this file. + // This implementation looks like it's blindly deferring to `E::lower` + // and hoping it does the right thing. + // + // In reality, however, the correctness of variant lowering relies on + // two subtle details of the `ValRaw` implementation in Wasmtime: + // + // 1. First the `ValRaw` value always contains little-endian values. + // This means that if a `u32` is written, a `u64` is read, and then + // the `u64` has its upper bits truncated the original value will + // always be retained. This is primarily here for big-endian + // platforms where if it weren't little endian then the opposite + // would occur and the wrong value would be read. + // + // 2. Second, and perhaps even more subtly, the `ValRaw` constructors + // for 32-bit types actually always initialize 64-bits of the + // `ValRaw`. In the component model flat ABI only 32 and 64-bit types + // are used so 64-bits is big enough to contain everything. This + // means that when a `ValRaw` is written into the destination it will + // always, whether it's needed or not, be "ready" to get extended up + // to 64-bits. + // + // Put together these two subtle guarantees means that all `Lower` + // implementations can be written "naturally" as one might naively + // expect. Variants will, on each arm, zero out remaining fields and all + // writes to the flat representation will automatically be 64-bit writes + // meaning that if the value is read as a 64-bit value, which isn't + // known at the time of the write, it'll still be correct. match self { Ok(e) => { map_maybe_uninit!(dst.tag).write(ValRaw::i32(0)); - e.lower(store, options, map_maybe_uninit!(dst.payload.ok))?; + unsafe { + lower_payload( + map_maybe_uninit!(dst.payload), + |payload| map_maybe_uninit!(payload.ok), + |dst| e.lower(store, options, dst), + ) + } } Err(e) => { map_maybe_uninit!(dst.tag).write(ValRaw::i32(1)); - e.lower(store, options, map_maybe_uninit!(dst.payload.err))?; + unsafe { + lower_payload( + map_maybe_uninit!(dst.payload), + |payload| map_maybe_uninit!(payload.err), + |dst| e.lower(store, options, dst), + ) + } } } - Ok(()) } fn store(&self, mem: &mut MemoryMut<'_, U>, offset: usize) -> Result<()> { diff --git a/crates/wasmtime/src/component/mod.rs b/crates/wasmtime/src/component/mod.rs index 3b0924dbed..bb9f3923b6 100644 --- a/crates/wasmtime/src/component/mod.rs +++ b/crates/wasmtime/src/component/mod.rs @@ -8,6 +8,7 @@ mod func; mod instance; mod linker; mod matching; +mod storage; mod store; pub mod types; mod values; @@ -28,8 +29,9 @@ pub use wasmtime_component_macro::{flags, ComponentType, Lift, Lower}; #[doc(hidden)] pub mod __internal { pub use super::func::{ - format_flags, typecheck_enum, typecheck_flags, typecheck_record, typecheck_union, - typecheck_variant, ComponentVariant, MaybeUninitExt, Memory, MemoryMut, Options, + format_flags, lower_payload, typecheck_enum, typecheck_flags, typecheck_record, + typecheck_union, typecheck_variant, ComponentVariant, MaybeUninitExt, Memory, MemoryMut, + Options, }; pub use crate::map_maybe_uninit; pub use crate::store::StoreOpaque; diff --git a/crates/wasmtime/src/component/storage.rs b/crates/wasmtime/src/component/storage.rs new file mode 100644 index 0000000000..4928bdb2bf --- /dev/null +++ b/crates/wasmtime/src/component/storage.rs @@ -0,0 +1,43 @@ +use crate::ValRaw; +use std::mem::{self, MaybeUninit}; +use std::slice; + +fn assert_raw_slice_compat() { + assert!(mem::size_of::() % mem::size_of::() == 0); + assert!(mem::align_of::() == mem::align_of::()); +} + +/// Converts a `::Lower` representation to a slice of +/// `ValRaw`. +pub unsafe fn storage_as_slice(storage: &T) -> &[ValRaw] { + assert_raw_slice_compat::(); + + slice::from_raw_parts( + (storage as *const T).cast(), + mem::size_of_val(storage) / mem::size_of::(), + ) +} + +/// Same as `storage_as_slice`, but mutable. +pub unsafe fn storage_as_slice_mut(storage: &mut T) -> &mut [ValRaw] { + assert_raw_slice_compat::(); + + slice::from_raw_parts_mut( + (storage as *mut T).cast(), + mem::size_of_val(storage) / mem::size_of::(), + ) +} + +/// Same as `storage_as_slice`, but in reverse and mutable. +pub unsafe fn slice_to_storage_mut(slice: &mut [ValRaw]) -> &mut MaybeUninit { + assert_raw_slice_compat::(); + + // This is an actual runtime assertion which if performance calls for we may + // need to relax to a debug assertion. This notably tries to ensure that we + // stay within the bounds of the number of actual values given rather than + // reading past the end of an array. This shouldn't actually trip unless + // there's a bug in Wasmtime though. + assert!(mem::size_of_val(slice) >= mem::size_of::()); + + &mut *slice.as_mut_ptr().cast() +} diff --git a/crates/wasmtime/src/component/values.rs b/crates/wasmtime/src/component/values.rs index fefffcbfe2..adfe6bc53d 100644 --- a/crates/wasmtime/src/component/values.rs +++ b/crates/wasmtime/src/component/values.rs @@ -875,10 +875,14 @@ impl Val { }) => { next_mut(dst).write(ValRaw::u32(*discriminant)); value.lower(store, options, dst)?; + + // For the remaining lowered representation of this variant that + // the payload didn't write we write out zeros here to ensure + // the entire variant is written. let value_flat = value.ty().canonical_abi().flat_count(usize::MAX).unwrap(); let variant_flat = self.ty().canonical_abi().flat_count(usize::MAX).unwrap(); for _ in (1 + value_flat)..variant_flat { - next_mut(dst).write(ValRaw::u32(0)); + next_mut(dst).write(ValRaw::u64(0)); } } Val::Enum(Enum { discriminant, .. }) => {