Stream: git-wasmtime

Topic: wasmtime / issue #9310 Replace `cranelift_codegen::ir::Tr...

Wasmtime GitHub notifications bot (Sep 25 2024 at 16:05):

fitzgen added the good first issue label to Issue #9310.

Wasmtime GitHub notifications bot (Sep 25 2024 at 16:05):

fitzgen added the cranelift label to Issue #9310.

Wasmtime GitHub notifications bot (Sep 25 2024 at 16:05):

fitzgen opened issue #9310:

We currently have an enum of various trap codes, but they are all pretty Wasm-/Wasmtime-specific, rather than things that general Cranelift users will want. And they certainly do not cover everything that general Cranelift users might want.

We should replace cranelift_codegen::ir::TrapCode with a u8 that is simply plumbed through cranelift and whose meaning is determined fully by the CLIF producer. Or perhaps instead of simply a u8, a pub struct TrapCode(pub u8), just so that it is that much harder to pass the wrong value around when we are shuffling these about inside the compiler.

This will also let us stop duplicating trap codes between cranelift_codegen::ir::TrapCode and wasmtime_environ::Trap, which will be a nice clean up.

Note: fixing this issue will mostly involve chasing down compile errors throughout Cranelift and Wasmtime.

Wasmtime GitHub notifications bot (Sep 25 2024 at 16:20):

bjorn3 commented on issue #9310:

TrapCode::User(u16) can already be used for passing custom trap codes. While some builtin trap codes are wasm specific, others are not. For example IntegerDivisionByZero and BadConversionToInteger directly correspond to an instruction that could trap. And UnreachableCodeReached is used by cg_clif for any block that should be unreachable. Also HeapOutOfBounds (which is a bit misnamed) is the default trap code for trapping loads and stores. We still need a default trap code for trapping loads and stores.

Wasmtime GitHub notifications bot (Sep 25 2024 at 16:34):

alexcrichton commented on issue #9310:

Perhaps something like:

#[derive(All, The, Traits)]
pub struct TrapCode(u8);

impl TrapCode {
    pub const INTEGER_DIVIDE_BY_ZERO: TrapCode = TrapCode(u8::MAX);
    pub const BAD_CONVERSION_TO_INTEGER: TrapCode = TrapCode(u8::MAX - 1);
    pub const MEMORY_INVALID: TrapCode = TrapCode(u8::MAX - 2);

    pub const fn user(n: u8) -> TrapCode {
        assert!(n < u8::MAX - 2);
        TrapCode(n)
    }

    pub fn code(&self) -> u8 { self.0 }
}

That way Cranelift can reserve a few trap codes for itself that lowerings generate but leave most of the 8-bit space open up to embedders?

Wasmtime GitHub notifications bot (Sep 25 2024 at 16:51):

fitzgen commented on issue #9310:

Division by zero is an interesting example, because Cranelift emits that trap code itself. Makes sense to reserve a constant for that one.

I think bad integer conversions may be in the same boat, but I'd need to double check.

But unreachable code reached and the others seem like things that could easily be defined by the CLIF producer, since they choose what code to attach to trap[n]z or in ir::MemFlags, and cranelift itself just plumbs that code through without actually inspecting it or doing different things depending on which code was attached.

Wasmtime GitHub notifications bot (Sep 25 2024 at 16:55):

bjorn3 commented on issue #9310:

I think bad integer conversions may be in the same boat, but I'd need to double check.

It is. Eg on x86_64: https://github.com/bytecodealliance/wasmtime/blob/1e37274a0795be7063d1c5db6eb282d57d117f02/cranelift/codegen/src/isa/x64/inst/emit.rs#L3695

For loads and stores the default is allowing traps, so there has to be a default trap code for loads and stores unless you want to force everyone to explicitly choose between a trap code or notrap.

Wasmtime GitHub notifications bot (Sep 25 2024 at 16:59):

cfallin commented on issue #9310:

For loads and stores the default is allowing traps, so there has to be a default trap code for loads and stores unless you want to force everyone to explicitly choose between a trap code or notrap.

That doesn't seem like the worst option actually -- those are indeed the choices, trap or no trap, and if trap, given that we're pushing the definition of trap codes upward as far as possible, why not require the user to specify?

Wasmtime GitHub notifications bot (Sep 25 2024 at 17:02):

alexcrichton commented on issue #9310:

Alternatively we could flip the defaults and disallow traps by default? Wasm translation already has to handle loads and stores with great care and assuming no traps seems like a more appropriate default for a Cranelift only context

Wasmtime GitHub notifications bot (Sep 25 2024 at 17:44):

bjorn3 commented on issue #9310:

Alternatively we could flip the defaults and disallow traps by default?

Currently you have to opt in to get any kind of UB in Cranelift IR. I would prefer if it stays that way. Making notrap the default would cause loads and stores to have UB by default.

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

alexcrichton closed issue #9310:

We currently have an enum of various trap codes, but they are all pretty Wasm-/Wasmtime-specific, rather than things that general Cranelift users will want. And they certainly do not cover everything that general Cranelift users might want.

We should replace cranelift_codegen::ir::TrapCode with a u8 that is simply plumbed through cranelift and whose meaning is determined fully by the CLIF producer. Or perhaps instead of simply a u8, a pub struct TrapCode(pub u8), just so that it is that much harder to pass the wrong value around when we are shuffling these about inside the compiler.

This will also let us stop duplicating trap codes between cranelift_codegen::ir::TrapCode and wasmtime_environ::Trap, which will be a nice clean up.

Note: fixing this issue will mostly involve chasing down compile errors throughout Cranelift and Wasmtime.

Last updated: Oct 23 2024 at 20:03 UTC