Function references #5288

CosineP · 2022-11-17T04:42:43Z

This patch, written by @dhil and I, implements the (complete) function references proposal, forming the wasmtime side of the existing pull request bytecodealliance/wasm-tools#701. It is missing some obvious things:

call_ref is simply translated as a cranelift indirect_call. A faster cranelift indirect call that does not check the callee signature can (and should) eventually be added and used. Further, no null check is elided.
The wasm-tools PR has advanced past the version we depend on here. We hope to update all at once, once the wasm-tools side has fully settled. This will result in the removal of a number of ValType::Bot branches, the addition of the type annotation on call_ref, and a handful of other convenient things. The version we depend on is called func-ref-2, if you need to reference it*.
Wasmtime, too, has of course advanced past our parent. I tried to rebase on the latest changes, but due to particular Cargo.toml changes, I think this will be much easier when we've retargetted to the latest wasm-tools#701 and can keep our dependencies consistent in the history

Otherwise, things should be mostly obvious. Typed function references are represented by the existing untyped function references. Even though it's held up by wasm-tools, we ended up with a fairly polished changeset that we figured we could send out and get feedback on. There's no rush on our end, as we've been happily using this in our typed continuations work for some time now. But we're happy to discuss any changes, problems, or suggestions!

* We added a public peek method to the validator to attain a signature for call_ref. This is now unnecesary because call_ref comes with a type annotation, and will disappear fairly conveniently.

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

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

github-actions · 2022-11-17T04:49:56Z

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "cranelift", "cranelift:wasm", "wasmtime:api", "wasmtime:config"

Thus the following users have been cc'd because of the following labels:

peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

github-actions · 2022-11-17T04:50:08Z

Label Messager: wasmtime:config

It looks like you are changing Wasmtime's configuration options. Make sure to
complete this check list:

If you added a new Config method, you wrote extensive documentation for
it.

Our documentation should be of the following form:

Short, simple summary sentence.

More details. These details can be multiple paragraphs. There should be
information about not just the method, but its parameters and results as
well.

Is this method fallible? If so, when can it return an error?

Can this method panic? If so, when does it panic?

# Example

Optional example here.

If you added a new Config method, or modified an existing one, you
ensured that this configuration is exercised by the fuzz targets.

For example, if you expose a new strategy for allocating the next instance
slot inside the pooling allocator, you should ensure that at least one of our
fuzz targets exercises that new strategy.

Often, all that is required of you is to ensure that there is a knob for this
configuration option in wasmtime_fuzzing::Config (or one
of its nested structs).

Rarely, this may require authoring a new fuzz target to specifically test this
configuration. See our docs on fuzzing for more details.
If you are enabling a configuration option by default, make sure that it
has been fuzzed for at least two weeks before turning it on by default.

To modify this label's message, edit the .github/label-messager/wasmtime-config.md file.

To add new label messages or remove existing label messages, edit the
.github/label-messager.json configuration file.

Learn more.

bjorn3 · 2022-11-17T09:35:49Z

call_ref is simply translated as a cranelift indirect_call. A faster cranelift indirect call that does not check the callee signature can (and should) eventually be added and used. Further, no null check is elided.

call_indirect doesn't check the function signature, nor does it check for null pointers.

alexcrichton

Thanks for this! I left a slew of comments in various places. At a high level one thing I'd encourage is to add your own *.wast tests as you see appropriate. The upstream test suite is rarely ever all that comprehensive so if you run into something that looks like an issue but the upstream test suite doesn't cover it feel free to add custom *.wast files here.

Otherwise one overall comment is that the embedding API here I think may need some relatively large work. One thing I might recommend is mostly trying to punt on it for now. For example not supporting tables of typed functions, arguments/parameters, runtime type information about this, etc. There's a fair bit of design that needs to happen here and if you're mostly interested in the stack switching proposal it might be best to basically skip the embedding API and jump only have the internal implementation necessary here.

cranelift/codegen/src/ir/trapcode.rs

cranelift/wasm/src/code_translator.rs

cranelift/wasm/src/environ/spec.rs

cranelift/wasm/src/func_translator.rs

cranelift/wasm/src/state/module_state.rs

crates/wasmtime/src/config.rs

crates/wasmtime/src/externals.rs

crates/wasmtime/src/types.rs

crates/wasmtime/src/values.rs

cranelift/wasm/src/code_translator.rs

fitzgen

Thanks! Excited to see this taking shape!

cranelift/wasm/src/code_translator.rs

crates/cranelift/src/func_environ.rs

crates/types/src/lib.rs

crates/wasmtime/src/trap.rs

CosineP · 2023-05-22T15:49:36Z

This needs to happen for typed continuations support as well. Currently we assume everything is a function, but if we do this refactor I think we can look up in the types table whether a ref is a function or a continuation, which we need to know in a number of places in Wasmtime (at least in the embedding API; we managed to fudge the table layout stuff). But as a result I can say firsthand it is absolutely a hefty undertaking which is why we resorted to our current hack. I may still have a local branch with part of this refactoring underway.

alexcrichton · 2023-05-22T18:44:46Z

If you're ok with it, I think it's ok to set the test to ignore and land this. It's true that this refactoring will require a bit of finesse, but it's one I'm happy to take on myself rather than having y'all do yet-more work on top of what you've already got here.

Otherwise I'd be happy to merge this with a green CI run (which will require ignoring tests) and a list of items to tackle tracked in an issue (such as fixing the failing tests, this possible refactoring, the embedder API, etc)

…tion-references

dhil · 2023-05-23T12:10:04Z

I have added the test type-equivalence.wast to the ignore set. How should we go about documenting the issues? Should I open each issue as a separate issue here on GitHub?

alexcrichton · 2023-05-23T13:33:21Z

I think one tracking issue should be good to start off with and it can branch from there as necessary. I'll work on giving this a final pass today. Thanks again for y'all's work here!

* 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

alexcrichton · 2023-05-24T14:41:53Z

@dhil to answer this question (over here for a bit more visibility) my thinking is that at the wasm AST layer there's only "this index" as a type but at the Wasmtime layer we'll be able to be more principled than that by having, for example:

enum WasmHeapType {
    Func,
    Extern,
    TypedFunc(SignatureIndex),
    Array,
    TypedArray(StorageType),
    Struct,
    TypedStruct(StructIndex),
    // ..
}

or something along the lines of that where WasmHeapType stores effectively the discriminant of what it refers to rather than requiring the indexing operation to then verify the type of the result.

One of the things I was worried about was that there was the pervasive assumption that WasmHeapType::Index(_) was a function which won't be true once the GC proposal was implemented. I wanted to head off issues there by having the Wasmtime-level name be called TypedFunc so it's clear that all the cases handling Func and TypedFunc are only there for handling function-related things. In the future when more variants are added it'll require adding more match arms to handle in all these places.

alexcrichton

Ok this looks good to go to me, thanks again @CosineP and @dhil!

I'd like another pair of eyes on this though given what's changing here, so I'm going to request that @fitzgen take a look as well. If y'all would be ok opening up a tracking issue for the remaining items that'd be appreciated!

dhil · 2023-05-25T11:41:07Z

I've opened a issue to track the unresolved bits of this PR. See #6455. I hopefully I managed to cover them all.

Merge with upstream

fitzgen

Let's get this merged! Thanks @dhil and @CosineP!

fitzgen · 2023-05-25T18:53:33Z

cranelift/wasm/src/translation_utils.rs

@@ -23,27 +22,18 @@ where
    return Ok(match ty {
        wasmparser::BlockType::Empty => {
            let params: &'static [wasmparser::ValType] = &[];
-            let results: &'static [wasmparser::ValType] = &[];
+            let results: std::vec::Vec<wasmparser::ValType> = vec![];


This function's allocations were previously showing up in DHAT for some profiling I did, which is why we ended up wit the super generic interface to this function and weird &'static [_] stuff going on. All that is to say that I think we should probably use a small vec here.

Fine to do as a follow up.

fitzgen · 2023-05-25T19:03:43Z

crates/cranelift/src/func_environ.rs

+        // FIXME: the wasm type system tracks enough information to know whether
+        // `callee` is a null reference or not. In some situations it can be
+        // statically known here that `callee` cannot be null in which case this
+        // null check can be elided. This requires feeding type information from
+        // wasmparser's validator into this function, however, which is not
+        // easily done at this time.


Can we file an issue to track this?

I've added this to #6455. I can hoist it into a separate issue if need be.

fitzgen · 2023-05-25T19:47:21Z

Seems like there are some conflicts and this needs a rebase/merge.

dhil · 2023-05-26T08:23:19Z

Seems like there are some conflicts and this needs a rebase/merge.

I have synchronised with upstream again. Hopefully everything ticks green again :-)

alexcrichton · 2023-05-26T16:07:02Z

🎊

dhil and others added 18 commits November 16, 2022 22:40

Make wasmtime-types type check

3799e10

Make wasmtime-environ type check.

7a4bc7e

Make wasmtime-runtime type check

6e73ec4

Make cranelift-wasm type check

cb0fa63

Make wasmtime-cranelift type check

11acbe2

Make wasmtime type check

37c32fc

Make wasmtime-wast type check

5af12e6

Make testsuite compile

98b26b0

Address Luna's comments

1c6eb7e

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

0c69cba

Add function refs feature flag; support testing

931feeb

Provide function references support in helpers

4897c59

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

Implement ref.as_non_null

fda8182

Add br_on_null

1959304

Update Cargo.lock to use wasm-tools with peek

e14fde5

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

6e31760

Support typed function references in ref.null

aa71b96

Implement br_on_non_null

1f411e6

github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:wasm wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:config Issues related to the configuration of Wasmtime labels Nov 17, 2022

alexcrichton mentioned this pull request Nov 17, 2022

Remove some dead code in the cranelift-wasm crate #5290

Merged

alexcrichton reviewed Nov 17, 2022

View reviewed changes

fitzgen reviewed Nov 17, 2022

View reviewed changes

cranelift/wasm/src/code_translator.rs Outdated Show resolved Hide resolved

fitzgen reviewed Nov 17, 2022

View reviewed changes

cranelift/wasm/src/code_translator.rs Outdated Show resolved Hide resolved

crates/cranelift/src/func_environ.rs Outdated Show resolved Hide resolved

crates/types/src/lib.rs Outdated Show resolved Hide resolved

crates/wasmtime/src/trap.rs Outdated Show resolved Hide resolved

Remove extraneous flag; default func refs false

a589b99

dhil added 3 commits May 23, 2023 13:34

Merge branch 'main' of github.com:bytecodealliance/wasmtime into func…

fb94ffb

…tion-references

Ignore type-equivalence test

c04a013

Replace TODOs with descriptive comments.

6f66bb5

alexcrichton mentioned this pull request May 23, 2023

Function references implementation for Wasmtime #6441

Closed

alexcrichton approved these changes May 24, 2023

View reviewed changes

alexcrichton requested review from fitzgen and removed request for abrown May 24, 2023 14:47

CosineP mentioned this pull request May 24, 2023

Various changes found during review effect-handlers/wasmtime#21

Merged

dhil mentioned this pull request May 25, 2023

Function references unresolved issues #6455

Closed

6 tasks

alexcrichton and others added 2 commits May 25, 2023 08:24

Merge remote-tracking branch 'origin/main' into function-references

0adcec6

Merge pull request #22 from alexcrichton/function-references

36e9f7e

Merge with upstream

fitzgen approved these changes May 25, 2023

View reviewed changes

dhil added 2 commits May 26, 2023 10:09

Merge with upstream

e467bba

Cargo fmt

aa4ffdc

Fix arity mismatch in aarch64/abi

96a9b55

alexcrichton enabled auto-merge May 26, 2023 15:08

alexcrichton added this pull request to the merge queue May 26, 2023

Merged via the queue into bytecodealliance:main with commit 92024ad May 26, 2023
19 checks passed

tw-atroehrsm mentioned this pull request Feb 6, 2024

Implement the WebAssembly GC Proposal #5032

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Function references #5288

Function references #5288

CosineP commented Nov 17, 2022 •

edited

github-actions bot commented Nov 17, 2022

github-actions bot commented Nov 17, 2022

bjorn3 commented Nov 17, 2022

alexcrichton left a comment

fitzgen left a comment

CosineP commented May 22, 2023

alexcrichton commented May 22, 2023

dhil commented May 23, 2023

alexcrichton commented May 23, 2023

alexcrichton commented May 24, 2023

alexcrichton left a comment

dhil commented May 25, 2023

fitzgen left a comment

fitzgen May 25, 2023

fitzgen May 25, 2023

dhil May 26, 2023

fitzgen commented May 25, 2023

dhil commented May 26, 2023

alexcrichton commented May 26, 2023

Function references #5288

Function references #5288

Conversation

CosineP commented Nov 17, 2022 • edited

github-actions bot commented Nov 17, 2022

Subscribe to Label Action

github-actions bot commented Nov 17, 2022

Label Messager: wasmtime:config

bjorn3 commented Nov 17, 2022

alexcrichton left a comment

Choose a reason for hiding this comment

fitzgen left a comment

Choose a reason for hiding this comment

CosineP commented May 22, 2023

alexcrichton commented May 22, 2023

dhil commented May 23, 2023

alexcrichton commented May 23, 2023

alexcrichton commented May 24, 2023

alexcrichton left a comment

Choose a reason for hiding this comment

dhil commented May 25, 2023

fitzgen left a comment

Choose a reason for hiding this comment

fitzgen May 25, 2023

Choose a reason for hiding this comment

fitzgen May 25, 2023

Choose a reason for hiding this comment

dhil May 26, 2023

Choose a reason for hiding this comment

fitzgen commented May 25, 2023

dhil commented May 26, 2023

alexcrichton commented May 26, 2023

CosineP commented Nov 17, 2022 •

edited