Support for funcrefs, ref.func, and table.grow with funcrefs

[fitzgen]

No description provided.

[github-actions]

Subscribe to Label Action

cc @bnjbvr, @peterhuene

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

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

• bnjbvr: cranelift
• peterhuene: wasmtime:api

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

Learn more.

[alexcrichton]

Nice, that was smaller than I expected! One thing we'll need to handle is that ValueType::from_wasmtime_type is incorrect if both externref and funcref translate to the same underlying cranelift type.

Other than that, though, I was hoping that we wouldn't have to allocate an ExternRef for dealing with funcref. There's no real need to GC them since they have the same lifetime of the Instance in theory, so it seems like wasted time to GC them and to allocate new data structures for them.

Thinking a bit about this, can we perhaps make the representation of funcref to be *mut VMCallerCheckedAnyfunc? That way it would still be word-sized (easy to manage) and we could easily implement calling that and such. Additionally it's very easy to convert that into a wasmtime::Func, and we'll probably just need to update wasmtime::Func to internally contain *mut VMCallerCheckedAnyfunc to make it easy to create a Func. "morally" at least I'd expect that going from funcref to wasmtime::Func should basically be "pair the internal representation with a Store and you're off to the races"

We statically know all possible indices that can be referenced by ref.func, so I think what we can do is something like this:

• The function index may be referenced by an element segment (passive/declared/active). In that case we don't currently ever drop those until the end of the Store, so assuming that we store all these element segments as Vec<VMCallerCheckedAnyfunc> (as we do today for tables) we could just load a constant offset from an element segment (e.g function N is at element segment A at offset B)
• Otherwise we can allocate a Vec<VMCallerCheckedAnyfunc> specifically for non-element-segment-referenced indices (those referenced through globals for now) and attach that the VMContext. I guess not really allocate a Vec<T> but we would just make the VMContext allocation bigger.

In any case I think it'd be best if we could leverage the current state of things to avoid GC/allocation overhead for funcref if we can. WDYT?

[alexcrichton]

FWIW this pattern with #[cfg] might be good to encapsulate somewhere with something like ir::types::rsize()

[fitzgen]

That definitely sounds like a better design, albeit more implementation effort. How strongly do you feel vs waiting until we actually care about the performance of funcref?

We statically know all possible indices that can be referenced by ref.func,

I know that this is true at the spec level with the refs member of the validation context, but do you know if we actually retain this info anywhere? Not sure this is exposed by wasmparser or cranelift_wasm.

[alexcrichton]

I think it's fine to take the shortest path for now regardless of perf to a complete implementation of reference types, but looking forward to things like table.copy and table.init on funcref tables I feel like we're gonna be jumping through a lot of hoops trying to convert back and forth between VMCallerCheckedAnyfunc and VMFuncRef. I'm fine deferring to you, but it feels to me that it'll be basically equivalent the amount of work to take either strategy (in that I'm not sure one is that much more onerous than the other).

Currently AFAIK the refs member isn't exposed, so cranelift-wasm would likely need to construct this.

[fitzgen]

My one concern with your proposed design is just figuring out where to stick the new side tables for VMCallerCheckedAnyfuncs and how to plumb that through everywhere. Its the kind of thing that seems like it might run into the annoying wasmtime/wasmtime-runtime crate boundary again. I'll investigate a little more and see if I can't make it work.

[alexcrichton]

Perhaps we could temporarily allocate a VMCallerCheckedAnyfunc for every function in the module, and shove that into the VMContext? That way the index of the function is the same as the index into that table. We could later optimize that to not allocate such a big table for functions that aren't referenced.

[fitzgen]

Pushed a wip commit that switches to using *const VMCallerCheckedAnyfunc for funcrefs. It compiles at least, but tests are failing.

[github-actions]

Subscribe to Label Action

cc @fitzgen

This issue or pull request has been labeled: "fuzzing"

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

• fitzgen: fuzzing

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

Learn more.

[fitzgen]

@alexcrichton okay I think this is ready for review again.

[github-actions]

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:c-api"

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

• peterhuene: wasmtime:c-api

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

Learn more.

[alexcrichton]

Looking great! My main comment below is about the change to Extern which had a pretty large impact, but I don't think should be necessary.

Otherwise, though, could you add some tests which acquire a Val::FuncRef and then test whether it's None or Some, and if Some calls the function to make sure it gets the right result?

Additionally I think some tests would be good for passing in various FuncRef values. For example Some, None, something from another store, something from another instance, etc.

[alexcrichton]

An idle thought, but we could perhaps have something like:

impl Func {
    pub(crate) fn into_raw(self, dst: &Store) -> Result<NonNull<VMCallerCheckedAnyfunc>> {
        // ... 
    }
}

to bake in how you can't access the raw value without using the right store. Anyway not necessary to do here, just a thought!

[fitzgen]

@alexcrichton okay, I've deduped the null func refs into the store, and made Extern::Func non-nullable.

Still need to write a couple tests that pass in Val::Func(None) to various things, but I think you can take another look a this while I do that.

[alexcrichton]

Looks great to me! I've got one final question though before landing, which I just thought of after reading this. Could we be even more clever and make the null funcref a literal null pointer? It looks like you were worried about the cost of call_indirect in terms of number of checks, but I think that it's the same number of checks as before, we're just testing anyfunc_ptr for whether it's null instead of func_addr, which we assume is always non-null.

With that in place translation of ref.null would be super simple, just a bag of zeros all the time!

[fitzgen]

Looks great to me! I've got one final question though before landing, which I just thought of after reading this. Could we be even more clever and make the null funcref a literal null pointer? It looks like you were worried about the cost of call_indirect in terms of number of checks, but I think that it's the same number of checks as before, we're just testing anyfunc_ptr for whether it's null instead of func_addr, which we assume is always non-null.

With that in place translation of ref.null would be super simple, just a bag of zeros all the time!

The concern wasn't about the path for call_indirect with a null funcref (which should be a super rare event, and leads to a trap anyways, so it isn't ever gonna be fast). The concern was introducing an extra potential cache miss in front of every non-trapping call_indirect before we actually end up calling the function.

Originally, we couldn't use a null pointer for a null func ref, but after all the refactorings this PR has been through, that might not be the case anymore. Let me double check. Either way, I think ref.null will be cheap enough either way. But this actually made me discover a latent bug: cranelift-wasm doesn't allow you customize ref.null's translation (currently). It also always assumes that all references are of the reference type (which is not true for funcrefs for us). So I have a bit more work to do here either way.

[alexcrichton]

Oh I'm not actually worried about the cost of ref.null, it's more that using a null pointer for ref.null felt clearer and easier to understand.

Could you elaborate on the cache misses, though? With this current incantation a call_indirect is: load anyref, load funcptr, check null, load type, check match, call. With a null pointer representation it would instead be load anyref, check null, load type, check match, load funcptr, call. I'm not sure how the cache misses would be different there?

[fitzgen]

The extra potential cache miss is from changing table elements from VMCallerCheckedAnyfunc to a pointer to a VMCallerCheckedAnyfunc (regardless whether that is a nullable pointer or not).

It makes it so that there is an extra pointer chase to get to the func_ptr member of an element in the table.

[alexcrichton]

Ah ok, I remember talking about that yeah. I thought you meant that the extra cache miss was about the representation of the null function in the sense of is it a null pointer or is it a sentinel.

[fitzgen]

phew. okay.

• I made ref.null func be a null pointer, rather than a VMCallerCheckedAnyfunc with a null func_ptr member.
• similarly, VMCallerCheckedAnyfunc::func_ptr is now NonNull.
• I added the extra translate_* hooks to cranelift_wasm::FuncEnvironment to allow for non-uniform reference types representations.

@alexcrichton I think this is ready for another (and hopefully the last!) round of review. Thanks for your patience with this one!

[alexcrichton]

Looks like there's some warnings on aarch64 causing CI to fail, but otherwise 👍 from me, thanks for being patient with me too :)

Mind adding a test or two for ref.is_null while you're at it?