Cranelift: make all non-tail, non-indirect calls patchable, and rename patchable ABI to preserve_all. (#12160)

* Cranelift: make all non-tail, non-indirect calls patchable, and rename patchable ABI to `preserve_all`.

As discussed in this week's Cranelift meeting, we've discovered a need
to generalize the `patchable_call` mechanism and corresponding
`patchable` ABI slightly. In particular, we will need patchable
`try_call` callsites as well in order to allow breakpoint handlers to
throw exceptions (desirable functionality eventually) and have this work
in the presence of inlining. Also, it's just a nice generalization to
say that patchability is an orthogonal dimension to the call ABI and the
other restrictions we initially imposed, and works as long as the basic
requirement (no return values) is met.

This also renames the `patchable` ABI to `preserve_all`, to make it
clear that its purpose is actually orthogonal, and it can be used
independently of patchable callsites. It also deletes the `cold` ABI,
which never actually did anything and is misleading in the presence of
an actual cold-ish (subzero temperature, actually) ABI like
`preserve_all`.

* Review feedback.

Author: cfallin

cranelift/codegen/meta/src/shared/instructions.rs

@@ -3918,31 +3918,4 @@ pub(crate) fn define(
         )
         .other_side_effects(),
     );
-
-    ig.push(
-        Inst::new(
-            "patchable_call",
-            r#"
-         A call instruction that can be turned on/off by patching machine code.
-
-         This call is different than an ordinary call in a few ways:
-         - It can only call functions with the `patchable` ABI.
-         - As a result of that, the called function cannot have any
-           return values.
-         - The call emits metadata into the MachBuffer that allows the consumer
-           of the machine code to edit the code to either make the call or not.
-
-         Note that the lack of return values is necessary: because the call
-         may or may not occur, dynamically, we cannot rely on any values
-         actually being defined.
-         "#,
-            &formats.call,
-        )
-        .operands_in(vec![
-            Operand::new("FN", &entities.func_ref)
-                .with_doc("function to call, declared by `function`"),
-            Operand::new("args", &entities.varargs).with_doc("call arguments"),
-        ])
-        .call(),
-    );
 }

cranelift/codegen/meta/src/shared/settings.rs

@@ -169,11 +169,11 @@ pub(crate) fn define() -> SettingGroup {
         vec![
             "isa_default",
             "fast",
-            "cold",
             "system_v",
             "windows_fastcall",
             "apple_aarch64",
             "probestack",
+            "preserve_all",
         ],
     );
 

cranelift/codegen/src/inline.rs

@@ -139,6 +139,14 @@ pub(crate) fn do_inlining(
             debug_assert_eq!(Some(block), cursor.func.layout.inst_block(inst));
 
             match cursor.func.dfg.insts[inst] {
+                ir::InstructionData::Call { func_ref, .. }
+                    if cursor.func.dfg.ext_funcs[func_ref].patchable =>
+                {
+                    // Can't inline patchable calls; they need to
+                    // remain patchable and inlining the whole body is
+                    // decidedly *not* patchable!
+                }
+
                 ir::InstructionData::Call {
                     opcode: opcode @ ir::Opcode::Call | opcode @ ir::Opcode::ReturnCall,
                     args: _,
@@ -227,14 +235,6 @@ pub(crate) fn do_inlining(
                     // Can't inline indirect calls; need to have some earlier
                     // pass rewrite them into direct calls first, when possible.
                 }
-                ir::InstructionData::Call {
-                    opcode: ir::Opcode::PatchableCall,
-                    ..
-                } => {
-                    // Can't inline patchable calls; they need to
-                    // remain patchable and inlining the whole body is
-                    // decidedly *not* patchable!
-                }
                 _ => {
                     debug_assert!(
                         !cursor.func.dfg.insts[inst].opcode().is_call(),
@@ -505,19 +505,7 @@ fn inline_one(
                         call_opcode == ir::Opcode::TryCall,
                         call_exception_table.is_some()
                     );
-                    // Note that we do not fix up patchable calls
-                    // inlined at a try-call to a try-call, because
-                    // the "patchable ABI" does not support catching
-                    // exceptions. This does mean that we cannot have
-                    // an exception-throw propagate out of a
-                    // breakpoint when we use patchable calls to set
-                    // up breakpoints, but we don't expect that to
-                    // occur.
-                    //
-                    // FIXME: consider making patchability an aspect
-                    // of any call; then we can remove this special
-                    // case.
-                    if call_opcode == ir::Opcode::TryCall && opcode != ir::Opcode::PatchableCall {
+                    if call_opcode == ir::Opcode::TryCall {
                         allocs
                             .calls_needing_exception_table_fixup
                             .push(inlined_inst);
@@ -1491,6 +1479,7 @@ fn create_func_refs(
         name,
         signature,
         colocated,
+        patchable,
     } in callee.dfg.ext_funcs.values()
     {
         func.dfg.ext_funcs.push(ir::ExtFuncData {
@@ -1507,6 +1496,7 @@ fn create_func_refs(
             },
             signature: entity_map.inlined_sig_ref(*signature),
             colocated: *colocated,
+            patchable: *patchable,
         });
     }
 

cranelift/codegen/src/inst_predicates.rs

@@ -149,11 +149,7 @@ pub fn has_memory_fence_semantics(op: Opcode) -> bool {
         | Opcode::Fence
         | Opcode::Debugtrap
         | Opcode::SequencePoint => true,
-        Opcode::Call
-        | Opcode::CallIndirect
-        | Opcode::TryCall
-        | Opcode::TryCallIndirect
-        | Opcode::PatchableCall => true,
+        Opcode::Call | Opcode::CallIndirect | Opcode::TryCall | Opcode::TryCallIndirect => true,
         op if op.can_trap() => true,
         _ => false,
     }

cranelift/codegen/src/ir/extfunc.rs

@@ -313,6 +313,14 @@ pub struct ExtFuncData {
     /// See the documentation for `RelocDistance` for more details. A `colocated` flag value of
     /// `true` implies `RelocDistance::Near`.
     pub colocated: bool,
+    /// Is this function "patchable"? If so, calls to this function will
+    /// emit additional metadata indicating how to patch them in or out.
+    ///
+    /// Calls to functions of any calling convention may be patchable,
+    /// *but* only calls with no return values are patchable. This is
+    /// because every SSA value must always be defined, and return
+    /// values would not be if the call were patched out.
+    pub patchable: bool,
 }
 
 impl ExtFuncData {
@@ -340,6 +348,9 @@ impl<'a> fmt::Display for DisplayableExtFuncData<'a> {
         if self.ext_func.colocated {
             write!(f, "colocated ")?;
         }
+        if self.ext_func.patchable {
+            write!(f, "patchable ")?;
+        }
         write!(
             f,
             "{} {}",
@@ -384,7 +395,8 @@ mod tests {
     fn call_conv() {
         for &cc in &[
             CallConv::Fast,
-            CallConv::Cold,
+            CallConv::PreserveAll,
+            CallConv::Tail,
             CallConv::SystemV,
             CallConv::WindowsFastcall,
         ] {

cranelift/codegen/src/isa/aarch64/abi.rs

@@ -1067,6 +1067,7 @@ impl ABIMachineSpec for AArch64MachineDeps {
                 callee_conv: call_conv,
                 callee_pop_size: 0,
                 try_call_info: None,
+                patchable: false,
             }),
         });
         insts
@@ -1101,8 +1102,14 @@ impl ABIMachineSpec for AArch64MachineDeps {
             (isa::CallConv::Tail, true) => ALL_CLOBBERS,
             (isa::CallConv::Winch, true) => ALL_CLOBBERS,
             (isa::CallConv::Winch, false) => WINCH_CLOBBERS,
+            // Note that "PreserveAll" actually preserves nothing at
+            // the callsite if used for a `try_call`, because the
+            // unwinder ABI for `try_call`s is still "no clobbered
+            // register restores" for this ABI (so as to work with
+            // Wasmtime).
+            (isa::CallConv::PreserveAll, true) => ALL_CLOBBERS,
             (isa::CallConv::SystemV, _) => DEFAULT_AAPCS_CLOBBERS,
-            (isa::CallConv::Patchable, _) => NO_CLOBBERS,
+            (isa::CallConv::PreserveAll, _) => NO_CLOBBERS,
             (_, false) => DEFAULT_AAPCS_CLOBBERS,
             (_, true) => panic!("unimplemented clobbers for exn abi of {call_conv:?}"),
         }
@@ -1184,7 +1191,9 @@ impl ABIMachineSpec for AArch64MachineDeps {
     fn exception_payload_regs(call_conv: isa::CallConv) -> &'static [Reg] {
         const PAYLOAD_REGS: &'static [Reg] = &[regs::xreg(0), regs::xreg(1)];
         match call_conv {
-            isa::CallConv::SystemV | isa::CallConv::Tail => PAYLOAD_REGS,
+            isa::CallConv::SystemV | isa::CallConv::Tail | isa::CallConv::PreserveAll => {
+                PAYLOAD_REGS
+            }
             _ => &[],
         }
     }
@@ -1271,7 +1280,7 @@ fn is_reg_saved_in_prologue(
     sig: &Signature,
     r: RealReg,
 ) -> bool {
-    if call_conv == isa::CallConv::Patchable {
+    if call_conv == isa::CallConv::PreserveAll {
         return true;
     }
 

cranelift/codegen/src/isa/aarch64/inst.isle

@@ -817,9 +817,6 @@
        ;; An indirect return-call macro instruction.
        (ReturnCallInd (info BoxReturnCallIndInfo))
 
-       ;; A patchable call instruction.
-       (PatchableCall (info BoxCallInfo))
-
        ;; A pseudo-instruction that captures register arguments in vregs.
        (Args
         (args VecArgPair))
@@ -4481,7 +4478,7 @@
 
 ;;;; Helpers for Emitting Calls ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
-(decl gen_call_info (Sig ExternalName CallArgList CallRetList OptionTryCallInfo) BoxCallInfo)
+(decl gen_call_info (Sig ExternalName CallArgList CallRetList OptionTryCallInfo bool) BoxCallInfo)
 (extern constructor gen_call_info gen_call_info)
 
 (decl gen_call_ind_info (Sig Reg CallArgList CallRetList OptionTryCallInfo) BoxCallIndInfo)
@@ -4513,11 +4510,6 @@
 (rule (return_call_ind_impl info)
       (SideEffectNoResult.Inst (MInst.ReturnCallInd info)))
 
-;; Helper for creating `MInst.PatchableCall` instructions.
-(decl patchable_call_impl (BoxCallInfo) SideEffectNoResult)
-(rule (patchable_call_impl info)
-      (SideEffectNoResult.Inst (MInst.PatchableCall info)))
-
 ;; Helpers for pinned register manipulation.
 
 (decl write_pinned_reg (Reg) SideEffectNoResult)

cranelift/codegen/src/isa/aarch64/inst/emit.rs

@@ -2941,8 +2941,8 @@ impl MachInstEmit for Inst {
                     sink.put4(0xd65f0bff | (op2 << 9)); // reta{key}
                 }
             }
-            &Inst::Call { ref info } | &Inst::PatchableCall { ref info } => {
-                let is_patchable = matches!(self, Inst::PatchableCall { .. });
+            &Inst::Call { ref info } => {
+                let start = sink.cur_offset();
                 let user_stack_map = state.take_stack_map();
                 sink.add_reloc(Reloc::Arm64Call, &info.dest, 0);
                 sink.put4(enc_jump26(0b100101, 0));
@@ -2956,8 +2956,6 @@ impl MachInstEmit for Inst {
                         Some(state.frame_layout.sp_to_fp()),
                         try_call.exception_handlers(&state.frame_layout),
                     );
-                } else if is_patchable {
-                    sink.add_patchable_call_site(4);
                 } else {
                     sink.add_call_site();
                 }
@@ -2970,12 +2968,16 @@ impl MachInstEmit for Inst {
                     }
                 }
 
-                // Load any stack-carried return values.
-                info.emit_retval_loads::<AArch64MachineDeps, _, _>(
-                    state.frame_layout().stackslots_size,
-                    |inst| inst.emit(sink, emit_info, state),
-                    |needed_space| Some(Inst::EmitIsland { needed_space }),
-                );
+                if info.patchable {
+                    sink.add_patchable_call_site(sink.cur_offset() - start);
+                } else {
+                    // Load any stack-carried return values.
+                    info.emit_retval_loads::<AArch64MachineDeps, _, _>(
+                        state.frame_layout().stackslots_size,
+                        |inst| inst.emit(sink, emit_info, state),
+                        |needed_space| Some(Inst::EmitIsland { needed_space }),
+                    );
+                }
 
                 // If this is a try-call, jump to the continuation
                 // (normal-return) block.

cranelift/codegen/src/isa/aarch64/inst/mod.rs

@@ -801,7 +801,7 @@ fn aarch64_get_operands(inst: &mut Inst, collector: &mut impl OperandVisitor) {
         }
         Inst::Ret { .. } | Inst::AuthenticatedRet { .. } => {}
         Inst::Jump { .. } => {}
-        Inst::Call { info, .. } | Inst::PatchableCall { info, .. } => {
+        Inst::Call { info, .. } => {
             let CallInfo { uses, defs, .. } = &mut **info;
             for CallArgPair { vreg, preg } in uses {
                 collector.reg_fixed_use(vreg, *preg);
@@ -1008,7 +1008,6 @@ impl MachInst for Inst {
         match self {
             Inst::Call { .. }
             | Inst::CallInd { .. }
-            | Inst::PatchableCall { .. }
             | Inst::ElfTlsGetAddr { .. }
             | Inst::MachOTlsGetAddr { .. } => CallType::Regular,
 
@@ -1093,7 +1092,7 @@ impl MachInst for Inst {
 
     fn is_safepoint(&self) -> bool {
         match self {
-            Inst::Call { .. } | Inst::CallInd { .. } | Inst::PatchableCall { .. } => true,
+            Inst::Call { .. } | Inst::CallInd { .. } => true,
             _ => false,
         }
     }
@@ -2599,9 +2598,6 @@ impl Inst {
                     .unwrap_or_default();
                 format!("blr {rn}{try_call}")
             }
-            &Inst::PatchableCall { .. } => {
-                format!("bl 0 ; patchable")
-            }
             &Inst::ReturnCall { ref info } => {
                 let mut s = format!(
                     "return_call {:?} new_stack_arg_size:{}",

cranelift/codegen/src/isa/aarch64/lower.isle

@@ -2520,7 +2520,7 @@
 
 ;;;; Rules for `func_addr` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
-(rule (lower (func_addr (func_ref_data _ extname dist)))
+(rule (lower (func_addr (func_ref_data _ extname dist _)))
       (load_ext_name (box_external_name extname) 0 dist))
 
 ;;;; Rules for `symbol_value` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
@@ -2542,17 +2542,17 @@
 ;; Rules for `call` and `call_indirect` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
 ;; Direct call to an in-range function.
-(rule 1 (lower (call (func_ref_data sig_ref name (RelocDistance.Near)) args))
+(rule 1 (lower (call (func_ref_data sig_ref name (RelocDistance.Near) patchable) args))
       (let ((output ValueRegsVec (gen_call_output sig_ref))
             (abi Sig (abi_sig sig_ref))
             (uses CallArgList (gen_call_args abi args))
             (defs CallRetList (gen_call_rets abi output))
-            (info BoxCallInfo (gen_call_info abi name uses defs (try_call_none)))
+            (info BoxCallInfo (gen_call_info abi name uses defs (try_call_none) patchable))
             (_ Unit (emit_side_effect (call_impl info))))
         output))
 
 ;; Direct call to an out-of-range function (implicitly via pointer).
-(rule (lower (call (func_ref_data sig_ref name dist) args))
+(rule (lower (call (func_ref_data sig_ref name dist false) args))
       (let ((output ValueRegsVec (gen_call_output sig_ref))
             (abi Sig (abi_sig sig_ref))
             (uses CallArgList (gen_call_args abi args))
@@ -2573,30 +2573,19 @@
             (_ Unit (emit_side_effect (call_ind_impl info))))
         output))
 
-;;;; Rules for `patchable_call` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
-
-;; Direct call to an in-range function.
-(rule (lower (patchable_call (func_ref_data sig_ref name (RelocDistance.Near)) args))
-      (let ((abi Sig (abi_sig sig_ref))
-            (uses CallArgList (gen_call_args abi args))
-            (defs CallRetList (gen_patchable_call_rets))
-            (info BoxCallInfo (gen_call_info abi name uses defs (try_call_none)))
-            (_ Unit (emit_side_effect (patchable_call_impl info))))
-        (output_none)))
-
 ;;;; Rules for `try_call` and `try_call_indirect` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
 ;; Direct call to an in-range function.
-(rule 1 (lower_branch (try_call (func_ref_data sig_ref name (RelocDistance.Near)) args et) targets)
+(rule 1 (lower_branch (try_call (func_ref_data sig_ref name (RelocDistance.Near) patchable) args et) targets)
       (let ((abi Sig (abi_sig sig_ref))
             (trycall OptionTryCallInfo (try_call_info et targets))
             (uses CallArgList (gen_call_args abi args))
             (defs CallRetList (gen_try_call_rets abi))
-            (info BoxCallInfo (gen_call_info abi name uses defs trycall)))
+            (info BoxCallInfo (gen_call_info abi name uses defs trycall patchable)))
         (emit_side_effect (call_impl info))))
 
 ;; Direct call to an out-of-range function (implicitly via pointer).
-(rule (lower_branch (try_call (func_ref_data sig_ref name dist) args et) targets)
+(rule (lower_branch (try_call (func_ref_data sig_ref name dist false) args et) targets)
       (let ((abi Sig (abi_sig sig_ref))
             (trycall OptionTryCallInfo (try_call_info et targets))
             (uses CallArgList (gen_call_args abi args))
@@ -2625,14 +2614,14 @@
 ;;;; Rules for `return_call` and `return_call_indirect` ;;;;;;;;;;;;;;;;;;;;;;;;
 
 ;; Direct call to an in-range function.
-(rule 1 (lower (return_call (func_ref_data sig_ref name (RelocDistance.Near)) args))
+(rule 1 (lower (return_call (func_ref_data sig_ref name (RelocDistance.Near) false) args))
       (let ((abi Sig (abi_sig sig_ref))
             (uses CallArgList (gen_return_call_args abi args))
             (info BoxReturnCallInfo (gen_return_call_info abi name uses)))
         (side_effect (return_call_impl info))))
 
 ;; Direct call to an out-of-range function (implicitly via pointer).
-(rule (lower (return_call (func_ref_data sig_ref name dist) args))
+(rule (lower (return_call (func_ref_data sig_ref name dist false) args))
       (let ((abi Sig (abi_sig sig_ref))
             (uses CallArgList (gen_return_call_args abi args))
             (target Reg (load_ext_name name 0 dist))