jit-trace: bridge the codewriter's PyObject.w_class read to the walker descr (append loop 40 → 33 ops) - #931
Conversation
…r descr
`make_descr_from_bh` already redirects a codewriter-lowered body's field descr
to the walker's canonical one for `W_ListObject.int_items.*`,
`ItemsBlock.capacity` and the box payloads, because the heapcache and the
optimizer's heap pass key on descr identity and a field carrying two descrs
breaks aliasing between them. The shared `PyObject` header's `w_class` had no
such arm.
`orthodox_list_append_commit` pins the appended value's class by reading
offset 8 through `w_class_descr()` and guarding it to a constant. The sub-walk
then evaluates `is_plain_int1(value)` (listobject.rs), whose `value.w_class`
read comes through the modelled `PyObject` parent — a different identity for
the same field, so the pinned constant never reached it. The steady loop body
carried the read twice, the second one followed by its own null test and
equality test.
Bridge `("PyObject" | "pyre_object::pyobject::PyObject", "w_class")` to
`w_class_descr()`. Placed ahead of the parent-group lookup for the reason the
neighbouring `int_items.*` arm documents: when the codewriter does model the
parent, that lookup answers with the parent's own entry and re-creates the
split.
Append loop steady body 40 -> 33 ops. The remaining pair is the single genuine
read and guard; `guard_class` cannot replace it because a builtin subclass
shares `ob_type` and only retags `w_class`.
Assisted-by: Claude
Walkthrough
Changesw_class descriptor matching
Estimated code review effort: 3 (Moderate) | ~15 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/63875f6ed156de2e54746c99fdc5fe4fe7c3f801/pyre-jit-trace/src/descr.rs#L4360
On the wasm32 backend, the incoming codewriter descriptor uses the target layout (a 4-byte pointer and the wasm w_class offset), but this unconditional return replaces it with w_class_descr(), whose new_w_class_field_descr deliberately uses the host-derived offset and hard-coded field_size: 8. Consequently, translated PyObject.w_class operations such as is_plain_int1 can access the wrong wasm field or include adjacent payload bytes, miscompiling the generated JIT even though the interpreter uses the correct layout. Either make the canonical descriptor target-layout-correct or avoid this bridge when its offset/size differs from the BhDescr.
AGENTS.md reference: AGENTS.md:L14-L19
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit b54316a). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
…s width `new_w_class_field_descr` hardcodes `field_size: 8` while the codewriter sizes a pointer field by `layout::target_word_size()` (`call.rs:7448-7452 get_type_flag`), so on a 32-bit target the two spellings of `PyObject.w_class` describe different accesses at the same offset: a 4-byte load from the codewriter and an 8-byte load from the canonical descr. The bridge returned the canonical descr unconditionally, widening the read over four bytes of the adjacent payload. Return the canonical descr only when its offset, width, and field type all equal the incoming `BhDescr`'s. The width split is deliberate and documented at `new_w_class_field_descr`; targets where it applies keep the descr they had before the bridge existed. Adds a decline test alongside the existing bridge test, both now deriving offset and width from the canonical descr instead of hardcoding them. Raised as P1 on #931. Assisted-by: Claude
|
The P1 is correct — and the tree already documented the trap it names.
Verified both halves:
So on a 32-bit target the bridge replaced a 4-byte load with an 8-byte load at the same offset, pulling four bytes of the adjacent payload into the class pointer. Same family as the Fix taken: the second of the two you offered. Making the canonical descr target-layout-correct is the first option, and the comment above records that it was already tried and regressed let canonical = &*W_CLASS_FIELD_DESCR;
if canonical.offset() == *offset
&& canonical.field_size() == *field_size
&& canonical.field_type() == *field_type
{
return w_class_descr();
}Where the widths disagree the bridge declines and the target keeps exactly the descr it had before #931 — the documented "two universes out of step" state stays as-is rather than being silently merged. Test coverage: the existing bridge test and a new decline test now both derive offset and width from the canonical descr instead of hardcoding Verification on this commit: — commented by Claude |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@pyre/pyre-jit-trace/src/descr.rs`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 86f0a094-fd60-45ae-9574-3de8b230ef9f
📒 Files selected for processing (1)
pyre/pyre-jit-trace/src/descr.rs
| /// 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", | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b54316acf5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| && canonical.field_size() == *field_size | ||
| && canonical.field_type() == *field_type | ||
| { | ||
| return w_class_descr(); |
There was a problem hiding this comment.
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 👍 / 👎.
1 participant
Recovers most of what #922 gave up, and this time by deleting a duplicate read rather than folding past a pending write.
The duplicate
make_descr_from_bhalready redirects a codewriter-lowered body's field descr to the walker's canonical one — forW_ListObject.int_items.*,ItemsBlock.capacityand the box payloads. The reason is stated in that function: the heapcache and the optimizer's heap pass both key on descr identity, so a field carrying two descrs silently breaks aliasing between them. "One field is one descr."The shared
PyObjectheader'sw_classhad no such arm, and it is read from both sides in the same loop:orthodox_list_append_commit(specialize.rs:6446) pins the appended value's class — reads offset 8 throughw_class_descr(), guards it to a constant,replace_box.is_plain_int1(value)(pyre-object/src/listobject.rs), whosevalue.w_classread comes through the modelledPyObjectparent — a different identity for the same field.So the pinned constant never reached the second read. In the steady body that showed up as the load appearing twice, the second followed by its own null test and equality test:
The fix
Bridge
("PyObject" | "pyre_object::pyobject::PyObject", "w_class")tow_class_descr().Placed ahead of the parent-group lookup, for the reason the neighbouring
int_items.*arm documents: when the codewriter does model the parent, that lookup answers with the parent's own entry for the same offset and re-creates the split. Putting the arm in the later(owner, name)match — where the box-payload bridges live — measured no change at all, because the parented path returns first.Result
Append loop steady body 40 → 33 ops, so 7 of the 9 #922 gave up.
The remaining pair is the single genuine read and its guard.
guard_classcannot replace it: a builtin subclass instance shares the payloadob_typeand passesguard_class, retagging onlyw_class(walker_frame_ops.rs:196-200). Removing it needs the object-model split behindbuiltin subclass shares the base layout, not another descr change.Verification
check.py --backend dynasm,cranelift: dynasm 352/352, cranelift 352/352cargo test --release -p pyre-jit-trace --features dynasm: 314 passed, 0 failed, including a new test asserting both owner spellings bridge to the sameArcretag_force.pymatches thePYRE_NO_JIToracle on both backends (25000 50000 1249975000 tuple MyTuple);w_subclass2.pyandsynth/callee_store_global_read_after_callunchanged— authored by Claude
Summary by CodeRabbit
Bug Fixes
Tests