Repository: bytecodealliance/wasmtime
Pull request #8809 (2024-06-14 19:13:06)

Cranelift: Remove resumable traps
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.

<!--
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
-->



Patch:
diff --git a/cranelift/codegen/meta/src/shared/instructions.rs b/cranelift/codegen/meta/src/shared/instructions.rs
index 909f80160..6019ce2d2 100644
--- 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
index 9b72e239b..9a9254264 100644
--- 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
index fc31e3d2a..3ac4953d2 100644
--- 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
index dc7ed36e4..5fd9bf67e 100644
--- 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
index 2e10a253a..d287e0c7b 100644
--- 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
index 40f748302..977a28c02 100644
--- 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
index 9857f8dd2..92d8b7478 100644
--- 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
index b8de7bcd7..fb14a5d8f 100644
--- 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/fuzzgen/src/function_generator.rs b/cranelift/fuzzgen/src/function_generator.rs
index 31d3cea93..af345c73b 100644
--- 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
index 96d465966..bb9703bfc 100644
--- 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
index c3c1b19d7..b496912d8 100644
--- 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));
         }


Test patch:
diff --git a/cranelift/filetests/filetests/isa/s390x/traps.clif b/cranelift/filetests/filetests/isa/s390x/traps.clif
index f231f0851..54a071823 100644
--- 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
-