Skip to content

Commit

Permalink
Implement subtype checking for [return_]call_indirect instructions
Browse files Browse the repository at this point in the history 
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).
  • Loading branch information
@fitzgen
fitzgen committed Oct 10, 2024
1 parent 0146260 commit 70bfce2
Show file tree
Hide file tree
Showing 6 changed files with 74 additions and 50 deletions.
4 changes: 2 additions & 2 deletions cranelift/codegen/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)*);
}
// }
};
}

Expand Down
96 changes: 62 additions & 34 deletions crates/cranelift/src/func_environ.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -1294,13 +1294,15 @@ 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,
callee: ir::Value,
call_args: &[ir::Value],
) -> WasmResult<Option<ir::Inst>> {
let (code_ptr, callee_vmctx) = match self.check_and_load_code_and_callee_vmctx(
features,
table_index,
ty_index,
callee,
Expand All @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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<Option<ir::Inst>> {
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(
Expand Down Expand Up @@ -2660,13 +2686,15 @@ 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,
callee: ir::Value,
call_args: &[ir::Value],
) -> WasmResult<()> {
Call::new_tail(builder, self).indirect_call(
features,
table_index,
ty_index,
sig_ref,
Expand Down
2 changes: 2 additions & 0 deletions crates/cranelift/src/translate/code_translator.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -650,6 +650,7 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(

let call = environ.translate_call_indirect(
builder,
validator.features(),
TableIndex::from_u32(*table_index),
TypeIndex::from_u32(*type_index),
sigref,
Expand Down Expand Up @@ -717,6 +718,7 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(

environ.translate_return_call_indirect(
builder,
validator.features(),
TableIndex::from_u32(*table_index),
TypeIndex::from_u32(*type_index),
sigref,
Expand Down
4 changes: 3 additions & 1 deletion crates/cranelift/src/translate/environ/spec.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
7 changes: 5 additions & 2 deletions crates/wasmtime/src/runtime/vm/libcalls.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
11 changes: 0 additions & 11 deletions tests/wast.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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" {
Expand All @@ -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));
}
}

Expand Down

0 comments on commit 70bfce2

Please sign in to comment.