Cranelift: Remove resumable traps (#8809)

These were originally a SpiderMonkey-ism and have been unused ever since. It was introduced for GC integration, where the runtime could do something to make Cranelift code hit a trap and pause for a GC and then resume execution once GC completed. But it is unclear that, as implemented, this is actually a useful mechanism for doing that (compared to, say, loading from some Well Known page and the GC protecting that page and catching signals to interrupt the mutator, or simply branching and doing a libcall). And if someone has that particular use case in the future (Wasmtime and its GC integration doesn't need exactly this) then we can design something for what is actually needed at that time, instead of carrying this cruft forward forever.
bytecodealliance · Jun 14, 2024 · 9ffc9e6 · 9ffc9e6
1 parent 34b6b67
commit 9ffc9e6
Show file tree

Hide file tree

Showing 12 changed files with 3 additions and 122 deletions.
diff --git a/cranelift/codegen/meta/src/shared/instructions.rs b/cranelift/codegen/meta/src/shared/instructions.rs
@@ -144,20 +144,6 @@ fn define_control_flow(
         .can_trap(),
     );
 
-    ig.push(
-        Inst::new(
-            "resumable_trap",
-            r#"
-        A resumable trap.
-
-        This instruction allows non-conditional traps to be used as non-terminal instructions.
-        "#,
-            &formats.trap,
-        )
-        .operands_in(vec![Operand::new("code", &imm.trapcode)])
-        .can_trap(),
-    );
-
     ig.push(
         Inst::new(
             "trapnz",
@@ -175,23 +161,6 @@ fn define_control_flow(
         .can_trap(),
     );
 
-    ig.push(
-        Inst::new(
-            "resumable_trapnz",
-            r#"
-        A resumable trap to be called when the passed condition is non-zero.
-
-        If ``c`` is zero, execution continues at the following instruction.
-        "#,
-            &formats.cond_trap,
-        )
-        .operands_in(vec![
-            Operand::new("c", ScalarTruthy).with_doc("Controlling value to test"),
-            Operand::new("code", &imm.trapcode),
-        ])
-        .can_trap(),
-    );
-
     ig.push(
         Inst::new(
             "return",

diff --git a/cranelift/codegen/src/ir/instructions.rs b/cranelift/codegen/src/ir/instructions.rs
@@ -193,14 +193,6 @@ impl Opcode {
     pub fn constraints(self) -> OpcodeConstraints {
         OPCODE_CONSTRAINTS[self as usize - 1]
     }
-
-    /// Returns true if the instruction is a resumable trap.
-    pub fn is_resumable_trap(&self) -> bool {
-        match self {
-            Opcode::ResumableTrap | Opcode::ResumableTrapnz => true,
-            _ => false,
-        }
-    }
 }
 
 // This trait really belongs in cranelift-reader where it is used by the `.clif` file parser, but since

diff --git a/cranelift/codegen/src/ir/trapcode.rs b/cranelift/codegen/src/ir/trapcode.rs
@@ -46,7 +46,6 @@ pub enum TrapCode {
     UnreachableCodeReached,
 
     /// Execution has potentially run too long and may be interrupted.
-    /// This trap is resumable.
     Interrupt,
 
     /// A user-defined trap code.

diff --git a/cranelift/codegen/src/isa/aarch64/lower.isle b/cranelift/codegen/src/isa/aarch64/lower.isle
@@ -1936,11 +1936,6 @@
 (rule (lower (trap trap_code))
       (side_effect (udf trap_code)))
 
-;;;; Rules for `resumable_trap` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
-
-(rule (lower (resumable_trap trap_code))
-      (side_effect (udf trap_code)))
-
 ;;;; Rules for `select` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
 (rule (lower (has_type ty

diff --git a/cranelift/codegen/src/isa/riscv64/lower.isle b/cranelift/codegen/src/isa/riscv64/lower.isle
@@ -2027,11 +2027,6 @@
   (lower (trap code))
   (udf code))
 
-;;;;;  Rules for `resumable_trap`;;;;;;;;;
-(rule
-  (lower (resumable_trap code))
-  (udf code))
-
 ;;;;;  Rules for `uload8`;;;;;;;;;
 (rule (lower (uload8 flags addr offset))
   (gen_load (amode addr offset) (LoadOP.Lbu) flags))

diff --git a/cranelift/codegen/src/isa/s390x/lower.isle b/cranelift/codegen/src/isa/s390x/lower.isle
@@ -3800,12 +3800,6 @@
       (side_effect (trap_impl trap_code)))
 
 
-;;;; Rules for `resumable_trap` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
-
-(rule (lower (resumable_trap trap_code))
-      (side_effect (trap_impl trap_code)))
-
-
 ;;;; Rules for `trapz` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
 (rule (lower (trapz val trap_code))
@@ -3818,12 +3812,6 @@
       (side_effect (trap_if_bool (value_nonzero val) trap_code)))
 
 
-;;;; Rules for `resumable_trapnz` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
-
-(rule (lower (resumable_trapnz val trap_code))
-      (side_effect (trap_if_bool (value_nonzero val) trap_code)))
-
-
 ;;;; Rules for `debugtrap` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
 (rule (lower (debugtrap))

diff --git a/cranelift/codegen/src/isa/x64/lower.isle b/cranelift/codegen/src/isa/x64/lower.isle
@@ -1821,11 +1821,6 @@
         (x64_add_with_flags_paired ty b a)
         (trap_if (CC.B) tc)))
 
-;;;; Rules for `resumable_trap` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
-
-(rule (lower (resumable_trap code))
-      (side_effect (x64_ud2 code)))
-
 ;;;; Rules for `return` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
 ;; N.B.: the Ret itself is generated by the ABI.

diff --git a/cranelift/codegen/src/legalizer/mod.rs b/cranelift/codegen/src/legalizer/mod.rs
@@ -64,8 +64,7 @@ pub fn simple_legalize(func: &mut ir::Function, cfg: &mut ControlFlowGraph, isa:
             match pos.func.dfg.insts[inst] {
                 // control flow
                 InstructionData::CondTrap {
-                    opcode:
-                        opcode @ (ir::Opcode::Trapnz | ir::Opcode::Trapz | ir::Opcode::ResumableTrapnz),
+                    opcode: opcode @ (ir::Opcode::Trapnz | ir::Opcode::Trapz),
                     arg,
                     code,
                 } => {
@@ -283,7 +282,7 @@ fn expand_cond_trap(
     // Parse the instruction.
     let trapz = match opcode {
         ir::Opcode::Trapz => true,
-        ir::Opcode::Trapnz | ir::Opcode::ResumableTrapnz => false,
+        ir::Opcode::Trapnz => false,
         _ => panic!("Expected cond trap: {}", func.dfg.display_inst(inst)),
     };
 
@@ -331,10 +330,6 @@ fn expand_cond_trap(
         ir::Opcode::Trapz | ir::Opcode::Trapnz => {
             pos.ins().trap(code);
         }
-        ir::Opcode::ResumableTrapnz => {
-            pos.ins().resumable_trap(code);
-            pos.ins().jump(new_block_resume, &[]);
-        }
         _ => unreachable!(),
     }
 

diff --git a/cranelift/filetests/filetests/isa/s390x/traps.clif b/cranelift/filetests/filetests/isa/s390x/traps.clif
@@ -2,7 +2,7 @@ test compile precise-output
 target s390x
 
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
-;; TRAP/RESUMABLE_TRAP
+;; TRAP
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
 function %trap() {
@@ -18,19 +18,6 @@ block0:
 ; block0: ; offset 0x0
 ;   .byte 0x00, 0x00 ; trap: user0
 
-function %resumable_trap() {
-block0:
-  trap user0
-}
-
-; VCode:
-; block0:
-;   .word 0x0000 # trap=user0
-;
-; Disassembled:
-; block0: ; offset 0x0
-;   .byte 0x00, 0x00 ; trap: user0
-
 function %trapz(i64) {
 block0(v0: i64):
   v1 = iconst.i64 42
@@ -83,32 +70,6 @@ block0(v0: i64):
 ; block2: ; offset 0xe
 ;   .byte 0x00, 0x00 ; trap: user0
 
-function %resumable_trapnz(i64) {
-block0(v0: i64):
-  v1 = iconst.i64 42
-  v2 = icmp eq v0, v1
-  trapnz v2, user0
-  return
-}
-
-; VCode:
-; block0:
-;   clgfi %r2, 42
-;   jge label2 ; jg label1
-; block1:
-;   br %r14
-; block2:
-;   .word 0x0000 # trap=user0
-;
-; Disassembled:
-; block0: ; offset 0x0
-;   clgfi %r2, 0x2a
-;   jge 0xe
-; block1: ; offset 0xc
-;   br %r14
-; block2: ; offset 0xe
-;   .byte 0x00, 0x00 ; trap: user0
-
 function %h() {
 block0:
   debugtrap
@@ -124,4 +85,3 @@ block0:
 ; block0: ; offset 0x0
 ;   .byte 0x00, 0x01
 ;   br %r14
-
diff --git a/cranelift/fuzzgen/src/function_generator.rs b/cranelift/fuzzgen/src/function_generator.rs
@@ -888,9 +888,7 @@ static OPCODE_SIGNATURES: Lazy<Vec<OpcodeSignature>> = Lazy::new(|| {
                 (Opcode::Debugtrap),
                 (Opcode::Trap),
                 (Opcode::Trapz),
-                (Opcode::ResumableTrap),
                 (Opcode::Trapnz),
-                (Opcode::ResumableTrapnz),
                 (Opcode::CallIndirect, &[I32]),
                 (Opcode::FuncAddr),
                 (Opcode::X86Pshufb),

diff --git a/cranelift/interpreter/src/step.rs b/cranelift/interpreter/src/step.rs
@@ -353,10 +353,8 @@ where
         }
         Opcode::Trap => ControlFlow::Trap(CraneliftTrap::User(trap_code())),
         Opcode::Debugtrap => ControlFlow::Trap(CraneliftTrap::Debug),
-        Opcode::ResumableTrap => ControlFlow::Trap(CraneliftTrap::Resumable),
         Opcode::Trapz => trap_when(!arg(0).into_bool()?, CraneliftTrap::User(trap_code())),
         Opcode::Trapnz => trap_when(arg(0).into_bool()?, CraneliftTrap::User(trap_code())),
-        Opcode::ResumableTrapnz => trap_when(arg(0).into_bool()?, CraneliftTrap::Resumable),
         Opcode::Return => ControlFlow::Return(args()),
         Opcode::Call | Opcode::ReturnCall => {
             let func_ref = if let InstructionData::Call { func_ref, .. } = inst {
@@ -1330,8 +1328,6 @@ pub enum CraneliftTrap {
     User(TrapCode),
     #[error("user debug")]
     Debug,
-    #[error("resumable")]
-    Resumable,
 }
 
 /// Compare two values using the given integer condition `code`.

diff --git a/fuzz/fuzz_targets/cranelift-fuzzgen.rs b/fuzz/fuzz_targets/cranelift-fuzzgen.rs
@@ -107,7 +107,6 @@ impl Default for Statistics {
         // Pre-Register all trap codes since we can't modify this hashmap atomically.
         let mut run_result_trap = HashMap::new();
         run_result_trap.insert(CraneliftTrap::Debug, AtomicU64::new(0));
-        run_result_trap.insert(CraneliftTrap::Resumable, AtomicU64::new(0));
         for trapcode in TrapCode::non_user_traps() {
             run_result_trap.insert(CraneliftTrap::User(*trapcode), AtomicU64::new(0));
         }