Skip to content

Commit 1f50af1

Browse files
committed
Make the dead-load-with-context instruction a MachInst at root.
This gives us a place to put regalloc constraints and to gather metadata (specifically, instruction locations). Add a compile disas test to make sure `dead_load_with_context` is still emitting an acceptable dead load. It is. The only difference is that it's loading into `rdx` rather than `edx` now, probably due to my new regalloc constraints: ``` - movl (%rdx), %edx + movq (%rdx), %rdx ``` Also... * Make the new instruction a `.call()` in consistency with `stack_switch` being one. * Move the RDI-specificity to the regalloc constraints, which lets us remove the preg_rdi ISLE constructor I had added. * Reserve r10 as a scratch register.
1 parent bb153ec commit 1f50af1

11 files changed

Lines changed: 79 additions & 15 deletions

File tree

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -395,6 +395,8 @@ fn define_control_flow(
395395
``VMContext``'s ``vm_store_context``) and can use the second
396396
reserved register to store a temp value, as needed on platforms
397397
where signal handlers cannot push stack frames.
398+
399+
On x64, RDI holds ``context``, and R10 is used as scratch space.
398400
"#,
399401
&formats.binary,
400402
)
@@ -403,8 +405,10 @@ fn define_control_flow(
403405
Operand::new("context", iAddr)
404406
.with_doc("arbitrary address-sized context to pass to signal handler"),
405407
])
406-
// Are we a call? stack_switch calls itself one "as it continues execution elsewhere".
407-
// .is_call()
408+
// Are we a call? stack_switch calls itself one "as it continues
409+
// execution elsewhere". See reasoning at
410+
// https://github.com/bytecodealliance/wasmtime/pull/9078#issuecomment-2273869774.
411+
.call()
408412
.can_load()
409413
// Don't optimize me out just because I don't def anything. TODO: Can we use side_effects_idempotent()?
410414
.other_side_effects(),

cranelift/codegen/src/ir/instructions.rs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -613,9 +613,13 @@ impl InstructionData {
613613
Self::Ternary {
614614
opcode: Opcode::StackSwitch,
615615
..
616+
}
617+
| Self::Binary {
618+
opcode: Opcode::DeadLoadWithContext,
619+
..
616620
} => {
617-
// `StackSwitch` is not actually a call, but has the .call() side
618-
// effect as it continues execution elsewhere.
621+
// These instructions aren't actually calls, but they have the
622+
// .call() side effect, as they continue execution elsewhere.
619623
CallInfo::NotACall
620624
}
621625
_ => {

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

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,9 @@
194194
(offset i64)
195195
(distance RelocDistance))
196196

197+
(DeadLoadWithContext (load_ptr Gpr)
198+
(context Gpr))
199+
197200
;; =========================================
198201
;; Instructions pertaining to atomic memory accesses.
199202

@@ -1342,6 +1345,12 @@
13421345
(rule (return_call_unknown info)
13431346
(SideEffectNoResult.Inst (MInst.ReturnCallUnknown info)))
13441347

1348+
;; Helper for creating `DeadLoadWithContext` instructions.
1349+
(decl x64_dead_load_with_context (Gpr Gpr) SideEffectNoResult)
1350+
(rule (x64_dead_load_with_context load_ptr context)
1351+
(SideEffectNoResult.Inst (MInst.DeadLoadWithContext load_ptr
1352+
context)))
1353+
13451354
;;;; Helpers for emitting stack switches ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
13461355

13471356
(decl x64_stack_switch_basic (Gpr Gpr Gpr) Gpr)
@@ -4064,9 +4073,6 @@
40644073
(decl preg_rsp () PReg)
40654074
(extern constructor preg_rsp preg_rsp)
40664075

4067-
(decl preg_rdi () PReg)
4068-
(extern constructor preg_rdi preg_rdi)
4069-
40704076
(decl preg_pinned () PReg)
40714077
(extern constructor preg_pinned preg_pinned)
40724078

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -645,6 +645,14 @@ pub(crate) fn emit(
645645
sink.bind_label(resume, state.ctrl_plane_mut());
646646
}
647647

648+
Inst::DeadLoadWithContext { .. } => {
649+
// The ISLE has already emitted the dead load. Put the address of
650+
// this instruction aside so we can later distinguish whether a
651+
// segfault is its fault.
652+
653+
// Search for "let pc_offset = layout.ip_offset as i32;" as a string to pull on.
654+
}
655+
648656
Inst::JmpKnown { dst } => uncond_jmp(sink, *dst),
649657

650658
Inst::WinchJmpIf { cc, taken } => one_way_jmp(sink, *cc, *taken),

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

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
//! This module defines x86_64-specific machine instruction types.
22
33
pub use emit_state::EmitState;
4+
use regalloc2::PRegSet;
45

56
use crate::binemit::{Addend, CodeOffset, Reloc};
67
use crate::ir::{ExternalName, LibCall, TrapCode, Type, types};
@@ -96,6 +97,7 @@ impl Inst {
9697
| Inst::Args { .. }
9798
| Inst::Rets { .. }
9899
| Inst::StackSwitchBasic { .. }
100+
| Inst::DeadLoadWithContext { .. }
99101
| Inst::TrapIf { .. }
100102
| Inst::TrapIfAnd { .. }
101103
| Inst::TrapIfOr { .. }
@@ -671,6 +673,12 @@ impl PrettyPrint for Inst {
671673
)
672674
}
673675

676+
Inst::DeadLoadWithContext { load_ptr, context } => {
677+
let load_ptr = pretty_print_reg(**load_ptr, 8);
678+
let context = pretty_print_reg(**context, 8);
679+
format!("dead_load_with_context {load_ptr}, {context}")
680+
}
681+
674682
Inst::JmpKnown { dst } => {
675683
let op = ljustify("jmp".to_string());
676684
let dst = dst.to_string();
@@ -1045,6 +1053,19 @@ fn x64_get_operands(inst: &mut Inst, collector: &mut impl OperandVisitor) {
10451053
collector.reg_clobbers(clobbers);
10461054
}
10471055

1056+
Inst::DeadLoadWithContext { load_ptr, context } => {
1057+
// load_ptr is an input param.
1058+
collector.reg_use(load_ptr);
1059+
// Demand context (vmctx) go into RDI.
1060+
// TODO: Do I still have to move it, or will regalloc make sure it's there?
1061+
collector.reg_fixed_use(context, regs::rdi());
1062+
// Reserve r10 as a place for the signal handler to stow the return
1063+
// address (which we're overwriting with that of the epoch-ending
1064+
// stub). Picking r10 because it's caller-saved but otherwise
1065+
// arbitrarily.
1066+
collector.reg_clobbers(PRegSet::empty().with(regs::gpr_preg(asm::gpr::enc::R10)));
1067+
}
1068+
10481069
Inst::ReturnCallKnown { info } => {
10491070
let ReturnCallInfo {
10501071
dest, uses, tmp, ..

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

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3583,10 +3583,16 @@
35833583
(rule (lower (dead_load_with_context load_ptr context))
35843584
(let (
35853585
;; Put vmctx into rdi. TODO: Do we have to shove context in a different reg first? The type system seems happy to allow it without.
3586-
(_ SideEffectNoResult (mov_to_preg (preg_rdi) context))
3586+
;; Actually, I suspect we don't even need to do this move; because we use reg_fixed_use() to constrain the `context` arg of DeadLoadWithContext to RDI, I hypothesize regalloc will insert any MOV needed to make that so. It is, after all, in the business of inserting MOVs for spills.
3587+
;; (_ SideEffectNoResult (mov_to_preg (preg_rdi) context))
3588+
35873589
;; Load from load_ptr to perhaps trigger a trap.
35883590
;; TODO: May be able to change x64_load to higher-level "load", from codegen/meta/src/shared/instructions.rs.
3589-
(_ Gpr (x64_load $I64 (to_amode (mem_flags_aligned_read_only) load_ptr (zero_offset)) (ExtKind.None))))
3591+
(_ Gpr (x64_load $I64 (to_amode (mem_flags_aligned_read_only)
3592+
load_ptr
3593+
(zero_offset))
3594+
(ExtKind.None)))
3595+
(_ SideEffectNoResult (x64_dead_load_with_context load_ptr context)))
35903596
(output_none)))
35913597

35923598
;;;; Rules for `get_{frame,stack}_pointer` and `get_return_address` ;;;;;;;;;;;;

cranelift/codegen/src/isa/x64/lower/isle.rs

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -658,11 +658,6 @@ impl Context for IsleContext<'_, '_, MInst, X64Backend> {
658658
regs::rsp().to_real_reg().unwrap().into()
659659
}
660660

661-
#[inline]
662-
fn preg_rdi(&mut self) -> PReg {
663-
regs::rdi().to_real_reg().unwrap().into()
664-
}
665-
666661
#[inline]
667662
fn preg_pinned(&mut self) -> PReg {
668663
regs::pinned_reg().to_real_reg().unwrap().into()

cranelift/codegen/src/isa/x64/pcc.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -209,6 +209,8 @@ pub(crate) fn check(
209209

210210
Inst::StackSwitchBasic { .. } => Err(PccError::UnimplementedInst),
211211

212+
Inst::DeadLoadWithContext { .. } => Err(PccError::UnimplementedInst),
213+
212214
Inst::LabelAddress { .. } => Err(PccError::UnimplementedInst),
213215

214216
Inst::SequencePoint { .. } => Ok(()),

cranelift/interpreter/src/step.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1316,6 +1316,7 @@ where
13161316
Opcode::X86Pmaddubsw => unimplemented!("X86Pmaddubsw"),
13171317
Opcode::X86Cvtt2dq => unimplemented!("X86Cvtt2dq"),
13181318
Opcode::StackSwitch => unimplemented!("StackSwitch"),
1319+
Opcode::DeadLoadWithContext => unimplemented!("DeadLoadWithContext"),
13191320

13201321
Opcode::TryCall => unimplemented!("TryCall"),
13211322
Opcode::TryCallIndirect => unimplemented!("TryCallIndirect"),
Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
;;! target = "x86_64"
2+
;;! test = "compile"
3+
;;! flags = ["-Wepoch-interruption-via-mmu=y"]
4+
5+
(module
6+
(memory 0)
7+
(func)
8+
)
9+
;; wasm[0]::function[0]:
10+
;; pushq %rbp
11+
;; movq %rsp, %rbp
12+
;; movq 8(%rdi), %rdx
13+
;; movq 0x10(%rdx), %rdx
14+
;; movq (%rdx), %rdx
15+
;; movq %rbp, %rsp
16+
;; popq %rbp
17+
;; retq

0 commit comments

Comments
 (0)