diff --git a/majit/majit-metainterp/src/optimizeopt/virtualize.rs b/majit/majit-metainterp/src/optimizeopt/virtualize.rs index b033a732082..60ff555b12c 100644 --- a/majit/majit-metainterp/src/optimizeopt/virtualize.rs +++ b/majit/majit-metainterp/src/optimizeopt/virtualize.rs @@ -768,46 +768,20 @@ impl OptVirtualize { return Some(OptimizationResult::Remove); } set_field(&mut vinfo.fields, field_idx, value_op.clone()); - debug_assert!( - (field_idx as usize) - < vinfo - .descr - .as_size_descr() - .map(|sd| sd.all_fielddescrs().len()) - .unwrap_or(0), - "Virtual field slot {} is outside its own descr's field list \ - (len {}, descr index {}); `set_field` just wrote past the \ - struct this PtrInfo describes", - field_idx, - vinfo - .descr - .as_size_descr() - .map(|sd| sd.all_fielddescrs().len()) - .unwrap_or(0), - vinfo.descr.index(), - ); + if let Some(err) = + field_slot_disagreement(&vinfo.descr, field_idx, field_descr) + { + panic!("Virtual {err}"); + } Some(OptimizationResult::Remove) } PtrInfo::VirtualStruct(vinfo) => { set_field(&mut vinfo.fields, field_idx, value_op.clone()); - debug_assert!( - (field_idx as usize) - < vinfo - .descr - .as_size_descr() - .map(|sd| sd.all_fielddescrs().len()) - .unwrap_or(0), - "VirtualStruct field slot {} is outside its own descr's field list \ - (len {}, descr index {}); `set_field` just wrote past the \ - struct this PtrInfo describes", - field_idx, - vinfo - .descr - .as_size_descr() - .map(|sd| sd.all_fielddescrs().len()) - .unwrap_or(0), - vinfo.descr.index(), - ); + if let Some(err) = + field_slot_disagreement(&vinfo.descr, field_idx, field_descr) + { + panic!("VirtualStruct {err}"); + } Some(OptimizationResult::Remove) } PtrInfo::Virtualizable(vstate) => { @@ -2374,6 +2348,79 @@ impl Optimization for OptVirtualize { // ── Field list helpers ── +/// The postcondition of a virtual `set_field`: slot `field_idx` of the struct +/// descr this PtrInfo carries is the field that supplied the index. +/// +/// `info.py:206` writes `self._fields[fielddescr.get_index()]`, and +/// `info.py:219-220 _force_elements` reads it back as +/// `for i, fielddescr in enumerate(descr.get_all_fielddescrs()): fld = +/// self._fields[i]` — two descrs, one index. Upstream cannot +/// disagree, because `heaptracker.py:60-72 all_fielddescrs` and `:97-109 +/// get_fielddescr_index_in` are one declaration-order walk. Nor can the two +/// descrs be a mismatched pair: `rclass.py:549` declares a subclass as +/// `MkStruct(name, ('super', rbase.object_type), *llfields)`, so the inherited +/// fields are walked first and a field's index is the same in a class as in +/// every subclass of it. `info.py:184-188` spends exactly that guarantee when +/// it swaps `self.descr` for "a more precise descr" and keeps the index. +/// +/// pyre reaches one such list from two producers that rank fields differently — +/// the codewriter walks declarations (`codewriter/assembler.rs +/// bh_all_field_specs_for_struct_into`), `jitcode/assembler.rs +/// register_struct_layout` sorts by byte offset — so the pairing above is a +/// postcondition to state rather than a property of the construction. Measured +/// over 120928 virtual setfields (1172 programs) it holds everywhere, including +/// the 2286 that did index a descr other than the field's own parent; the +/// reachable failure needs a reordered list AND that cross-descr step at once, +/// which nothing in the corpus produces. A census over what ran cannot promise +/// what the producers can emit, so check it here — the same argument +/// `jitcode/assembler.rs field_descr_position_disagreement` already makes for +/// the other end of this pipe. +/// +/// Returns the disagreement as a message so the caller's panic names both the +/// slot and the field. Compiled out of release builds: it is a debug assertion, +/// written as a function only because the message needs the same walk the +/// predicate does. +fn field_slot_disagreement( + descr: &DescrRef, + field_idx: u32, + field: &dyn FieldDescr, +) -> Option { + if !cfg!(debug_assertions) { + return None; + } + let fields = descr.as_size_descr()?.all_fielddescrs(); + let Some(slot) = fields.get(field_idx as usize) else { + return Some(format!( + "field slot {field_idx} is outside its own descr's field list (len {}, descr \ + index {}); `set_field` just wrote past the struct this PtrInfo describes", + fields.len(), + descr.index(), + )); + }; + // Both halves must agree. The name is the better key but is not always + // carried — the flattened inline aggregates (`ob_header`, an enum's + // `__pos_0`) reach here under the documented empty-name fallback — so the + // name is compared only when both sides have one, and the offset is + // compared always. Neither alone is sufficient: a name can be absent, and a + // flattened layout puts an aggregate and its first leaf at one address + // (`heaptracker.py:68-69`). + let named_apart = !field.field_name().is_empty() + && !slot.field_name().is_empty() + && slot.field_name() != field.field_name(); + if named_apart || slot.offset() != field.offset() { + return Some(format!( + "field {:?} at offset {} claims slot {field_idx} of descr index {}, but that \ + slot holds {:?} at offset {}", + field.field_name(), + field.offset(), + descr.index(), + slot.field_name(), + slot.offset(), + )); + } + None +} + fn set_field(fields: &mut Vec<(u32, Operand)>, field_idx: u32, value: Operand) { for entry in fields.iter_mut() { if entry.0 == field_idx { diff --git a/pyre/bench/fannkuch.wasm.jitstats b/pyre/bench/fannkuch.wasm.jitstats index 2cceb0b9d90..8d8a0b964d4 100644 --- a/pyre/bench/fannkuch.wasm.jitstats +++ b/pyre/bench/fannkuch.wasm.jitstats @@ -3,6 +3,8 @@ descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 guard_failures=5049 internal_compile_panics=0 loops_aborted=0 diff --git a/pyre/bench/fib_loop.wasm.jitstats b/pyre/bench/fib_loop.wasm.jitstats index c2c1f34ede9..ef0f65b3b1e 100644 --- a/pyre/bench/fib_loop.wasm.jitstats +++ b/pyre/bench/fib_loop.wasm.jitstats @@ -3,6 +3,8 @@ descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 guard_failures=189 internal_compile_panics=0 loops_aborted=0 diff --git a/pyre/bench/fib_recursive.wasm.jitstats b/pyre/bench/fib_recursive.wasm.jitstats index 8b8d8bd32bc..13f6bf4928d 100644 --- a/pyre/bench/fib_recursive.wasm.jitstats +++ b/pyre/bench/fib_recursive.wasm.jitstats @@ -3,6 +3,8 @@ descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 guard_failures=406 internal_compile_panics=0 loops_aborted=0 diff --git a/pyre/bench/float_loop.wasm.jitstats b/pyre/bench/float_loop.wasm.jitstats index 8beed56f050..62c98090fec 100644 --- a/pyre/bench/float_loop.wasm.jitstats +++ b/pyre/bench/float_loop.wasm.jitstats @@ -3,6 +3,8 @@ descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 guard_failures=1 internal_compile_panics=0 loops_aborted=0 diff --git a/pyre/bench/inline_helper.wasm.jitstats b/pyre/bench/inline_helper.wasm.jitstats index 1cc731febcf..a0796ff2cd2 100644 --- a/pyre/bench/inline_helper.wasm.jitstats +++ b/pyre/bench/inline_helper.wasm.jitstats @@ -3,6 +3,8 @@ descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 guard_failures=1 internal_compile_panics=0 loops_aborted=0 diff --git a/pyre/bench/int_loop.wasm.jitstats b/pyre/bench/int_loop.wasm.jitstats index 8beed56f050..62c98090fec 100644 --- a/pyre/bench/int_loop.wasm.jitstats +++ b/pyre/bench/int_loop.wasm.jitstats @@ -3,6 +3,8 @@ descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 guard_failures=1 internal_compile_panics=0 loops_aborted=0 diff --git a/pyre/bench/nbody.wasm.jitstats b/pyre/bench/nbody.wasm.jitstats index 434fcead556..8707432d056 100644 --- a/pyre/bench/nbody.wasm.jitstats +++ b/pyre/bench/nbody.wasm.jitstats @@ -3,6 +3,8 @@ descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 guard_failures=1547 internal_compile_panics=0 loops_aborted=0 diff --git a/pyre/bench/nested_loop.wasm.jitstats b/pyre/bench/nested_loop.wasm.jitstats index 114b48b9fd0..3276873edf5 100644 --- a/pyre/bench/nested_loop.wasm.jitstats +++ b/pyre/bench/nested_loop.wasm.jitstats @@ -3,6 +3,8 @@ descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 guard_failures=201 internal_compile_panics=0 loops_aborted=0 diff --git a/pyre/bench/raise_catch_loop.wasm.jitstats b/pyre/bench/raise_catch_loop.wasm.jitstats index 114b48b9fd0..3276873edf5 100644 --- a/pyre/bench/raise_catch_loop.wasm.jitstats +++ b/pyre/bench/raise_catch_loop.wasm.jitstats @@ -3,6 +3,8 @@ descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 guard_failures=201 internal_compile_panics=0 loops_aborted=0 diff --git a/pyre/bench/spectral_norm.wasm.jitstats b/pyre/bench/spectral_norm.wasm.jitstats index fc43af8db38..c55603e53a8 100644 --- a/pyre/bench/spectral_norm.wasm.jitstats +++ b/pyre/bench/spectral_norm.wasm.jitstats @@ -3,6 +3,8 @@ descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 guard_failures=520 internal_compile_panics=0 loops_aborted=0 diff --git a/pyre/bench/synth/closure_per_call.wasm.jitstats b/pyre/bench/synth/closure_per_call.wasm.jitstats index fb6a485e39d..04e0b5011b3 100644 --- a/pyre/bench/synth/closure_per_call.wasm.jitstats +++ b/pyre/bench/synth/closure_per_call.wasm.jitstats @@ -3,7 +3,9 @@ descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 -guard_failures=470 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 +guard_failures=468 internal_compile_panics=0 loops_aborted=0 loops_compiled=4 diff --git a/pyre/bench/synth/exception_traceback_frame_lineno.wasm.jitstats b/pyre/bench/synth/exception_traceback_frame_lineno.wasm.jitstats index f1d4098f127..95b7ea20405 100644 --- a/pyre/bench/synth/exception_traceback_frame_lineno.wasm.jitstats +++ b/pyre/bench/synth/exception_traceback_frame_lineno.wasm.jitstats @@ -3,7 +3,9 @@ descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 -guard_failures=820 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 +guard_failures=819 internal_compile_panics=0 loops_aborted=0 loops_compiled=17 diff --git a/pyre/bench/synth/gc_iterator_source_drop.wasm.jitstats b/pyre/bench/synth/gc_iterator_source_drop.wasm.jitstats index 51649cfb022..6e3b5767b12 100644 --- a/pyre/bench/synth/gc_iterator_source_drop.wasm.jitstats +++ b/pyre/bench/synth/gc_iterator_source_drop.wasm.jitstats @@ -3,7 +3,9 @@ descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 -guard_failures=613 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 +guard_failures=614 internal_compile_panics=0 loops_aborted=0 loops_compiled=4 diff --git a/pyre/bench/synth/recursive_call_frame_relocation.wasm.jitstats b/pyre/bench/synth/recursive_call_frame_relocation.wasm.jitstats index d98f1879e71..4bc1b8e3595 100644 --- a/pyre/bench/synth/recursive_call_frame_relocation.wasm.jitstats +++ b/pyre/bench/synth/recursive_call_frame_relocation.wasm.jitstats @@ -3,7 +3,9 @@ descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 -guard_failures=649 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 +guard_failures=648 internal_compile_panics=0 loops_aborted=0 loops_compiled=3