Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 101 additions & 0 deletions pyre/pyre-jit-trace/src/descr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3628,6 +3628,70 @@ mod tests {
}
}

/// A `PyObject.w_class` `BhDescr` describing the same access as the
/// canonical header descr, for both owner spellings the codewriter emits.
fn w_class_bh(owner: &str, field_size: usize) -> majit_translate::jitcode::BhDescr {
use majit_ir::descr::ArrayFlag;
use majit_translate::jitcode::BhDescr;

BhDescr::Field {
offset: pyre_object::pyobject::W_CLASS_OFFSET,
field_size,
field_type: Type::Ref,
field_flag: ArrayFlag::Signed,
is_field_signed: false,
is_immutable: false,
is_quasi_immutable: false,
// slot 0 is `ob_type`; `w_class` is slot 1 of the header.
index_in_parent: 1,
parent: None,
name: "w_class".into(),
owner: owner.into(),
}
}

/// The shared `PyObject` header's `w_class` bridges to the same descr the
/// walker pins a value's class through, so a codewriter-lowered subclass
/// test (`is_plain_int1`) reads the header the walker already guarded
/// instead of emitting a second, uncacheable read of the same offset.
#[test]
fn make_descr_from_bh_bridges_pyobject_w_class_to_the_walker_descr() {
let canonical = w_class_descr();
let width = W_CLASS_FIELD_DESCR.field_size();

for owner in ["PyObject", "pyre_object::pyobject::PyObject"] {
let descr = make_descr_from_bh(&w_class_bh(owner, width));
assert!(
std::sync::Arc::ptr_eq(&descr, &canonical),
"{owner}.w_class must bridge to the walker's w_class descr Arc",
);
}
}

/// …but only when the two spellings describe the same access. The canonical
/// descr hardcodes an 8-byte width while the codewriter sizes a pointer by
/// `target_word_size()`, so on a 32-bit target the incoming descr is a
/// narrower load at the same offset. Bridging there would widen the read
/// over the adjacent payload, so the mismatch declines instead.
#[test]
fn make_descr_from_bh_declines_w_class_bridge_on_a_width_mismatch() {
let canonical = w_class_descr();
let narrower = W_CLASS_FIELD_DESCR.field_size() / 2;

for owner in ["PyObject", "pyre_object::pyobject::PyObject"] {
let descr = make_descr_from_bh(&w_class_bh(owner, narrower));
assert!(
!std::sync::Arc::ptr_eq(&descr, &canonical),
"{owner}.w_class must not bridge to a descr of a different width",
);
assert_eq!(
descr.as_field_descr().map(|f| f.field_size()),
Some(narrower),
"the declined descr must keep the width the codewriter asked for",
);
}
}
Comment on lines +3631 to +3693

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover every bridge rejection guard.

The tests only reject a mismatched field_size. Add rejection cases for a mismatched offset and field_type. Both cases must return a noncanonical descriptor. This protects the access-compatibility contract at Lines 4405-4407.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pyre/pyre-jit-trace/src/descr.rs` around lines 3631 - 3693, Extend
make_descr_from_bh_declines_w_class_bridge_on_a_width_mismatch to also construct
bridge descriptors with mismatched offset and field_type, then assert each
result is not Arc-pointer-equal to the canonical w_class_descr. Preserve the
existing width-mismatch coverage and verify the returned descriptors retain
their requested access properties.


#[test]
fn make_descr_from_bh_struct_array_preserves_type_and_interior_fields() {
use majit_ir::descr::ArrayFlag;
Expand Down Expand Up @@ -4308,6 +4372,43 @@ pub fn make_descr_from_bh(bh: &majit_translate::jitcode::BhDescr) -> DescrRef {
// (one skipped `list.pop(0)` per compiled loop entry). One field is
// one descr — `metainterp_sd.all_descrs` has no second entry for a
// field just because a different interpreter reached it.
// Same split, one struct up: the shared `PyObject` header's
// `w_class`. The walker pins a value's Python-level class by
// reading offset 8 through `w_class_descr()`; a codewriter-lowered
// body testing the same header — `is_plain_int1` reading
// `value.w_class` (listobject.rs) — reaches it through the modelled
// `PyObject` parent, whose group entry is a different identity for
// the same field. The pinned constant then never reached the
// second read, so the strict subclass test stayed symbolic and
// re-emitted the load plus its null and equality tests.
//
// Like the leaves below this runs BEFORE the parent-group lookup,
// which would otherwise answer with the parent's own entry.
//
// Only when the two spellings describe the same memory access.
// `new_w_class_field_descr` hardcodes `field_size: 8` while the
// codewriter sizes a pointer field by `layout::target_word_size()`
// (`call.rs get_type_flag`), so on wasm32 the incoming descr is a
// 4-byte load and the canonical one an 8-byte load at the same
// offset. Merging them there would widen the read over four bytes
// of the adjacent payload. That size split is deliberate and
// documented at `new_w_class_field_descr`; until it is resolved the
// bridge declines rather than papering over it, leaving those
// targets exactly as they were before the bridge existed.
if name.as_str() == "w_class"
&& matches!(
owner.as_str(),
"PyObject" | "pyre_object::pyobject::PyObject"
)
{
let canonical = &*W_CLASS_FIELD_DESCR;
if canonical.offset() == *offset
&& canonical.field_size() == *field_size
&& canonical.field_type() == *field_type
{
return w_class_descr();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep w_class stores valid for virtual objects

When a codewriter-lowered body stores PyObject.w_class on a freshly allocated virtual object—for example, while retagging a builtin-subclass instance—this unconditional descriptor bridge also redirects the SetfieldGc, not just the intended read. w_class_descr() has no parent descriptor, while OptVirtualize::optimize_setfield_gc treats every field except typeptr as positional and calls get_parent_descr().expect(...); the resulting trace therefore panics during optimization. Make the canonical descriptor valid for virtual stores or avoid applying this read-oriented bridge to store operands.

AGENTS.md reference: AGENTS.md:L205-L207

Useful? React with 👍 / 👎.

}
}
if owner.as_str() == "W_ListObject" {
match name.as_str() {
"int_items.len" => return list_int_items_len_descr(),
Expand Down
Loading