Skip to content

Commit

Permalink
Remove the NullI31Ref trap code
Browse files Browse the repository at this point in the history
To precisely match the Wasm spec tests, we would also need `NullStructRef` and
`NullArrayRef`, etc... This is not practical, given the encoding space we have
available. We are already matching expected "null FOO reference" trap messages
when running the spec tests to our own "null reference" messages, so we can do
that for `i31`s as well.
  • Loading branch information
fitzgen committed Sep 24, 2024
1 parent 435169c commit 699a588
Show file tree
Hide file tree
Showing 6 changed files with 3 additions and 16 deletions.
3 changes: 1 addition & 2 deletions cranelift/codegen/src/ir/memflags.rs
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ impl MemFlags {
0b1001 => Some(TrapCode::UnreachableCodeReached),
0b1010 => Some(TrapCode::Interrupt),
0b1011 => Some(TrapCode::NullReference),
0b1100 => Some(TrapCode::NullI31Ref),
// 0b1100 => {} not allocated
// 0b1101 => {} not allocated
// 0b1110 => {} not allocated
0b1111 => None,
Expand Down Expand Up @@ -390,7 +390,6 @@ impl MemFlags {
Some(TrapCode::UnreachableCodeReached) => 0b1001,
Some(TrapCode::Interrupt) => 0b1010,
Some(TrapCode::NullReference) => 0b1011,
Some(TrapCode::NullI31Ref) => 0b1100,
None => 0b1111,

Some(TrapCode::User(_)) => panic!("cannot set user trap code in mem flags"),
Expand Down
5 changes: 0 additions & 5 deletions cranelift/codegen/src/ir/trapcode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,6 @@ pub enum TrapCode {

/// A null reference was encountered which was required to be non-null.
NullReference,

/// A null `i31ref` was encountered which was required to be non-null.
NullI31Ref,
}

impl TrapCode {
Expand Down Expand Up @@ -95,7 +92,6 @@ impl Display for TrapCode {
Interrupt => "interrupt",
User(x) => return write!(f, "user{x}"),
NullReference => "null_reference",
NullI31Ref => "null_i31ref",
};
f.write_str(identifier)
}
Expand All @@ -119,7 +115,6 @@ impl FromStr for TrapCode {
"unreachable" => Ok(UnreachableCodeReached),
"interrupt" => Ok(Interrupt),
"null_reference" => Ok(NullReference),
"null_i31ref" => Ok(NullI31Ref),
_ if s.starts_with("user") => s[4..].parse().map(User).map_err(|_| ()),
_ => Err(()),
}
Expand Down
1 change: 0 additions & 1 deletion crates/cranelift-shared/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ pub fn mach_trap_to_trap(trap: &MachTrap) -> Option<TrapInformation> {
ir::TrapCode::User(ALWAYS_TRAP_CODE) => Trap::AlwaysTrapAdapter,
ir::TrapCode::User(CANNOT_ENTER_CODE) => Trap::CannotEnterComponent,
ir::TrapCode::NullReference => Trap::NullReference,
ir::TrapCode::NullI31Ref => Trap::NullI31Ref,

// These do not get converted to wasmtime traps, since they
// shouldn't ever be hit in theory. Instead of catching and handling
Expand Down
4 changes: 2 additions & 2 deletions crates/cranelift/src/func_environ.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1851,7 +1851,7 @@ impl<'module_environment> cranelift_wasm::FuncEnvironment for FuncEnvironment<'m
// TODO: If we knew we have a `(ref i31)` here, instead of maybe a `(ref
// null i31)`, we could omit the `trapz`. But plumbing that type info
// from `wasmparser` and through to here is a bit funky.
self.trapz(builder, i31ref, ir::TrapCode::NullI31Ref);
self.trapz(builder, i31ref, ir::TrapCode::NullReference);
Ok(builder.ins().sshr_imm(i31ref, 1))
}

Expand All @@ -1863,7 +1863,7 @@ impl<'module_environment> cranelift_wasm::FuncEnvironment for FuncEnvironment<'m
// TODO: If we knew we have a `(ref i31)` here, instead of maybe a `(ref
// null i31)`, we could omit the `trapz`. But plumbing that type info
// from `wasmparser` and through to here is a bit funky.
self.trapz(builder, i31ref, ir::TrapCode::NullI31Ref);
self.trapz(builder, i31ref, ir::TrapCode::NullReference);
Ok(builder.ins().ushr_imm(i31ref, 1))
}

Expand Down
1 change: 0 additions & 1 deletion crates/cranelift/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,6 @@ fn clif_trap_to_env_trap(trap: ir::TrapCode) -> Option<Trap> {
ir::TrapCode::User(ALWAYS_TRAP_CODE) => Trap::AlwaysTrapAdapter,
ir::TrapCode::User(CANNOT_ENTER_CODE) => Trap::CannotEnterComponent,
ir::TrapCode::NullReference => Trap::NullReference,
ir::TrapCode::NullI31Ref => Trap::NullI31Ref,

// These do not get converted to wasmtime traps, since they
// shouldn't ever be hit in theory. Instead of catching and handling
Expand Down
5 changes: 0 additions & 5 deletions crates/environ/src/trap_encoding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,6 @@ pub enum Trap {
/// Call to a null reference.
NullReference,

/// Attempt to get the bits of a null `i31ref`.
NullI31Ref,

/// When the `component-model` feature is enabled this trap represents a
/// scenario where one component tried to call another component but it
/// would have violated the reentrance rules of the component model,
Expand Down Expand Up @@ -114,7 +111,6 @@ impl Trap {
OutOfFuel
AtomicWaitNonSharedMemory
NullReference
NullI31Ref
CannotEnterComponent
}

Expand Down Expand Up @@ -142,7 +138,6 @@ impl fmt::Display for Trap {
OutOfFuel => "all fuel consumed by WebAssembly",
AtomicWaitNonSharedMemory => "atomic wait on non-shared memory",
NullReference => "null reference",
NullI31Ref => "null i31 reference",
CannotEnterComponent => "cannot enter component instance",
};
write!(f, "wasm trap: {desc}")
Expand Down

0 comments on commit 699a588

Please sign in to comment.