Stream: git-wasmtime

Topic: wasmtime / PR #9338 Convert `TrapCode` to a single byte


view this post on Zulip Wasmtime GitHub notifications bot (Sep 30 2024 at 21:46):

alexcrichton opened PR #9338 from alexcrichton:make-trap-just-a-byte to bytecodealliance:main:

This commit refactors the representation of
cranelift_codegen::ir::TrapCode to be a single byte. The previous enumeration is replaced with an opaque byte-sized structure. Previous variants that Cranelift uses internally are now associated const values on TrapCode itself. For example TrapCode::IntegerOverflow is now TrapCode::INTEGER_OVERFLOW. All non-Cranelift traps are now removed and exclusively live in the wasmtime-cranelift crate now.

The representation of a TrapCode is now:

This enables embedders to have 255-N trap codes which is more than enough for Wasmtime for example. Cranelift reserves a few built-in codes for itself which shouldn't eat too much into the trap space. Additionally if Cranelift needs to grow a new trap it can do so pretty easily too.

The overall intent of this commit is to reduce the coupling of Wasmtime and Cranelift further and generally refactor Wasmtime to use user traps more often. This additionally shrinks the size of TrapCode for storage in various locations, notably it can now infallibly be represented inside of a MemFlags.

Closes #9310

<!--
Please make sure you include the following information:

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

view this post on Zulip Wasmtime GitHub notifications bot (Sep 30 2024 at 21:46):

alexcrichton requested fitzgen for a review on PR #9338.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 30 2024 at 21:46):

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

view this post on Zulip Wasmtime GitHub notifications bot (Sep 30 2024 at 21:46):

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

view this post on Zulip Wasmtime GitHub notifications bot (Sep 30 2024 at 21:54):

alexcrichton updated PR #9338.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 30 2024 at 23:15):

alexcrichton updated PR #9338.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 01 2024 at 16:12):

fitzgen submitted PR review:

Nice! A few nitpicks inline below.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 01 2024 at 16:12):

fitzgen created PR review comment:

To make it obvious to readers that we aren't losing bits or anything here:

            Some(code) => u16::from(code.as_raw()),

view this post on Zulip Wasmtime GitHub notifications bot (Oct 01 2024 at 16:12):

fitzgen created PR review comment:

If we are being careful, we should be properly careful

        match byte.checked_add(Self::RESERVED_START).and_then(NonZeroU8::new) {

view this post on Zulip Wasmtime GitHub notifications bot (Oct 01 2024 at 16:12):

fitzgen created PR review comment:

Should this take a NonZeroU8 or return an Option?

I would sort of expect that either both this and from_raw do not return Options and assert validity, or both do the checks and return an option. Having them make different choices is a little funky.

Could always have fn user(u8) -> Option<TrapCode> and fn unwrap_user(u8) -> TrapCode too I guess.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 01 2024 at 16:12):

fitzgen created PR review comment:

Unrelated to this PR, but it seems like we should print heap oob traps, since it is possible to not have any traps...

view this post on Zulip Wasmtime GitHub notifications bot (Oct 01 2024 at 16:12):

fitzgen created PR review comment:

What do you think about renaming this to TRAP_INTERNAL_ASSERT while we're here? We emit this not just in debug builds but also in release builds for certain asserts, so the "debug" nomenclature has been kind of nagging at me recently.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 01 2024 at 16:14):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 01 2024 at 16:14):

alexcrichton created PR review comment:

Alas const rust thwarts this as u16::from isn't available in a const context

view this post on Zulip Wasmtime GitHub notifications bot (Oct 01 2024 at 16:16):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 01 2024 at 16:16):

fitzgen created PR review comment:

Ah, gotcha

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

alexcrichton submitted PR review.

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

alexcrichton created PR review comment:

Lack of trap though prints as notrap, so I think we're good?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 01 2024 at 16:28):

alexcrichton updated PR #9338.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 01 2024 at 16:31):

alexcrichton updated PR #9338.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 01 2024 at 17:03):

alexcrichton updated PR #9338.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 01 2024 at 17:04):

alexcrichton has enabled auto merge for PR #9338.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 01 2024 at 17:52):

alexcrichton updated PR #9338.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 01 2024 at 18:22):

alexcrichton merged PR #9338.


Last updated: Dec 23 2024 at 12:05 UTC