Function references (bytecodealliance#5288)

* Make wasmtime-types type check

* Make wasmtime-environ type check.

* Make wasmtime-runtime type check

* Make cranelift-wasm type check

* Make wasmtime-cranelift type check

* Make wasmtime type check

* Make wasmtime-wast type check

* Make testsuite compile

* Address Luna's comments

* Restore compatibility with effect-handlers/wasm-tools#func-ref-2

* Add function refs feature flag; support testing

* Provide function references support in helpers

- Always support Index in blocktypes
- Support Index as table type by pretending to be Func
- Etc

* Implement ref.as_non_null

* Add br_on_null

* Update Cargo.lock to use wasm-tools with peek

This will ultimately be reverted when we refer to
wasm-tools#function-references, which doesn't have peek, but does have type
annotations on CallRef

* Add call_ref

* Support typed function references in ref.null

* Implement br_on_non_null

* Remove extraneous flag; default func refs false

* Use IndirectCallToNull trap code for call_ref

* Factor common call_indirect / call_ref into a fn

* Remove copypasta clippy attribute / format

* Add a some more tests for typed table instructions

There certainly need to be many more, but this at least catches the bugs fixed
in the next commit

* Fix missing typed cases for table_grow, table_fill

* Document trap code; remove answered question

* Mark wasm-tools to wasmtime reftype infallible

* Fix reversed conditional

* Scope externref/funcref shorthands within WasmRefType

* Merge with upstream

* Make wasmtime compile again

* Fix warnings

* Remove Bot from the type algebra

* Fix table tests.

`wast::Cranelift::spec::function_references::table`
`wast::Cranelift::spec::function_references::table_pooling`

* Fix table{get,set} tests.

```
wast::Cranelift::misc::function_references::table_get
wast::Cranelift::misc::function_references::table_get_pooling
wast::Cranelift::misc::function_references::table_set
wast::Cranelift::misc::function_references::table_set_pooling
```

* Insert subtype check to fix local_get tests.

```
wast::Cranelift::spec::function_references::local_get
wast::Cranelift::spec::function_references::local_get_pooling
```

* Fix compilation of `br_on_non_null`.

The branch destinations were the other way round... :-)

Fixes the following test failures:
```
wast::Cranelift::spec::function_references::br_on_non_null
wast::Cranelift::spec::function_references::br_on_non_null_pooling
```

* Fix ref_as_non_null tests.

The test was failing due to the wrong error message being printed. As
per upstream folks' suggest we were using the trap code
`IndirectCallToNull`, but it produces an unexpected error message.

This commit reinstates the `NullReference` trap code. It produces the
expected error message. We will have to chat with the maintainers
upstream about how to handle these "test failures".

Fixes the following test failures:

```
wast::Cranelift::spec::function_references::ref_as_non_null
wast::Cranelift::spec::function_references::ref_as_non_null_pooling
```

* Fix a call_ref regression.

* Fix global tests.

Extend `is_matching_assert_invalid_error_message` to circumvent the textual error message failure.

Fixes the following test failures:
```
wast::Cranelift::spec::function_references::global
wast::Cranelift::spec::function_references::global_pooling
```

* Cargo update

* Update

* Spell out some cases in match_val

* Disgusting hack to subvert limitations of type reconstruction.

In the function `wasmtime::values::Val::ty()` attempts to reconstruct
the type of its underlying value purely based on the shape of the
value. With function references proposal this sort of reconstruction
is no longer complete as a source reference type may have been
nullable. Nullability is not inferrable by looking at the shape of the
runtime object alone.

Consequently, the runtime cannot reconstruct the type for
`Val::FuncRef` and `Val::ExternRef` by looking at their respective
shapes.

* Address workflows comments.

* null reference => null_reference for CLIF parsing compliance.

* Delete duplicate-loads-dynamic-memory-egraph (again)

* Idiomatic code change.

* Nullability subtyping + fix non-null storage check.

This commit removes the `hacky_eq` check in `func.rs`. Instead it is
replaced by a subtype check. This subtype check occurs in
`externals.rs` too.

This commit also fixes a bug. Previously, it was possible to store a
null reference into a non-null table cell. I have added to new test
cases for this bug: one for funcrefs and another for externrefs.

* Trigger unimplemented for typed function references. Format values.rs

* run cargo fmt

* Explicitly match on HeapType::Extern.

* Address cranelift-related feedback

* Remove PartialEq,Eq from ValType, RefType, HeapType.

* Pin wasmparser to a fairly recent commit.

* Run cargo fmt

* Ignore tail call tests.

* Remove garbage

* Revert changes to wasmtime public API.

* Run cargo fmt

* Get more CI passing (bytecodealliance#19)

* Undo Cargo.lock changes

* Fix build of cranelift tests

* Implement link-time matches relation. Disable tests failing due to lack of public API support.

* Run cargo fmt

* Run cargo fmt

* Initial implementation of eager table initialization

* Tidy up eager table initialisation

* Cargo fmt

* Ignore type-equivalence test

* Replace TODOs with descriptive comments.

* Various changes found during review (bytecodealliance#21)

* Clarify a comment

This isn't only used for null references

* Resolve a TODO in local init

Don't initialize non-nullable locals to null, instead skip
initialization entirely and wasm validation will ensure it's always
initialized in the scope where it's used.

* Clarify a comment and skipping the null check.

* Remove a stray comment

* Change representation of `WasmHeapType`

Use a `SignatureIndex` instead of a `u32` which while not 100% correct
should be more correct. This additionally renames the `Index` variant to
`TypedFunc` to leave space for future types which aren't functions to
not all go into an `Index` variant.

This required updates to Winch because `wasmtime_environ` types can no
longer be converted back to their `wasmparser` equivalents. Additionally
this means that all type translation needs to go through some form of
context to resolve indices which is now encapsulated in a `TypeConvert`
trait implemented in various locations.

* Refactor table initialization

Reduce some duplication and simplify some data structures to have a more
direct form of table initialization and a bit more graceful handling of
element-initialized tables. Additionally element-initialize tables are
now treated the same as if there's a large element segment initializing
them.

* Clean up some unrelated chagnes

* Simplify Table bindings slightly

* Remove a no-longer-needed TODO

* Add a FIXME for `SignatureIndex` in `WasmHeapType`

* Add a FIXME for panicking on exposing function-references types

* Fix a warning on nightly

* Fix tests for winch and cranelift

* Cargo fmt

* Fix arity mismatch in aarch64/abi

---------

Co-authored-by: Daniel Hillerström <[email protected]>
Co-authored-by: Daniel Hillerström <[email protected]>
Co-authored-by: Alex Crichton <[email protected]>

build.rs

@@ -30,6 +30,7 @@ fn main() -> anyhow::Result<()> {
             test_directory_module(out, "tests/misc_testsuite/threads", strategy)?;
             test_directory_module(out, "tests/misc_testsuite/memory64", strategy)?;
             test_directory_module(out, "tests/misc_testsuite/component-model", strategy)?;
+            test_directory_module(out, "tests/misc_testsuite/function-references", strategy)?;
             Ok(())
         })?;
 
@@ -39,6 +40,11 @@ fn main() -> anyhow::Result<()> {
             // out.
             if spec_tests > 0 {
                 test_directory_module(out, "tests/spec_testsuite/proposals/memory64", strategy)?;
+                test_directory_module(
+                    out,
+                    "tests/spec_testsuite/proposals/function-references",
+                    strategy,
+                )?;
                 test_directory_module(
                     out,
                     "tests/spec_testsuite/proposals/multi-memory",
@@ -187,6 +193,30 @@ fn ignore(testsuite: &str, testname: &str, strategy: &str) -> bool {
         return true;
     }
 
+    // Tail calls are not yet implemented.
+    if testname.contains("return_call") {
+        return true;
+    }
+
+    if testsuite == "function_references" {
+        // The following tests fail due to function references not yet
+        // being exposed in the public API.
+        if testname == "ref_null" || testname == "local_init" {
+            return true;
+        }
+        // This test fails due to incomplete support for the various
+        // table/elem syntactic sugar in wasm-tools/wast.
+        if testname == "br_table" {
+            return true;
+        }
+        // This test fails due to the current implementation of type
+        // canonicalisation being broken as a result of
+        // #[derive(hash)] on WasmHeapType.
+        if testname == "type_equivalence" {
+            return true;
+        }
+    }
+
     match env::var("CARGO_CFG_TARGET_ARCH").unwrap().as_str() {
         "s390x" => {
             // FIXME: These tests fail under qemu due to a qemu bug.

cranelift/codegen/src/ir/trapcode.rs

@@ -51,6 +51,9 @@ pub enum TrapCode {
 
     /// A user-defined trap code.
     User(u16),
+
+    /// A null reference was encountered which was required to be non-null.
+    NullReference,
 }
 
 impl TrapCode {
@@ -68,6 +71,7 @@ impl TrapCode {
             TrapCode::BadConversionToInteger,
             TrapCode::UnreachableCodeReached,
             TrapCode::Interrupt,
+            TrapCode::NullReference,
         ]
     }
 }
@@ -88,6 +92,7 @@ impl Display for TrapCode {
             UnreachableCodeReached => "unreachable",
             Interrupt => "interrupt",
             User(x) => return write!(f, "user{}", x),
+            NullReference => "null_reference",
         };
         f.write_str(identifier)
     }
@@ -110,6 +115,7 @@ impl FromStr for TrapCode {
             "bad_toint" => Ok(BadConversionToInteger),
             "unreachable" => Ok(UnreachableCodeReached),
             "interrupt" => Ok(Interrupt),
+            "null_reference" => Ok(NullReference),
             _ if s.starts_with("user") => s[4..].parse().map(User).map_err(|_| ()),
             _ => Err(()),
         }

cranelift/codegen/src/verifier/mod.rs

@@ -1175,7 +1175,10 @@ impl<'a> Verifier<'a> {
                 errors.report((
                     inst,
                     self.context(inst),
-                    format!("has an invalid controlling type {}", ctrl_type),
+                    format!(
+                        "has an invalid controlling type {} (allowed set is {:?})",
+                        ctrl_type, value_typeset
+                    ),
                 ));
             }
 

cranelift/filetests/src/test_wasm/env.rs

@@ -13,6 +13,7 @@ use cranelift_codegen::{
 };
 use cranelift_wasm::{
     DummyEnvironment, FuncEnvironment, FuncIndex, ModuleEnvironment, TargetEnvironment,
+    TypeConvert, TypeIndex, WasmHeapType,
 };
 
 pub struct ModuleEnv {
@@ -235,6 +236,12 @@ impl<'data> ModuleEnvironment<'data> for ModuleEnv {
     }
 }
 
+impl TypeConvert for ModuleEnv {
+    fn lookup_heap_type(&self, _index: TypeIndex) -> WasmHeapType {
+        todo!()
+    }
+}
+
 pub struct FuncEnv<'a> {
     pub inner: cranelift_wasm::DummyFuncEnvironment<'a>,
     pub config: TestConfig,
@@ -261,6 +268,12 @@ impl<'a> FuncEnv<'a> {
     }
 }
 
+impl TypeConvert for FuncEnv<'_> {
+    fn lookup_heap_type(&self, _index: TypeIndex) -> WasmHeapType {
+        todo!()
+    }
+}
+
 impl<'a> TargetEnvironment for FuncEnv<'a> {
     fn target_config(&self) -> TargetFrontendConfig {
         self.inner.target_config()
@@ -622,4 +635,14 @@ impl<'a> FuncEnvironment for FuncEnv<'a> {
     fn is_x86(&self) -> bool {
         self.config.target.contains("x86_64")
     }
+
+    fn translate_call_ref(
+        &mut self,
+        _builder: &mut cranelift_frontend::FunctionBuilder<'_>,
+        _ty: ir::SigRef,
+        _func: ir::Value,
+        _args: &[ir::Value],
+    ) -> cranelift_wasm::WasmResult<ir::Inst> {
+        unimplemented!()
+    }
 }

cranelift/wasm/src/code_translator.rs

@@ -81,7 +81,6 @@ use crate::translation_utils::{
 };
 use crate::wasm_unsupported;
 use crate::{FuncIndex, GlobalIndex, MemoryIndex, TableIndex, TypeIndex, WasmResult};
-use core::convert::TryInto;
 use core::{i32, u32};
 use cranelift_codegen::ir::condcodes::{FloatCC, IntCC};
 use cranelift_codegen::ir::immediates::Offset32;
@@ -1152,7 +1151,8 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
             translate_fcmp(FloatCC::LessThanOrEqual, builder, state)
         }
         Operator::RefNull { hty } => {
-            state.push1(environ.translate_ref_null(builder.cursor(), (*hty).try_into()?)?)
+            let hty = environ.convert_heap_type(*hty);
+            state.push1(environ.translate_ref_null(builder.cursor(), hty)?)
         }
         Operator::RefIsNull => {
             let value = state.pop1();
@@ -2335,16 +2335,74 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
             state.push1(builder.ins().iadd(dot32, c));
         }
 
-        Operator::CallRef { .. }
-        | Operator::ReturnCallRef { .. }
-        | Operator::BrOnNull { .. }
-        | Operator::BrOnNonNull { .. }
-        | Operator::RefAsNonNull => {
+        Operator::ReturnCallRef { type_index: _ } => {
             return Err(wasm_unsupported!(
-                "proposed function-references operator {:?}",
+                "proposed tail-call operator for function references {:?}",
                 op
             ));
         }
+        Operator::BrOnNull { relative_depth } => {
+            let r = state.pop1();
+            let (br_destination, inputs) = translate_br_if_args(*relative_depth, state);
+            let is_null = environ.translate_ref_is_null(builder.cursor(), r)?;
+            let else_block = builder.create_block();
+            canonicalise_brif(builder, is_null, br_destination, inputs, else_block, &[]);
+
+            builder.seal_block(else_block); // The only predecessor is the current block.
+            builder.switch_to_block(else_block);
+            state.push1(r);
+        }
+        Operator::BrOnNonNull { relative_depth } => {
+            // We write this a bit differently from the spec to avoid an extra
+            // block/branch and the typed accounting thereof. Instead of the
+            // spec's approach, it's described as such:
+            // Peek the value val from the stack.
+            // If val is ref.null ht, then: pop the value val from the stack.
+            // Else: Execute the instruction (br relative_depth).
+            let is_null = environ.translate_ref_is_null(builder.cursor(), state.peek1())?;
+            let (br_destination, inputs) = translate_br_if_args(*relative_depth, state);
+            let else_block = builder.create_block();
+            canonicalise_brif(builder, is_null, else_block, &[], br_destination, inputs);
+
+            // In the null case, pop the ref
+            state.pop1();
+
+            builder.seal_block(else_block); // The only predecessor is the current block.
+
+            // The rest of the translation operates on our is null case, which is
+            // currently an empty block
+            builder.switch_to_block(else_block);
+        }
+        Operator::CallRef { type_index } => {
+            // Get function signature
+            // `index` is the index of the function's signature and `table_index` is the index of
+            // the table to search the function in.
+            let (sigref, num_args) = state.get_indirect_sig(builder.func, *type_index, environ)?;
+            let callee = state.pop1();
+
+            // Bitcast any vector arguments to their default type, I8X16, before calling.
+            let args = state.peekn_mut(num_args);
+            bitcast_wasm_params(environ, sigref, args, builder);
+
+            let call =
+                environ.translate_call_ref(builder, sigref, callee, state.peekn(num_args))?;
+
+            let inst_results = builder.inst_results(call);
+            debug_assert_eq!(
+                inst_results.len(),
+                builder.func.dfg.signatures[sigref].returns.len(),
+                "translate_call_ref results should match the call signature"
+            );
+            state.popn(num_args);
+            state.pushn(inst_results);
+        }
+        Operator::RefAsNonNull => {
+            let r = state.pop1();
+            let is_null = environ.translate_ref_is_null(builder.cursor(), r)?;
+            builder.ins().trapnz(is_null, ir::TrapCode::NullReference);
+            state.push1(r);
+        }
+
         Operator::I31New | Operator::I31GetS | Operator::I31GetU => {
             unimplemented!("GC operators not yet implemented")
         }

cranelift/wasm/src/environ/dummy.rs

@@ -11,8 +11,8 @@ use crate::state::FuncTranslationState;
 use crate::WasmType;
 use crate::{
     DataIndex, DefinedFuncIndex, ElemIndex, FuncIndex, Global, GlobalIndex, GlobalInit, Heap,
-    HeapData, HeapStyle, Memory, MemoryIndex, Table, TableIndex, TypeIndex, WasmFuncType,
-    WasmResult,
+    HeapData, HeapStyle, Memory, MemoryIndex, Table, TableIndex, TypeConvert, TypeIndex,
+    WasmFuncType, WasmHeapType, WasmResult,
 };
 use core::convert::TryFrom;
 use cranelift_codegen::cursor::FuncCursor;
@@ -251,6 +251,12 @@ impl<'dummy_environment> DummyFuncEnvironment<'dummy_environment> {
     }
 }
 
+impl<'dummy_environment> TypeConvert for DummyFuncEnvironment<'dummy_environment> {
+    fn lookup_heap_type(&self, _index: TypeIndex) -> WasmHeapType {
+        unimplemented!()
+    }
+}
+
 impl<'dummy_environment> TargetEnvironment for DummyFuncEnvironment<'dummy_environment> {
     fn target_config(&self) -> TargetFrontendConfig {
         self.mod_info.config
@@ -279,7 +285,7 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ
                 WasmType::F32 => ir::types::F32,
                 WasmType::F64 => ir::types::F64,
                 WasmType::V128 => ir::types::I8X16,
-                WasmType::FuncRef | WasmType::ExternRef => ir::types::R64,
+                WasmType::Ref(_) => ir::types::R64,
             },
         })
     }
@@ -464,6 +470,16 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ
         Ok(pos.ins().Call(ir::Opcode::Call, INVALID, callee, args).0)
     }
 
+    fn translate_call_ref(
+        &mut self,
+        _builder: &mut FunctionBuilder,
+        _sig_ref: ir::SigRef,
+        _callee: ir::Value,
+        _call_args: &[ir::Value],
+    ) -> WasmResult<ir::Inst> {
+        todo!("Implement dummy translate_call_ref")
+    }
+
     fn translate_memory_grow(
         &mut self,
         mut pos: FuncCursor,
@@ -658,6 +674,12 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ
     }
 }
 
+impl TypeConvert for DummyEnvironment {
+    fn lookup_heap_type(&self, _index: TypeIndex) -> WasmHeapType {
+        unimplemented!()
+    }
+}
+
 impl TargetEnvironment for DummyEnvironment {
     fn target_config(&self) -> TargetFrontendConfig {
         self.info.config
@@ -683,7 +705,7 @@ impl<'data> ModuleEnvironment<'data> for DummyEnvironment {
                 WasmType::F32 => ir::types::F32,
                 WasmType::F64 => ir::types::F64,
                 WasmType::V128 => ir::types::I8X16,
-                WasmType::FuncRef | WasmType::ExternRef => reference_type,
+                WasmType::Ref(_) => reference_type,
             })
         };
         sig.params.extend(wasm.params().iter().map(&mut cvt));

cranelift/wasm/src/environ/spec.rs

@@ -9,8 +9,8 @@
 use crate::state::FuncTranslationState;
 use crate::{
     DataIndex, ElemIndex, FuncIndex, Global, GlobalIndex, GlobalInit, Heap, HeapData, Memory,
-    MemoryIndex, SignatureIndex, Table, TableIndex, Tag, TagIndex, TypeIndex, WasmError,
-    WasmFuncType, WasmResult, WasmType,
+    MemoryIndex, SignatureIndex, Table, TableIndex, Tag, TagIndex, TypeConvert, TypeIndex,
+    WasmError, WasmFuncType, WasmHeapType, WasmResult,
 };
 use core::convert::From;
 use cranelift_codegen::cursor::FuncCursor;
@@ -44,7 +44,7 @@ pub enum GlobalVariable {
 }
 
 /// Environment affecting the translation of a WebAssembly.
-pub trait TargetEnvironment {
+pub trait TargetEnvironment: TypeConvert {
     /// Get the information needed to produce Cranelift IR for the given target.
     fn target_config(&self) -> TargetFrontendConfig;
 
@@ -70,7 +70,7 @@ pub trait TargetEnvironment {
     /// 32-bit architectures. If you override this, then you should also
     /// override `FuncEnvironment::{translate_ref_null, translate_ref_is_null}`
     /// as well.
-    fn reference_type(&self, ty: WasmType) -> ir::Type {
+    fn reference_type(&self, ty: WasmHeapType) -> ir::Type {
         let _ = ty;
         match self.pointer_type() {
             ir::types::I32 => ir::types::R32,
@@ -208,6 +208,22 @@ pub trait FuncEnvironment: TargetEnvironment {
         Ok(pos.ins().call(callee, call_args))
     }
 
+    /// Translate a `call_ref` WebAssembly instruction at `pos`.
+    ///
+    /// Insert instructions at `pos` for an indirect call to the
+    /// function `callee`. The `callee` value will have type `Ref`.
+    ///
+    /// The signature `sig_ref` was previously created by `make_indirect_sig()`.
+    ///
+    /// Return the call instruction whose results are the WebAssembly return values.
+    fn translate_call_ref(
+        &mut self,
+        builder: &mut FunctionBuilder,
+        sig_ref: ir::SigRef,
+        callee: ir::Value,
+        call_args: &[ir::Value],
+    ) -> WasmResult<ir::Inst>;
+
     /// Translate a `memory.grow` WebAssembly instruction.
     ///
     /// The `index` provided identifies the linear memory to grow, and `heap` is the heap reference
@@ -373,7 +389,11 @@ pub trait FuncEnvironment: TargetEnvironment {
     /// null sentinel is not a null reference type pointer for your type. If you
     /// override this method, then you should also override
     /// `translate_ref_is_null` as well.
-    fn translate_ref_null(&mut self, mut pos: FuncCursor, ty: WasmType) -> WasmResult<ir::Value> {
+    fn translate_ref_null(
+        &mut self,
+        mut pos: FuncCursor,
+        ty: WasmHeapType,
+    ) -> WasmResult<ir::Value> {
         let _ = ty;
         Ok(pos.ins().null(self.reference_type(ty)))
     }
@@ -554,7 +574,7 @@ pub trait FuncEnvironment: TargetEnvironment {
 /// An object satisfying the `ModuleEnvironment` trait can be passed as argument to the
 /// [`translate_module`](fn.translate_module.html) function. These methods should not be called
 /// by the user, they are only for `cranelift-wasm` internal use.
-pub trait ModuleEnvironment<'data> {
+pub trait ModuleEnvironment<'data>: TypeConvert {
     /// Provides the number of types up front. By default this does nothing, but
     /// implementations can use this to preallocate memory if desired.
     fn reserve_types(&mut self, _num: u32) -> WasmResult<()> {