codewriter parity, and the virtualizable array escape: the entry framestate was never copied - #1087
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change updates virtualizable field and array lowering, frame-local access paths, graph cleanup, label formatting, nested-struct indexing, and related regression checks. It also adds runtime hints and LLBC projection validation. ChangesVirtualizable lowering and frame access
Codewriter corrections
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
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
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ab468e689
ℹ️ 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".
| // the recursion was handed our own `cur_index`, so the index it | ||
| // reports back already counts the fields walked before the nested | ||
| // struct; adding it again would count them twice | ||
| cur_index = -r - 1; |
There was a problem hiding this comment.
Although this assignment fixes the non-leading nested-struct arithmetic, it deliberately diverges from the vendored RPython implementation at rpython/jit/codewriter/heaptracker.py:108, which still uses cur_index += -r - 1. This repository requires strict line-by-line structural parity, so the upstream/vendored implementation should be corrected first and then mirrored here rather than introducing a pyre-only rewrite that future parity syncs will undo.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed as suggested — the vendored RPython side is now corrected first and the majit port mirrors it.
rpython/jit/codewriter/heaptracker.py now uses cur_index = -r - 1, with a new rpython/jit/codewriter/test/test_heaptracker.py; three of its four cases fail without it (assert 4 == 3, the same symptom the majit test shows). Vendored suites: codewriter 524 passed, backend/llsupport/test/test_descr.py 20 passed, metainterp/test/test_ajit.py 217 passed, backend/llgraph 138 passed.
For context, this is the same defect as pypy/pypy#5537, which is still open upstream — so a future parity sync will now find the two sides already in agreement rather than reverting the port.
— commented by Claude
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
youknowone
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@majit/majit-translate/src/codewriter/jtransform.rs`:
- Around line 9352-9365: Extend the assertions in the block validation near the
last_exception exitswitch checks to verify that the sole retained exit targets
continuation. Keep the existing exit-count and exc_block exclusion assertions,
and add an explicit target assertion using the retained edge’s target and the
continuation symbol.
- Around line 719-723: Restore the upstream ordering in the loop around
rewrite_operation: assign count_before_last_operation immediately after
self.rewrite_operation returns, before any RewriteResult::Replace handling
extends new_ops. Preserve the existing rewritten-operation processing while
moving only this assignment.
🪄 Autofix
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: 71b2c07a-3b1e-4fe2-be4d-6bb33d6302ee
📒 Files selected for processing (2)
majit/majit-translate/src/codewriter/heaptracker.rsmajit/majit-translate/src/codewriter/jtransform.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffc38dab9b
ℹ️ 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".
| // exits and the `last_exception` exitswitch have to go with it — | ||
| // otherwise the block keeps an exception edge for an operation that | ||
| // is not in it any more. | ||
| if graph.blocks[block_idx].canraise() && new_ops.len() == count_before_last_operation { |
There was a problem hiding this comment.
When a block has no operations but retains ExitSwitch::LastException, this condition treats the initialized zero count as evidence that a nonexistent last operation was elided, then truncates its exception exits—or panics if it has none. Upstream jtransform.py:73-75 explicitly returns before processing every empty block, so port that guard before initializing this counter; the new condition otherwise introduces behavior precisely where upstream avoids reading count_before_last_operation.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
youknowone
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 270f8e1f25
ℹ️ 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".
| // land. And a projection whose *address* was taken is not a read | ||
| // of the field at all; tracking it would drop the op and leave the | ||
| // address's consumer with an undefined operand. | ||
| let fresh_virtualizable = fresh_virtualizable || field.suppresses_virtualizable(); |
There was a problem hiding this comment.
When the generated JIT executes FrameLocalsRoot::new with the GC hook active, this suppression leaves addr_of_mut!((*frame_ptr).locals_cells_stack_w) represented as a plain FieldRead, whose result is the FixedObjectArray pointer rather than the address of the frame field. try_gc_add_root therefore registers the array body as a mutable root slot; a collection during frame setup can interpret or overwrite its header instead of forwarding frame.locals_cells_stack_w. Add a real place-address operation and pass that result through rather than merely disabling virtualizable lowering.
AGENTS.md reference: AGENTS.md:L252-L254
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@majit/majit-translate/src/codewriter/jtransform.rs`:
- Around line 7773-7833: Add three regression tests alongside
vable_array_escaping_the_block_on_a_link_arg_is_rejected, covering the fused
exitswitch operand, call argument, and setfield operand escape paths. Build
minimal graphs that cause a virtualizable array to escape through each route,
run transform_graph, and annotate each test with #[should_panic(expected =
"...")] using the exact route strings "fused exitswitch operand", "call
argument", and "setfield operand".
- Around line 7915-7981: The write-side coverage is missing for address-taken
virtualizable array fields. Add a test alongside
address_of_a_vable_array_field_is_not_tracked that constructs the same
FieldDescriptor with with_base_is_deref(true) and with_taken_by_address(true),
emits the corresponding SetField, and verifies no virtualizable rewrite occurs
while the ordinary write remains. Use the test result to update
rewrite_op_setfield’s suppression logic to the broader predicate if needed;
otherwise document the alternative reason near its current
base_is_local_aggregate() check.
In `@majit/majit-translate/src/front/mir.rs`:
- Around line 7607-7648: Add an ignored real-LLBC regression test for the
FixedObjectArray length retarget, following the existing anchors
set_ref_set_locals_w_real and vec_index_mut_fill_user_function_args_real. Target
the _check_stack_index graph, assert it contains zero residual calls matching
["object_array", "<Impl>", "len"], and assert it emits at least one
OpKind::ArrayLen.
In `@majit/majit-translate/src/model.rs`:
- Around line 485-550: Correct the aliased `Rvalue::Ref` field-access
classification so `let r = &q; (*r).f` is recognized as a non-dereferenced local
aggregate rather than relying on the `Field` projection kind. Update the
producer or `FieldDescriptor::suppresses_virtualizable()` to suppress
virtualizable lowering for this resolved shape while preserving true
pointer-based accesses. Add a regression comparing the generated behavior with
the PyPy oracle under `MAJIT_STATS=1`.
🪄 Autofix
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: 15c7a369-9c0e-4178-83c6-83e964042e66
📒 Files selected for processing (36)
.github/workflows/pyre-ci.ymlmajit/majit-metainterp/src/jit.rsmajit/majit-translate/src/codewriter/flatten.rsmajit/majit-translate/src/codewriter/format.rsmajit/majit-translate/src/codewriter/heaptracker.rsmajit/majit-translate/src/codewriter/jtransform.rsmajit/majit-translate/src/front/bool_then.rsmajit/majit-translate/src/front/from_size_align.rsmajit/majit-translate/src/front/mir.rsmajit/majit-translate/src/front/option_closure_select.rsmajit/majit-translate/src/front/option_expect.rsmajit/majit-translate/src/front/option_is_none.rsmajit/majit-translate/src/front/option_map_or.rsmajit/majit-translate/src/front/option_try.rsmajit/majit-translate/src/front/option_unwrap.rsmajit/majit-translate/src/front/option_unwrap_or.rsmajit/majit-translate/src/front/range_iter.rsmajit/majit-translate/src/front/result_exc.rsmajit/majit-translate/src/front/slice_index.rsmajit/majit-translate/src/model.rsmajit/majit-translate/src/translator/backendopt/mod.rsmajit/majit-translate/src/translator/rtyper/cutover.rsmajit/majit-translate/tests/test_vable_array_len.rspyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/eval.rspyre/pyre-interpreter/src/pyframe.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-jit-trace/src/trace_opcode.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/eval.rspyre/scripts/vable-projection-census.pyrpython/jit/codewriter/heaptracker.pyrpython/jit/codewriter/test/test_heaptracker.py
| /// `jtransform.py:126-127` + `:145-168 _check_no_vable_array` — the | ||
| /// array a virtualizable field read produced may not leave the block | ||
| /// along a link argument; the block that would consume it has no | ||
| /// `vable_array_vars` entry for it, so no lowering could ever happen. | ||
| /// | ||
| /// Drives the whole `transform_graph`, so it is evidence that the | ||
| /// transform enforces the rule and not only that the predicate is | ||
| /// correct. This is the same graph as | ||
| /// `fresh_virtualizable_suppresses_vable_array_tracking` minus the | ||
| /// hint: there the array never becomes a virtualizable array and the | ||
| /// link is fine, here it does and the link is not. | ||
| /// | ||
| /// The expectation names the escape route, not just the upstream | ||
| /// header: all four call sites share one panic body, so without the | ||
| /// route the message cannot say which one fired. | ||
| #[test] | ||
| #[should_panic(expected = "Escaped via: link argument")] | ||
| fn vable_array_escaping_the_block_on_a_link_arg_is_rejected() { | ||
| use crate::model::Link; | ||
|
|
||
| let mut graph = FunctionGraph::new("vable_array_escape"); | ||
| let frame_var = graph.alloc_value_var(); | ||
| graph.push_inputarg_var(graph.startblock, frame_var.clone()); | ||
| let array_var = graph | ||
| .push_op_var( | ||
| graph.startblock, | ||
| OpKind::FieldRead { | ||
| base: frame_var, | ||
| field: crate::model::FieldDescriptor::new( | ||
| "locals_stack_w", | ||
| Some("Frame".into()), | ||
| ), | ||
| ty: ValueType::Ref(None), | ||
| pure: false, | ||
| }, | ||
| true, | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| // The consumer block receives the array as a link argument instead | ||
| // of reading it itself, so its `vable_array_vars` — rebuilt per | ||
| // block — has no entry to lower the access against. | ||
| let consumer = graph.create_block(); | ||
| let phi = graph.alloc_value_var(); | ||
| graph.push_inputarg_var(consumer, phi.clone()); | ||
| graph.set_return(consumer, Some(phi)); | ||
| let link = Link::from_variables(&graph, vec![array_var], consumer, None); | ||
| graph.set_control_flow_metadata(graph.startblock, None, vec![link]); | ||
|
|
||
| let config = GraphTransformConfig { | ||
| vable_arrays: vec![VirtualizableFieldDescriptor::new_with_arraydescr( | ||
| "locals_stack_w", | ||
| Some("Frame".into()), | ||
| 0, | ||
| 8, | ||
| true, | ||
| )], | ||
| ..Default::default() | ||
| }; | ||
| transform_graph(&graph, &config); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add regression coverage for the other three escape routes.
The doc comment on check_no_vable_array names four routes: "link argument", "fused exitswitch operand", "call argument", and "setfield operand". It also records that enumerating routes one at a time is what let _check_stack_index pass a green test while escaping by an unchecked route.
Only the link-argument route has a test. Add three #[should_panic] tests that pin the remaining route strings, so a future change that drops a call site fails a test instead of silently widening what the pass accepts.
🤖 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 `@majit/majit-translate/src/codewriter/jtransform.rs` around lines 7773 - 7833,
Add three regression tests alongside
vable_array_escaping_the_block_on_a_link_arg_is_rejected, covering the fused
exitswitch operand, call argument, and setfield operand escape paths. Build
minimal graphs that cause a virtualizable array to escape through each route,
run transform_graph, and annotate each test with #[should_panic(expected =
"...")] using the exact route strings "fused exitswitch operand", "call
argument", and "setfield operand".
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 23e4152). 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
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
majit/majit-translate/src/front/option_closure_select.rs (1)
268-317: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit test coverage for the niche-result
Map/AndThenpath.The
result_nichebranches (lines 268-287 for theSomearm, 306-317 for theNonearm) are new logic with no direct unit test. Every existing test uses thesite()helper, which hardcodesresult_niche: false(line 508). The niche path changes both the emitted shape (identity/null_mut()instead of anOptionaggregate) and the owner keys used, so a regression here would only surface through the#[ignore]d real-LLBC tests inmir.rs, which do not run by default.Add a test that builds a
Map(and ideallyAndThen) site withresult_niche: trueand asserts:
- The
Somearm emits noSyntheticTransparentCtor("Option", ..)and forwardscall_once's result directly.- The
Nonearm emits anull_mut()call instead of anOptionctor.♻️ Suggested test skeleton
fn site(kind: ClosureCombinator, result_var: Variable) -> ClosureSelectSite { ClosureSelectSite { kind, result_var, option_owner: "core::option::Option".into(), some_owner: "core::option::Option::Some".into(), call_once_owner: "test::closure".into(), payload_ty: ValueType::Int, call_result_ty: ValueType::Int, args_tuple_suffix: String::new(), niche: false, result_option_owner: "core::option::Option".into(), result_some_owner: "core::option::Option::Some".into(), result_niche: false, } } + + fn niche_result_site(kind: ClosureCombinator, result_var: Variable) -> ClosureSelectSite { + ClosureSelectSite { + result_niche: true, + ..site(kind, result_var) + } + } + + #[test] + fn map_with_niche_result_returns_closure_value_and_null_on_none() { + // Build result = opt.map(env) with niche_result_site(..) and assert: + // - no SyntheticTransparentCtor("Option", ..) in the Some arm + // - a null_mut() call in the None arm + }Also applies to: 495-510
🤖 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 `@majit/majit-translate/src/front/option_closure_select.rs` around lines 268 - 317, Extend the unit tests around the `site()` helper and combinator emission to construct `Map` (and preferably `AndThen`) cases with `result_niche: true`. Assert that the `Some` branch in the relevant emission function forwards the `call_once` result without a `SyntheticTransparentCtor("Option", ..)`, while the `None` branch emits `null_mut()` rather than an Option constructor.majit/majit-translate/src/translator/rtyper/cutover.rs (1)
5110-5125: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExercise the production blocked-block path.
The test removes the blocked block before calling
transform_allocate. It then performs no assertion. The test passes even if production code stops excluding blocked blocks.Call the production block-selection or specialization path, or extract the selection into a shared helper and test that helper. Assert that the blocked block is not transformed.
🤖 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 `@majit/majit-translate/src/translator/rtyper/cutover.rs` around lines 5110 - 5125, Update the test around the blocked-block selection to exercise the production block-selection or specialization path instead of filtering the block locally before transform_allocate. Assert that the blocked block is excluded from transformation, optionally extracting the selection logic into a shared helper used by both production code and the test.
🤖 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.
Outside diff comments:
In `@majit/majit-translate/src/front/option_closure_select.rs`:
- Around line 268-317: Extend the unit tests around the `site()` helper and
combinator emission to construct `Map` (and preferably `AndThen`) cases with
`result_niche: true`. Assert that the `Some` branch in the relevant emission
function forwards the `call_once` result without a
`SyntheticTransparentCtor("Option", ..)`, while the `None` branch emits
`null_mut()` rather than an Option constructor.
In `@majit/majit-translate/src/translator/rtyper/cutover.rs`:
- Around line 5110-5125: Update the test around the blocked-block selection to
exercise the production block-selection or specialization path instead of
filtering the block locally before transform_allocate. Assert that the blocked
block is excluded from transformation, optionally extracting the selection logic
into a shared helper used by both production code and the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f1e7ccfc-97c5-4d36-bb86-275b79b418d6
📒 Files selected for processing (14)
majit/majit-translate/src/front/mir.rsmajit/majit-translate/src/front/option_closure_select.rsmajit/majit-translate/src/front/slice_index.rsmajit/majit-translate/src/translator/rtyper/cutover.rspyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/eval.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit-trace/src/jitcode_dispatch/tests.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-jit-trace/src/trace_opcode.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/eval.rs
The recursion into an inlined sub-struct is handed the caller's own cur_index, so the "not found" value it returns already counts the fields walked before that sub-struct. Adding it back with `+=` counted them twice, giving every field after a non-leading nested struct an index one too high per preceding field. That breaks the invariant field_pos_in() documents and descr.rs derive_index_in_parent() repairs: all_fielddescrs_into() numbers the same fields flat, so the two walkers disagreed. The stale number survives wherever derive_index_in_parent() returns None -- no parent descr, an empty parent all_fielddescrs(), or an unresolved field name. Assisted-by: Claude
optimize_block() did not port jtransform.py:116-118: the exception exits and the last_exception exitswitch belong to the block's last operation, so when rewriting that operation contributes no operation at all they have to go with it. Without this the block keeps an exception edge for an operation that is no longer in it. Ported in upstream order -- the count is taken right after rewrite_operation() returns, and the exits are truncated before the exitswitch/exits remap, matching _killed_exception_raising_operation() at jtransform.py:176-179. Latent today. A census over the pyre-object/pyre-interpreter/pyre-jit corpus that build.rs analyzes counted 1590 canraise blocks and none whose last operation rewrote to nothing, so no wrong exception edge is being emitted; the test pins the behaviour with cast_int_to_uint, which rewrites to RewriteResult::Identity. Assisted-by: Claude
…ub-struct Same defect as the majit port, on the RPython side it was ported from. The recursion is given the caller's own cur_index, so the "not found" value it returns already counts the fields walked before the inlined sub-struct; adding it back with += counted them twice and gave every field after a non-leading sub-struct an index one too high per preceding field. all_fielddescrs() numbers the same fields flat, and descr.py:228 pairs the two, so they disagreed for that shape. New test file; three of its four cases fail without the fix (assert 4 == 3). codewriter 524 passed, backend/llsupport/test/test_descr.py 20 passed, metainterp/test/test_ajit.py 217 passed, backend/llgraph 138 passed. Assisted-by: Claude
Addresses a review note on the elided-last-operation test: it proved one non-exception edge remained but not that the edge targeted the continuation block. Assisted-by: Claude
`count_before_last_operation` was initialised to 0, so a block with no operations satisfied `len(newoperations) == count_before_last_operation` and took the `jtransform.py:117` branch: its `last_exception` exitswitch and exception exits were dropped. Upstream never binds the name when the loop body does not run, so the comparison cannot evaluate there. The counter is now `Option<usize>`; `None` means there was no last operation and the test does not apply. Also ports `jtransform.py:74-75`, the early return for the graph's return and except blocks. Both are already inert for every remaining step of `optimize_block`, so this changes no output today. Assisted-by: Claude
`format_assembler`'s first pass assigned each label its ordinal as it scanned jump targets, and the output loop's `FlatOp::Label` arm could only look up a number a jump had already claimed. `format.py:37-44` only marks targets with a sentinel; `format.py:45-50 getlabelname` assigns the number at print time, reached from both the jump operand (`format.py:24-25`) and the label line (`format.py:61-63`). Over a back edge the two orders differ: the loop header's label line prints before the back edge that targets it, so pyre numbered the forward exit first and inverted every label in the loop. The first pass now marks only, and numbering moves into the output loop. Two tests cover it, one on a hand-built SSARepr and one end-to-end over the back-edge graph. `format_assembler` is read by the flatten parity tests and by `JitCode::dump()`, which has no callers. Assisted-by: Claude
…enced `FieldDescriptor` gains `base_is_deref: Option<bool>`, recorded by the MIR front in `resolve_place` / `emit_projection_write`. `base_is_local_aggregate()` (`== Some(false)`) joins `fresh_virtualizable` as a condition that keeps a field access off the `getfield_vable_*` / `setfield_vable_*` path: a projection whose base is a local aggregate rather than a pointer is not reaching a live virtualizable. `None` means "not recorded" and keeps the old lowering, so a producer that does not set the flag cannot silently disable the protocol. Measured over the three extracted crates (32181 fns, 46652 field projections), the twelve non-dereferenced projections of a `PyFrame` virtualizable field are all in `pyframe::<Impl>::new`, which takes its frame by value. `majit_metainterp::jit` gains `hint_access_directly` / `hint_fresh_virtualizable` (rlib/jit.py:88-93). Placement differs from upstream: pyre dispatches one helper per kwarg and has no `hook_access_field`, so the suppression covers only the basic block the call sits in. `simplify_lowered_graph` runs `eliminate_empty_blocks` + `clear_unreachable_blocks` a second time after the dead-code sweeps. `all_passes` (simplify.py:1065-1078) orders `transform_dead_op_vars` before `eliminate_empty_blocks`; the existing head call runs before every sweep, which inverts that. The head call cannot move: it collapses the MIR front's own empty blocks, rewiring a link on 998 of 1255 graphs. `pyre_object::object_array::<Impl>::len` lowers to `OpKind::ArrayLen`. It was matched by `is_container_len` and lowered to `Call ["__len"]` with the array as an argument. `int_array` / `float_array` stay on `__len`; neither is virtualizable. `_check_no_vable_array` (jtransform.py:124-127, 421, 912) is ported, with a route tag added to upstream's message. It is not wired to its four call sites in this commit. Assisted-by: Claude
`PyFrame::locals_w` / `locals_w_mut` become `locals_w!` / `locals_w_mut!`
macros and their 108 call sites are converted. The accessor form put the
field read in a separate graph from every consumer, so the array crossed a
call boundary as a return value; the macro emits the read and its consumer
into one basic block, which is what the codewriter's virtualizable-array
handling requires. Charon output for the two forms:
accessor: bb0: _4 = locals_w<'7>(move _5) -> bb2
macro: bb0: _4 = &(*((*f_1)).locals_cells_stack_w); ... -> bb2
`FrameBox::new` taking its frame by value is now guarded at compile time
(`const _: fn(PyFrame) -> FrameBox = FrameBox::new;`). Allocating first and
initialising through a pointer would turn its twelve field projections into
dereferences.
Assisted-by: Claude
`pyre/scripts/vable-projection-census.py` counts field projections over the already-extracted Charon LLBC and fails unless every non-dereferenced projection of a `PyFrame` virtualizable field is in `pyframe::<Impl>::new`. The field list is parsed from `virtualizable_spec.rs` rather than duplicated, and an unrecognised base shape fails rather than being assumed harmless. The CI step goes in `&pyre-check-steps` after `Verify prepared Charon/LLBC`, where both artefacts are already downloaded. That block is aliased by the macOS and Windows jobs, so the step is gated `if: runner.os == 'Linux'` — charon resolves per-platform and has no Windows mapping. The workflow's path filters gain `scripts/llbc_extract.py`, the module `scripts/extract-llbc.py` imports and where most of the extraction logic lives; a change to it alone did not trigger this workflow. Assisted-by: Claude
`lower_framestate`'s first-edge arm handed a block its predecessor's exit framestate verbatim, so its inputargs were the same `Variable` identities as the link args feeding them — measured at 142598 of 153927 blocks. Upstream's `make_next_block` (flowcontext.py:464-471) is `newstate = state.copy()`, and `FrameState.copy` (framestate.py:42) is "make a copy of this state in which all Variables are fresh". That distinctness is what makes positional liveness meaningful in `transform_dead_op_vars`: with the identities shared, a value read anywhere in the predecessor is in `read_vars`, so a link arg naming it is never dropped however dead it is in the target. `prune_dead_phis` loses its `if dirty` gate. `transform_dead_op_vars` is the first entry in `all_passes` (simplify.py:1067) and `simplify_graph` (:1080-1086) applies every pass to every graph. The gate was written while the inputargs aliased, when the sweep found little on graphs no other pass had touched. Both are load-bearing, measured by reverting each in place against `_check_stack_index`: with only the copy the array still escapes at link arg 8, with only the ungating at arg 5, with both it does not escape. The four `_check_no_vable_array` call sites are wired (jtransform.py:124-127, 421, 912). `vable_array_escaping_the_block_on_a_link_arg_is_rejected` now drives `transform_graph` rather than calling the check directly. `FieldDescriptor` gains `taken_by_address`. `&raw mut (*p).f` reaches the codewriter as a `FieldRead`, because the front models a reference as an alias of its referent. For a virtualizable field that is not survivable: the read is recorded in `vable_array_vars` and the op is dropped, leaving the address's consumer with an undefined operand. `pyframe::FrameLocalsRoot::new` is the case — it registers `addr_of_mut!((*frame).locals_cells_stack_w)` with `try_gc_add_root` — and it is what the newly wired check rejected. Marking it suppresses the virtualizable lowering only; the aliasing itself is unchanged, here as at every other place-address in the corpus. Only the outermost projection is marked, and only when the place is a projection that emitted a read. A local's Variable is the Variable of the op that produced it, so matching on the last operation alone marks whatever came before — and `self.method()` on `&mut self` is a `Rvalue::Ref` over a local at every call site. Assisted-by: Claude
`simplify_lowered_graph` takes `sweep_dead_vars`. The end-of-lowering call and the panic-chain call pass `true` and run `prune_dead_phis` unconditionally; the result-exc pre-pass passes `false` and keeps the `dirty` gate, so the `dirty` accumulation over `fuse_boxing_alloc`, `prune_dead_boxing_remnants`, `remove_dead_aggregates`, `remove_assertion_errors` and `fold_constant_exitswitch` is restored. `rewire_result_exc_call_sites` follows one `Variable` by identity from a link's args into the target block's inputargs. A `Result` that is dead downstream is what the sweep removes, and `follow_single_exit` then fails the graph with "block N's exit does not carry the tracked value". Sweeping in the pre-pass stopped three `_io` `__init__` graphs from lowering and took the `reader_reset_buf` and `writer_reset_buf` callees they queue: 2356 jitcodes -> 2350. With the pre-pass gate kept, the corpus is back to 2356 with no name-level difference. Assisted-by: Claude
…by colour `keyword_builtin_wrapper_finds_colored_argument_slice_item_descr` asserted the argument slice is not coloured r0, then took the first `arraylen_gc` on that register. The colour is an allocation outcome: with the entry framestate copied and the dead-var sweep running on every graph, the `__pyre_wrap_getrandbits` wrapper loses 5 operations (153 -> 148) and the allocator moves the slice from r11 to r0. The wrapper is otherwise unchanged across the two arms -- same five array operations in the same order, same length and item descriptor pool entries, same array read throughout -- so the assertion pinned the allocation and not a property of the wrapper. Select the nearest `arraylen_gc` on the slice's register preceding the extraction instead. That also drops the reason the colour guard existed: the first `arraylen_gc` can be the splitter's own `args.len()` read once that inlines and the wrapper input is dead, and taking the nearest preceding one names the slice's read under either colouring. Assisted-by: Claude
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38501fdb28
ℹ️ 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".
| macro_rules! locals_w_mut { | ||
| ($frame:expr) => { | ||
| (unsafe { &mut *$frame.locals_cells_stack_w }) | ||
| }; |
There was a problem hiding this comment.
locals_w_mut!
Because the macro dereferences the copied raw field pointer inside its own unsafe block, safe callers can invoke locals_w_mut! through a shared &PyFrame or invoke it twice while the first result remains live, creating aliased mutable references and triggering undefined behavior. The removed accessor required &mut self, so preserve that exclusivity requirement while still expanding the field access at the call site.
Useful? React with 👍 / 👎.
| // wherever the access happens to sit. A single hint call | ||
| // cannot: it covers only the accesses in its own block. | ||
| // A redirected field access on the same instance in any | ||
| // other block — the far side of a branch, a loop body, an | ||
| // assertion guarded by a null test — still lowers to |
There was a problem hiding this comment.
When a constructor uses the new hint_fresh_virtualizable equivalent once and then accesses a redirected field after a branch or inside a loop, optimize_block clears vable_flags for that block, so the access is incorrectly lowered through the virtualizable protocol even though the object is still fresh. Requiring another hint in every block does not preserve upstream semantics; port the per-access flag propagation (hook_access_field/instance annotation) so the single source hint remains effective across the function.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 @.github/workflows/pyre-ci.yml:
- Around line 408-410: Update the run command for the vable-projection census
step to invoke the pinned interpreter from steps.cpython.outputs.python-path
instead of bare python3, while preserving the existing script and arguments.
In `@majit/majit-translate/src/codewriter/jtransform.rs`:
- Around line 3074-3076: Update the comment above the write-side
fresh_virtualizable assignment to explicitly state that FieldWrite represents a
store rather than an address-taking projection, so the taken_by_address arm does
not apply. Keep the existing base_is_local_aggregate logic unchanged.
In `@majit/majit-translate/src/front/mir.rs`:
- Around line 5243-5298: Add a focused FunctionGraph-based unit test following
block_emptied_after_the_head_collapse_is_still_rewired_past that exercises
mark_place_address_of through Ref/RawPtr over a Projection(Field). Assert
taken_by_address is set only on the outermost FieldRead, while bare-local &x and
a no-op Deref collapse leave the relevant FieldRead unmarked.
In `@pyre/scripts/vable-projection-census.py`:
- Around line 93-97: Update the pretty-print subprocess invocation in the
surrounding function to surface stderr when check=True raises
CalledProcessError, preserving the existing stdout decoding on success. Ensure
the failure path logs or re-raises an error containing the captured stderr so CI
output includes Charon’s diagnostic.
🪄 Autofix
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: ebcd1981-921c-42ae-aa27-c2d9ffbce8bc
📒 Files selected for processing (37)
.github/workflows/pyre-ci.ymlmajit/majit-metainterp/src/jit.rsmajit/majit-translate/src/codewriter/flatten.rsmajit/majit-translate/src/codewriter/format.rsmajit/majit-translate/src/codewriter/heaptracker.rsmajit/majit-translate/src/codewriter/jtransform.rsmajit/majit-translate/src/front/bool_then.rsmajit/majit-translate/src/front/from_size_align.rsmajit/majit-translate/src/front/mir.rsmajit/majit-translate/src/front/option_closure_select.rsmajit/majit-translate/src/front/option_expect.rsmajit/majit-translate/src/front/option_is_none.rsmajit/majit-translate/src/front/option_map_or.rsmajit/majit-translate/src/front/option_try.rsmajit/majit-translate/src/front/option_unwrap.rsmajit/majit-translate/src/front/option_unwrap_or.rsmajit/majit-translate/src/front/range_iter.rsmajit/majit-translate/src/front/result_exc.rsmajit/majit-translate/src/front/slice_index.rsmajit/majit-translate/src/model.rsmajit/majit-translate/src/translator/backendopt/mod.rsmajit/majit-translate/src/translator/rtyper/cutover.rsmajit/majit-translate/tests/test_vable_array_len.rspyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/eval.rspyre/pyre-interpreter/src/pyframe.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit-trace/src/jitcode_dispatch/tests.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-jit-trace/src/trace_opcode.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/eval.rspyre/scripts/vable-projection-census.pyrpython/jit/codewriter/heaptracker.pyrpython/jit/codewriter/test/test_heaptracker.py
| if: runner.os == 'Linux' | ||
| shell: bash | ||
| run: python3 pyre/scripts/vable-projection-census.py build/llbc/pyre-object.ullbc build/llbc/pyre-interpreter.ullbc build/llbc/pyre-jit.ullbc |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Use the pinned interpreter instead of the system python3.
Every other Python step in this job runs ${{ steps.cpython.outputs.python-path }}. This step runs bare python3, so it binds to the runner image's system interpreter rather than the pinned 3.14. The script uses only the standard library today, so behavior does not change; the inconsistency is what invites drift when the image's default moves.
♻️ Proposed change
- run: python3 pyre/scripts/vable-projection-census.py build/llbc/pyre-object.ullbc build/llbc/pyre-interpreter.ullbc build/llbc/pyre-jit.ullbc
+ run: ${{ steps.cpython.outputs.python-path }} pyre/scripts/vable-projection-census.py build/llbc/pyre-object.ullbc build/llbc/pyre-interpreter.ullbc build/llbc/pyre-jit.ullbc📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if: runner.os == 'Linux' | |
| shell: bash | |
| run: python3 pyre/scripts/vable-projection-census.py build/llbc/pyre-object.ullbc build/llbc/pyre-interpreter.ullbc build/llbc/pyre-jit.ullbc | |
| if: runner.os == 'Linux' | |
| shell: bash | |
| run: ${{ steps.cpython.outputs.python-path }} pyre/scripts/vable-projection-census.py build/llbc/pyre-object.ullbc build/llbc/pyre-interpreter.ullbc build/llbc/pyre-jit.ullbc |
🤖 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 @.github/workflows/pyre-ci.yml around lines 408 - 410, Update the run command
for the vable-projection census step to invoke the pinned interpreter from
steps.cpython.outputs.python-path instead of bare python3, while preserving the
existing script and arguments.
| /// Record that the `Variable` just resolved for a place is the | ||
| /// **address** of a field, not its value. | ||
| /// | ||
| /// The `Ref` / `RawPtr` arms alias the reference to its referent, so | ||
| /// `&raw mut (*p).f` leaves an ordinary `FieldRead` behind. A | ||
| /// virtualizable field read is not kept as a value — the codewriter | ||
| /// records the array in `vable_array_vars` and drops the op — so the | ||
| /// address's consumer would be handed an undefined operand. Flagging | ||
| /// the descriptor lets the codewriter decline the virtualizable path | ||
| /// for exactly this projection. | ||
| /// | ||
| /// Only the **outermost** projection is marked: it is the one whose | ||
| /// result is the Variable the address stands for. An inner step of | ||
| /// `&(*p).a.b` is a genuine value read of `a` and keeps its lowering, | ||
| /// so a nested read of a virtualizable field is not collaterally | ||
| /// suppressed. | ||
| /// | ||
| /// Both guards are load-bearing, because a local's Variable *is* the | ||
| /// Variable of whatever op produced it: | ||
| /// | ||
| /// - `projection` — `&mut x` on a bare local resolves to that local's | ||
| /// Variable with nothing emitted. Matching on the last op's result | ||
| /// alone would then mark the op that produced `x`, which may be an | ||
| /// unrelated earlier field read. Autoref makes this the common | ||
| /// case, not a corner: `self.method()` on `&mut self` is a | ||
| /// `Rvalue::Ref` over a local at every call site. | ||
| /// - `before` — a projection that emits nothing (a bare `Deref`, or a | ||
| /// field the front resolves without a read) leaves the preceding op | ||
| /// as `last`, with the same false-marking risk. | ||
| /// | ||
| /// A false mark here is not inert: on a virtualizable field it would | ||
| /// silently disable the protocol for a genuine access, which is the | ||
| /// failure the `Option` default on `base_is_deref` exists to prevent. | ||
| fn mark_place_address_of( | ||
| &mut self, | ||
| mir_bb: usize, | ||
| projection: bool, | ||
| before: usize, | ||
| v: &Variable, | ||
| ) { | ||
| if !projection { | ||
| return; | ||
| } | ||
| let bb_id = self.block_id[mir_bb]; | ||
| let block = self.graph.block_mut(bb_id); | ||
| if block.operations.len() <= before { | ||
| return; | ||
| } | ||
| if let Some(op) = block.operations.last_mut() | ||
| && op.result.as_ref() == Some(v) | ||
| && let OpKind::FieldRead { field, .. } = &mut op.kind | ||
| { | ||
| field.taken_by_address = true; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Add a unit test for mark_place_address_of.
mark_place_address_of decides whether a FieldRead keeps a virtualizable field readable as a value or gets dropped by the codewriter's vable protocol. The function's own doc comment states the failure mode: a false mark on a virtualizable field silently disables the protocol for a genuine access, and this bug class is invisible until a specific address-taking shape appears.
The guard logic (projection, before, "only the last op", niche __pos_0 alias skip) has several interacting branches, but no direct unit test exercises it in this file. Add a small FunctionGraph-based test (following the pattern of block_emptied_after_the_head_collapse_is_still_rewired_past) that builds a Ref/RawPtr over a Projection(Field) place and asserts taken_by_address is set only on the outermost FieldRead, and stays unset for a bare-local &x and for a no-op Deref collapse.
🤖 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 `@majit/majit-translate/src/front/mir.rs` around lines 5243 - 5298, Add a
focused FunctionGraph-based unit test following
block_emptied_after_the_head_collapse_is_still_rewired_past that exercises
mark_place_address_of through Ref/RawPtr over a Projection(Field). Assert
taken_by_address is set only on the outermost FieldRead, while bare-local &x and
a no-op Deref collapse leave the relevant FieldRead unmarked.
| return subprocess.run( | ||
| [str(charon_bin()), "pretty-print", str(path)], | ||
| capture_output=True, | ||
| check=True, | ||
| ).stdout.decode(errors="replace") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Surface Charon's stderr when pretty-print fails.
capture_output=True with check=True discards stderr into the exception object. CalledProcessError renders only the command and the exit status, so a CI red gives no reason for the failure. This script exists to diagnose a CI failure, so the diagnostic must reach the log.
🛠️ Proposed fix
- return subprocess.run(
- [str(charon_bin()), "pretty-print", str(path)],
- capture_output=True,
- check=True,
- ).stdout.decode(errors="replace")
+ proc = subprocess.run(
+ [str(charon_bin()), "pretty-print", str(path)],
+ capture_output=True,
+ check=False,
+ )
+ if proc.returncode != 0:
+ raise SystemExit(
+ f"charon pretty-print failed on {path} (exit {proc.returncode})\n"
+ + proc.stderr.decode(errors="replace")
+ )
+ return proc.stdout.decode(errors="replace")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return subprocess.run( | |
| [str(charon_bin()), "pretty-print", str(path)], | |
| capture_output=True, | |
| check=True, | |
| ).stdout.decode(errors="replace") | |
| proc = subprocess.run( | |
| [str(charon_bin()), "pretty-print", str(path)], | |
| capture_output=True, | |
| check=False, | |
| ) | |
| if proc.returncode != 0: | |
| raise SystemExit( | |
| f"charon pretty-print failed on {path} (exit {proc.returncode})\n" | |
| proc.stderr.decode(errors="replace") | |
| ) | |
| return proc.stdout.decode(errors="replace") |
🧰 Tools
🪛 Ruff (0.16.1)
[error] 93-93: subprocess call: check for execution of untrusted input
(S603)
🤖 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/scripts/vable-projection-census.py` around lines 93 - 97, Update the
pretty-print subprocess invocation in the surrounding function to surface stderr
when check=True raises CalledProcessError, preserving the existing stdout
decoding on success. Ensure the failure path logs or re-raises an error
containing the captured stderr so CI output includes Charon’s diagnostic.
… a count `EXPECTED` pinned `FrameBox::new` at 12 non-dereferenced projections. Reworking that function's GC-root bracket took it to 10, so the step failed on Linux against a number measured before that landed. The count tracks how often the constructor happens to touch a virtualizable field, which moves with work unrelated to the protocol. Gate on the set of functions instead. `FrameBox::new` takes the frame by value, so its `frame.<field>` projections are off a local aggregate; what suppresses the virtualizable lowering is another function acquiring one, and that is now what fails. Also fail when the census matches nothing at all. If the render or the projection pattern stops resolving, every count goes to zero and a set-membership check alone would report a clean tree while checking nothing. Assisted-by: Claude
1 participant
Next round of the codewriter parity audit against
rpython/jit/codewriter/.Two gaps found and fixed; the rest of the round is recorded below as negatives
so the same ground is not re-walked.
1.
get_fielddescr_index_in()double-counts before a nested structget_fielddescr_index_in()hands the recursion its owncur_index, so the"not found" value the recursion returns already counts the fields walked
before the nested struct. Adding it back with
+=counted them twice.Measured on
Outer { a, inner: Inner{x,y}, b }:all_fielddescrs_intoget_fielddescr_index_inEvery field after a non-leading nested struct is one too high per preceding
field of the enclosing struct.
Both pre-existing tests (
heaptracker.rs,call.rs) put the nested structfirst, so the recursion always started at
cur_index == 0and the bug wasinvisible. The new test places it after a field.
Why it matters
field_pos_in()states the invariant it depends on:all_fielddescrs(S)[i] .get_index() == i, which holds upstream because one walker answers bothquestions. pyre has three numberers, and this was the only one that
disagreed —
all_fielddescrs_into()and the assembler'sbh_all_field_specs_for_struct_into()both count flat into a shared list.Blast radius is bounded:
descr.rs derive_index_in_parent()re-derives theindex from the parent that will actually be indexed and overrides the
caller's number. The stale value survives only where that returns
None—no parent descr (
FIELD_PARENT_ABSENT), an empty parentall_fielddescrs()(
FIELD_PARENT_EMPTY), or an unresolved field name(
FIELD_INDEX_UNRESOLVED).descr.rsdescribes what happens then: atoo-high index "either runs off the end or silently names a DIFFERENT field
and emits the store against it".
No real pyre struct with a non-leading inlined sub-struct has been exhibited,
so the miscompile is latent; the wrong number on a load-bearing path is not.
This is the same defect as
heaptracker.py:130upstream, reported separatelyas pypy/pypy#5537 — pyre inherited it verbatim in the port. The upstream red
symptom recorded there ("index 4 for a field that
all_fielddescrs()puts at3") reproduces here unchanged.
2. A block keeps its exception exits after its last operation is elided
optimize_block()did not portjtransform.py:116-118. The exception exitsand the
last_exceptionexitswitch belong to the block's last operation,so when rewriting that operation contributes no operation at all they have to
go with it — otherwise the block carries an exception edge for an operation
that is not in it any more.
Ported in upstream order: the count is taken right after
rewrite_operation()returns, and the exits are truncated before the exitswitch/exits remap,
matching
_killed_exception_raising_operation()(jtransform.py:176-179).Block::canraise()already means exactly upstream'sexitswitch == c_last_exception, andLink.exitcaseis already there, so the port ismechanical.
Latent today, and measured rather than assumed. A census over the
pyre-object/pyre-interpreter/pyre-jitcorpus thatbuild.rsanalyzes:
canraiseblocks reachedThe 1590 is what makes the 0 meaningful — the path is heavily exercised, and
no rewrite currently produces the combination. So no wrong exception edge is
being emitted today. The instrumentation was removed after measuring; the
test pins the behaviour with
cast_int_to_uint, one of the no-op casts thatrewrite to
RewriteResult::Identity.Audit method
Line-count ratios were tried first and discarded:
effectinfo.rsreads 0.3xupstream only because the
EffectInfodata class is extracted intomajit-ir(2134 lines combined, 3.9x — normal). Instead, all 400 top-leveldefs underrpython/jit/codewriter/were diffed against an identifierindex of
majit/+pyre/.Negatives, so the ground is not re-walked
jtransform.rs_ => RewriteResult::Keep. Upstream raises on anunregistered opname, and pass-through is gated on the blackhole actually
carrying a
bhimpl_(_add_default_ops: "All other operations areforbidden"). pyre has no such gate. But
OpKindis a closed enum and all45 variants that reach the default arm are named by the assembler, so
there is no unencodable-op hole today. Structural deviation only.
jtransform_shadow.rsmirrors_add_default_ops, but is explicitlyinert: gated on
PYRE_JTRANSFORM_SHADOW, "never feeds the productionjitcode path", and currently reads zero. It is an audit gauge, not
enforcement.
InteriorFieldRead/Writereach the default armand are encoded as
{get,set}interiorfield_gc_{i,r,f}. All six handlersare wired in
blackhole.rs, matching upstream's sixbhimpl_s. Clean.c__padskip-set asymmetry.all_fielddescrs_into()skipsc__pad*and
get_fielddescr_index_in()does not — the same asymmetry upstream has.Dead in pyre:
c__padappears only in skip predicates, never as a producedfield name.
Verification
cargo test --all --features dynasm— 7496 passed across 101 suites, 0failed. Both new tests were checked red before their fix and green after:
left: 4, right: 3for the index walk, andthe last_exception exitswitch must go with the elided operationfor the exception exits.One note on getting there: the gate first aborted in
pyre-jit-trace's buildscript with
duplicate variable v398(flowspace/model.rscheckgraph).That was not this change — reverting the one line in place reproduced it
identically — and not
maineither.build/llbc/was 9 days stale (154source files newer than their fingerprints) after the branch was rebased;
re-running
pyre/scripts/extract-llbc.pycleared it and the build scriptthen completed. Worth knowing before anyone else reads that panic as a code
defect.
Second round: the virtualizable array escape
Everything below landed on this branch after the section above was written.
It starts from one wired assertion and ends with the front-end bug that
assertion was unable to be wired against.
3.
optimize_blocktreated an operation-less block as an elisionFollow-up to §2. The elision test
len(newoperations) == count_beforealsofires for a block that had no operations to begin with, so an untouched
empty block had its exception exits stripped. Upstream reaches the same
check only from inside the
for op in block.operationsloop, which an emptyblock never enters.
The predicate that decides this is
is_bool_branchon the exitswitch value,and it is load-bearing: it is what separates "this block's switch belongs to
an operation" from "this block's switch is a plain bool test".
4. Virtualizable lowering is gated on whether the field base is dereferenced
FieldDescriptorcarriesbase_is_deref: Option<bool>, and thevirtualizable protocol only fires when it is
Some(true).None— thedefault — is deliberately not treated as "yes": a descriptor built by a
path that has not established the base is a real dereference must not
silently enable the protocol.
Also in this commit:
object_array::lenis retargeted toArrayLen. It had been matched to a__lenCall, i.e. it was not unmatched — it was matched to the wrongthing, and the call form is what carried the array into an argument
position.
_check_no_vable_arrayported fromjtransform.py:124-127, initially#[allow(dead_code)].5.
locals_w()accessors defeated the protocol; expanded as macrosThe interpreter reached the virtualizable array through
locals_w(&self)helper methods. A helper's body reads the field off its own
&selfparameter, so the base is a fresh local rather than the frame the caller
holds, and the protocol cannot see through it.
108 call sites now expand
locals_w!/locals_w_mut!in place. Aby-value compile-time guard on
FrameBox::newkeeps the shape fromregressing silently.
6. Census + CI step
pyre/scripts/vable-projection-census.pycounts non-dereferencedPyFramevirtualizable-field projections per LLBC artefact and fails on anyfunction not on the expected list. Current state:
pyre-object.ullbcpyre-interpreter.ullbcFrameBox::new, expectedpyre-jit.ullbcWired as a Linux-gated CI step.
scripts/llbc_extract.pywas also missingfrom both CI path filters, so an edit to the extractor could not trigger the
jobs that consume its output.
7. The front did not copy the entry framestate
Wiring
_check_no_vable_arraywas blocked by a real escape:PyFrame::_check_stack_indexthreaded the virtualizable array into asuccessor block that never reads it. Two independent causes, both needed:
(a)
make_next_blockdid not copy.flowcontext.py:464-471isnewstate = state.copy(), andframestate.py:42FrameState.copyis "makea copy of this state in which all Variables are fresh" — per slot, so two
slots holding one Variable become two distinct Variables. pyre carried the
state through unchanged on a first-arrival edge, which made the target's
inputargs literally the predecessor's own Variables. Measured at 142598
of 153927 blocks.
transform_dead_op_vars' liveness is positional, sowith the Variables shared it could not tell a read in the predecessor from a
read in the target, and kept every arg.
(b) the sweep was gated on
dirty.transform_dead_op_varsis thefirst entry in
all_passes(simplify.py:1067) andsimplify_graphappliesevery pass to every graph — there is no "only if something changed" gate
upstream.
_check_stack_indexreports no removal from any other pass, sothe gate meant the sweep never ran on it.
Both halves were verified load-bearing by in-place revert: copy alone still
escapes at link arg 8, ungating alone still escapes at arg 5, both together
is clean.
_check_no_vable_arrayis now wired at all four upstream sites —optimize_block(fused exitswitch operands and every link arg),rewrite_call_three_lists(call arguments), andrewrite_op_setfield.8. What the wired check then found:
addr_of_mut!on the array slotThe front lowers a place-address as a
FieldRead(6372 sites corpus-wide),so the codewriter recorded the array in
vable_array_varsand dropped theop, leaving
try_gc_add_rootwith an undefined operand. It was silentbefore; with the check wired it aborts the build, which is how it surfaced.
Fixed narrowly:
FieldDescriptor::taken_by_address, set only when thervalue is
Ref/RawPtrover a projection and the marked op is thefield read that rvalue just produced. Both guards matter — a local's
Variable is the Variable of the op that produced it, so an unguarded match
marks an unrelated earlier read, and
self.method()on&mut selfis aRefover a local at every call site. A false mark disables the protocolsilently, the same failure the
Optiondefault onbase_is_derefexists toprevent.
This does not make the aliasing right — the getfield still yields the
array's value where the source asked for the slot's address, as every
other place-address in the corpus does. It stops the virtualizable path from
turning that into a dropped operand. The general fix is tracked separately.
9. The sweep had to stay out of the result-exc pre-pass
Making the sweep unconditional cost 6 jitcodes (2356 → 2350). Not waved
off as noise, and worth recording how nearly it was: the first attempt to
dismiss it scanned jitcode bodies for callee
fnaddrintegers and reported100% of all 2356 jitcodes unreferenced — which falsifies the method, not the
regression. Callee references resolve through
fnaddr_bindings.bin.Real cause: exactly three
_io__init__graphs failed to lower withblock N's exit does not carry the tracked value, taking thereader_reset_buf/writer_reset_bufcallees they queue. pyre'sresult_excdiamond walk follows oneVariableby identity from linkargs into the target's inputargs, and a
Resultdead downstream is exactlywhat the sweep removes.
simplify_lowered_graphnow takessweep_dead_vars. End-of-lowering passestrue; the pyre-only pre-pass keeps itsdirtygate. That also leavesone sweep per graph, which is upstream's shape.
Verification
extract-llbc.py_check_no_vable_arrayabortscargo test -p majit-translateThe jitcode count and the per-name diff are both reported because the count
alone hides a swap.
reader_reset_buf1,writer_reset_buf2,__init__10 — the three the regression had zeroed.
Also on the branch: jitcode dump labels are numbered by first printed
mention rather than by internal id, so a dump diff is readable.
Summary by CodeRabbit
Bug Fixes
Tests