Stream: git-wasmtime

Topic: wasmtime / PR #8159 Don't check for null in `call_indirec...

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2024 at 18:28):

alexcrichton opened PR #8159 from alexcrichton:no-null-chekc-functions to bytecodealliance:main:

This PR is an implementation of #5291 to slightly optimize the lowering of call_indirect and call_ref in Wasmtime. Explicitly checks for null function pointers are no longer present and instead we let a segfault happen when loading from a null function pointer. This segfault is caught and the relevant instruction is annotated with the appropriate trap code.

This support first starts by refactoring the MemFlags API to no longer purely be flags but instead be a mixture of flags and "flag regions". The vmctx/heap/table alias regions are bundled into two bits now and the various trap-related bits are now bundled into four bits. This enables putting arbitrary trap codes in a MemFlags so long as they aren't TrapCode::User(_).

Closes #5291

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2024 at 18:28):

alexcrichton requested wasmtime-compiler-reviewers for a review on PR #8159.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2024 at 18:28):

alexcrichton requested cfallin for a review on PR #8159.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2024 at 18:28):

alexcrichton requested wasmtime-core-reviewers for a review on PR #8159.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2024 at 18:28):

alexcrichton requested fitzgen for a review on PR #8159.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2024 at 18:30):

alexcrichton updated PR #8159.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2024 at 18:44):

github-actions[bot] commented on PR #8159:

Subscribe to Label Action

cc @peterhuene

<details>
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:

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

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2024 at 21:27):

cfallin submitted PR review:

LGTM, nice!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2024 at 21:27):

cfallin submitted PR review:

LGTM, nice!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2024 at 21:27):

cfallin created PR review comment:

s/the interpreted/interpreted/

view this post on Zulip Wasmtime GitHub notifications bot (Mar 18 2024 at 00:16):

jameysharp submitted PR review:

I would kind of prefer to land the MemFlags changes separately from the "don't explicitly check for null function pointers" changes, if that's not too much trouble to split up. The commit history here is almost right for doing that with a quick rebase but I think only part of the "Trim the MemFlags API" commit applies.

I'm interested in whether you or Chris or anyone else have opinions on the other comments I've left below too.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 18 2024 at 00:16):

jameysharp submitted PR review:

I would kind of prefer to land the MemFlags changes separately from the "don't explicitly check for null function pointers" changes, if that's not too much trouble to split up. The commit history here is almost right for doing that with a quick rebase but I think only part of the "Trim the MemFlags API" commit applies.

I'm interested in whether you or Chris or anyone else have opinions on the other comments I've left below too.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 18 2024 at 00:16):

jameysharp created PR review comment:

Since I just added this tabletrap flag and haven't merged the work that would have used it yet, I don't think we need to preserve compatibility for it here.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 18 2024 at 00:16):

jameysharp created PR review comment:

There's a very slight cost to attaching trap metadata to an instruction, right? Just a few extra bytes in read-only data, I assume, but not completely free? This comment would still apply to avoiding that cost by plumbing the non-nullability through and avoiding setting the metadata at all. Is it worth keeping part of this comment here?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 18 2024 at 00:16):

jameysharp created PR review comment:

I'm inclined to think that we don't need to track whether heap_oob was set explicitly or just used as a default, so my instinct is to use just one encoding for it. I could easily be talked into the approach you've taken here, but I think it at least needs a bit more of a comment explaining why it appears twice here, and why the zero case doesn't appear in with_trap_code below.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 18 2024 at 04:13):

alexcrichton commented on PR #8159:

prefer to land the MemFlags changes separately

Certainly!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 18 2024 at 04:14):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 18 2024 at 04:14):

alexcrichton created PR review comment:

Good point, I'll be sure to add this back in when rebasing

view this post on Zulip Wasmtime GitHub notifications bot (Mar 18 2024 at 04:14):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 18 2024 at 04:14):

alexcrichton created PR review comment:

Nah I think that makes the most sense in retrospect too, no need to be needlessly complicated.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 18 2024 at 15:53):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 18 2024 at 15:53):

fitzgen created PR review comment:

We should have a test somewhere, maybe here, that mem::size_of<VMFuncRef>() < PAGE_SIZE, since we should only rely on the presence of a single null guard page.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 18 2024 at 15:53):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 18 2024 at 16:51):

alexcrichton updated PR #8159.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 18 2024 at 16:52):

alexcrichton commented on PR #8159:

Updated and rebased!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 18 2024 at 19:08):

jameysharp submitted PR review:

This feels very satisfying! Also TIL that you can do static asserts by putting assert! in a const-eval block.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 18 2024 at 19:55):

alexcrichton merged PR #8159.

Last updated: Dec 23 2024 at 13:07 UTC