Descend pos_inner for +x, and key a virtual's class word off the layout

majit/majit-ir/src/ptr_info.rs

@@ -152,18 +152,32 @@ pub struct VStringConcatInfo {
 ///
 /// Fields are tracked as OpRefs to the operations that produce their values.
 ///
-/// ## Invariant: `fields` NEVER contains typeptr (offset 0)
+/// ## Invariant: an entry's key is a slot of `all_fielddescrs()`
 ///
 /// Matches RPython upstream: `heaptracker.py all_fielddescrs()` skips
 /// `typeptr`, so `info.py AbstractStructPtrInfo.init_fields` sizes
-/// `_fields` with typeptr excluded from the indexable range. The typeptr
-/// (offset 0) is tracked separately via `known_class` and emitted by the
-/// GC rewriter's `gen_initialize_vtable` path (rewrite.py:479-484), NOT
-/// from the force-path field loop.
+/// `_fields` with it excluded from the indexable range. The typeptr (offset 0)
+/// is therefore never a key here; it is tracked via `known_class` and emitted
+/// by the GC rewriter's `gen_initialize_vtable` path (rewrite.py), NOT from
+/// the force-path field loop.
+///
+/// Pyre's additional Python-level `w_class` header is not the same case: a
+/// layout that lists it holds it at an ordinary slot (`W_BaseException` at 1),
+/// and an entry keyed by that slot resolves like any other. What must never
+/// appear is a key that names no slot — `index_in_parent` off a gc-only header
+/// edge (always 0, which is some value field), or one of
+/// `heap::OptHeap::field_slot_index`'s header/unslotted bands, which are keys
+/// for an association lookup and not positions in any list. `force_box` reads
+/// this list back through `all_fielddescrs()` BY POSITION, so such a key
+/// resolves to the wrong field or to nothing at all.
 ///
 /// Enforced by:
-/// - `virtualize.rs optimize_setfield_gc` Virtual arm: runtime check that
-///   returns early on `offset == Some(0)` before calling `set_field`.
+/// - `virtualize.rs optimize_setfield_gc` Virtual arm: returns early on
+///   typeptr, and keys a class-word store off the layout's
+///   `class_word_index_in_parent` rather than the op's descr.
+/// - `optimizeopt/mod.rs structinfo_setfield`: declines a header word on a
+///   virtual, whose key there comes from `heap::OptHeap::field_slot_index`'s
+///   band and names no slot.
 /// - `virtualize.rs force_virtual_instance`: `debug_assert_no_typeptr`
 ///   at the entry of the field-emit loop.
 /// - `virtualstate.rs export_single_value`:
@@ -182,7 +196,8 @@ pub struct VirtualInfo {
     /// SetfieldGc(ob_type) without polluting `fields` (which feeds rd_virtuals).
     pub ob_type_descr: Option<DescrRef>,
     /// Field values: `(field_descr_index, value_opref)`.
-    /// **Invariant**: never contains typeptr (offset 0) — see struct-level docs.
+    /// **Invariant**: every key is a slot of the descr's `all_fielddescrs()`;
+    /// typeptr is never one — see struct-level docs.
     pub fields: Vec<(u32, Operand)>,
     /// info.py:91-92
     pub last_guard_pos: i32,

majit/majit-metainterp/src/optimizeopt/info.rs

@@ -68,11 +68,10 @@ pub(crate) fn resolve_gc_tid(
 /// `descr.get_all_fielddescrs()` (info.py:217-225), where `typeptr` is absent,
 /// so upstream never emits this header store from the force path. This is the
 /// same optimizer-layer "value already there" elision as heap.py:88-101.
-fn w_class_store_is_covered_by_alloc(
+pub(crate) fn w_class_value_is_covered_by_alloc(
     size_descr: &DescrRef,
     field_descr: &DescrRef,
-    value: &Operand,
-    ctx: &crate::optimizeopt::OptContext,
+    value: Option<Value>,
 ) -> bool {
     let Some(field) = field_descr.as_field_descr().filter(|fd| fd.is_w_class()) else {
         return false;
@@ -97,9 +96,22 @@ fn w_class_store_is_covered_by_alloc(
         return false;
     }
     matches!(
+        value,
+        Some(Value::Ref(value)) if value == GcRef(w_class as usize)
+    )
+}
+
+fn w_class_store_is_covered_by_alloc(
+    size_descr: &DescrRef,
+    field_descr: &DescrRef,
+    value: &Operand,
+    ctx: &crate::optimizeopt::OptContext,
+) -> bool {
+    w_class_value_is_covered_by_alloc(
+        size_descr,
+        field_descr,
         ctx.resolve_operand_operand_opt(value)
             .and_then(|resolved| resolved.const_value()),
-        Some(Value::Ref(value)) if value == GcRef(w_class as usize)
     )
 }
 
@@ -1315,9 +1327,23 @@ fn force_box_impl(
             for (field_idx, value_ref) in std::mem::take(&mut vinfo.fields) {
                 let value_ref = force_child(&value_ref, ctx);
                 let descr = lookup_field_descr(&cached_fielddescrs, field_idx);
-                let descr = descr.expect(
-                    "force_box: field_idx must resolve through descr.get_all_fielddescrs()[i]",
-                );
+                // The key that failed to resolve is the whole diagnosis, and
+                // the list it was looked up in says which numbering it came
+                // from, so name both rather than only the rule they broke.
+                let descr = descr.unwrap_or_else(|| {
+                    panic!(
+                        "force_box: field_idx must resolve through descr.get_all_fielddescrs()[i] \
+                         (field_idx={field_idx} len={} keys={:?})",
+                        cached_fielddescrs.len(),
+                        cached_fielddescrs
+                            .iter()
+                            .map(|d| d
+                                .as_field_descr()
+                                .map(|f| (f.field_key().to_string(), f.index_in_parent()))
+                                .unwrap_or_else(|| ("?".to_string(), 0)))
+                            .collect::<Vec<_>>(),
+                    )
+                });
                 if w_class_store_is_covered_by_alloc(&vinfo.descr, &descr, &value_ref, ctx) {
                     continue;
                 }

majit/majit-metainterp/src/optimizeopt/mod.rs

@@ -8395,11 +8395,28 @@ impl OptContext {
             }
             return;
         }
+        // A virtual's field list is addressed by position, and the key this
+        // site carries is not one.  [`heap::OptHeap::field_slot_index`] answers
+        // `typeptr` and `w_class` out of a band of their own, above every
+        // position a parent's field list can hand out, precisely because they
+        // resolve through no field list at all.  That is a workable key for the
+        // association lists an `Instance`/`Struct` carries, but `force_box`
+        // walks a virtual's `fields` back through `descr.get_all_fielddescrs()`
+        // BY POSITION, so a banded slot there resolves to nothing.  A virtual
+        // that owns a listed class word records it from
+        // `virtualize.rs optimize_setfield_gc`, which reads the slot off the
+        // layout; nothing is lost by declining here.
+        let is_header_word = op
+            .with_field_descr(|fd| fd.is_typeptr() || fd.is_w_class())
+            .unwrap_or(false);
         // info.py AbstractStructPtrInfo.setfield: mutate `_fields`
         // in the PtrInfo object stored in the operand's `_forwarded` slot.
         // PyPy has the same single-object behavior via `box._forwarded`.
         self.with_ensured_ptr_info_arg0(op, |mut handle| {
             if let Some(mut pi) = handle.as_mut() {
+                if is_header_word && pi.is_virtual() {
+                    return;
+                }
                 pi.setfield(field_idx, value.clone());
             }
         });