From 70bfce269e0e819d2ca4812af1d1a59de0ea80cf Mon Sep 17 00:00:00 2001 From: Nick Fitzgerald Date: Wed, 9 Oct 2024 23:56:05 -0700 Subject: [PATCH] Implement subtype checking for `[return_]call_indirect` instructions When Wasm GC is enabled, the `[return_]call_indirect` instructions must do full subtyping checks, rather than simple strict equality type checks. This adds an additional branch and slow path to indirect calls, so we only emit code for this check when Wasm GC is enabled, even though it would otherwise be correct to always emit it (because the `is_subtype` check would always fail for non-equal types, since there is no subtyping before Wasm GC). --- cranelift/codegen/src/lib.rs | 4 +- crates/cranelift/src/func_environ.rs | 96 ++++++++++++------- .../src/translate/code_translator.rs | 2 + .../cranelift/src/translate/environ/spec.rs | 4 +- crates/wasmtime/src/runtime/vm/libcalls.rs | 7 +- tests/wast.rs | 11 --- 6 files changed, 74 insertions(+), 50 deletions(-) diff --git a/cranelift/codegen/src/lib.rs b/cranelift/codegen/src/lib.rs index 475014e0f550..71227bc1a6cd 100644 --- a/cranelift/codegen/src/lib.rs +++ b/cranelift/codegen/src/lib.rs @@ -97,9 +97,9 @@ pub mod incremental_cache; #[macro_export] macro_rules! trace { ($($tt:tt)*) => { - if cfg!(feature = "trace-log") { + // if cfg!(feature = "trace-log") { ::log::trace!($($tt)*); - } + // } }; } diff --git a/crates/cranelift/src/func_environ.rs b/crates/cranelift/src/func_environ.rs index e1fcb8c4fbfd..0127d4489f97 100644 --- a/crates/cranelift/src/func_environ.rs +++ b/crates/cranelift/src/func_environ.rs @@ -17,7 +17,7 @@ use cranelift_frontend::FunctionBuilder; use cranelift_frontend::Variable; use smallvec::SmallVec; use std::mem; -use wasmparser::Operator; +use wasmparser::{Operator, WasmFeatures}; use wasmtime_environ::{ BuiltinFunctionIndex, DataIndex, ElemIndex, EngineOrModuleTypeIndex, FuncIndex, GlobalIndex, IndexType, Memory, MemoryIndex, MemoryPlan, MemoryStyle, Module, ModuleInternedTypeIndex, @@ -1294,6 +1294,7 @@ impl<'a, 'func, 'module_env> Call<'a, 'func, 'module_env> { /// Do an indirect call through the given funcref table. pub fn indirect_call( mut self, + features: &WasmFeatures, table_index: TableIndex, ty_index: TypeIndex, sig_ref: ir::SigRef, @@ -1301,6 +1302,7 @@ impl<'a, 'func, 'module_env> Call<'a, 'func, 'module_env> { call_args: &[ir::Value], ) -> WasmResult> { let (code_ptr, callee_vmctx) = match self.check_and_load_code_and_callee_vmctx( + features, table_index, ty_index, callee, @@ -1316,6 +1318,7 @@ impl<'a, 'func, 'module_env> Call<'a, 'func, 'module_env> { fn check_and_load_code_and_callee_vmctx( &mut self, + features: &WasmFeatures, table_index: TableIndex, ty_index: TypeIndex, callee: ir::Value, @@ -1333,7 +1336,8 @@ impl<'a, 'func, 'module_env> Call<'a, 'func, 'module_env> { ); // If necessary, check the signature. - let check = self.check_indirect_call_type_signature(table_index, ty_index, funcref_ptr); + let check = + self.check_indirect_call_type_signature(features, table_index, ty_index, funcref_ptr); let trap_code = match check { // `funcref_ptr` is checked at runtime that its type matches, @@ -1367,6 +1371,7 @@ impl<'a, 'func, 'module_env> Call<'a, 'func, 'module_env> { fn check_indirect_call_type_signature( &mut self, + features: &WasmFeatures, table_index: TableIndex, ty_index: TypeIndex, funcref_ptr: ir::Value, @@ -1402,33 +1407,40 @@ impl<'a, 'func, 'module_env> Call<'a, 'func, 'module_env> { }; } - // Otherwise if the types don't match then either (a) this is a - // null pointer or (b) it's a pointer with the wrong type. - // Figure out which and trap here. - // - // If it's possible to have a null here then try to load the - // type information. If that fails due to the function being a - // null pointer, then this was a call to null. Otherwise if it - // succeeds then we know it won't match, so trap anyway. - if table.table.ref_type.nullable { - if self.env.signals_based_traps() { - let mem_flags = ir::MemFlags::trusted().with_readonly(); - self.builder.ins().load( - sig_id_type, - mem_flags.with_trap_code(Some(crate::TRAP_INDIRECT_CALL_TO_NULL)), - funcref_ptr, - i32::from(self.env.offsets.ptr.vm_func_ref_type_index()), - ); - } else { - self.env.trapz( - self.builder, - funcref_ptr, - crate::TRAP_INDIRECT_CALL_TO_NULL, - ); + if features.gc() { + // If we are in the Wasm GC world, then we need to perform + // an actual subtype check at runtime. Fall through to below + // to do that. + } else { + // Otherwise if the types don't match then either (a) this + // is a null pointer or (b) it's a pointer with the wrong + // type. Figure out which and trap here. + // + // If it's possible to have a null here then try to load the + // type information. If that fails due to the function being + // a null pointer, then this was a call to null. Otherwise + // if it succeeds then we know it won't match, so trap + // anyway. + if table.table.ref_type.nullable { + if self.env.signals_based_traps() { + let mem_flags = ir::MemFlags::trusted().with_readonly(); + self.builder.ins().load( + sig_id_type, + mem_flags.with_trap_code(Some(crate::TRAP_INDIRECT_CALL_TO_NULL)), + funcref_ptr, + i32::from(self.env.offsets.ptr.vm_func_ref_type_index()), + ); + } else { + self.env.trapz( + self.builder, + funcref_ptr, + crate::TRAP_INDIRECT_CALL_TO_NULL, + ); + } } + self.env.trap(self.builder, crate::TRAP_BAD_SIGNATURE); + return CheckIndirectCallTypeSignature::StaticTrap; } - self.env.trap(self.builder, crate::TRAP_BAD_SIGNATURE); - return CheckIndirectCallTypeSignature::StaticTrap; } // Tables of `nofunc` can only be inhabited by null, so go ahead and @@ -1480,12 +1492,18 @@ impl<'a, 'func, 'module_env> Call<'a, 'func, 'module_env> { self.env .load_funcref_type_index(&mut self.builder.cursor(), mem_flags, funcref_ptr); - // Check that they match. - let cmp = self - .builder - .ins() - .icmp(IntCC::Equal, callee_sig_id, caller_sig_id); - self.env.trapz(self.builder, cmp, crate::TRAP_BAD_SIGNATURE); + // Check that they match: in the case of Wasm GC, this means doing a + // full subtype check. Otherwise, we do a simple equality check. + let matches = if features.gc() { + self.env + .is_subtype(self.builder, callee_sig_id, caller_sig_id) + } else { + self.builder + .ins() + .icmp(IntCC::Equal, callee_sig_id, caller_sig_id) + }; + self.env + .trapz(self.builder, matches, crate::TRAP_BAD_SIGNATURE); CheckIndirectCallTypeSignature::Runtime } @@ -2617,13 +2635,21 @@ impl<'module_environment> crate::translate::FuncEnvironment fn translate_call_indirect( &mut self, builder: &mut FunctionBuilder, + features: &WasmFeatures, table_index: TableIndex, ty_index: TypeIndex, sig_ref: ir::SigRef, callee: ir::Value, call_args: &[ir::Value], ) -> WasmResult> { - Call::new(builder, self).indirect_call(table_index, ty_index, sig_ref, callee, call_args) + Call::new(builder, self).indirect_call( + features, + table_index, + ty_index, + sig_ref, + callee, + call_args, + ) } fn translate_call( @@ -2660,6 +2686,7 @@ impl<'module_environment> crate::translate::FuncEnvironment fn translate_return_call_indirect( &mut self, builder: &mut FunctionBuilder, + features: &WasmFeatures, table_index: TableIndex, ty_index: TypeIndex, sig_ref: ir::SigRef, @@ -2667,6 +2694,7 @@ impl<'module_environment> crate::translate::FuncEnvironment call_args: &[ir::Value], ) -> WasmResult<()> { Call::new_tail(builder, self).indirect_call( + features, table_index, ty_index, sig_ref, diff --git a/crates/cranelift/src/translate/code_translator.rs b/crates/cranelift/src/translate/code_translator.rs index d768b2b3b0dc..a1d3ba57f37c 100644 --- a/crates/cranelift/src/translate/code_translator.rs +++ b/crates/cranelift/src/translate/code_translator.rs @@ -650,6 +650,7 @@ pub fn translate_operator( let call = environ.translate_call_indirect( builder, + validator.features(), TableIndex::from_u32(*table_index), TypeIndex::from_u32(*type_index), sigref, @@ -717,6 +718,7 @@ pub fn translate_operator( environ.translate_return_call_indirect( builder, + validator.features(), TableIndex::from_u32(*table_index), TypeIndex::from_u32(*type_index), sigref, diff --git a/crates/cranelift/src/translate/environ/spec.rs b/crates/cranelift/src/translate/environ/spec.rs index 2225e4890fd6..1d1418d342f4 100644 --- a/crates/cranelift/src/translate/environ/spec.rs +++ b/crates/cranelift/src/translate/environ/spec.rs @@ -15,7 +15,7 @@ use cranelift_codegen::isa::{TargetFrontendConfig, TargetIsa}; use cranelift_entity::PrimaryMap; use cranelift_frontend::FunctionBuilder; use smallvec::SmallVec; -use wasmparser::Operator; +use wasmparser::{Operator, WasmFeatures}; use wasmtime_environ::{ DataIndex, ElemIndex, FuncIndex, GlobalIndex, MemoryIndex, TableIndex, TypeConvert, TypeIndex, WasmHeapType, WasmRefType, WasmResult, @@ -196,6 +196,7 @@ pub trait FuncEnvironment: TargetEnvironment { fn translate_call_indirect( &mut self, builder: &mut FunctionBuilder, + features: &WasmFeatures, table_index: TableIndex, sig_index: TypeIndex, sig_ref: ir::SigRef, @@ -235,6 +236,7 @@ pub trait FuncEnvironment: TargetEnvironment { fn translate_return_call_indirect( &mut self, builder: &mut FunctionBuilder, + features: &WasmFeatures, table_index: TableIndex, sig_index: TypeIndex, sig_ref: ir::SigRef, diff --git a/crates/wasmtime/src/runtime/vm/libcalls.rs b/crates/wasmtime/src/runtime/vm/libcalls.rs index bbe86f3b9f32..e3dcdf999ffc 100644 --- a/crates/wasmtime/src/runtime/vm/libcalls.rs +++ b/crates/wasmtime/src/runtime/vm/libcalls.rs @@ -1012,11 +1012,14 @@ unsafe fn is_subtype( let actual = VMSharedTypeIndex::from_u32(actual_engine_type); let expected = VMSharedTypeIndex::from_u32(expected_engine_type); - (*instance.store()) + let is_subtype: bool = (*instance.store()) .engine() .signatures() .is_subtype(actual, expected) - .into() + .into(); + + log::trace!("is_subtype(actual={actual:?}, expected={expected:?}) -> {is_subtype}",); + is_subtype } // Implementation of `memory.atomic.notify` for locally defined memories. diff --git a/tests/wast.rs b/tests/wast.rs index 992db084f026..80d8f6846710 100644 --- a/tests/wast.rs +++ b/tests/wast.rs @@ -204,8 +204,6 @@ fn should_fail(test: &Path, strategy: Strategy) -> bool { } } - let unsupported_gc_tests = ["type-subtyping.wast"]; - for part in test.iter() { // Not implemented in Wasmtime yet if part == "exception-handling" { @@ -228,15 +226,6 @@ fn should_fail(test: &Path, strategy: Strategy) -> bool { { return true; } - if unsupported_gc_tests.iter().any(|i| test.ends_with(i)) { - return true; - } - } - - // Implementation of the GC proposal is a work-in-progress, this is - // a list of all currently known-to-fail tests. - if part == "gc" { - return unsupported_gc_tests.iter().any(|i| test.ends_with(i)); } }