From 699a588e0a7fb52e1da911a5a76019af0ccdede7 Mon Sep 17 00:00:00 2001 From: Nick Fitzgerald Date: Tue, 24 Sep 2024 08:25:08 -0700 Subject: [PATCH] Remove the `NullI31Ref` trap code 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. --- cranelift/codegen/src/ir/memflags.rs | 3 +-- cranelift/codegen/src/ir/trapcode.rs | 5 ----- crates/cranelift-shared/src/lib.rs | 1 - crates/cranelift/src/func_environ.rs | 4 ++-- crates/cranelift/src/lib.rs | 1 - crates/environ/src/trap_encoding.rs | 5 ----- 6 files changed, 3 insertions(+), 16 deletions(-) diff --git a/cranelift/codegen/src/ir/memflags.rs b/cranelift/codegen/src/ir/memflags.rs index f604a324c0a2..ffd46825de49 100644 --- a/cranelift/codegen/src/ir/memflags.rs +++ b/cranelift/codegen/src/ir/memflags.rs @@ -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, @@ -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"), diff --git a/cranelift/codegen/src/ir/trapcode.rs b/cranelift/codegen/src/ir/trapcode.rs index e33a655456a3..c0f343631b74 100644 --- a/cranelift/codegen/src/ir/trapcode.rs +++ b/cranelift/codegen/src/ir/trapcode.rs @@ -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 { @@ -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) } @@ -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(()), } diff --git a/crates/cranelift-shared/src/lib.rs b/crates/cranelift-shared/src/lib.rs index 408c266e0d5c..1b72ef884d3f 100644 --- a/crates/cranelift-shared/src/lib.rs +++ b/crates/cranelift-shared/src/lib.rs @@ -86,7 +86,6 @@ pub fn mach_trap_to_trap(trap: &MachTrap) -> Option { 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 diff --git a/crates/cranelift/src/func_environ.rs b/crates/cranelift/src/func_environ.rs index 56d88d663d72..f2392754639d 100644 --- a/crates/cranelift/src/func_environ.rs +++ b/crates/cranelift/src/func_environ.rs @@ -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)) } @@ -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)) } diff --git a/crates/cranelift/src/lib.rs b/crates/cranelift/src/lib.rs index cbfcc7a5b600..ee3c55b917cb 100644 --- a/crates/cranelift/src/lib.rs +++ b/crates/cranelift/src/lib.rs @@ -248,7 +248,6 @@ fn clif_trap_to_env_trap(trap: ir::TrapCode) -> Option { 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 diff --git a/crates/environ/src/trap_encoding.rs b/crates/environ/src/trap_encoding.rs index 32d9e19e78f3..242beba2fc26 100644 --- a/crates/environ/src/trap_encoding.rs +++ b/crates/environ/src/trap_encoding.rs @@ -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, @@ -114,7 +111,6 @@ impl Trap { OutOfFuel AtomicWaitNonSharedMemory NullReference - NullI31Ref CannotEnterComponent } @@ -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}")