diff --git a/majit/majit-ir/src/descr.rs b/majit/majit-ir/src/descr.rs index 231a082ff67..66b392210c5 100644 --- a/majit/majit-ir/src/descr.rs +++ b/majit/majit-ir/src/descr.rs @@ -690,6 +690,63 @@ static FIELD_INDEX_REDERIVED: std::sync::atomic::AtomicUsize = static FIELD_INDEX_UNRESOLVED: std::sync::atomic::AtomicUsize = std::sync::atomic::AtomicUsize::new(0); +/// Counters behind [`GcCache::spec_position_census`] — the same question the +/// four above ask, put to the producer instead of to the reader. +static FIELD_SPEC_CHECKED: std::sync::atomic::AtomicUsize = std::sync::atomic::AtomicUsize::new(0); +static FIELD_SPEC_MISPLACED: std::sync::atomic::AtomicUsize = + std::sync::atomic::AtomicUsize::new(0); + +/// Counters behind [`GcCache::attached_position_census`]. +static FIELD_ATTACHED_CHECKED: std::sync::atomic::AtomicUsize = + std::sync::atomic::AtomicUsize::new(0); +static FIELD_ATTACHED_MISPLACED: std::sync::atomic::AtomicUsize = + std::sync::atomic::AtomicUsize::new(0); + +/// One field descr's two halves, compared against each other. +/// +/// `expected` is the slot of the attached parent's `all_fielddescrs` that this +/// field's own offset occupies; `actual` is the `index_in_parent` +/// (`descr.py:228`) the producer stamped on it. Upstream cannot disagree — +/// `heaptracker.py:60-72` and `:96-112` are one walker — so every disagreement +/// is a descr declaring itself to be at a slot its parent fills with a +/// different field. +/// +/// Caller supplies `expected`, because only it holds the parent the producer +/// actually attached. Once the descr reaches `get_field_descr` the parent is +/// whatever `_cache_size` holds for the key, which is a different question. +pub fn census_attached_index(expected: usize, actual: usize) { + use std::sync::atomic::Ordering::Relaxed; + FIELD_ATTACHED_CHECKED.fetch_add(1, Relaxed); + if expected != actual { + FIELD_ATTACHED_MISPLACED.fetch_add(1, Relaxed); + } +} + +/// `field_specs[i].index_in_parent == i`, counted on the list as handed in. +/// +/// This is [`GcCache::positional_invariant_census`]'s predicate moved from the +/// published list to the submitted one, and the move is the whole point. +/// `get_field_descr` re-derives every index against the parent before the list +/// is frozen, so a census taken afterwards reads the reader's answer; the +/// producer's own number survives nowhere else. Taken here it needs no parent, +/// no name and no cache — the caller supplies both halves of the claim. +/// +/// Both factories feed this, so it also covers the `cache_key == 0` fresh-mint +/// groups that never enter `_cache_size` and are therefore outside +/// `positional_invariant_census`'s domain entirely. +fn census_spec_positions(field_specs: &[SimpleFieldDescrSpec]) { + use std::sync::atomic::Ordering::Relaxed; + FIELD_SPEC_CHECKED.fetch_add(field_specs.len(), Relaxed); + let misplaced = field_specs + .iter() + .enumerate() + .filter(|(i, spec)| spec.index_in_parent != *i) + .count(); + if misplaced > 0 { + FIELD_SPEC_MISPLACED.fetch_add(misplaced, Relaxed); + } +} + /// descr.py:14-23 GcCache. /// /// Per-type descriptor caches keyed by LLType (structural equality). @@ -934,6 +991,53 @@ impl GcCache { ] } + /// `[checked, misplaced]` over every field spec handed to a group factory. + /// + /// Read this next to [`field_position_census`]'s `rederived`, which cannot + /// answer the same question: `derive_index_in_parent` is both the judge and + /// the repairman. It counts only the disagreements it *reached* — a parent + /// already in `_cache_size`, a non-empty list, a name that resolves — and it + /// overwrites the caller's number in the same expression, so a nonzero + /// `rederived` is a repair log, not a defect report, and a zero one is + /// silence about a population it mostly never examined + /// (`parent_absent` is the count of mints where the question was skipped + /// outright, and it dwarfs `rederived` by three orders of magnitude, because + /// `make_simple_descr_group_keyed_with_headerless` mints every field BEFORE + /// `register_keyed_size` publishes the parent they will be indexed against). + /// + /// `misplaced` here is the same defect measured where nothing can have + /// repaired it yet. + /// + /// [`field_position_census`]: Self::field_position_census + pub fn spec_position_census() -> [usize; 2] { + use std::sync::atomic::Ordering::Relaxed; + [ + FIELD_SPEC_CHECKED.load(Relaxed), + FIELD_SPEC_MISPLACED.load(Relaxed), + ] + } + + /// `[checked, misplaced]` over field descrs compared against the parent + /// their own producer attached — see [`census_attached_index`]. + /// + /// This is the surface [`spec_position_census`] does NOT cover. That one + /// reads a parent's positional list, which every producer builds by + /// enumerating the list it just sorted, so it is self-consistent by + /// construction and reads zero even while a standalone field descr pointing + /// INTO that list carries a stale rank. The assembler mints exactly such a + /// descr: `add_struct_field_descr` resolves the rank against the layout as + /// it stands at the emit site, and `register_struct_layout` re-indexes the + /// layout on every later merge. + /// + /// [`spec_position_census`]: Self::spec_position_census + pub fn attached_position_census() -> [usize; 2] { + use std::sync::atomic::Ordering::Relaxed; + [ + FIELD_ATTACHED_CHECKED.load(Relaxed), + FIELD_ATTACHED_MISPLACED.load(Relaxed), + ] + } + /// How many published parents still list no fields, and how many of those /// are shadowing a layout this same cache already knows. /// @@ -4868,6 +4972,9 @@ pub fn make_simple_descr_group_keyed_with_headerless( extra_gc_fielddescrs: &[Arc], ) -> SimpleDescrGroup { let struct_key = LLType::struct_key(cache_key); + // Before `get_field_descr` below can normalise anything: the producer's own + // `index_in_parent` against the position it hands the field in. + census_spec_positions(field_specs); let mut gc = gc_cache().lock().unwrap(); // descr.py:218-239 — cache-or-mint each FieldDescr by // `(STRUCT, fieldname)` before freezing this producer's positional list. @@ -4935,6 +5042,7 @@ fn make_simple_descr_group_inner( headerless: bool, field_specs: &[SimpleFieldDescrSpec], ) -> SimpleDescrGroup { + census_spec_positions(field_specs); let field_descrs_cell = std::cell::RefCell::new(Vec::>::new()); let field_specs = field_specs.to_vec(); let size_descr = Arc::new_cyclic(|weak_size: &Weak| { 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 8516c000575..73dba90d0dd 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 @@ -777,6 +777,7 @@ impl<'c> Lowerer<'c> { #base_reg, ::core::mem::offset_of!(#struct_path, #member), #tid, + stringify!(#member), ); }, ); @@ -811,6 +812,7 @@ impl<'c> Lowerer<'c> { #base_reg, ::core::mem::offset_of!(#struct_path, #member), #tid, + stringify!(#member), ); }, ); @@ -880,6 +882,7 @@ impl<'c> Lowerer<'c> { #base_reg, ::core::mem::offset_of!(#struct_path, #member), #tid, + stringify!(#member), ); }, ); @@ -913,6 +916,7 @@ impl<'c> Lowerer<'c> { #base_reg, ::core::mem::offset_of!(#struct_path, #member), #tid, + stringify!(#member), ); }, ); @@ -1075,6 +1079,7 @@ impl<'c> Lowerer<'c> { #src, ::core::mem::offset_of!(#struct_path, #member), #tid, + stringify!(#member), ); }, ); @@ -1107,6 +1112,7 @@ impl<'c> Lowerer<'c> { #src, ::core::mem::offset_of!(#struct_path, #member), #tid, + stringify!(#member), ); }, ); @@ -1175,6 +1181,7 @@ impl<'c> Lowerer<'c> { #src, ::core::mem::offset_of!(#struct_path, #member), #tid, + stringify!(#member), ); }, ); @@ -1206,6 +1213,7 @@ impl<'c> Lowerer<'c> { #src, ::core::mem::offset_of!(#struct_path, #member), #tid, + stringify!(#member), ); }, ); diff --git a/majit/majit-macros/src/jit_interp/jitcode_lower/lower_value.rs b/majit/majit-macros/src/jit_interp/jitcode_lower/lower_value.rs index c1e14eb99c8..4c2ca6bea22 100644 --- a/majit/majit-macros/src/jit_interp/jitcode_lower/lower_value.rs +++ b/majit/majit-macros/src/jit_interp/jitcode_lower/lower_value.rs @@ -588,6 +588,7 @@ impl<'c> Lowerer<'c> { #value_reg, ::core::mem::offset_of!(#struct_path, #member), #type_id, + stringify!(#member), ); }, ), @@ -599,6 +600,7 @@ impl<'c> Lowerer<'c> { #value_reg, ::core::mem::offset_of!(#struct_path, #member), #type_id, + stringify!(#member), ); }, ), diff --git a/majit/majit-metainterp/src/jitcode/assembler.rs b/majit/majit-metainterp/src/jitcode/assembler.rs index 7680b3afc2d..8f017ce239c 100644 --- a/majit/majit-metainterp/src/jitcode/assembler.rs +++ b/majit/majit-metainterp/src/jitcode/assembler.rs @@ -45,6 +45,37 @@ pub(crate) fn scalar_size(ty: majit_ir::value::Type) -> usize { } } +/// Which slot of `fields` a field descr occupies, keyed the way +/// `heaptracker.py:60-72 get_fielddescr_index_in(STRUCT, fieldname)` keys it. +/// +/// Upstream has only the name, because upstream always has one. Every emit site +/// here carries one too — `jitcode_lower` passes the same `stringify!(#member)` +/// it hands `register_struct_layout`, and `newlist_clear` passes the `"length"` +/// / `"items"` it registered — so the name arm is the normal path. The byte +/// offset stands in for the residue: a name the layout *this* site saw does not +/// list, and the empty name a caller outside those paths would supply. +/// +/// The offset arm is deliberately narrow. Offset is NOT an identity: measured on +/// this tree, 7 of 1714 submitted field specs sit at an offset another field of +/// the same parent also occupies (a flattened inline aggregate and its first +/// leaf share an address, `heaptracker.py:68-69`). Resolving against an +/// ambiguous offset would silently name a sibling, so this returns `None` and +/// leaves the caller's number alone — the same refusal `descr.rs +/// find_index_in_parent` documents for the runtime lookup, for the same reason. +fn field_slot_in(fields: &[BhFieldSpec], name: &str, offset: usize) -> Option { + if !name.is_empty() + && let Some(idx) = fields.iter().position(|fd| fd.name == name) + { + return Some(idx); + } + let mut at_offset = fields + .iter() + .enumerate() + .filter(|(_, fd)| fd.offset == offset); + let (idx, _) = at_offset.next()?; + at_offset.next().is_none().then_some(idx) +} + #[derive(Default)] pub struct JitCodeBuilder { /// RPython `jitcode.py:15` `self.name = name`. Propagated to the @@ -727,6 +758,7 @@ impl JitCodeBuilder { offset: usize, field_type: majit_ir::value::Type, type_id: u64, + field_name: &str, ) -> u16 { let parent = self.struct_size_specs.get(&type_id).cloned(); let Some(parent_spec) = parent.as_ref() else { @@ -742,12 +774,25 @@ impl JitCodeBuilder { // (offset-ordered) layout `new_struct` registered, not the caller's // store order. descr.py:227 name = '%s.%s' % (STRUCT._name, // fieldname): take it from the same layout so the repr matches. - let (index_in_parent, name) = parent_spec - .all_fielddescrs - .iter() - .position(|fd| fd.offset == offset) - .map(|idx| (idx, parent_spec.all_fielddescrs[idx].name.clone())) - .unwrap_or((0, String::new())); + // + // `field_name` is the `fieldname` argument itself, carried from the + // emit site — `jitcode_lower` already hands `register_struct_layout` + // the same `stringify!(#member)` in the same expansion, so read and + // write agree by construction. It is what makes this a real + // `get_fielddescr_index_in(STRUCT, fieldname)` rather than an + // offset lookup wearing its name. + // + // The offset remains the fallback for a caller that has no name (`""`), + // and it is not an identity: a flattened layout puts an inline + // aggregate and its first leaf at one address (`heaptracker.py:68-69`). + // Where the offset names two fields, take the unresolved fallback + // rather than the first of them. A guessed NAME is the damaging half: + // it becomes the descr's `_cache_field` key downstream, so naming the + // aggregate hands the leaf's access the aggregate's descr. + let (index_in_parent, name) = + field_slot_in(&parent_spec.all_fielddescrs, field_name, offset) + .map(|idx| (idx, parent_spec.all_fielddescrs[idx].name.clone())) + .unwrap_or((0, String::new())); self.add_bh_descr(CanonicalBhDescr::Field { offset, field_size: scalar_size(field_type), @@ -767,10 +812,18 @@ impl JitCodeBuilder { /// store the int in `value_reg` into `struct_reg`'s field at `offset`. /// `type_id` + `offset` identify the field against the layout /// `new_struct` registered, so the optimizer can virtualize the store. - pub fn setfield_gc_i(&mut self, struct_reg: u16, value_reg: u16, offset: usize, type_id: u64) { + pub fn setfield_gc_i( + &mut self, + struct_reg: u16, + value_reg: u16, + offset: usize, + type_id: u64, + field_name: &str, + ) { self.touch_ref_reg(struct_reg); self.touch_reg(value_reg); - let descr = self.add_struct_field_descr(offset, majit_ir::value::Type::Int, type_id); + let descr = + self.add_struct_field_descr(offset, majit_ir::value::Type::Int, type_id, field_name); self.write_insn("setfield_gc_i/rid"); self.push_reg_u8(struct_reg, "setfield_gc_i struct"); self.push_reg_u8(value_reg, "setfield_gc_i value"); @@ -781,10 +834,18 @@ impl JitCodeBuilder { /// store the ref in `value_reg` into `struct_reg`'s field at `offset`. /// `type_id` + `offset` identify the field against the layout /// `new_struct` registered, so the optimizer can virtualize the store. - pub fn setfield_gc_r(&mut self, struct_reg: u16, value_reg: u16, offset: usize, type_id: u64) { + pub fn setfield_gc_r( + &mut self, + struct_reg: u16, + value_reg: u16, + offset: usize, + type_id: u64, + field_name: &str, + ) { self.touch_ref_reg(struct_reg); self.touch_ref_reg(value_reg); - let descr = self.add_struct_field_descr(offset, majit_ir::value::Type::Ref, type_id); + let descr = + self.add_struct_field_descr(offset, majit_ir::value::Type::Ref, type_id, field_name); self.write_insn("setfield_gc_r/rrd"); self.push_reg_u8(struct_reg, "setfield_gc_r struct"); self.push_reg_u8(value_reg, "setfield_gc_r value"); @@ -798,9 +859,17 @@ impl JitCodeBuilder { /// /// Encoding: `[BC_SETFIELD_GC_I_C][struct_reg u8][value i8] /// [descr_idx lo u8][descr_idx hi u8]`. - pub fn setfield_gc_i_c(&mut self, struct_reg: u16, value: i8, offset: usize, type_id: u64) { + pub fn setfield_gc_i_c( + &mut self, + struct_reg: u16, + value: i8, + offset: usize, + type_id: u64, + field_name: &str, + ) { self.touch_ref_reg(struct_reg); - let descr = self.add_struct_field_descr(offset, majit_ir::value::Type::Int, type_id); + let descr = + self.add_struct_field_descr(offset, majit_ir::value::Type::Int, type_id, field_name); self.write_insn("setfield_gc_i/rcd"); self.push_reg_u8(struct_reg, "setfield_gc_i_c struct"); self.push_u8(value as u8); @@ -817,10 +886,18 @@ impl JitCodeBuilder { /// A `type_id` whose layout was never registered degrades to a parentless /// scalar descr (`add_struct_field_descr` returns the scalar form), which /// keeps existing callers correct. - pub fn getfield_gc_i(&mut self, dest: u16, struct_reg: u16, offset: usize, type_id: u64) { + pub fn getfield_gc_i( + &mut self, + dest: u16, + struct_reg: u16, + offset: usize, + type_id: u64, + field_name: &str, + ) { self.touch_ref_reg(struct_reg); self.touch_reg(dest); - let descr = self.add_struct_field_descr(offset, majit_ir::value::Type::Int, type_id); + let descr = + self.add_struct_field_descr(offset, majit_ir::value::Type::Int, type_id, field_name); self.write_insn("getfield_gc_i/rd>i"); self.push_reg_u8(struct_reg, "getfield_gc_i struct"); self.push_u16(descr); @@ -831,10 +908,18 @@ impl JitCodeBuilder { /// load `struct_reg`'s ref field at `offset` into `dest`. /// /// See [`Self::getfield_gc_i`] for the `type_id` parent-descr contract. - pub fn getfield_gc_r(&mut self, dest: u16, struct_reg: u16, offset: usize, type_id: u64) { + pub fn getfield_gc_r( + &mut self, + dest: u16, + struct_reg: u16, + offset: usize, + type_id: u64, + field_name: &str, + ) { self.touch_ref_reg(struct_reg); self.touch_ref_reg(dest); - let descr = self.add_struct_field_descr(offset, majit_ir::value::Type::Ref, type_id); + let descr = + self.add_struct_field_descr(offset, majit_ir::value::Type::Ref, type_id, field_name); self.write_insn("getfield_gc_r/rd>r"); self.push_reg_u8(struct_reg, "getfield_gc_r struct"); self.push_u16(descr); @@ -1759,10 +1844,18 @@ impl JitCodeBuilder { is_gc_managed: true, }); // lengthdescr / itemsdescr: parent-carrying field descrs. - let length_descr = - self.add_struct_field_descr(length_offset, majit_ir::value::Type::Int, struct_type_id); - let items_descr = - self.add_struct_field_descr(items_offset, majit_ir::value::Type::Ref, struct_type_id); + let length_descr = self.add_struct_field_descr( + length_offset, + majit_ir::value::Type::Int, + struct_type_id, + "length", + ); + let items_descr = self.add_struct_field_descr( + items_offset, + majit_ir::value::Type::Ref, + struct_type_id, + "items", + ); // arraydescr: length-prefixed items block (length word at offset 0). let is_item_signed = !matches!( item_type, @@ -4952,6 +5045,11 @@ impl JitCodeBuilder { self.patch_switch_descrs(); self.patch_const_refs(); self.patch_field_descr_parents(); + if cfg!(debug_assertions) { + if let Some(disagreement) = self.field_descr_position_disagreement() { + panic!("{disagreement}"); + } + } self.patch_const_u8_refs(); // RPython `jitcode.py:47 self._resulttypes = resulttypes`. // Upstream `assembler.py:217-219` records the result-kind @@ -5521,16 +5619,132 @@ impl JitCodeBuilder { /// This is the pyre analogue of PyPy always calling `get_size_descr` /// at descr-creation time (which returns the single canonical /// SizeDescr with all fields populated by `heaptracker.all_fielddescrs`). + /// + /// `index_in_parent` and `name` are re-resolved against that same final + /// spec, because `add_struct_field_descr` derived both from the snapshot + /// this pass is replacing. Swapping only the parent leaves the two halves + /// of one descr disagreeing: the field is declared to sit at slot `i` of a + /// list whose slot `i` is a different offset. + /// + /// The key is the FIELD NAME, `heaptracker.py:60-72 + /// get_fielddescr_index_in(STRUCT, fieldname)`, whenever the mint recorded + /// one. Offset is not an identity: measured on this tree, 7 of 1714 + /// submitted field specs sit at an offset another field of the same parent + /// also occupies, so `position(|fd| fd.offset == offset)` can name a + /// sibling. It is used only for the descrs whose recorded name the final + /// spec does not list — every emit site carries the declared member name + /// into `add_struct_field_descr`, so what reaches the offset arm is a field + /// the layout never enumerated, not a field that arrived anonymous — and + /// then only when the offset is UNAMBIGUOUS. Where it is not, this leaves + /// the descr alone: `descr.rs find_index_in_parent` refuses to invent a + /// position for exactly this reason, and a pass that guesses is worse than + /// one that declines. + /// + /// A field still absent from the final spec keeps what the mint left it — + /// `add_struct_field_descr`'s `unwrap_or((0, String::new()))`. Those are + /// the inline aggregates the flattened layout only covers through their + /// leaves (`ob_header`, `int_items`, an enum's `__pos_0`), which + /// `heaptracker.py:68-69` mints no descr for at all. fn patch_field_descr_parents(&mut self) { for entry in &mut self.descrs { - if let RuntimeBhDescr::Descr(CanonicalBhDescr::Field { parent, .. }) = entry { - if let Some(p) = parent { - if let Some(final_spec) = self.struct_size_specs.get(&p.type_id) { - *p = final_spec.clone(); + let RuntimeBhDescr::Descr(CanonicalBhDescr::Field { + offset, + index_in_parent, + parent, + name, + .. + }) = entry + else { + continue; + }; + let Some(p) = parent else { continue }; + let Some(final_spec) = self.struct_size_specs.get(&p.type_id) else { + continue; + }; + *p = final_spec.clone(); + let Some(idx) = field_slot_in(&p.all_fielddescrs, name, *offset) else { + continue; + }; + *index_in_parent = idx; + name.clone_from(&p.all_fielddescrs[idx].name); + } + } + + /// The postcondition of [`patch_field_descr_parents`], checked rather than + /// assumed: every emitted `Field` descr agrees with the parent attached to + /// it about which slot the field occupies. + /// + /// This exists because the corpus cannot be the detector. The defect the + /// pass fixes is *input-order dependent* — it needs a field minted before a + /// lower-offset sibling registers — so a program that happens to register in + /// offset order exercises the producer without exercising the bug. Measured: + /// with the two re-resolution lines removed, `field_pos_rederived`, + /// `field_pos_spec_misplaced`, `field_pos_attached_misplaced` and + /// `positional_misplaced` all still read 0 over the whole corpus. A census + /// counts what ran; only a construction-site check covers what the producer + /// can emit. + /// + /// Three shapes, all of them real states this pass can leave behind: + /// + /// * the field resolves by [`field_slot_in`] — index and name must be that + /// slot's. This is the re-resolution above, so it guards future edits + /// rather than today's tree. + /// * it does not resolve — the descr must still carry + /// `add_struct_field_descr`'s `(0, String::new())` fallback untouched. + /// Those are the inline aggregates (`ob_header`, `int_items`, an enum's + /// `__pos_0`) the flattened layout represents only through their leaves. + /// * the parent's `type_id` is absent from `struct_size_specs` — the + /// `continue` above, which leaves the mint-time snapshot in place. Nothing + /// patched it, so nothing has established the invariant for it either. + /// + /// The claim is stated over [`field_slot_in`], not over a bare + /// `position(offset)`: the tree has parents with two fields at one offset, + /// so an offset-keyed claim would panic on a descr that names its field + /// correctly and merely shares an address with a sibling. + /// + /// Returns the first disagreement as a message, so the caller's panic says + /// which descr and both numbers. Gated on `cfg!(debug_assertions)` at the + /// call site rather than wrapped in `debug_assert!`, because the message + /// needs the same walk the predicate does and `debug_assert!` would run it + /// twice. + /// + /// [`patch_field_descr_parents`]: Self::patch_field_descr_parents + fn field_descr_position_disagreement(&self) -> Option { + for entry in &self.descrs { + let RuntimeBhDescr::Descr(CanonicalBhDescr::Field { + offset, + index_in_parent, + parent: Some(p), + name, + .. + }) = entry + else { + continue; + }; + match field_slot_in(&p.all_fielddescrs, name, *offset) { + Some(idx) => { + let slot = &p.all_fielddescrs[idx]; + if *index_in_parent != idx || *name != slot.name { + return Some(format!( + "field descr at offset {offset} of type_id {:#x} claims slot \ + {index_in_parent} named {name:?}, but that offset is slot {idx} \ + named {:?}", + p.type_id, slot.name, + )); } } + None if *index_in_parent != 0 || !name.is_empty() => { + return Some(format!( + "field descr at offset {offset} does not resolve in type_id {:#x}'s \ + layout yet carries slot {index_in_parent} named {name:?} instead of \ + the unresolved fallback", + p.type_id, + )); + } + None => {} } } + None } /// RPython `assembler.py:131-138` resolves a const-source operand @@ -5833,6 +6047,165 @@ mod tests { ); } + /// `add_struct_field_descr` resolves `index_in_parent` / `name` against + /// the layout accumulated SO FAR, and `register_struct_layout` re-indexes + /// every entry on each merge, so a field minted before a lower-offset + /// sibling is registered carries a rank the merge then invalidates. + /// + /// `patch_field_descr_parents` installs the final merged spec as the + /// parent, so a stale rank left beside it declares the field at a slot the + /// parent fills with a different offset — `descr.py:228`'s index and + /// `descr.py:238`'s parent_descr describing two different fields. + /// + /// Emission order here is high-offset first, which is what makes the + /// prefix rank (0) differ from the final rank (1). + #[test] + fn field_descr_index_follows_the_final_layout_not_the_mint_time_prefix() { + const TID: u64 = 0x5747_5F49_4458; + let mut builder = JitCodeBuilder::new(); + // Site 1 registers only the HIGH offset, so the mint ranks it 0. + builder.register_struct_layout(24, TID, false, false, &[(16, false, "hi")]); + builder.getfield_gc_i(0, 1, 16, TID, "hi"); + // Site 2 registers the LOW offset; the merge re-indexes to {8→0, 16→1}. + builder.register_struct_layout(24, TID, false, false, &[(8, false, "lo")]); + builder.getfield_gc_i(2, 1, 8, TID, "lo"); + let jitcode = builder.finish(); + + let fields: Vec<_> = jitcode + .exec + .descrs + .iter() + .filter_map(|entry| match entry { + RuntimeBhDescr::Descr(CanonicalBhDescr::Field { + offset, + index_in_parent, + parent, + name, + .. + }) => Some((*offset, *index_in_parent, name.clone(), parent.clone())), + _ => None, + }) + .collect(); + assert_eq!(fields.len(), 2, "one Field descr per getfield site"); + for (offset, index_in_parent, name, parent) in fields { + let parent = parent.expect("a registered type_id gives the field a parent"); + assert_eq!( + parent.all_fielddescrs.len(), + 2, + "the parent is the final merged spec, not the mint-time prefix", + ); + let (expected_index, expected_name) = match offset { + 8 => (0, "lo"), + 16 => (1, "hi"), + other => panic!("unexpected field offset {other}"), + }; + assert_eq!( + index_in_parent, expected_index, + "field at offset {offset} must rank by the final layout", + ); + assert_eq!(name, expected_name, "and name from that same layout"); + assert_eq!( + parent.all_fielddescrs[index_in_parent].offset, offset, + "`all_fielddescrs[index_in_parent]` must be this very field \ + (`heaptracker.py:60-72` / `:96-112` share one walker upstream)", + ); + } + } + + /// Two fields at one offset — a flattened inline aggregate and its first + /// leaf — must not be arbitrated by offset. Neither the re-resolution nor + /// the debug postcondition may name one of them. + /// + /// Measured on the real corpus: 7 of 1714 submitted field specs share an + /// offset with a sibling, so this is a reachable state and not a + /// hypothetical. An offset-keyed pass would rewrite `index_in_parent` to + /// whichever of the two happens to sort first, and the postcondition would + /// then panic on any descr correctly naming the other. + /// + /// The named access resolves through it; the unnamed one declines. Both + /// halves matter: `heaptracker.get_fielddescr_index_in(STRUCT, fieldname)` + /// keys on the name, and the offset is only what is left when a caller has + /// none. + #[test] + fn an_ambiguous_offset_is_arbitrated_by_name_and_otherwise_declined() { + const TID: u64 = 0x414D_4249_4755; + let field_of = |name: &str| { + let mut builder = JitCodeBuilder::new(); + // One `register_struct_layout` call, so the merge's offset dedup + // does not apply and both fields survive at offset 8. + builder.register_struct_layout( + 24, + TID, + false, + false, + &[(8, false, "agg"), (8, true, "leaf")], + ); + builder.getfield_gc_i(0, 1, 8, TID, name); + // `finish` runs the postcondition under `cfg!(debug_assertions)`; a + // resolution that guessed would trip it before this returns. + let jitcode = builder.finish(); + let (index_in_parent, name, parent) = jitcode + .exec + .descrs + .iter() + .find_map(|entry| match entry { + RuntimeBhDescr::Descr(CanonicalBhDescr::Field { + index_in_parent, + parent, + name, + .. + }) => Some((*index_in_parent, name.clone(), parent.clone())), + _ => None, + }) + .expect("one Field descr for the getfield site"); + let parent = parent.expect("a registered type_id gives the field a parent"); + assert_eq!(parent.all_fielddescrs.len(), 2, "both fields are listed"); + assert_eq!( + parent.all_fielddescrs[0].offset, parent.all_fielddescrs[1].offset, + "the fixture's point is that one offset names two fields", + ); + (index_in_parent, name) + }; + + let (idx, name) = field_of("leaf"); + assert_eq!( + (idx, name.as_str()), + (1, "leaf"), + "the emit site's `fieldname` names the field the shared offset cannot", + ); + let (idx, name) = field_of(""); + assert_eq!( + (idx, name.as_str()), + (0, ""), + "without a name the ambiguous offset resolves to nothing, so the \ + mint's `unwrap_or((0, String::new()))` fallback stands", + ); + } + + /// …but a name always wins, even when the offset it sits at is ambiguous. + /// That is `heaptracker.py:60-72 get_fielddescr_index_in(STRUCT, fieldname)` + /// — the offset is only a stand-in for the mint sites that carry no name. + #[test] + fn a_named_field_resolves_by_name_through_an_ambiguous_offset() { + const TID: u64 = 0x4E41_4D45_4B59; + let fields = [(0, false, "head"), (8, false, "agg"), (8, true, "leaf")]; + assert_eq!( + super::field_slot_in(&JitCodeBuilder::field_specs_from_layout(&fields), "leaf", 8,), + Some(2), + "the name names the field; the shared offset does not", + ); + assert_eq!( + super::field_slot_in(&JitCodeBuilder::field_specs_from_layout(&fields), "", 8), + None, + "without a name the shared offset arbitrates nothing", + ); + assert_eq!( + super::field_slot_in(&JitCodeBuilder::field_specs_from_layout(&fields), "", 0), + Some(0), + "an unambiguous offset still resolves", + ); + } + #[test] fn typed_vable_helpers_record_resulttypes_at_end_pc() { // RPython assembler.py:217-219 records `argcodes[-1]` at diff --git a/majit/majit-metainterp/src/pyjitpl/dispatch.rs b/majit/majit-metainterp/src/pyjitpl/dispatch.rs index d5cc577cf77..a33dc71ae69 100644 --- a/majit/majit-metainterp/src/pyjitpl/dispatch.rs +++ b/majit/majit-metainterp/src/pyjitpl/dispatch.rs @@ -142,6 +142,44 @@ pub fn field_descr_ref_from_bh(descr: &crate::blackhole::BhDescr) -> (usize, maj } => { if let Some(p) = parent { if !p.all_fielddescrs.is_empty() { + // The descr's own two halves, before any of the branches + // below hand it to a reader that would reconcile them: + // `index_in_parent` (`descr.py:228`) must name the slot of + // the attached parent's list that this field's offset + // occupies. `get_field_descr`'s `derive_index_in_parent` + // cannot answer this — it runs only on the mint path, only + // once `_cache_size` already holds the parent, and it + // overwrites the number it disagrees with. + // + // Keyed by field name, `heaptracker.py:60-72 + // get_fielddescr_index_in(STRUCT, fieldname)`; the byte + // offset stands in only for the mint sites that carry no + // name, and only when exactly one field sits there. A + // flattened layout puts an inline aggregate and its first + // leaf at one address, so an offset-keyed census would + // report a correctly-named descr as misplaced and put a + // false reading behind `JITSTATS_BADNESS_FIELDS`. + // + // A miss is the inline-aggregate floor (`ob_header`, + // `int_items`, an enum's `__pos_0`) carrying the documented + // `(0, "")` fallback the branches below already describe, + // not this defect, so it is not counted either way. + let expected = p + .all_fielddescrs + .iter() + .position(|f| !name.is_empty() && f.name == *name) + .or_else(|| { + let mut at = p + .all_fielddescrs + .iter() + .enumerate() + .filter(|(_, f)| f.offset == *offset); + let (idx, _) = at.next()?; + at.next().is_none().then_some(idx) + }); + if let Some(expected) = expected { + majit_ir::descr::census_attached_index(expected, *index_in_parent); + } let mut specs: Vec<_> = p.all_fielddescrs.iter().map(field_spec_from_bh).collect(); if p.type_id == 0 { @@ -10765,10 +10803,10 @@ mod tests { &[(0, false, "value"), (8, true, "next")], ); // ref reg 0 = Node* builder.load_const_i_value(0, 99); // int reg 0 = 99 - builder.setfield_gc_i(0, 0, 0, 0xCD); // Node.value = 99 - builder.setfield_gc_r(0, 0, 8, 0xCD); // Node.next = Node (self-ref) - builder.getfield_gc_i(1, 0, 0, 0xCD); // int reg 1 = Node.value - builder.getfield_gc_r(1, 0, 8, 0xCD); // ref reg 1 = Node.next + builder.setfield_gc_i(0, 0, 0, 0xCD, "value"); // Node.value = 99 + builder.setfield_gc_r(0, 0, 8, 0xCD, "next"); // Node.next = Node (self-ref) + builder.getfield_gc_i(1, 0, 0, 0xCD, "value"); // int reg 1 = Node.value + builder.getfield_gc_r(1, 0, 8, 0xCD, "next"); // ref reg 1 = Node.next let jitcode = builder.finish(); let mut ctx = TraceCtx::for_test(0); @@ -10836,7 +10874,7 @@ mod tests { false, &[(0, false, "value"), (8, true, "next")], ); - builder.setfield_gc_i_c(0, -7, 0, 0xCE); // Node.value = -7 (inline const) + builder.setfield_gc_i_c(0, -7, 0, 0xCE, "value"); // Node.value = -7 (inline const) let jitcode = builder.finish(); let mut ctx = TraceCtx::for_test(0); diff --git a/majit/majit-metainterp/src/trace_ctx.rs b/majit/majit-metainterp/src/trace_ctx.rs index b8d51d8f6de..bdd26c80f8f 100644 --- a/majit/majit-metainterp/src/trace_ctx.rs +++ b/majit/majit-metainterp/src/trace_ctx.rs @@ -844,6 +844,22 @@ impl TraceCtx { /// RPython executes the allocation before recording the matching trace op, /// so later residual calls and field operations observe a real pointer /// while the optimizer remains free to virtualize the recorded allocation. + /// + /// Rooting contract: the result is returned unrooted, and the caller must + /// stamp it onto the op it records for this allocation + /// (`set_opref_concrete`) before performing any GC allocation. That stamp + /// is what makes the object a root — `MetaInterp::walk_active_trace_refs` + /// forwards every recorder `Op`/`InputArg` `value` cell holding a + /// `Value::Ref`, which is the `history.py:803-807` `*FrontendOp(pos, + /// value)` slot upstream reaches through the object graph. Between the + /// `bh_new` here and that stamp there is no root at all, so the caller's + /// window must contain no GC allocation; recording the op and populating + /// the heapcache allocate from the Rust heap only, which is why the + /// existing call sites are sound. + /// + /// A side list of executed allocations is NOT the way to widen that + /// window: it duplicates a root the op graph already owns, and it hands + /// the collector shapes the op graph never exposes it to. pub fn execute_new_allocation(&self, descr: &DescrRef, with_vtable: bool) -> Option { let cpu = unsafe { &*self.cpu? }; let bh_descr = descr_to_bh_size_descr(descr)?; diff --git a/majit/majit-translate/src/codewriter/assembler.rs b/majit/majit-translate/src/codewriter/assembler.rs index d8fe7409161..941bc092919 100644 --- a/majit/majit-translate/src/codewriter/assembler.rs +++ b/majit/majit-translate/src/codewriter/assembler.rs @@ -3725,6 +3725,23 @@ fn heuristic_struct_size_for_bh(cc: &CallControl, owner: &str) -> Option Some((offset + align - 1) & !(align - 1)) } +/// The one slot of `fields` at `offset`, or `None` when no field or more than +/// one sits there. +/// +/// A flattened layout puts an inline aggregate and its first leaf at the same +/// address (`heaptracker.py:68-69` recurses into a nested `lltype.Struct` +/// without minting a descr for the container), so an offset can name two +/// fields. Where it does, there is no answer to give and inventing one names a +/// sibling — `descr.rs find_index_in_parent` refuses the same way. +fn unique_slot_at_offset(fields: &[crate::jitcode::BhFieldSpec], offset: usize) -> Option { + let mut at_offset = fields + .iter() + .enumerate() + .filter(|(_, f)| f.offset == offset); + let (idx, _) = at_offset.next()?; + at_offset.next().is_none().then_some(idx) +} + fn fielddescrof( field: &crate::model::FieldDescriptor, ty: &crate::model::ValueType, @@ -3830,8 +3847,54 @@ fn fielddescrof( is_immutable = rank.is_immutable(); is_quasi_immutable = rank.is_quasi_immutable(); } + // Both fallbacks above take `offset` from a second source — the + // `struct_layout_for` registry or `heuristic_field_layout` — and neither + // touches `index_in_parent`, which is still its `0` initialiser while + // `parent` carries the whole flattened list. Slot 0 of that list is + // some other field, and `all_fielddescrs()[index_in_parent]` is what + // `optimizeopt/info.rs force_box` indexes by, so the descr would name + // one field and address another. Resolve the slot against the parent + // that is actually attached, the way `descr.py:228 + // heaptracker.get_fielddescr_index_in` derives it from the STRUCT + // itself. + // + // Only the byte offset is available here — reaching this arm means the + // name lookup above already missed — and offset is not an identity: a + // flattened inline aggregate shares an address with its first leaf + // (`heaptracker.py:68-69`). So resolve only when exactly one field sits + // there, and otherwise leave the caller's number rather than name a + // sibling. The `found_parent_field` arm is untouched for the converse + // reason: it picked its spec BY NAME, which is the better key. + if !found_parent_field + && let Some(parent_spec) = parent.as_ref() + && let Some(pos) = unique_slot_at_offset(&parent_spec.all_fielddescrs, offset) + { + index_in_parent = pos; + } } + // The descr's two halves, counted where they are produced rather than at a + // reader that would repair them (`GcCache::derive_index_in_parent` + // re-derives by name and overwrites, so it can only report repairs it + // already applied — see `field_position_jit_stats`). + // + // Still meaningful after the re-resolution above, which only covers the + // fallback arms: the `found_parent_field` arm takes `index_in_parent` from + // the matched spec's STORED number, and nothing here guarantees that number + // equals the spec's position in the list it was stored in. + // + // Name first, and an ambiguous offset is not counted at all — a descr that + // names its field correctly while sharing an address with a sibling is not + // misplaced, and counting it would put a false reading behind a gate. + if let Some(parent_spec) = parent.as_ref() + && let Some(pos) = parent_spec + .all_fielddescrs + .iter() + .position(|spec| spec.field_key() == field_key) + .or_else(|| unique_slot_at_offset(&parent_spec.all_fielddescrs, offset)) + { + majit_ir::descr::census_attached_index(pos, index_in_parent); + } crate::jitcode::BhDescr::Field { offset, field_size, @@ -4962,6 +5025,83 @@ mod tests { ); } + #[test] + fn fielddescrof_resolves_the_slot_when_the_offset_comes_from_the_layout_registry() { + use crate::call::{CallControl, StructFieldLayout, StructLayout}; + use crate::model::FieldDescriptor; + + let owner = "assembler_fielddescrof_layout_registry_test::Owner"; + let owner_id = majit_ir::descr::StructId::from_canonical(owner); + majit_ir::descr::register_struct_ids(HashMap::from([(owner.to_string(), Some(owner_id))])); + + let mut cc = CallControl::new(); + let mut struct_fields = crate::front::StructFieldRegistry::default(); + struct_fields.fields.insert( + owner.to_string(), + vec![ + ("visible_zero".to_string(), "i64".to_string()), + ("visible_eight".to_string(), "i64".to_string()), + ], + ); + cc.set_struct_fields(struct_fields); + cc.set_struct_layout( + owner_id, + StructLayout { + size: 16, + fields: vec![ + StructFieldLayout { + name: "visible_zero".to_string(), + offset: 0, + size: 8, + flag: majit_ir::descr::ArrayFlag::Signed, + field_type: majit_ir::value::Type::Int, + rank: None, + }, + StructFieldLayout { + name: "visible_eight".to_string(), + offset: 8, + size: 8, + flag: majit_ir::descr::ArrayFlag::Signed, + field_type: majit_ir::value::Type::Int, + rank: None, + }, + // `bh_size_spec_from_callcontrol` omits void fields from + // its flattened list, leaving this layout-only name to + // exercise fielddescrof's registry fallback. + StructFieldLayout { + name: "hidden".to_string(), + offset: 8, + size: 0, + flag: majit_ir::descr::ArrayFlag::Void, + field_type: majit_ir::value::Type::Void, + rank: None, + }, + ], + }, + ); + + let crate::jitcode::BhDescr::Field { + offset, + index_in_parent, + parent, + .. + } = fielddescrof( + &FieldDescriptor::new("hidden", Some(owner.to_string())), + &crate::model::ValueType::Int, + Some(&cc), + ) + else { + panic!("fielddescrof must produce a field descriptor"); + }; + + assert_eq!(offset, 8); + assert_eq!(index_in_parent, 1); + assert_eq!( + parent.as_ref().unwrap().all_fielddescrs[index_in_parent].offset, + offset + ); + } + #[test] fn explicit_result_variant_size_inherits_discriminant_slot() { use crate::call::CallControl; diff --git a/pyre/check.py b/pyre/check.py index cd74e3374f1..90f90ac594e 100644 --- a/pyre/check.py +++ b/pyre/check.py @@ -703,6 +703,18 @@ def _parse_jit_stats(snapshot): # fourth one fail CI instead of surfacing as an intermittent asyncio failure. # Like `loops_aborted` it is not absolute: the baseline pins the instances that # exist today and a rise means a new one. +# +# `field_pos_spec_misplaced` and `field_pos_attached_misplaced` are the same kind +# of assertion one layer down: a field descr's `index_in_parent` (`descr.py:228`) +# must name the slot its own parent puts the field in, which upstream gets for +# free because `heaptracker.py:60-72` and `:96-112` are one walker. Both are +# counted where the producer's number is still readable — before +# `get_field_descr` re-derives it — because the counter that looks like it +# already gates this, `field_pos_rederived`, cannot: `derive_index_in_parent` +# overwrites the number it disagrees with, and skips the check entirely whenever +# the parent is not yet published, which is most of them. So a printed +# `field_pos_rederived=0` never meant the producers agreed, and gating it would +# have gated nothing. JITSTATS_BADNESS_FIELDS = ( "loops_aborted", "internal_compile_panics", @@ -710,6 +722,8 @@ def _parse_jit_stats(snapshot): "descr_set_ambiguous", "descr_set_stale_absent", "fbw_rolled_back_with_effects", + "field_pos_spec_misplaced", + "field_pos_attached_misplaced", ) # The three count-valued counters, and what a move in either direction means: diff --git a/pyre/pyre-jit-trace/src/descr.rs b/pyre/pyre-jit-trace/src/descr.rs index 36cdd770daf..9a9064916e7 100644 --- a/pyre/pyre-jit-trace/src/descr.rs +++ b/pyre/pyre-jit-trace/src/descr.rs @@ -5167,6 +5167,17 @@ pub fn make_descr_from_bh(bh: &majit_translate::jitcode::BhDescr) -> DescrRef { spec.offset == *offset && spec.field_key() == field_key }) { + // `pos` is the slot this reader will index; the descr's + // own `index_in_parent` (`descr.py:228`) is what every + // later consumer indexes by + // (`optimizeopt/info.rs force_box`). They are two + // producers' answers to one question and nothing else + // compares them: the cache-hit return above and + // `get_field_descr`'s own `derive_index_in_parent` both + // resolve by NAME, so a stale rank is silently replaced + // rather than reported. Count it where it is still + // visible. + majit_ir::descr::census_attached_index(pos, *index_in_parent); if let Some(descr) = group.field_descrs.get(pos) { return descr.clone() as DescrRef; } diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs index 775b1b03cc9..1e207dc4e6a 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs @@ -176,6 +176,18 @@ thread_local! { /// via [`fbw_finish_concrete_root_walker`]. `None` for ungated / /// loop-closing / float (no concrete float shadow bank) walks → the /// portal degrades to the legacy `ContinueRunningNormally` replay. + /// + /// This root exists because of the *outliving*, not because a walk-time + /// concrete is otherwise unrooted. Every value the walk computes is + /// stamped onto its frontend op (`set_opref_concrete` → + /// `history.py:803-807` `*FrontendOp(pos, value)`), and + /// `MetaInterp::walk_active_trace_refs` forwards every recorder + /// `Op`/`InputArg` `value` cell holding a `Value::Ref` — so for the + /// duration of the walk the recorder IS the root set. The compile path + /// then does `self.tracing.take()`, dropping that recorder while this + /// stash is still live, which is exactly the window no op-graph slot + /// covers. A walk-scoped side list of the same values would be a second + /// source of truth for a window the op graph already owns. static FBW_FINISH_CONCRETE: std::cell::Cell> = const { std::cell::Cell::new(None) }; diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs index 6cf18db2594..42a7b6a677c 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs @@ -9876,6 +9876,11 @@ fn handle( "new/d>r" => { let descr = read_descr(code, op, 0, ctx)?; let concrete = ctx.trace_ctx.execute_new_allocation(&descr, false); + // The `set_opref_concrete` below is what roots this object: it + // stamps the allocation onto the recorded op's `value` cell, which + // `MetaInterp::walk_active_trace_refs` forwards. Nothing between + // here and there allocates from the GC heap, so no collection can + // observe the object before it is reachable from that root. // pyjitpl.py:624-629 `execute_new`. ctx.trace_ctx .profiler() @@ -9911,6 +9916,7 @@ fn handle( "new_with_vtable/d>r" => { let descr = read_descr(code, op, 0, ctx)?; let concrete = ctx.trace_ctx.execute_new_allocation(&descr, true); + // Rooted by the `set_opref_concrete` stamp below, as in `new/d>r`. if let Some(Value::Ref(majit_ir::GcRef(ptr))) = concrete && let Some(w_class) = descr.as_size_descr().and_then(|size| size.w_class_obj()) { diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs index 29934068465..f16d8f2d460 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs @@ -1147,7 +1147,14 @@ fn int_ovf_jump_declines_when_an_operand_is_not_concrete() { /// Drive one of the `d>r` struct-allocation handlers (`new`, /// `new_with_vtable`): both record the descr alone and write the /// allocation into the ref bank. -fn drive_alloc_with_descr(opname: &str, expected_opcode: OpCode) { +/// +/// Returns the concrete stamped onto the recorded allocation op, which is +/// `None` without a cpu (no `bh_new` to execute). +fn drive_alloc_with_descr( + opname: &str, + expected_opcode: OpCode, + cpu: Option<&dyn majit_backend::Backend>, +) -> Option { let nwv_byte = *insns_opname_to_byte() .get(opname) .unwrap_or_else(|| panic!("`{opname}` must be in the runtime instruction table")); @@ -1159,6 +1166,7 @@ fn drive_alloc_with_descr(opname: &str, expected_opcode: OpCode) { let _ = test_outer_resume_jitcode_index(); let descr_pool = vec![crate::descr::w_int_size_descr()]; let mut tc = TraceCtx::for_test_types(&[]); + tc.set_cpu(cpu); let mut regs_r = vec![OpRef::NONE]; let mut concrete_r = vec![ConcreteValue::Null]; let session = std::cell::RefCell::new(WalkSession::default()); @@ -1217,16 +1225,118 @@ fn drive_alloc_with_descr(opname: &str, expected_opcode: OpCode) { "the `>r` decorator writes the allocation into the ref bank" ); assert_eq!(next_pc, 4, "`d>r` consumes a 2B descr plus a 1B dst"); + tc.lookup_opref_concrete(dst) } #[test] fn new_with_vtable_records_the_alloc_and_writes_the_ref_dst() { - drive_alloc_with_descr("new_with_vtable/d>r", OpCode::NewWithVtable); + drive_alloc_with_descr("new_with_vtable/d>r", OpCode::NewWithVtable, None); } #[test] fn new_records_the_alloc_and_writes_the_ref_dst() { - drive_alloc_with_descr("new/d>r", OpCode::New); + drive_alloc_with_descr("new/d>r", OpCode::New, None); +} + +/// Backend stub for the allocation-rooting tests: `bh_new*` hands back one +/// caller-owned block so the handler observes a real, dereferenceable +/// pointer (`new_with_vtable` writes `w_class` into it). +struct AllocTestCpu { + block: i64, +} +impl majit_backend::Backend for AllocTestCpu { + fn bh_new(&self, _sizedescr: &majit_translate::jitcode::BhDescr) -> i64 { + self.block + } + fn bh_new_with_vtable(&self, _sizedescr: &majit_translate::jitcode::BhDescr) -> i64 { + self.block + } + fn compile_loop( + &mut self, + _inputargs: &[majit_ir::InputArg], + _ops: &[majit_ir::OpRc], + _token: &majit_backend::JitCellToken, + ) -> Result { + unimplemented!("AllocTestCpu::compile_loop") + } + fn compile_bridge( + &mut self, + _fail_descr: &dyn majit_ir::FailDescr, + _inputargs: &[majit_ir::InputArg], + _ops: &[majit_ir::OpRc], + _original_token: &majit_backend::JitCellToken, + _previous_tokens: &[std::sync::Arc], + _caller_recovery_layout: Option<&majit_backend::ExitRecoveryLayout>, + ) -> Result { + unimplemented!("AllocTestCpu::compile_bridge") + } + fn execute_token( + &self, + _token: &majit_backend::JitCellToken, + _args: &[majit_ir::Value], + ) -> majit_backend::DeadFrame { + unimplemented!("AllocTestCpu::execute_token") + } + fn get_latest_descr<'a>( + &'a self, + _frame: &'a majit_backend::DeadFrame, + ) -> &'a dyn majit_ir::FailDescr { + unimplemented!("AllocTestCpu::get_latest_descr") + } + fn get_latest_descr_arc( + &self, + _frame: &majit_backend::DeadFrame, + ) -> std::sync::Arc { + unimplemented!("AllocTestCpu::get_latest_descr_arc") + } + fn get_int_value(&self, _frame: &majit_backend::DeadFrame, _index: usize) -> i64 { + unimplemented!("AllocTestCpu::get_int_value") + } + fn get_float_value(&self, _frame: &majit_backend::DeadFrame, _index: usize) -> f64 { + unimplemented!("AllocTestCpu::get_float_value") + } + fn get_ref_value(&self, _frame: &majit_backend::DeadFrame, _index: usize) -> majit_ir::GcRef { + unimplemented!("AllocTestCpu::get_ref_value") + } + fn invalidate_loop(&self, _token: &majit_backend::JitCellToken) { + unimplemented!("AllocTestCpu::invalidate_loop") + } +} + +/// The object `execute_new[_with_vtable]` allocates is rooted by being +/// stamped onto the op recorded for it: `MetaInterp::walk_active_trace_refs` +/// forwards every recorder `Op` `value` cell holding a `Value::Ref` +/// (`history.py:803-807` `*FrontendOp(pos, value)`), and that stamp is the +/// only thing putting a walk-time allocation in the root set — the walker's +/// `concrete_registers_r` shadow is a borrowed slice, not a root. +/// +/// Losing the stamp would leave the allocation reachable only from that +/// shadow, so pin it here rather than relying on a side list of executed +/// allocations, which would duplicate a root the op graph already owns. +fn alloc_result_is_stamped_onto_its_recorded_op(opname: &str, expected_opcode: OpCode) { + // Real backing storage: `new_with_vtable` writes `w_class` into the + // returned block, so a synthetic address would be a wild store. + let block: Box<[usize; 32]> = Box::new([0; 32]); + let cpu = AllocTestCpu { + block: block.as_ref().as_ptr() as i64, + }; + let stamped = drive_alloc_with_descr(opname, expected_opcode, Some(&cpu)); + assert_eq!( + stamped, + Some(Value::Ref(majit_ir::GcRef(cpu.block as usize))), + "`{opname}` must stamp the allocation onto its recorded op — that cell \ + is the GC root the walk relies on", + ); +} + +#[test] +fn new_stamps_its_allocation_onto_the_recorded_op() { + alloc_result_is_stamped_onto_its_recorded_op("new/d>r", OpCode::New); +} + +#[test] +fn new_with_vtable_stamps_its_allocation_onto_the_recorded_op() { + alloc_result_is_stamped_onto_its_recorded_op("new_with_vtable/d>r", OpCode::NewWithVtable); } /// Step one record-only opcode (the heapcache hints, the raw-memory pair) diff --git a/pyre/pyre-jit-trace/src/jitcode_runtime.rs b/pyre/pyre-jit-trace/src/jitcode_runtime.rs index 3d5f7b3d2ca..8c33732ddc1 100644 --- a/pyre/pyre-jit-trace/src/jitcode_runtime.rs +++ b/pyre/pyre-jit-trace/src/jitcode_runtime.rs @@ -718,11 +718,36 @@ pub fn descr_set_jit_stats() -> String { /// one skip set, so `all_fielddescrs(S)[i].get_index() == i` holds by /// construction and there is nothing to count. /// -/// `rederived` is the one to watch: it counts fields whose caller-supplied index -/// disagreed with the parent that will actually be indexed at -/// `optimizeopt/info.rs force_box`. Every one of those was, before this was -/// derived rather than transported, either an out-of-range panic or a store -/// emitted against a DIFFERENT field. +/// `attached_misplaced` and `spec_misplaced` are the two to watch. Both count a +/// producer-supplied `index_in_parent` that disagrees with where the field +/// actually sits, read before anything downstream can normalise it, and between +/// them they cover the two shapes a producer can emit: `spec_misplaced` a +/// parent's own positional list, `attached_misplaced` a standalone field descr +/// pointing into one. The second is not implied by the first — a producer builds +/// a list by enumerating what it just sorted, so the list is self-consistent by +/// construction while a descr minted against an earlier state of it is not. +/// +/// `rederived` looks like it asks that question and does not. `derive_index_in_parent` +/// is the judge and the repairman: it replaces the caller's number with the +/// parent's in the same expression, so a nonzero reading is a log of repairs +/// already applied, never a defect still present. Worse, it counts only the +/// mints it reached — `parent_absent` is the ones where it never asked, and that +/// is the *majority* of them, because +/// `make_simple_descr_group_keyed_with_headerless` mints every field before +/// `register_keyed_size` publishes the parent those fields will be indexed +/// against. `field_pos_rederived=0` therefore states nothing about the +/// producers; it has to be read as a fraction of `parent_absent`, and a +/// producer defect can sit at zero forever. `spec_misplaced` is the same defect +/// measured where no reader has had the chance to repair it. +/// +/// The defect being measured: `all_fielddescrs()[index_in_parent]` is a +/// load-bearing lookup (`optimizeopt/info.rs force_box`), so an index naming a +/// different slot than the field occupies either runs off the end or emits the +/// store against a DIFFERENT field. +/// +/// `positional_misplaced` is the output-side companion — the same predicate on +/// the published list, after `get_field_descr` has reconciled it. The gap +/// between the two is exactly how much the reader is absorbing. /// /// `size_shell_*` is the producer-side companion. `parent_empty` counts mint /// attempts that found a fieldless parent, so one shell hit by many fields reads @@ -733,6 +758,9 @@ pub fn descr_set_jit_stats() -> String { pub fn field_position_jit_stats() -> String { let [parent_absent, parent_empty, rederived, unresolved] = majit_ir::descr::GcCache::field_position_census(); + let [spec_checked, spec_misplaced] = majit_ir::descr::GcCache::spec_position_census(); + let [attached_checked, attached_misplaced] = + majit_ir::descr::GcCache::attached_position_census(); let ( [published, fieldless, shadowing, aliased, aliased_multi], [slots, misplaced], @@ -757,6 +785,9 @@ pub fn field_position_jit_stats() -> String { format!( "field_pos_parent_absent={parent_absent} field_pos_parent_empty={parent_empty} \ field_pos_rederived={rederived} field_pos_unresolved={unresolved} \ + field_pos_spec_checked={spec_checked} field_pos_spec_misplaced={spec_misplaced} \ + field_pos_attached_checked={attached_checked} \ + field_pos_attached_misplaced={attached_misplaced} \ size_shell_published={published} size_shell_fieldless={fieldless} \ size_shell_shadowing={shadowing} size_shell_aliased={aliased} \ size_shell_aliased_multi={aliased_multi} \ @@ -791,6 +822,41 @@ pub struct DescrSetCounts { pub stale_absent: u64, } +/// The two producer-side field-position invariants as numbers, for the same +/// reason [`descr_set_counts`] exists: the wasm guest has no stderr, so it +/// exports them individually (`pyre_jit_field_pos_*` in `pyre-wasm`) and the +/// runner prints the line. +/// +/// This matters more on wasm than the name suggests. The invariant is stated in +/// terms of BYTE OFFSETS — `index_in_parent` must name the slot the field's +/// offset occupies — and wasm32's word is 4 bytes (`symbolic.py:12 WORD = +/// sizeof(lltype.Signed)`), so every struct is laid out differently there. A +/// producer that ranks correctly on a 64-bit host is not thereby correct on +/// wasm, and without these exports a wasm-only rise reads as absent-and- +/// therefore-zero, i.e. healthy. +pub fn field_position_counts() -> FieldPositionCounts { + let [spec_checked, spec_misplaced] = majit_ir::descr::GcCache::spec_position_census(); + let [attached_checked, attached_misplaced] = + majit_ir::descr::GcCache::attached_position_census(); + FieldPositionCounts { + spec_checked: spec_checked as u64, + spec_misplaced: spec_misplaced as u64, + attached_checked: attached_checked as u64, + attached_misplaced: attached_misplaced as u64, + } +} + +/// `*_checked` are the denominators — reported so a run that checked nothing +/// cannot read the same as one that checked everything, but host-dependent and +/// therefore not in `JITSTATS_SNAPSHOT_FIELDS`. The two `*_misplaced` are +/// `JITSTATS_BADNESS_FIELDS` members and healthy only at zero. +pub struct FieldPositionCounts { + pub spec_checked: u64, + pub spec_misplaced: u64, + pub attached_checked: u64, + pub attached_misplaced: u64, +} + /// Name every member whose container has been registered since its raw set was /// frozen, under `PYRE_DESCR_SPELLING_GATE=1`. /// diff --git a/pyre/pyre-jit/src/lib.rs b/pyre/pyre-jit/src/lib.rs index 78a9873a999..888a679272d 100644 --- a/pyre/pyre-jit/src/lib.rs +++ b/pyre/pyre-jit/src/lib.rs @@ -56,7 +56,7 @@ mod trace_verify; // Re-export auto-generated trace functions from pyre-jit-trace pub use pyre_jit_trace::jitcode_runtime::{ descr_set_counts, descr_set_jit_stats, descr_spelling_gate_recheck_now, - field_descr_identity_census_now, field_position_jit_stats, + field_descr_identity_census_now, field_position_counts, field_position_jit_stats, }; pub use pyre_jit_trace::{ trace_box_float, trace_box_int, trace_float_binop, trace_float_compare, trace_int_binop, diff --git a/pyre/pyre-wasm-runner/src/main.rs b/pyre/pyre-wasm-runner/src/main.rs index d86b8cedd5f..3d749012882 100644 --- a/pyre/pyre-wasm-runner/src/main.rs +++ b/pyre/pyre-wasm-runner/src/main.rs @@ -837,6 +837,12 @@ fn run(module_path: &PathBuf, source: &str, script: &Path) -> Result { let descr_set_absent = counter("pyre_jit_descr_set_absent", &mut missing); let descr_set_ambiguous = counter("pyre_jit_descr_set_ambiguous", &mut missing); let descr_set_stale_absent = counter("pyre_jit_descr_set_stale_absent", &mut missing); + let field_pos_spec_checked = counter("pyre_jit_field_pos_spec_checked", &mut missing); + let field_pos_spec_misplaced = counter("pyre_jit_field_pos_spec_misplaced", &mut missing); + let field_pos_attached_checked = + counter("pyre_jit_field_pos_attached_checked", &mut missing); + let field_pos_attached_misplaced = + counter("pyre_jit_field_pos_attached_misplaced", &mut missing); // Walks that ended uncommitted after a residual had already run an // irreversible effect. Reached through the slot-indexed `pyre_fbw_diag` // export rather than a counter of its own; slot 1 is @@ -872,7 +878,11 @@ fn run(module_path: &PathBuf, source: &str, script: &Path) -> Result { descr_set_absent={descr_set_absent} \ descr_set_ambiguous={descr_set_ambiguous} \ descr_set_stale_absent={descr_set_stale_absent} \ - fbw_rolled_back_with_effects={fbw_rolled_back_with_effects}" + fbw_rolled_back_with_effects={fbw_rolled_back_with_effects} \ + field_pos_spec_checked={field_pos_spec_checked} \ + field_pos_spec_misplaced={field_pos_spec_misplaced} \ + field_pos_attached_checked={field_pos_attached_checked} \ + field_pos_attached_misplaced={field_pos_attached_misplaced}" ); } let packed = match run_result { diff --git a/pyre/pyre-wasm/src/lib.rs b/pyre/pyre-wasm/src/lib.rs index 682c0fdaefe..810421f137e 100644 --- a/pyre/pyre-wasm/src/lib.rs +++ b/pyre/pyre-wasm/src/lib.rs @@ -482,6 +482,42 @@ pub extern "C" fn pyre_jit_descr_set_resolved() -> u64 { pyre_jit::descr_set_counts().resolved } +/// The producer-side field-position invariants, the other two +/// `JITSTATS_BADNESS_FIELDS` members, exported for the same reason as the +/// `descr_set_*` block above. +/// +/// Not redundant with the native backends' reading of them: the invariant is +/// that `index_in_parent` names the slot the field's BYTE OFFSET occupies, and +/// wasm32 lays every struct out on a 4-byte word (`symbolic.py:12`). A +/// producer's ranking is a function of the target's offsets, so a 64-bit host +/// reading zero says nothing about wasm. +#[cfg(all(target_arch = "wasm32", feature = "wasm-host"))] +#[unsafe(no_mangle)] +pub extern "C" fn pyre_jit_field_pos_spec_misplaced() -> u64 { + pyre_jit::field_position_counts().spec_misplaced +} + +#[cfg(all(target_arch = "wasm32", feature = "wasm-host"))] +#[unsafe(no_mangle)] +pub extern "C" fn pyre_jit_field_pos_attached_misplaced() -> u64 { + pyre_jit::field_position_counts().attached_misplaced +} + +/// The denominators for the two above, on the same footing as +/// `pyre_jit_descr_set_resolved`: reported so a run that checked nothing cannot +/// read the same as one that checked everything, host-dependent, not gated. +#[cfg(all(target_arch = "wasm32", feature = "wasm-host"))] +#[unsafe(no_mangle)] +pub extern "C" fn pyre_jit_field_pos_spec_checked() -> u64 { + pyre_jit::field_position_counts().spec_checked +} + +#[cfg(all(target_arch = "wasm32", feature = "wasm-host"))] +#[unsafe(no_mangle)] +pub extern "C" fn pyre_jit_field_pos_attached_checked() -> u64 { + pyre_jit::field_position_counts().attached_checked +} + #[cfg(any(feature = "web", feature = "wasm-host"))] static PANIC_HOOK: Once = Once::new();