Stream: git-wasmtime

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

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:

• 0 - invalid, used in MemFlags for "no trap code"
• 1..256-N - user traps
• 256-N..256 - built-in Cranelift traps (it uses N of these)

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:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

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

Wasmtime GitHub notifications bot (Sep 30 2024 at 21:46):

alexcrichton requested fitzgen for a review on PR #9338.

Wasmtime GitHub notifications bot (Sep 30 2024 at 21:46):

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

Wasmtime GitHub notifications bot (Sep 30 2024 at 21:46):

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

Wasmtime GitHub notifications bot (Sep 30 2024 at 21:54):

alexcrichton updated PR #9338.

Wasmtime GitHub notifications bot (Sep 30 2024 at 23:15):

alexcrichton updated PR #9338.

Wasmtime GitHub notifications bot (Oct 01 2024 at 16:12):

fitzgen submitted PR review:

Nice! A few nitpicks inline below.

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()),

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) {

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.

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...

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.

Wasmtime GitHub notifications bot (Oct 01 2024 at 16:14):

alexcrichton submitted PR review.

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

Wasmtime GitHub notifications bot (Oct 01 2024 at 16:16):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Oct 01 2024 at 16:16):

fitzgen created PR review comment:

Ah, gotcha

Wasmtime GitHub notifications bot (Oct 01 2024 at 16:18):

alexcrichton submitted PR review.

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?

Wasmtime GitHub notifications bot (Oct 01 2024 at 16:28):

alexcrichton updated PR #9338.

Wasmtime GitHub notifications bot (Oct 01 2024 at 16:31):

alexcrichton updated PR #9338.

Wasmtime GitHub notifications bot (Oct 01 2024 at 17:03):

alexcrichton updated PR #9338.

Wasmtime GitHub notifications bot (Oct 01 2024 at 17:04):

alexcrichton has enabled auto merge for PR #9338.

Wasmtime GitHub notifications bot (Oct 01 2024 at 17:52):

alexcrichton updated PR #9338.

Wasmtime GitHub notifications bot (Oct 01 2024 at 18:22):

alexcrichton merged PR #9338.

Last updated: Oct 23 2024 at 20:03 UTC