diff --git a/majit/majit-backend-dynasm/src/aarch64/assembler.rs b/majit/majit-backend-dynasm/src/aarch64/assembler.rs index c01e9992297..1993d72e188 100644 --- a/majit/majit-backend-dynasm/src/aarch64/assembler.rs +++ b/majit/majit-backend-dynasm/src/aarch64/assembler.rs @@ -9,6 +9,7 @@ /// _assemble — assembler.py:779 (walk ops + emit code) /// patch_jump_for_descr — assembler.py:965 /// redirect_call_assembler — assembler.py:1138 +use crate::regloc::ebp_loc_pat; use indexmap::IndexMap; use majit_ir::IndexMapExt; use std::sync::Arc; @@ -7945,16 +7946,16 @@ impl<'a> crate::jump::RegallocMoves for AssemblerARM64<'a> { dynasm!(self.mc ; .arch aarch64 ; fmov D(d.value), X(s.value)); } } - (Loc::Reg(s), Loc::Frame(f)) => { - let ofs = f.ebp_loc.value; + (Loc::Reg(s), ebp_loc_pat!(e)) => { + let ofs = e.value; if s.is_xmm { self.emit_str_fp_d(s.value, ofs); } else { self.emit_str_fp(s.value, ofs); } } - (Loc::Frame(f), Loc::Reg(d)) => { - let ofs = f.ebp_loc.value; + (ebp_loc_pat!(e), Loc::Reg(d)) => { + let ofs = e.value; if d.is_xmm { self.emit_ldr_fp_d(d.value, ofs); } else { @@ -7969,19 +7970,23 @@ impl<'a> crate::jump::RegallocMoves for AssemblerARM64<'a> { self.emit_mov_imm64(d.value as u32, i.value); } } - (Loc::Immed(i), Loc::Frame(f)) => { - let ofs = f.ebp_loc.value; + (Loc::Immed(i), ebp_loc_pat!(e)) => { + let ofs = e.value; self.emit_mov_imm64(16, i.value); self.emit_str_fp(16, ofs); } - (Loc::Frame(f1), Loc::Frame(f2)) if f1.position == f2.position => {} - (Loc::Frame(f1), Loc::Frame(f2)) => { - let o1 = f1.ebp_loc.value; - let o2 = f2.ebp_loc.value; + (ebp_loc_pat!(e1), ebp_loc_pat!(e2)) if e1.value == e2.value => {} + (ebp_loc_pat!(e1), ebp_loc_pat!(e2)) => { + let o1 = e1.value; + let o2 = e2.value; self.emit_ldr_fp(16, o1); self.emit_str_fp(16, o2); } - _ => {} + _ => panic!( + "parallel move {src:?} -> {dst:?} is outside the RegallocMoves \ + operand contract; emitting nothing here would leave the \ + destination stale", + ), } } @@ -7993,15 +7998,18 @@ impl<'a> crate::jump::RegallocMoves for AssemblerARM64<'a> { Loc::Reg(r) => { dynasm!(self.mc ; .arch aarch64 ; str X(r.value), [sp, #-16]!); } - Loc::Frame(f) if f.ebp_loc.is_float => { - self.emit_ldr_fp_d(15, f.ebp_loc.value); + ebp_loc_pat!(e) if e.is_float => { + self.emit_ldr_fp_d(15, e.value); dynasm!(self.mc ; .arch aarch64 ; str D(15), [sp, #-16]!); } - Loc::Frame(f) => { - self.emit_ldr_fp(16, f.ebp_loc.value); + ebp_loc_pat!(e) => { + self.emit_ldr_fp(16, e.value); dynasm!(self.mc ; .arch aarch64 ; str x16, [sp, #-16]!); } - _ => {} + _ => panic!( + "parallel move cannot park {loc:?} on the stack; emitting nothing \ + here would leave the matching pop unbalanced", + ), } } @@ -8013,15 +8021,18 @@ impl<'a> crate::jump::RegallocMoves for AssemblerARM64<'a> { Loc::Reg(r) => { dynasm!(self.mc ; .arch aarch64 ; ldr X(r.value), [sp], #16); } - Loc::Frame(f) if f.ebp_loc.is_float => { + ebp_loc_pat!(e) if e.is_float => { dynasm!(self.mc ; .arch aarch64 ; ldr D(15), [sp], #16); - self.emit_str_fp_d(15, f.ebp_loc.value); + self.emit_str_fp_d(15, e.value); } - Loc::Frame(f) => { + ebp_loc_pat!(e) => { dynasm!(self.mc ; .arch aarch64 ; ldr x16, [sp], #16); - self.emit_str_fp(16, f.ebp_loc.value); + self.emit_str_fp(16, e.value); } - _ => {} + _ => panic!( + "parallel move cannot restore {loc:?} from the stack; emitting \ + nothing here would leave the stack pointer shifted", + ), } } } diff --git a/majit/majit-backend-dynasm/src/jump.rs b/majit/majit-backend-dynasm/src/jump.rs index ea1229694f5..51b9a1bf822 100644 --- a/majit/majit-backend-dynasm/src/jump.rs +++ b/majit/majit-backend-dynasm/src/jump.rs @@ -25,9 +25,20 @@ use indexmap::IndexMap; /// The three emitters `remap_frame_layout` drives, named as `jump.py` calls /// them on the `assembler` it is handed. /// +/// **Operand contract.** A destination is a register or a frame-pointer +/// location in either spelling — `Loc::Frame`, which knows its stack position, +/// or the bare `Loc::Ebp`, which does not (`regloc.py:113 class +/// FrameLoc(RawEbpLoc)`). A source is one of those or an immediate. The one +/// location left out is `Loc::Addr`, which upstream cannot key either. An +/// implementation must fault on anything outside this rather than emit +/// nothing: a move that is counted in `pending_dests` and then silently +/// dropped leaves the destination holding a stale value, and a dropped +/// `regalloc_pop` leaves the machine stack pointer shifted as well. +/// /// `regalloc_push`/`regalloc_pop` exist only to serve the cycle-breaking arm /// below — parking one value of a cycle on the machine stack is the whole -/// reason a cycle can be resolved at all. +/// reason a cycle can be resolved at all. They are only ever handed a +/// destination, so an immediate never reaches them. pub(crate) trait RegallocMoves { /// `assembler.py:1145 regalloc_mov(from_loc, to_loc)`. fn regalloc_mov(&mut self, src: &Loc, dst: &Loc); @@ -37,20 +48,70 @@ pub(crate) trait RegallocMoves { /// A location's identity for the dependency bookkeeping. /// -/// Registers and frame slots share one number space, so the constants keep the -/// classes apart; an immediate has no identity because it is never anyone's -/// destination and can be re-materialised at will. +/// Two locations must get the same key exactly when they are the same storage. +/// Handing two distinct destinations one key collapses them to a single +/// `IndexMap` entry, and the move for whichever one loses is never emitted. +/// +/// Registers and stack slots are told apart **by sign**: a register key is +/// positive, a stack key is `!offset` and so negative. Upstream instead keys a +/// register on its bare number and a frame slot on its byte offset, and argues +/// the two cannot meet because offsets start above the register file — +/// `regloc.py:117-120` says so in as many words and asserts `ebp_offset >= 8 + +/// 8 * IS_X86_64` rather than trusting it. That argument does not survive the +/// per-class bias below: an offset of 4096 is an ordinary frame for a large +/// trace and is exactly the general-register base. +/// +/// The bias is still worth keeping. Upstream gives `r0` and `xmm0` the same key +/// — harmless there because the two files are remapped in separate calls, but +/// `remap_frame_layout_mixed` compares one call's destination keys against the +/// other's sources, which is precisely across the two files. pub(crate) fn loc_as_key(loc: &Loc) -> i32 { + /// Keeps the two register files apart; both stay positive. + const XMM_KEY_BASE: i32 = 0x2000; + const GPR_KEY_BASE: i32 = 0x1000; + match loc { - Loc::Reg(r) if r.is_xmm => 0x2000 + i32::from(r.value), - Loc::Reg(r) => 0x1000 + i32::from(r.value), - Loc::Frame(f) => f.ebp_loc.value, - Loc::Ebp(e) => e.value, + Loc::Reg(r) if r.is_xmm => XMM_KEY_BASE + i32::from(r.value), + Loc::Reg(r) => GPR_KEY_BASE + i32::from(r.value), + // `!offset` for a non-negative offset is negative, so no stack slot can + // ever land on a register key however deep the frame gets. + Loc::Frame(f) => stack_key(f.ebp_loc.value), + Loc::Ebp(e) => stack_key(e.value), + // Never a destination and re-materialisable at will, so it needs no + // identity — only a value no real location can take. Loc::Immed(_) => i32::MIN, - Loc::Addr(a) => a.offset, + // `AddressLoc` is the one location class upstream leaves without a key: + // it overrides neither `_getregkey` nor, for its `'a'`/`'m'` codes, the + // `value` the inherited one reads (`regloc.py:207-250`), so a parallel + // move handed one raises there as well. Minting a key here instead + // would put an entry in `pending_dests` that no emitter can retire. + Loc::Addr(a) => panic!( + "parallel move over an address location (offset {}), which no \ + regalloc_mov can emit", + a.offset, + ), } } +/// The key for a stack slot at `offset` bytes from the frame pointer. +fn stack_key(offset: i32) -> i32 { + debug_assert!( + offset >= 0, + "a negative frame offset ({offset}) inverts back into the positive \ + register key space", + ); + !offset +} + +/// The key of the slot one machine word past `key`. +/// +/// Stack keys run backwards against offsets — `!(offset + WORD)` is +/// `!offset - WORD` — so the neighbour is found by subtracting, and writing +/// `key + WORD` here would silently read the slot on the wrong side. +fn stack_key_next_word(key: i32) -> i32 { + key - WORD as i32 +} + pub(crate) fn loc_width(loc: &Loc) -> usize { match loc { Loc::Reg(r) => r.get_width(), @@ -74,7 +135,16 @@ pub(crate) fn remap_frame_layout( let mut pending_dests = dst_locations.len() as i32; let mut srccount: IndexMap = IndexMap::new(); for dst in dst_locations { - srccount.insert(loc_as_key(dst), 0); + // `jump.py:7 assert key not in srccount`. A repeated destination shares + // one entry while `pending_dests` counts both, so the second one can + // never be retired: every key reaches -1, the loop stops making + // progress, and the cycle-breaking arm finds no key left at or above + // zero to park — the whole call spins. `insert` returns the displaced + // value, so the check costs nothing beyond the store already made. + assert!( + srccount.insert(loc_as_key(dst), 0).is_none(), + "duplicate value in dst_locations!", + ); } for i in 0..dst_locations.len() { let src = src_locations[i]; @@ -178,7 +248,7 @@ pub(crate) fn remap_frame_layout_mixed( if loc.is_stack() { let key = loc_as_key(&loc); if dst_keys.contains_key(&key) - || (loc_width(&loc) > WORD && dst_keys.contains_key(&(key + WORD as i32))) + || (loc_width(&loc) > WORD && dst_keys.contains_key(&stack_key_next_word(key))) { asm.regalloc_push(&loc); extrapushes.push(dstloc); diff --git a/majit/majit-backend-dynasm/src/regloc.rs b/majit/majit-backend-dynasm/src/regloc.rs index 8b0c02abcbb..a7c9a9314ac 100644 --- a/majit/majit-backend-dynasm/src/regloc.rs +++ b/majit/majit-backend-dynasm/src/regloc.rs @@ -159,12 +159,35 @@ pub enum Loc { Addr(AddressLoc), } +/// Matches either spelling of a frame-pointer location, binding its +/// `RawEbpLoc`. +/// +/// `regloc.py:113 class FrameLoc(RawEbpLoc)` — the two are one type upstream, +/// separated here only because Rust has no inheritance. Every operand that +/// addresses through the frame pointer accepts both (`mov`'s `'b'` code), so a +/// match naming just one spelling silently excludes the other. +macro_rules! ebp_loc_pat { + ($e:ident) => { + $crate::regloc::Loc::Frame($crate::regloc::FrameLoc { ebp_loc: $e, .. }) + | $crate::regloc::Loc::Ebp($e) + }; +} + +pub(crate) use ebp_loc_pat; + impl Loc { pub fn is_reg(&self) -> bool { matches!(self, Loc::Reg(_)) } + /// `regloc.py:82 RawEbpLoc.is_stack` returns True, and `FrameLoc` inherits + /// it — a frame slot is a raw ebp location that also knows its stack + /// position, so both spellings are stack locations. + /// + /// Naming only `Frame` here makes an `Ebp` operand look like a register to + /// the parallel move, which then skips the scratch register and hands the + /// backend a memory-to-memory move no machine encodes. pub fn is_stack(&self) -> bool { - matches!(self, Loc::Frame(_)) + matches!(self, Loc::Frame(_) | Loc::Ebp(_)) } pub fn is_immed(&self) -> bool { matches!(self, Loc::Immed(_)) diff --git a/majit/majit-backend-dynasm/src/x86/assembler.rs b/majit/majit-backend-dynasm/src/x86/assembler.rs index 7349b5f57cb..4b7b9549b25 100644 --- a/majit/majit-backend-dynasm/src/x86/assembler.rs +++ b/majit/majit-backend-dynasm/src/x86/assembler.rs @@ -9,6 +9,7 @@ /// _assemble — assembler.py:779 (walk ops + emit code) /// patch_jump_for_descr — assembler.py:965 /// redirect_call_assembler — assembler.py:1138 +use crate::regloc::ebp_loc_pat; use indexmap::IndexMap; use std::sync::Arc; @@ -8827,16 +8828,16 @@ impl<'a> crate::jump::RegallocMoves for Assembler386<'a> { dynasm!(self.mc ; .arch x64 ; movq Rx(d.value), Rq(s.value)); } } - (Loc::Reg(s), Loc::Frame(f)) => { - let ofs = f.ebp_loc.value; + (Loc::Reg(s), ebp_loc_pat!(e)) => { + let ofs = e.value; if s.is_xmm { dynasm!(self.mc ; .arch x64 ; movsd [rbp + ofs], Rx(s.value)); } else { dynasm!(self.mc ; .arch x64 ; mov [rbp + ofs], Rq(s.value)); } } - (Loc::Frame(f), Loc::Reg(d)) => { - let ofs = f.ebp_loc.value; + (ebp_loc_pat!(e), Loc::Reg(d)) => { + let ofs = e.value; if d.is_xmm { dynasm!(self.mc ; .arch x64 ; movsd Rx(d.value), [rbp + ofs]); } else { @@ -8854,25 +8855,29 @@ impl<'a> crate::jump::RegallocMoves for Assembler386<'a> { dynasm!(self.mc ; .arch x64 ; mov Rq(d.value), QWORD i.value); } } - (Loc::Immed(i), Loc::Frame(f)) => { - let ofs = f.ebp_loc.value; + (Loc::Immed(i), ebp_loc_pat!(e)) => { + let ofs = e.value; let scratch = crate::regloc::X86_64_SCRATCH_REG.value; dynasm!(self.mc ; .arch x64 ; mov Rq(scratch), QWORD i.value ; mov [rbp + ofs], Rq(scratch) ); } - (Loc::Frame(f1), Loc::Frame(f2)) if f1.position == f2.position => {} - (Loc::Frame(f1), Loc::Frame(f2)) => { - let o1 = f1.ebp_loc.value; - let o2 = f2.ebp_loc.value; + (ebp_loc_pat!(e1), ebp_loc_pat!(e2)) if e1.value == e2.value => {} + (ebp_loc_pat!(e1), ebp_loc_pat!(e2)) => { + let o1 = e1.value; + let o2 = e2.value; let scratch = crate::regloc::X86_64_SCRATCH_REG.value; dynasm!(self.mc ; .arch x64 ; mov Rq(scratch), [rbp + o1] ; mov [rbp + o2], Rq(scratch) ); } - _ => {} + _ => panic!( + "parallel move {src:?} -> {dst:?} is outside the RegallocMoves \ + operand contract; emitting nothing here would leave the \ + destination stale", + ), } } @@ -8884,13 +8889,16 @@ impl<'a> crate::jump::RegallocMoves for Assembler386<'a> { Loc::Reg(r) => { dynasm!(self.mc ; .arch x64 ; push Rq(r.value)); } - Loc::Frame(f) if f.ebp_loc.is_float => { - dynasm!(self.mc ; .arch x64 ; sub rsp, 8 ; movsd xmm15, [rbp + f.ebp_loc.value] ; movsd [rsp], xmm15); + ebp_loc_pat!(e) if e.is_float => { + dynasm!(self.mc ; .arch x64 ; sub rsp, 8 ; movsd xmm15, [rbp + e.value] ; movsd [rsp], xmm15); } - Loc::Frame(f) => { - dynasm!(self.mc ; .arch x64 ; push QWORD [rbp + f.ebp_loc.value]); + ebp_loc_pat!(e) => { + dynasm!(self.mc ; .arch x64 ; push QWORD [rbp + e.value]); } - _ => {} + _ => panic!( + "parallel move cannot park {loc:?} on the stack; emitting nothing \ + here would leave the matching pop unbalanced", + ), } } @@ -8902,13 +8910,16 @@ impl<'a> crate::jump::RegallocMoves for Assembler386<'a> { Loc::Reg(r) => { dynasm!(self.mc ; .arch x64 ; pop Rq(r.value)); } - Loc::Frame(f) if f.ebp_loc.is_float => { - dynasm!(self.mc ; .arch x64 ; movsd xmm15, [rsp] ; add rsp, 8 ; movsd [rbp + f.ebp_loc.value], xmm15); + ebp_loc_pat!(e) if e.is_float => { + dynasm!(self.mc ; .arch x64 ; movsd xmm15, [rsp] ; add rsp, 8 ; movsd [rbp + e.value], xmm15); } - Loc::Frame(f) => { - dynasm!(self.mc ; .arch x64 ; pop QWORD [rbp + f.ebp_loc.value]); + ebp_loc_pat!(e) => { + dynasm!(self.mc ; .arch x64 ; pop QWORD [rbp + e.value]); } - _ => {} + _ => panic!( + "parallel move cannot restore {loc:?} from the stack; emitting \ + nothing here would leave the stack pointer shifted", + ), } } } diff --git a/majit/majit-macros/src/jit_interp/jitcode_lower/lower_vable.rs b/majit/majit-macros/src/jit_interp/jitcode_lower/lower_vable.rs index 1b9d95eae37..ce8a2d3cb29 100644 --- a/majit/majit-macros/src/jit_interp/jitcode_lower/lower_vable.rs +++ b/majit/majit-macros/src/jit_interp/jitcode_lower/lower_vable.rs @@ -6,8 +6,10 @@ use super::*; /// struct. `descr.py:218-239 get_field_descr` derives both from `FIELDTYPE`, /// but the macro sees only the field's name at the access site, so a sub-word /// integer field has to be named in `int_fields` to be registered as one. -/// Anything undeclared keeps the machine-word default — and has to be eight -/// bytes wide to keep it, which the returned witness enforces. +/// Anything undeclared is registered as an `i64` — and has to be that wide to +/// stay so, which the returned witness enforces. `i64`, not `usize`: the two +/// part company on a 32-bit target such as wasm32, and it is the eight-byte +/// word that is claimed here. pub(super) fn field_scalar_tokens( config: &LowererConfig, key: &str, @@ -78,11 +80,18 @@ pub(super) fn field_scalar_tokens( // Names the field's type without spelling it, which // is the only handle available here: the macro sees // the member, not its declaration. - const fn __field_width(_: fn(&#struct_path) -> T) -> usize { + // + // Through a reference, not by value. `|s| s.field` + // returns `T` and so moves out of a shared borrow, + // which only compiles for a `Copy` field — an + // eight-byte field that is not `Copy` would fail + // expansion with a borrow error naming generated + // code, for a struct with nothing wrong with it. + const fn __field_width(_: fn(&#struct_path) -> &T) -> usize { ::core::mem::size_of::() } assert!( - __field_width(|__s: &#struct_path| __s.#member) + __field_width(|__s: &#struct_path| &__s.#member) == ::core::mem::size_of::(), #message, ); diff --git a/majit/majit-metainterp/tests/jit_interp_array_field_write_kind.rs b/majit/majit-metainterp/tests/jit_interp_array_field_write_kind.rs index 4baf8d10dd2..99accdca7e8 100644 --- a/majit/majit-metainterp/tests/jit_interp_array_field_write_kind.rs +++ b/majit/majit-metainterp/tests/jit_interp_array_field_write_kind.rs @@ -29,8 +29,22 @@ use majit_metainterp::{Assembler, JitDriver}; struct PointerFieldStack { data: *mut i64, size: usize, + /// An eight-byte field that is not `Copy`, named in the write set below but + /// declared in none of the field maps, so it takes the undeclared default + /// and the macro emits its width witness over it. + /// + /// A witness written as `|s| s.field` returns the field by value and so + /// moves out of a shared borrow, which only compiles when the field is + /// `Copy`. This member is the standing check that it is not written that + /// way: if it regresses, this crate stops compiling rather than quietly + /// refusing a struct that has nothing wrong with it. + generation: Generation, } +/// Deliberately not `Copy` and deliberately eight bytes. +#[repr(transparent)] +struct Generation(i64); + /// An opaque in-place mutator. The body is irrelevant — the JIT never looks /// inside a residual — but it has to exist for the concrete path. extern "C" fn jit_scramble_pointer_field(stack: usize) { @@ -40,6 +54,7 @@ extern "C" fn jit_scramble_pointer_field(stack: usize) { } unsafe { (*stack).size = (*stack).size; + (*stack).generation = Generation((*stack).generation.0 + 1); } } @@ -59,9 +74,24 @@ struct PointerFieldState { state_fields = { a: int, sel: ref(PointerFieldStack) }, greens = [], array_fields = { PointerFieldStack::data => i64 }, + int_fields = { + // Declared for its width, not its kind. `usize` is four bytes on a + // 32-bit target, and naming a field in a write set is what sends it + // through the undeclared-scalar default, whose witness demands eight — + // so leaving it undeclared fails macro expansion on wasm32 before any + // of this runs. The control below still reads whatever kind the + // write-set path decided. + PointerFieldStack::size => usize, + }, calls = { jit_scramble_pointer_field => residual_void }, residual_writes = { + // `size` is here only so the scalar control below has a descr to read. + // Naming a field in a write-set layout is what mints its descr, and + // without one the control skips: it asserted nothing, which is the + // failure mode it exists to rule out for the pointer field. sel.data => [jit_scramble_pointer_field], + sel.size => [jit_scramble_pointer_field], + sel.generation => [jit_scramble_pointer_field], }, )] #[allow(unused_assignments, unused_variables)] @@ -165,15 +195,19 @@ fn a_scalar_field_of_the_same_struct_is_still_a_scalar() { .get(&majit_ir::descr::LLType::Struct(type_id)) .and_then(|fields| fields.get("size")) .cloned(); - // `size` is not named by any declaration or access here, so the control is - // only meaningful if something registered it. Skipping when nothing did is - // honest; asserting on an absent slot would pass for the wrong reason. - if let Some(descr) = size_field { - assert_eq!( - descr.field_type(), - majit_ir::Type::Int, - "`size` is a scalar; only the field a pointer declaration names may \ - become a Ref", - ); - } + // Require the slot rather than skipping when it is absent. This test read + // `if let Some(..)` and `size` was in no declaration, so the assertion below + // never ran once — a control that does not execute rules nothing out, which + // is exactly what it was written to prevent for the pointer field. The + // fixture now names `size` in its write set so the descr exists. + let descr = size_field.expect( + "`size` must be registered for this control to assert anything; the \ + fixture's `residual_writes` names it", + ); + assert_eq!( + descr.field_type(), + majit_ir::Type::Int, + "`size` is a scalar; only the field a pointer declaration names may \ + become a Ref", + ); }