wasmtime: Implement table.get and table.set

[fitzgen]

These instructions have fast, inline JIT paths for the common cases, and only
call out to host VM functions for the slow paths. This required some changes to
cranelift-wasm's FuncEnvironment: instead of taking a FuncCursor to insert
an instruction sequence within the current basic block,
FuncEnvironment::translate_table_{get,set} now take a &mut FunctionBuilder
so that they can create whole new basic blocks. This is necessary for
implementing GC read/write barriers that involve branching (e.g. checking for
null, or whether a store buffer is at capacity).

Furthermore, it required that the load, load_complex, and store
instructions handle loading and storing through an r{32,64} rather than just
i{32,64} addresses. This involved making r{32,64} types acceptable
instantiations of the iAddr type variable, plus a few new instruction
encodings.

Part of #929

[github-actions]

Subscribe to Label Action

cc @bnjbvr, @kubkon

This issue or pull request has been labeled: "cranelift", "cranelift:meta", "cranelift:wasm", "wasi"

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

• bnjbvr: cranelift
• kubkon: wasi

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

Learn more.

[alexcrichton]

Nice!

Could this add some more tests too? Some things like:

• Using table.get/table.set on a funcref table
• Calling table.set with a ref.null
• Calling table.get returning a ref.null
• calling table.set where the previous element is null
• Calling table.set where the previous element had a refcount of 1 (this may be tricky?)

[fitzgen]

Could this add some more tests too? Some things like:

All of these cases are in the spec tests (except explicitly targeting replacing a ref_count == 1 element), and enabled in this PR. I don't think it makes sense to duplicate them (again, excpet for ref_count == 1).

[alexcrichton]

Hm so I do think that it's worthwhile to add some more tests here. I'm a bit hesitant to rely on pure wast tests since all the interesting bits are about how host code interacts with this.

For example this program will hit "attempt to subtract with overflow" because the reference count is decremented in wasm but then drop_in_place tries to decrement it again. If that's fixed then the program segfaults since it tries to drop A twice. The problem is that we're in the process of dropping a slot in the table when at the same time we still have access to read that slot. What needs to happen is we need to move out of the table, move the new entry into the table, then drop the previous entry. That way if in the destructor of the previous value we read the table slot we get the new value.

I do realize that the spec tests try to be somewhat exhaustive, but given that this was pretty easy to segfault I think it's worthwhile trying to be at least somewhat flavorful with tests that exercise the embedding where we have more control over GC and such.

[fitzgen]

I do realize that the spec tests try to be somewhat exhaustive, but given that this was pretty easy to segfault I think it's worthwhile trying to be at least somewhat flavorful with tests that exercise the embedding where we have more control over GC and such.

Yeah I aleady had a fix for the drop_in_place bit, but the destructor-touching-the-table bit needs some tests for sure. I have a half written one already.

I don't think we need to duplicate anything that's already in the spec tests, just the ref_count == 1 paths, and dtors that touch the table.

[fitzgen]

Okay, I've added a bunch of tests for pathological gc-in-externref-destructor-while-sweeping behaviors and other various re-entry into GC.

Working on a fuzz target specifically for exercising table.get, table.set, and GC now, but it will probably be a follow up PR.

[alexcrichton]

Oh man gc-in-dtor is pretty gnarly, totally forgot to think about that! Very nice tests though 👍

This all looks great to me, but as you mentioned at the beginning someone should take a look at the cranelift pieces too.

[fitzgen]

Alright, I got the fuzz target written and passing as well!

[github-actions]

Subscribe to Label Action

cc @fitzgen, @peterhuene

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

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

• fitzgen: fuzzing
• peterhuene: wasmtime:api

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

Learn more.

[sunfishcode]

The meta and frontend parts look good to me, with one minor refactoring request: