From a96e33e34cc222736fcc10ae3d3309ff007bd8c3 Mon Sep 17 00:00:00 2001 From: Nick Fitzgerald Date: Wed, 9 Oct 2024 21:01:17 -0700 Subject: [PATCH 1/2] Enable some miscellaneous GC spec tests These weren't caught by our checks that `should_fail` tests do not pass, I think because they only contain modules that should validate, and not any actual assertions. --- tests/wast.rs | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/tests/wast.rs b/tests/wast.rs index 657e19a4a34..7cf6b7eadb6 100644 --- a/tests/wast.rs +++ b/tests/wast.rs @@ -203,15 +203,8 @@ fn should_fail(test: &Path, strategy: Strategy) -> bool { } } } - let unsupported_gc_tests = [ - "binary_gc.wast", - "table_sub.wast", - "type_canon.wast", - "type_equivalence.wast", - "type-rec.wast", - "type-subtyping.wast", - "unreached_valid.wast", - ]; + + let unsupported_gc_tests = ["type-rec.wast", "type-subtyping.wast"]; for part in test.iter() { // Not implemented in Wasmtime yet From 7e780474effcf09b26183b594aca7bc7848b50d6 Mon Sep 17 00:00:00 2001 From: Nick Fitzgerald Date: Wed, 9 Oct 2024 22:53:34 -0700 Subject: [PATCH 2/2] Require declared subtyping when linking module import/export functions We were previously incorrectly doing a (shallow) match on structure when we should have been doing a subtyping check. --- crates/environ/src/module_types.rs | 2 - crates/wasmtime/src/runtime/type_registry.rs | 38 +++-- crates/wasmtime/src/runtime/types/matching.rs | 29 +--- tests/all/func.rs | 144 +++++++++--------- tests/all/linker.rs | 87 +++++++---- tests/wast.rs | 2 +- 6 files changed, 166 insertions(+), 136 deletions(-) diff --git a/crates/environ/src/module_types.rs b/crates/environ/src/module_types.rs index 4a96a85e7e3..ed59103ee5e 100644 --- a/crates/environ/src/module_types.rs +++ b/crates/environ/src/module_types.rs @@ -85,8 +85,6 @@ impl ModuleTypes { #[cfg(feature = "compile")] impl ModuleTypes { /// Associate `trampoline_ty` as the trampoline type for `for_ty`. - /// - /// This is really only for use by the `ModuleTypesBuilder`. pub fn set_trampoline_type( &mut self, for_ty: ModuleInternedTypeIndex, diff --git a/crates/wasmtime/src/runtime/type_registry.rs b/crates/wasmtime/src/runtime/type_registry.rs index a2a35c262e4..d23e3160742 100644 --- a/crates/wasmtime/src/runtime/type_registry.rs +++ b/crates/wasmtime/src/runtime/type_registry.rs @@ -119,12 +119,15 @@ impl TypeCollection { .write() .register_module_types(&**gc_runtime, module_types); + log::trace!("Begin building module's shared-to-module-trampoline-types map"); let mut trampolines = SecondaryMap::with_capacity(types.len()); - for (module_ty, trampoline) in module_types.trampoline_types() { + for (module_ty, module_trampoline_ty) in module_types.trampoline_types() { let shared_ty = types[module_ty]; - let trampoline_ty = registry.trampoline_type(shared_ty); - trampolines[trampoline_ty] = Some(trampoline).into(); + let trampoline_shared_ty = registry.trampoline_type(shared_ty); + trampolines[trampoline_shared_ty] = Some(module_trampoline_ty).into(); + log::trace!("--> shared_to_module_trampolines[{trampoline_shared_ty:?}] = {module_trampoline_ty:?}"); } + log::trace!("Done building module's shared-to-module-trampoline-types map"); Self { engine, @@ -711,14 +714,21 @@ impl TypeRegistryInner { // type in the rec group. for shared_type_index in entry.0.shared_type_indices.iter().copied() { let slab_id = shared_type_index_to_slab_id(shared_type_index); - if let Some(f) = self.types[slab_id].as_func() { - match f.trampoline_type() { - Cow::Borrowed(_) => { + let sub_ty = &self.types[slab_id]; + if let Some(f) = sub_ty.as_func() { + let trampoline = f.trampoline_type(); + match &trampoline { + Cow::Borrowed(_) if sub_ty.is_final && sub_ty.supertype.is_none() => { // The function type is its own trampoline type. Leave // its entry in `type_to_trampoline` empty to signal // this. + log::trace!( + "function type is its own trampoline type: \n\ + --> trampoline_type[{shared_type_index:?}] = {shared_type_index:?}\n\ + --> trampoline_type[{f}] = {f}" + ); } - Cow::Owned(trampoline) => { + Cow::Borrowed(_) | Cow::Owned(_) => { // This will recursively call into rec group // registration, but at most once since trampoline // function types are their own trampoline type. @@ -728,13 +738,23 @@ impl TypeRegistryInner { is_final: true, supertype: None, composite_type: wasmtime_environ::WasmCompositeType::Func( - trampoline, + trampoline.into_owned(), ), }, ); let trampoline_index = trampoline_entry.0.shared_type_indices[0]; log::trace!( - "Registering trampoline {trampoline_index:?} for function type {shared_type_index:?}" + "Registering trampoline type:\n\ + --> trampoline_type[{shared_type_index:?}] = {trampoline_index:?}\n\ + --> trampoline_type[{f}] = {g}", + f = { + let slab_id = shared_type_index_to_slab_id(shared_type_index); + self.types[slab_id].unwrap_func() + }, + g = { + let slab_id = shared_type_index_to_slab_id(trampoline_index); + self.types[slab_id].unwrap_func() + } ); debug_assert_ne!(shared_type_index, trampoline_index); self.type_to_trampoline[shared_type_index] = Some(trampoline_index).into(); diff --git a/crates/wasmtime/src/runtime/types/matching.rs b/crates/wasmtime/src/runtime/types/matching.rs index 96e9c36bb00..7b133226f21 100644 --- a/crates/wasmtime/src/runtime/types/matching.rs +++ b/crates/wasmtime/src/runtime/types/matching.rs @@ -1,6 +1,5 @@ -use crate::type_registry::RegisteredType; -use crate::{linker::DefinitionType, Engine, FuncType}; -use crate::{prelude::*, ArrayType, StructType}; +use crate::prelude::*; +use crate::{linker::DefinitionType, Engine}; use wasmtime_environ::{ EntityType, Global, IndexType, Memory, ModuleTypes, Table, TypeTrace, VMSharedTypeIndex, WasmHeapType, WasmRefType, WasmSubType, WasmValType, @@ -17,30 +16,10 @@ impl MatchCx<'_> { } fn type_reference(&self, expected: VMSharedTypeIndex, actual: VMSharedTypeIndex) -> Result<()> { - // Avoid matching on structure for subtyping checks when we have - // precisely the same type. - let matches = expected == actual || { - let expected = RegisteredType::root(self.engine, expected).unwrap(); - let actual = RegisteredType::root(self.engine, actual).unwrap(); - if expected.is_array() && actual.is_array() { - let expected = ArrayType::from_registered_type(expected); - let actual = ArrayType::from_registered_type(actual); - actual.matches(&expected) - } else if expected.is_func() && actual.is_func() { - let expected = FuncType::from_registered_type(expected); - let actual = FuncType::from_registered_type(actual); - actual.matches(&expected) - } else if expected.is_struct() && actual.is_struct() { - let expected = StructType::from_registered_type(expected); - let actual = StructType::from_registered_type(actual); - actual.matches(&expected) - } else { - false - } - }; - if matches { + if self.engine.signatures().is_subtype(actual, expected) { return Ok(()); } + let msg = "types incompatible"; let expected = match self.engine.signatures().borrow(expected) { Some(ty) => ty, diff --git a/tests/all/func.rs b/tests/all/func.rs index ce7a1610356..cc92d732006 100644 --- a/tests/all/func.rs +++ b/tests/all/func.rs @@ -2147,28 +2147,43 @@ fn wasm_to_host_trampolines_and_subtyping(config: &mut Config) -> Result<()> { let engine = Engine::new(&config)?; + let ft0 = FuncType::with_finality_and_supertype( + &engine, + Finality::NonFinal, + None, + [], + [ValType::Ref(RefType::new(true, HeapType::Extern))], + )?; + let ft1 = FuncType::with_finality_and_supertype( + &engine, + Finality::NonFinal, + Some(&ft0), + [], + [ValType::Ref(RefType::new(false, HeapType::Extern))], + )?; + let module = Module::new( &engine, r#" (module - (type $ty (func (result (ref null extern)))) + (type $ft0 (sub (func (result (ref null extern))))) - (import "" "a" (func $imported_func (type $ty))) - (import "" "return_func" (func $return_func (result (ref $ty)))) - (global $g (export "g") (mut (ref null $ty)) (ref.null $ty)) - (table $t (export "t") 10 10 (ref null $ty)) + (import "" "a" (func $imported_func (type $ft0))) + (import "" "return_func" (func $return_func (result (ref $ft0)))) + (global $g (export "g") (mut (ref null $ft0)) (ref.null $ft0)) + (table $t (export "t") 10 10 (ref null $ft0)) (func (export "f") (drop (call $imported_func)) - (drop (call_ref $ty (call $return_func))) - (drop (call_ref $ty (global.get $g))) - (drop (call_ref $ty (table.get $t (i32.const 0)))) + (drop (call_ref $ft0 (call $return_func))) + (drop (call_ref $ft0 (global.get $g))) + (drop (call_ref $ft0 (table.get $t (i32.const 0)))) ) ) "#, )?; - // This defines functions of type `(func (result (ref extern)))` and not of + // This defines a function of type `(func (result (ref extern)))` and not of // type `(func (result (ref null extern)))` that the Wasm imports. But it is // still a subtype of the Wasm import's type, so we should be able to use // the Wasm's precompiled trampolines with this host function. @@ -2176,78 +2191,61 @@ fn wasm_to_host_trampolines_and_subtyping(config: &mut Config) -> Result<()> { // But this means we also need to look up the trampoline by the caller's // type, rather than the callee's type. // - // So take each kind of function we can define, and give it to Wasm in every - // way that Wasm can receive a function, and make sure we find the correct - // trampolines in all cases and everything works. - // - // Note that we create multiple versions of the same function by calling - // these constructors multiple times in the loop below. This is to avoid the - // scenario where we reuse the function, the first time it is exposed to - // Wasm its trampoline is filled in, and then all subsequent uses don't - // actually exercise their associated code paths for finding and - // initializing their trampoline. - let func_ty = FuncType::new(&engine, [], [RefType::new(false, HeapType::Extern).into()]); - let func_ctors: Vec) -> Func>> = vec![ - Box::new(|store| { - Func::wrap(store, |mut caller: Caller<'_, ()>| { - ExternRef::new(&mut caller, 100) - }) - }), - Box::new(|store| { - Func::new( - store, - func_ty.clone(), - |mut caller: Caller<'_, ()>, _args, results| { - results[0] = ExternRef::new(&mut caller, 200)?.into(); - Ok(()) - }, - ) - }), - ]; + // So give it to Wasm in every way that Wasm can receive a function, and + // make sure we find the correct trampolines in all cases and everything + // works. + let make_func = |store: &mut Store<_>| { + Func::new( + store, + ft1.clone(), + |mut caller: Caller<'_, ()>, _args, results| { + results[0] = ExternRef::new(&mut caller, 200)?.into(); + Ok(()) + }, + ) + }; let return_func_ty = module.imports().nth(1).unwrap().ty().unwrap_func().clone(); - for make_func in func_ctors { - let mut store = Store::new(&engine, ()); + let mut store = Store::new(&engine, ()); - let imported_func = make_func(&mut store); - assert!(FuncType::eq(&imported_func.ty(&store), &func_ty)); + let imported_func = make_func(&mut store); + assert!(FuncType::eq(&imported_func.ty(&store), &ft1)); - let returned_func = make_func(&mut store); - assert!(FuncType::eq(&returned_func.ty(&store), &func_ty)); + let returned_func = make_func(&mut store); + assert!(FuncType::eq(&returned_func.ty(&store), &ft1)); - let return_func = Func::new( - &mut store, - return_func_ty.clone(), - move |_caller, _args, results| { - results[0] = returned_func.into(); - Ok(()) - }, - ); + let return_func = Func::new( + &mut store, + return_func_ty.clone(), + move |_caller, _args, results| { + results[0] = returned_func.into(); + Ok(()) + }, + ); - let instance = Instance::new( - &mut store, - &module, - &[imported_func.into(), return_func.into()], - )?; + let instance = Instance::new( + &mut store, + &module, + &[imported_func.into(), return_func.into()], + )?; - let g = make_func(&mut store); - assert!(FuncType::eq(&g.ty(&store), &func_ty)); - instance - .get_global(&mut store, "g") - .unwrap() - .set(&mut store, g.into())?; - - let t = make_func(&mut store); - assert!(FuncType::eq(&t.ty(&store), &func_ty)); - instance - .get_table(&mut store, "t") - .unwrap() - .set(&mut store, 0, t.into())?; - - let f = instance.get_typed_func::<(), ()>(&mut store, "f")?; - f.call(&mut store, ())?; - } + let g = make_func(&mut store); + assert!(FuncType::eq(&g.ty(&store), &ft1)); + instance + .get_global(&mut store, "g") + .unwrap() + .set(&mut store, g.into())?; + + let t = make_func(&mut store); + assert!(FuncType::eq(&t.ty(&store), &ft1)); + instance + .get_table(&mut store, "t") + .unwrap() + .set(&mut store, 0, t.into())?; + + let f = instance.get_typed_func::<(), ()>(&mut store, "f")?; + f.call(&mut store, ())?; Ok(()) } diff --git a/tests/all/linker.rs b/tests/all/linker.rs index e02ea2d5471..888584826d4 100644 --- a/tests/all/linker.rs +++ b/tests/all/linker.rs @@ -521,46 +521,81 @@ fn linker_instantiate_with_concrete_func_refs() -> Result<()> { #[test] #[cfg_attr(miri, ignore)] fn linker_defines_func_subtype() -> Result<()> { + let _ = env_logger::try_init(); + let mut config = Config::new(); config.wasm_function_references(true); config.wasm_gc(true); let engine = Engine::new(&config)?; - let mut linker = Linker::new(&engine); - linker.func_new( - "env", - "f", - FuncType::new(&engine, Some(ValType::FUNCREF), None), - |_caller, _args, _results| Ok(()), + let ft0 = FuncType::with_finality_and_supertype( + &engine, + Finality::NonFinal, + None, + [ValType::NULLFUNCREF], + [], )?; - linker.func_new( - "env", - "g", - FuncType::new(&engine, None, Some(ValType::NULLFUNCREF)), - |_caller, _args, _results| Ok(()), + let ft1 = FuncType::with_finality_and_supertype( + &engine, + Finality::NonFinal, + Some(&ft0), + [ValType::FUNCREF], + [], )?; - let nop_ty = FuncType::new(&engine, None, None); - let ref_null_nop = ValType::from(RefType::new(true, HeapType::ConcreteFunc(nop_ty))); - linker.func_new( - "env", - "h", - FuncType::new(&engine, Some(ref_null_nop.clone()), Some(ref_null_nop)), - |_caller, _args, _results| Ok(()), + + let ft2 = FuncType::with_finality_and_supertype( + &engine, + Finality::NonFinal, + None, + [], + [ValType::FUNCREF], + )?; + let ft3 = FuncType::with_finality_and_supertype( + &engine, + Finality::NonFinal, + Some(&ft2), + [], + [ValType::NULLFUNCREF], )?; + let nop = FuncType::new(&engine, [], []); + let ft4 = FuncType::with_finality_and_supertype( + &engine, + Finality::NonFinal, + None, + [ValType::NULLFUNCREF], + [ValType::FUNCREF], + )?; + let ft5 = FuncType::with_finality_and_supertype( + &engine, + Finality::NonFinal, + Some(&ft4), + [ValType::Ref(RefType::new( + true, + HeapType::ConcreteFunc(nop.clone()), + ))], + [ValType::Ref(RefType::new( + true, + HeapType::ConcreteFunc(nop.clone()), + ))], + )?; + + let mut linker = Linker::new(&engine); + linker.func_new("env", "f", ft1, |_caller, _args, _results| Ok(()))?; + linker.func_new("env", "g", ft3, |_caller, _args, _results| Ok(()))?; + linker.func_new("env", "h", ft5, |_caller, _args, _results| Ok(()))?; + let module = Module::new( &engine, r#" (module - ;; wasm's declared nullfuncref <: f's actual funcref - (import "env" "f" (func (param nullfuncref))) - - ;; g's actual nullfuncref <: wasm's declared funcref - (import "env" "g" (func (result funcref))) + (type $ft0 (sub (func (param nullfuncref)))) + (type $ft2 (sub (func (result funcref)))) + (type $ft4 (sub (func (param nullfuncref) (result funcref)))) - ;; wasm's declared nullfuncref param <: h's actual (ref null $nop) param, and - ;; h's actual (ref null $nop) result <: wasm's declared funcref result - (import "env" "h" (func (param nullfuncref) (result funcref))) + (import "env" "f" (func (type $ft0))) + (import "env" "g" (func (type $ft2))) + (import "env" "h" (func (type $ft4))) ) "#, )?; diff --git a/tests/wast.rs b/tests/wast.rs index 7cf6b7eadb6..992db084f02 100644 --- a/tests/wast.rs +++ b/tests/wast.rs @@ -204,7 +204,7 @@ fn should_fail(test: &Path, strategy: Strategy) -> bool { } } - let unsupported_gc_tests = ["type-rec.wast", "type-subtyping.wast"]; + let unsupported_gc_tests = ["type-subtyping.wast"]; for part in test.iter() { // Not implemented in Wasmtime yet