Stream: git-wasmtime

Topic: wasmtime / PR #4442 support dynamic function calls in com...

Wasmtime GitHub notifications bot (Jul 13 2022 at 17:13):

dicej opened PR #4442 from component-dynamic-type to main:

This addresses #4310, introducing a new component::values::Val type for
representing component values dynamically. It also adds a call method to
component::func::Func, which takes a slice of Vals as parameters and returns
a Result<Val> representing the result.

As an implementation detail, I've also added a component::values::Type type,
which is an owned, despecialized version of
wasmtime_environ::component::InterfaceType. That serves two purposes:

• It allows us to despecialize as the first step, which reduces the number of cases to consider when typechecking and lowering.
• It avoids needing to borrow the store both mutably and immutably when lowering, as we would if we used InterfaceTypes.

Finally, I happened to notice that the ComponentType::SIZE32 calculation in
expand_record_for_component_type needed a correction, so I did that.

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Jul 13 2022 at 18:22):

dicej updated PR #4442 from component-dynamic-type to main.

Wasmtime GitHub notifications bot (Jul 13 2022 at 22:01):

dicej updated PR #4442 from component-dynamic-type to main.

Wasmtime GitHub notifications bot (Jul 13 2022 at 22:27):

dicej updated PR #4442 from component-dynamic-type to main.

Wasmtime GitHub notifications bot (Jul 14 2022 at 00:48):

jameysharp created PR review comment:

I don't love making a function named _to_str visible outside the current module. But I see all you have at the call site is a &StoreOpaque so you can't call to_str instead. Unless @alexcrichton has a better idea I guess this is fine.

Wasmtime GitHub notifications bot (Jul 14 2022 at 00:48):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jul 14 2022 at 00:48):

jameysharp created PR review comment:

Would it be reasonable to use FlagsSize::from_count everywhere instead of using ceiling_divide directly?

Wasmtime GitHub notifications bot (Jul 14 2022 at 00:48):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jul 14 2022 at 00:48):

jameysharp created PR review comment:

If you got count from Type::Flags here instead of from Val::Flags, I don't see that you'd need to store a count in Val::Flags at all. It looks like everywhere that you need the count, you have the type available.

I guess you did it this way to force the host application to specify how many flags they expect to be passing in as parameters, so you can verify in typecheck that their expectation was correct—and so you can inform the host application about how many flags they received.

I go back and forth on whether that kind of checking is worth doing in this sort of dynamic call situation. @alexcrichton, what's your take?

Wasmtime GitHub notifications bot (Jul 14 2022 at 00:48):

jameysharp created PR review comment:

This is totally a nitpick but would you mind putting these cases in the same order as the enums they correspond to? I see Type and InterfaceType have the primitive types in the same order, so this list is the only one that doesn't match.

Wasmtime GitHub notifications bot (Jul 14 2022 at 00:48):

jameysharp created PR review comment:

I think you can use Iterator::try_for_each here for a little more clarity.

Wasmtime GitHub notifications bot (Jul 14 2022 at 14:13):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jul 14 2022 at 14:13):

dicej created PR review comment:

Yeah, that felt awkward to me, too. Perhaps we could rename it to something like to_str_with_store_opaque? I haven't yet wrapped my head around all the different flavors of store, so perhaps there's a better option.

Wasmtime GitHub notifications bot (Jul 14 2022 at 14:36):

dicej updated PR #4442 from component-dynamic-type to main.

Wasmtime GitHub notifications bot (Jul 14 2022 at 14:41):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2022 at 04:00):

dicej updated PR #4442 from component-dynamic-type to main.

Wasmtime GitHub notifications bot (Jul 21 2022 at 14:25):

dicej updated PR #4442 from component-dynamic-type to main.

Wasmtime GitHub notifications bot (Jul 21 2022 at 15:03):

dicej updated PR #4442 from component-dynamic-type to main.

Wasmtime GitHub notifications bot (Jul 21 2022 at 15:48):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2022 at 15:48):

alexcrichton created PR review comment:

While you're here and doing this could this also add variants for lifting Vec<T> from WasmSlice<T> and such?

The macro could possibly look something like:

forward_lifts! {
    WasmStr => { Box<str>, Rc<str>, Arc<str>, String }
    WasmList<T> => { Box<[T]>, ...}
}

(or similar)

Wasmtime GitHub notifications bot (Jul 21 2022 at 15:48):

alexcrichton created PR review comment:

Idiomatically we try to avoid as casts where possible since in isolation they could indicate that a lossy conversion is happening. Could the casts here be replaced with i32::from(*value)? That should signify that no data is lost.

Wasmtime GitHub notifications bot (Jul 21 2022 at 15:48):

alexcrichton created PR review comment:

I think this + index may not be right since each of the values may take up more than one ValRaw slot.

It might make more sense for this function to take &mut std::slice::Iter<'_, MaybeUninit<ValRaw>> instead of the slice/offset it currently has perhaps?

Wasmtime GitHub notifications bot (Jul 21 2022 at 15:48):

alexcrichton created PR review comment:

I worry with impl Iterator that this will excessively monomorphize into callers, so could this use something concrete like &mut &[ValRaw] or &mut std::slice::Iter<'_, ValRaw>?

Wasmtime GitHub notifications bot (Jul 21 2022 at 15:48):

alexcrichton created PR review comment:

Could this have a preceding debug_assert! about alignment?

Wasmtime GitHub notifications bot (Jul 21 2022 at 15:48):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2022 at 15:48):

alexcrichton created PR review comment:

This seems like it may not be right for multiple arguments since the offset may always be zero?

Wasmtime GitHub notifications bot (Jul 21 2022 at 15:48):

alexcrichton created PR review comment:

I think this could delegate to <str as Lower>::store?

Wasmtime GitHub notifications bot (Jul 21 2022 at 15:48):

alexcrichton created PR review comment:

This is another case that I think is not quite right for subtle reasons. The main one is that unused ValRaw entries (and upper bits which are unused here) need to all be set to zero. Otherwise this runs the risk of leaking undefined values into wasm modules.

Additionally this is another location that needs to inform the caller that a nonzero amount of ValRaw slots are consumed. Given value here you may also consume more slots than value takes up because of how variants are "flattened"

Wasmtime GitHub notifications bot (Jul 21 2022 at 15:48):

alexcrichton created PR review comment:

I think this is actually subtly incorrect because it would get the alignment incorrect for the call to realloc. The type of the list element may need to get passed in here.

Wasmtime GitHub notifications bot (Jul 21 2022 at 15:48):

alexcrichton created PR review comment:

I think that this may lead to unfortunately surprising results in that a string type is only considered equivalent if it comes from the same types origin but it's actually unique across all components. I think that this is sufficient for a "succeed fast" test but this may need to be expanded for deep equality checking as well.

Wasmtime GitHub notifications bot (Jul 21 2022 at 15:48):

alexcrichton created PR review comment:

GIven how many types are in this module and how Type isn't the only export I think it may actually make sense to have this be a pub mod types and don't reexport Type from inside. Otherwise external users can't name types like Field and such.

Wasmtime GitHub notifications bot (Jul 21 2022 at 15:48):

alexcrichton created PR review comment:

I think these methods may make more sense on List (in the values module) along the lines of:

impl List {
    pub fn new(elements: Box<[Val]>, ty: types::List) -> Result<List> {
        // ...
    }
}

(using the different structure of types I mentioned above).

Then there could be impl From<List> for Val to convert to a Val as necessary.

Wasmtime GitHub notifications bot (Jul 21 2022 at 15:48):

alexcrichton created PR review comment:

This doesn't necessarily have to be handled here, but I think this typecheck will eventually want to be more recursive for better error mesages. Right now I think you could get expected record, got record which doesn't describe which record fields mismatch or similar.

Wasmtime GitHub notifications bot (Jul 21 2022 at 15:48):

alexcrichton created PR review comment:

This is a bit bike-sheddy, but instead of

pub struct Handle<T> { ... }
pub struct TupleIndex(TypeTupleIndex);
pub enum Type {
    // ...
    Tuple(Handle<TupleIndex>),
    // ...
}

could this instead be:

struct Handle<T> { ... }
pub struct Tuple(Handle<TypeTupleIndex>);
pub enum Type {
    // ...
    Tuple(Tuple),
    // ...
}

That I think will allow us to be a bit more flexible in the future with internal representations ideally.

Wasmtime GitHub notifications bot (Jul 21 2022 at 15:48):

alexcrichton created PR review comment:

I think it would still be worth filling this in for other reasons though. For example all of the parameters to a host function are lifted which is where these could show up inline.

Eventually we'll want an equivalent to crate::Linker::func_new within component::Linker although it doesn't have to be added as part of that PR. When that's added this'll get exercised I believe.

Wasmtime GitHub notifications bot (Jul 21 2022 at 15:48):

alexcrichton created PR review comment:

One nice part of perhaps using MaybeUninit<ValRaw> is that you could delegate to the Lower for T implementations? Or otherwise I think it would be worthwhile figuring out how to delegate to that for primitives and strings/lists perhaps (may require an unsafe method or two to extract MaybeUninit<[ValRaw; 2]> from &mut std::slice::Iter<'_, MaybeUninit<ValRaw>> though.

Wasmtime GitHub notifications bot (Jul 21 2022 at 15:48):

alexcrichton created PR review comment:

I think that this is going to need special logic to skip extra fields within src to ensure that flatten_count entries are always popped off that iterator.

Wasmtime GitHub notifications bot (Jul 21 2022 at 15:48):

alexcrichton created PR review comment:

Is this still necessary? This seems like a bit of an odd query that I'm not sure what it would be used for

Wasmtime GitHub notifications bot (Jul 21 2022 at 15:48):

alexcrichton created PR review comment:

I think this organization also helps method discovery since today there's lots of impls for Handle<T> for different T but for Tuple there will be just the methods related to tuples.

Wasmtime GitHub notifications bot (Jul 21 2022 at 17:57):

dicej created PR review comment:

@alexcrichton Is there a convenient way to make a &mut std::slice::Iter<'_, &mut MaybeUninit<ValRaw>> from a &mut MaybeUninit<[ValRaw; N]? I could do something like (0..N).map(|i| map_maybe_uninit!(dst[i]), but I don't think that's what you meant.

Wasmtime GitHub notifications bot (Jul 21 2022 at 17:57):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2022 at 18:05):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2022 at 18:05):

alexcrichton created PR review comment:

Not easily, but you can transmute &mut MaybeUninit<[ValRaw; N]> to &mut [MaybeUninit<ValRaw>; N] "safely" and then .iter() from there

Wasmtime GitHub notifications bot (Jul 21 2022 at 18:19):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2022 at 18:19):

dicej created PR review comment:

I'm not sure I understand this. Type::String doesn't have a Handle<T>, so this code wouldn't be reached in that case. Am I misunderstanding what you meant by "string type"?

Wasmtime GitHub notifications bot (Jul 21 2022 at 19:00):

dicej created PR review comment:

That's what I originally considered, but I ended up using these methods on Type instead in the interest of ergonomics. You can see them used in the dynamic.rs tests, along with Type::nested. This avoids forcing the programmer to write a bunch of if let Type::List(ty) = ty { List::new(elements, ty) } else { unreachable!() }.

This is the ergonomic challenge I was referring to earlier in the discussion about whether Vals need to be associated with a specific Type. I'm certainly willing to reconsider; I just want to understand how you expect this API to be used vs how I'm using it in the dynamic.rs tests. Do you not expect real-world code to need a bunch of if let ... { ... } else { unreachable!() } when building Vals from Types in the scheme you outlined?

Wasmtime GitHub notifications bot (Jul 21 2022 at 19:00):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2022 at 19:00):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2022 at 19:00):

dicej created PR review comment:

Please see my comment above about ergonomics and how it is used in the dynamic.rs tests.

Wasmtime GitHub notifications bot (Jul 21 2022 at 19:09):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2022 at 19:09):

dicej created PR review comment:

Any e.g. record field mismatch would have been caught already in Type::new_record before we got this far. The only time you'd see expected record, got record would be if you used Vals from one component in another, or if your component had two identical records (i.e. same field names and types) and you mixed up which one you created the Val for. Either way, more recursion wouldn't necessarily help. This is somewhat analogous to when rustc reports a type mismatch between foo::Bar and foo::Bar because you're using two different versions of the foo crate. It's even worse here since types don't really have names in the component model.

But yes, I agree we should give a friendlier diagnostic here, e.g. hinting based on whether Handle::index or Handle::types are different, or both.

Wasmtime GitHub notifications bot (Jul 21 2022 at 20:35):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2022 at 20:35):

alexcrichton created PR review comment:

Ah sorry bad example, but instead I think this would consider (u32, u32) not an equivalent type if it comes from two different components.

Wasmtime GitHub notifications bot (Jul 21 2022 at 20:43):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2022 at 20:43):

alexcrichton created PR review comment:

Ah ok thanks for pointing that out! I think this may be a case where I'm used to slightly more verbose Rust-based idioms perhaps? For example the Type::new_list API as-is can return two kinds of errors (not a list type or element of the wrong type) where what I would otherwise expect is to split those two error cases across two functions. Instead of:

    let input = ty.new_list(Box::new([
        Val::U32(32343),
        Val::U32(79023439),
        Val::U32(2084037802),
    ]))?;

I'd expect:

    let input = ty.unwrap_list().new(Box::new([
        Val::U32(32343),
        Val::U32(79023439),
        Val::U32(2084037802),
    ]))?;

(or similar)

I don't mean to say we should go all the way to writing a bunch of wordy if let but I think helper methods can help scope precisely what errors are coming out of each function. In this case unwrap_list would be a clear indication of "I expect this type to be a list" and the new error would be clearly "I expect each element to be of this type".

Wasmtime GitHub notifications bot (Jul 21 2022 at 23:10):

dicej updated PR #4442 from component-dynamic-type to main.

Wasmtime GitHub notifications bot (Jul 22 2022 at 14:12):

dicej created PR review comment:

@alexcrichton any further concerns about this method? I'm going to mark this "resolved" for now, but feel free to unresolve it.

Wasmtime GitHub notifications bot (Jul 22 2022 at 14:12):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jul 22 2022 at 14:15):

dicej updated PR #4442 from component-dynamic-type to main.

Wasmtime GitHub notifications bot (Jul 22 2022 at 16:04):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 22 2022 at 16:04):

alexcrichton created PR review comment:

Could this be replaced with a debug_assert! that the memory is the same as the store that this slice originally came from?

Wasmtime GitHub notifications bot (Jul 22 2022 at 16:04):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 22 2022 at 16:04):

alexcrichton created PR review comment:

This is a minor nit, but idiomatically I'd expect that values::List::new would also exist as a function and this function would simply be values::List::new(values, self) (or something like that). This is just random API additions though so happy to defer to future PRs.

Wasmtime GitHub notifications bot (Jul 22 2022 at 16:04):

alexcrichton created PR review comment:

Would it be possible to avoid this and only have the version that takes StoreOpaque? I'm otherwise worried about making this method too general because the indexes are only guaranteed to be valid for the original memory slice and if one day we remove the bounds checks in release mode (which we should be able to do now I'm just being conservative) it would be memory unsafe to pass in an arbitrary slice here.

Wasmtime GitHub notifications bot (Jul 22 2022 at 16:04):

alexcrichton created PR review comment:

Is Lift + Lower needed here? Ideally I think just Lift would suffice for a trait bound

Wasmtime GitHub notifications bot (Jul 22 2022 at 16:04):

alexcrichton created PR review comment:

Unfortunately we can't rely on the .len() reported from an ExactSizeIterator with unsafe Rust. Given that we have an unsafe guarantee that a Val is always valid with respect to its type I think that the length will need to be checked after the fields are collected rather than before.

Wasmtime GitHub notifications bot (Jul 22 2022 at 17:00):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jul 22 2022 at 17:00):

dicej created PR review comment:

This is what I get when I remove the + Lower:

error[E0277]: the trait bound `T: component::func::typed::Lower` is not satisfied
    --> crates/wasmtime/src/component/func/typed.rs:536:31
     |
536  |           unsafe impl <T: Lift> Lift for $a {
     |                                 ^^^^ the trait `component::func::typed::Lower` is not implemented for `T`
...
550  | / forward_list_lifts! {
551  | |     Box<[T]>,
552  | |     std::rc::Rc<[T]>,
553  | |     std::sync::Arc<[T]>,
554  | |     Vec<T>,
555  | | }
     | |_- in this macro invocation
     |
note: required because of the requirements on the impl of `component::func::typed::Lower` for `[T]`
    --> crates/wasmtime/src/component/func/typed.rs:1006:16
     |
1006 | unsafe impl<T> Lower for [T]
     |                ^^^^^     ^^^
note: required because of the requirements on the impl of `component::func::typed::ComponentType` for `Box<[T]>`
    --> crates/wasmtime/src/component/func/typed.rs:475:37
     |
475  |           unsafe impl <$($generics)*> ComponentType for $a {
     |                                       ^^^^^^^^^^^^^
...
504  | / forward_impls! {
505  | |     (T: Lower + ?Sized) &'_ T => T,
506  | |     (T: Lower + ?Sized) Box<T> => T,
     | |                         ^^^^^^
507  | |     (T: Lower + ?Sized) std::rc::Rc<T> => T,
...    |
510  | |     (T: Lower) Vec<T> => [T],
511  | | }
     | |_- in this macro invocation
note: required by a bound in `component::func::typed::Lift`
    --> crates/wasmtime/src/component/func/typed.rs:444:32
     |
444  | pub unsafe trait Lift: Sized + ComponentType {
     |                                ^^^^^^^^^^^^^ required by this bound in `component::func::typed::Lift`
     = note: this error originates in the macro `forward_list_lifts` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider restricting type parameter `T`
     |
536  |         unsafe impl <T: component::func::typed::Lower Lift> Lift for $a {
     |                         +++++++++++++++++++++++++++++
...

Wasmtime GitHub notifications bot (Jul 22 2022 at 17:52):

alexcrichton created PR review comment:

Oh that bug is here where right now there's <T: Lower> ComponentType for Box<T> where that should be T: ComponentType. I was trying to be "too clever" by shoving it all into one macro but I think it can be split by refactoring the forward macro into multiple macros

Wasmtime GitHub notifications bot (Jul 22 2022 at 17:52):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 22 2022 at 18:28):

dicej updated PR #4442 from component-dynamic-type to main.

Wasmtime GitHub notifications bot (Jul 22 2022 at 18:33):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jul 22 2022 at 18:33):

dicej created PR review comment:

I've improved the error message so you'll never get expected record, got record. I attempted to go much further than that and recursively compare the types, but reusing typed::typecheck_tuple etc. turned out to be difficult in a dynamic context, and I didn't want to duplicate them either.

Also, I'm having trouble finding guidance in https://github.com/WebAssembly/component-model about when interface types are to be considered equal and when not, so I'm not sure what the correct logic is anyway. Are types considered equal if they are structurally identical?

Wasmtime GitHub notifications bot (Jul 22 2022 at 18:34):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jul 22 2022 at 18:34):

dicej created PR review comment:

I've address this for tuple, option, and expected. Not sure if I should do the same for e.g. record, though. See my comment in the other conversation about type equality.

Wasmtime GitHub notifications bot (Jul 25 2022 at 15:06):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 25 2022 at 15:06):

alexcrichton created PR review comment:

Idiomatically I think it's fine to call these constructors new rather than try_new since try_* is typically only used if the base name is already a method elsewhere (and otherwise there's no Option::new)

Wasmtime GitHub notifications bot (Jul 25 2022 at 15:06):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 25 2022 at 15:06):

alexcrichton created PR review comment:

Would it be possible to move lift/load to the Val module and keep all the internals of all the Val private (e.g. not pub(crate))?

Wasmtime GitHub notifications bot (Jul 25 2022 at 15:06):

alexcrichton created PR review comment:

I think that the tuple/option/expected bits are probably not going to be necessary in the long-term if the original issue here is fixed. I think this is fine to leave as a FIXME with a follow-up issue for now though.

Otherwise though the current logic in tuple/option/expected I would expect to be baked in here or otherwise into some trait impl on T perhaps. I don't think we'd want to duplicate things and have some using PartialEq and some using manual impls.

Wasmtime GitHub notifications bot (Jul 25 2022 at 15:06):

alexcrichton created PR review comment:

I don't think we'll get a ton of guidance from the component model itself, but for now from Wasmtime's perspective given how trampolines and lifting/lowering/etc are all implemented we require exact structural equality for the embedder API. I think the intention is that subtyping would eventually be used but we don't have a great means of implementing that right now.

Wasmtime GitHub notifications bot (Jul 25 2022 at 17:44):

dicej updated PR #4442 from component-dynamic-type to main.

Wasmtime GitHub notifications bot (Jul 25 2022 at 18:38):

alexcrichton merged PR #4442.

Last updated: Nov 22 2024 at 17:03 UTC