Conversation
6b6f675 to
cd2282b
Compare
Subscribe to Label Actioncc @fitzgen DetailsThis issue or pull request has been labeled: "wasmtime:api", "wasmtime:c-api", "wasmtime:config", "wizer"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
Label Messager: wasmtime:configIt looks like you are changing Wasmtime's configuration options. Make sure to
DetailsTo modify this label's message, edit the To add new label messages or remove existing label messages, edit the |
|
On a skim this looks like it's all in the right direction, thanks! As as a heads up the wasm-tools deps will be updated in #12254 which'll avoid the need for git deps. I'll take a closer look once this is further along in CI passing tests |
db45a3a to
83dd7a7
Compare
Subscribe to Label Actioncc @fitzgen DetailsThis issue or pull request has been labeled: "fuzzing"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
83dd7a7 to
b9ca740
Compare
alexcrichton
left a comment
There was a problem hiding this comment.
This is looking quite good to me, thanks for the thorough tests!
I haven't scrutinized the trampoline generation nor the lifting/lowering yet, but I can do that once the tests added here are passing (the #[ignore] ones at least).
If you can one thing I'd also recommend is modeling as many tests as possible as a *.wast test since that's generally the easiest to run and share (albeit difficult to write and debug)
301e19b to
d291adf
Compare
ddac63a to
b6376cb
Compare
alexcrichton
left a comment
There was a problem hiding this comment.
Mostly some thoughts about deduplication/sharing of the gnarliest bits below --
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…model This commit introduces the Map and MapEntry classes, enabling the representation of map values in the component model. The Map class allows for the creation and iteration of key/value pairs, enhancing the functionality of the wasmtime component API. Additionally, the .gitignore file is updated to exclude build artifacts from the crates/c-api directory.
This commit introduces support for HashMap<K, V> in the component model, allowing maps to be represented as list<tuple<K, V>> in the canonical ABI. It includes implementations for the ComponentType, Lower, and Lift traits for HashMap, enabling type checking, lowering to flat representations, and lifting from memory. Additionally, the maximum depth for type generation in the fuzzing utility is updated to accommodate the new map type.
This commit removes the previous wasm features configuration and adds new functions for creating a map-configured engine. The `map_config` and `map_engine` functions are introduced to facilitate the use of the component model with maps in tests, ensuring that the engine is properly configured for map types in the component model.
…existing tests This commit introduces a new WAST test file specifically for testing various map types in the component model. Additionally, it removes the redundant map type definitions from the existing types.wast file to streamline the test suite.
…ved iteration and memory management. Introduce new implementations for ComponentType, Lower, and Lift traits for std::collections::HashMap, enhancing support for map types in the component model.
b6376cb to
e644b8b
Compare
The translate_map function had two categories of bugs preventing map adapter trampolines from working: 1. Wasm stack discipline: local_set_new_tmp emits LocalSet which pops from the stack, but was called when the stack was empty (to "pre-allocate" locals). Fixed by computing values first, then calling local_set_new_tmp to consume them—matching translate_list's pattern. Also removed an erroneous LocalTee that left an orphan value on the stack. Affected: src_byte_len, dst_byte_len, cur_src_ptr, cur_dst_ptr. 2. Pointer advancement: after value translation, the pointer still points at the value start. The code only advanced by trailing padding instead of value_size + trailing_padding, causing every loop iteration to re-read the same memory. Also fixes entry layout to use proper record alignment rules (entry align = max(key_align, value_align), value at aligned offset).
Val::Map already holds Vec<(Val, Val)> which derefs to &[(Val, Val)], matching lower_map's signature directly. The intermediate Vec allocation and deep clone of every key/value pair was redundant.
|
@alexcrichton addressed the comments, about the sharing stuff, feedback welcome there, I am not sure if it is the best way or not; not sure what you had in mind |
| let tuple_alignment = key_abi.align32.max(value_abi.align32); | ||
| let tuple_size = usize::try_from(offset).unwrap(); | ||
| let tuple_size = (tuple_size + usize::try_from(tuple_alignment)? - 1) | ||
| & !(usize::try_from(tuple_alignment)? - 1); |
There was a problem hiding this comment.
Reading over crates/environ/src/component/types.rs again, I think it would be best to store the "tuple ABI" inside of the TypeMap structure. That way it wouldn't need to be caluclated at runtime either here or in the typed.rs case and it would only be calculated in one location.
There was a problem hiding this comment.
That might also be a good location to store a value_offset: u32 value which is the offset from the start of the tuple to the value, so it also wouldn't need to be recomputed here for example.
| #[cfg(feature = "std")] | ||
| unsafe impl<K, V> Lift for std::collections::HashMap<K, V> | ||
| where | ||
| K: Lift + Eq + Hash, | ||
| V: Lift, | ||
| { | ||
| fn linear_lift_from_flat( | ||
| cx: &mut LiftContext<'_>, | ||
| ty: InterfaceType, | ||
| src: &Self::Lower, | ||
| ) -> Result<Self> { | ||
| let (key_ty, value_ty) = match ty { | ||
| InterfaceType::Map(i) => { | ||
| let m = &cx.types[i]; | ||
| (m.key, m.value) | ||
| } | ||
| _ => bad_type_info(), | ||
| }; | ||
| // FIXME(#4311): needs memory64 treatment | ||
| let ptr = src[0].get_u32(); | ||
| let len = src[1].get_u32(); | ||
| let (ptr, len) = (usize::try_from(ptr)?, usize::try_from(len)?); | ||
| lift_std_map(cx, key_ty, value_ty, ptr, len) | ||
| } | ||
|
|
||
| fn linear_lift_from_memory( | ||
| cx: &mut LiftContext<'_>, | ||
| ty: InterfaceType, | ||
| bytes: &[u8], | ||
| ) -> Result<Self> { | ||
| let (key_ty, value_ty) = match ty { | ||
| InterfaceType::Map(i) => { | ||
| let m = &cx.types[i]; | ||
| (m.key, m.value) | ||
| } | ||
| _ => bad_type_info(), | ||
| }; | ||
| debug_assert!((bytes.as_ptr() as usize) % (Self::ALIGN32 as usize) == 0); | ||
| // FIXME(#4311): needs memory64 treatment | ||
| let ptr = u32::from_le_bytes(bytes[..4].try_into().unwrap()); | ||
| let len = u32::from_le_bytes(bytes[4..].try_into().unwrap()); | ||
| let (ptr, len) = (usize::try_from(ptr)?, usize::try_from(len)?); | ||
| lift_std_map(cx, key_ty, value_ty, ptr, len) | ||
| } | ||
| } |
There was a problem hiding this comment.
For lifting here what I'd recommend is that since this is implemented in terms of the fallible hash map this can delegate to the fallible version, then convert the fallible version to the std version.
Fore xample linear_lift_from_flat here would call HashMap::linear_lift_form_flat(...), and if successful, would convert that to std::collections::HashMap
| fn typecheck(ty: &InterfaceType, types: &InstanceType<'_>) -> Result<()> { | ||
| match ty { | ||
| InterfaceType::Map(t) => { | ||
| let map_ty = &types.types[*t]; | ||
| K::typecheck(&map_ty.key, types)?; | ||
| V::typecheck(&map_ty.value, types)?; | ||
| Ok(()) | ||
| } | ||
| other => bail!("expected `map` found `{}`", desc(other)), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This would be a good place to delegate to HashMap::typecheck(...) as well for example as opposed to duplicating the method body here.
| fn linear_lower_to_flat<U>( | ||
| &self, | ||
| cx: &mut LowerContext<'_, U>, | ||
| ty: InterfaceType, | ||
| dst: &mut MaybeUninit<[ValRaw; 2]>, | ||
| ) -> Result<()> { | ||
| let (key_ty, value_ty) = match ty { | ||
| InterfaceType::Map(i) => { | ||
| let m = &cx.types[i]; | ||
| (m.key, m.value) | ||
| } | ||
| _ => bad_type_info(), | ||
| }; | ||
| let (ptr, len) = lower_map_iter(cx, key_ty, value_ty, self.len(), self.iter())?; | ||
| // See "WRITEPTR64" above for why this is always storing a 64-bit | ||
| // integer. | ||
| map_maybe_uninit!(dst[0]).write(ValRaw::i64(ptr as i64)); | ||
| map_maybe_uninit!(dst[1]).write(ValRaw::i64(len as i64)); | ||
| Ok(()) | ||
| } | ||
|
|
||
| fn linear_lower_to_memory<U>( | ||
| &self, | ||
| cx: &mut LowerContext<'_, U>, | ||
| ty: InterfaceType, | ||
| offset: usize, | ||
| ) -> Result<()> { | ||
| let (key_ty, value_ty) = match ty { | ||
| InterfaceType::Map(i) => { | ||
| let m = &cx.types[i]; | ||
| (m.key, m.value) | ||
| } | ||
| _ => bad_type_info(), | ||
| }; | ||
| debug_assert!(offset % (Self::ALIGN32 as usize) == 0); | ||
| let (ptr, len) = lower_map_iter(cx, key_ty, value_ty, self.len(), self.iter())?; | ||
| *cx.get(offset + 0) = u32::try_from(ptr).unwrap().to_le_bytes(); | ||
| *cx.get(offset + 4) = u32::try_from(len).unwrap().to_le_bytes(); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
What I might recommend for deduplicating this with the above HashMap implementation is to have two helpers on top of lower_map_iter, specifically linear_lower_map_to_memory and linear_lower_map_to_flat. That would have the boilerplate here (plus a generic iterator) and that way the hash map lower implementations would be one-liners to the shared functions.
Context
This is adding support for
mapbased on WebAssembly/component-model#554References