majit dynasm: partition the parallel-move keys, fault outside the operand contract, and fix the field-width witness

majit/majit-backend-dynasm/src/aarch64/assembler.rs

@@ -7981,7 +7981,11 @@ impl<'a> crate::jump::RegallocMoves for AssemblerARM64<'a> {
                 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",
+            ),
         }
     }
 
@@ -8001,7 +8005,10 @@ impl<'a> crate::jump::RegallocMoves for AssemblerARM64<'a> {
                 self.emit_ldr_fp(16, f.ebp_loc.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",
+            ),
         }
     }
 
@@ -8021,7 +8028,10 @@ impl<'a> crate::jump::RegallocMoves for AssemblerARM64<'a> {
                 dynasm!(self.mc ; .arch aarch64 ; ldr x16, [sp], #16);
                 self.emit_str_fp(16, f.ebp_loc.value);
             }
-            _ => {}
+            _ => panic!(
+                "parallel move cannot restore {loc:?} from the stack; emitting \
+                 nothing here would leave the stack pointer shifted",
+            ),
         }
     }
 }

majit/majit-backend-dynasm/src/jump.rs

@@ -25,9 +25,17 @@ 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 slot; a source
+/// is one of those or an immediate. Nothing else is a location this algorithm
+/// can schedule, and an implementation must fault on anything else 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 +45,67 @@ 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,
+        // Not in the operand contract above. Minting a key for it 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 +129,16 @@ pub(crate) fn remap_frame_layout<A: RegallocMoves + ?Sized>(
     let mut pending_dests = dst_locations.len() as i32;
     let mut srccount: IndexMap<i32, i32> = 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 +242,7 @@ pub(crate) fn remap_frame_layout_mixed<A: RegallocMoves + ?Sized>(
         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);

majit/majit-backend-dynasm/src/x86/assembler.rs

@@ -8872,7 +8872,11 @@ impl<'a> crate::jump::RegallocMoves for Assembler386<'a> {
                     ; mov [rbp + o2], Rq(scratch)
                 );
             }
-            _ => {}
+            _ => panic!(
+                "parallel move {src:?} -> {dst:?} is outside the RegallocMoves \
+                 operand contract; emitting nothing here would leave the \
+                 destination stale",
+            ),
         }
     }
 
@@ -8890,7 +8894,10 @@ impl<'a> crate::jump::RegallocMoves for Assembler386<'a> {
             Loc::Frame(f) => {
                 dynasm!(self.mc ; .arch x64 ; push QWORD [rbp + f.ebp_loc.value]);
             }
-            _ => {}
+            _ => panic!(
+                "parallel move cannot park {loc:?} on the stack; emitting nothing \
+                 here would leave the matching pop unbalanced",
+            ),
         }
     }
 
@@ -8908,7 +8915,10 @@ impl<'a> crate::jump::RegallocMoves for Assembler386<'a> {
             Loc::Frame(f) => {
                 dynasm!(self.mc ; .arch x64 ; pop QWORD [rbp + f.ebp_loc.value]);
             }
-            _ => {}
+            _ => panic!(
+                "parallel move cannot restore {loc:?} from the stack; emitting \
+                 nothing here would leave the stack pointer shifted",
+            ),
         }
     }
 }

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<T>(_: 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<T>(_: fn(&#struct_path) -> &T) -> usize {
                                 ::core::mem::size_of::<T>()
                             }
                             assert!(
-                                __field_width(|__s: &#struct_path| __s.#member)
+                                __field_width(|__s: &#struct_path| &__s.#member)
                                     == ::core::mem::size_of::<i64>(),
                                 #message,
                             );

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);
     }
 }
 
@@ -61,7 +76,13 @@ struct PointerFieldState {
     array_fields = { PointerFieldStack::data => i64 },
     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 +186,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",
+    );
 }