Skip to content

Implement subtype checking for [return_]call_indirect instructions #9448

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
fitzgen merged 2 commits into from
Oct 10, 2024
Merged

Implement subtype checking for [return_]call_indirect instructions #9448

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b89dcb6
Implement subtype checking for `[return_]call_indirect` instructions
fitzgen Oct 10, 2024
91634f5
fix no-gc builds
fitzgen Oct 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 69 additions & 34 deletions crates/cranelift/src/func_environ.rs
View file
Open in desktop
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() {
Copy link
Copy Markdown
Member

@alexcrichton alexcrichton Oct 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be features.function_references() || features.gc()? Or just features.function_references()?

Copy link
Copy Markdown
Member Author

@fitzgen fitzgen Oct 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe just 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,25 @@ 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() {
#[cfg(feature = "gc")]
{
self.env
.is_subtype(self.builder, callee_sig_id, caller_sig_id)
}
#[cfg(not(feature = "gc"))]
{
unreachable!()
}
} 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 +2642,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 +2693,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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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