gc: put the per-thread GC box behind a gc_box feature and gate its remaining dynasm probes - #1098
Merged
Conversation
WalkthroughThis change adds conditional GC-box support and centralizes GC access in three JIT backends. It also restores vstack mirror state across reorder excursions, updates Windows descriptor stat conversion, and adds a residual-loop benchmark with backend statistics. ChangesConditional GC-box runtime
Vstack reorder restoration
Windows stat conversion
Residual-loop benchmark
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant JITBackend
participant GCBox
participant GCSync
JITBackend->>GCBox: Check installed allocator
alt GC box installed
JITBackend->>GCBox: Perform guarded or reentrant GC operation
else No GC box installed
JITBackend->>GCSync: Perform singleton GC operation
end
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
https://github.com/youknowone/pyre/blob/a8e9544867c6a7677e9d4ec10a862a7153d8b207/pyre-interpreter/src/module/posix/interp_posix.rs#L3615-L3616
On non-sandbox Windows builds with host_env, this branch makes os.stat(path) return the real file index and device, but fstat_fd still passes std::fs::Metadata to make_stat_result, whose Windows arm hardcodes st_ino and st_dev to zero. Consequently os.stat(path) and os.fstat(open(path).fileno()) now report different identities for every ordinary file, breaking callers that compare the two; the descriptor path needs to populate the same StatFields representation.
ℹ️ 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".
Comment on lines
+1647
to
+1649
| pub fn note_gc_box_installed() { | ||
| panic!( | ||
| "a per-thread GC box was installed in a build without the `gc_box` feature; \ |
There was a problem hiding this comment.
In a normal downstream build, calling the still-public JitDriverPair::set_gc_allocator or any backend's set_gc_allocator reaches this unconditional panic because none of those crates exposes or enables majit-gc/gc_box; the backend crates' new dev-dependencies only affect their own tests and do not participate when the crates are consumed as dependencies. Thus the advertised custom-allocator API now always aborts outside those test targets unless consumers discover that they must add a direct feature-unifying majit-gc dependency; propagate an explicit feature through the public crate or restrict/remove the API rather than leaving it operational but panicking.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyre/pyre-jit-trace/src/jitcode_dispatch/vstack_mirror.rs (1)
447-521: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReset
vstack_reorder_savedat mirror revalidation entry points.
seed_vstack_mirror,seed_callee_vstack_mirror, and the outer exception-handler reseeding path rebuild the vstack mirror and can revivevstack_valid, but they leave any priorvstack_reorder_savedintact. A stale saved(py_pc, depth, boxes)can later match a coincidental(new_pypc, new_depth)pair and restore boxes from the old walk. Clearvstack_reorder_savedtogether withvstack_reorder_ceilingat these revalidation points.🤖 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/jitcode_dispatch/vstack_mirror.rs` around lines 447 - 521, Reset vstack_reorder_saved alongside vstack_reorder_ceiling in seed_vstack_mirror, seed_callee_vstack_mirror, and the outer exception-handler reseeding path. Ensure each mirror revalidation clears any prior saved boxes before rebuilding the vstack mirror and reviving vstack_valid, preventing stale state from being consumed by returned_to_arm_point.
🤖 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-gc/src/lib.rs`:
- Around line 1642-1653: Update install_gc_box() to call note_gc_box_installed()
before disarm_published_nursery(), ensuring unsupported gc_box builds panic
before mutating nursery publishing state. Preserve the existing installation
flow for builds with gc_box support.
In `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs`:
- Around line 3382-3420: Update the Windows `fstat_fd` path to obtain a
`rustpython_host_env::fileutils::StatStruct` through the host API instead of
converting `std::fs::Metadata` with `make_stat_result`. Pass that result through
`stat_fields_from_statstruct`, matching `win_stat_fields` so `os.stat(path)` and
`os.fstat(fd)` expose the same `st_ino` and `st_dev`; add or preserve the
regression covering path/descriptor identity and `os.path.samestat`.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs`:
- Line 444: Add a focused test in the existing jitcode-dispatch test helpers
that exercises the `vstack_reorder_saved` restore path: build a synthetic
jitcode fixture with a non-`cfg_successor` boundary that arms the reorder
region, performs an out-of-order excursion, then returns to the same `(py_pc,
depth)`. Assert that the restored `vstack_boxes` exactly matches the
pre-excursion snapshot, while initializing the new field to `None` remains
unchanged in existing fixtures.
---
Outside diff comments:
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/vstack_mirror.rs`:
- Around line 447-521: Reset vstack_reorder_saved alongside
vstack_reorder_ceiling in seed_vstack_mirror, seed_callee_vstack_mirror, and the
outer exception-handler reseeding path. Ensure each mirror revalidation clears
any prior saved boxes before rebuilding the vstack mirror and reviving
vstack_valid, preventing stale state from being consumed by
returned_to_arm_point.
🪄 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: 01970bbd-c2b2-48ec-b690-8d9e7e53c612
📒 Files selected for processing (19)
majit/majit-backend-cranelift/Cargo.tomlmajit/majit-backend-dynasm/Cargo.tomlmajit/majit-backend-dynasm/src/runner.rsmajit/majit-backend-wasm/Cargo.tomlmajit/majit-gc/Cargo.tomlmajit/majit-gc/src/lib.rspyre/bench/synth/getframe_method_call_residual_body_once.cranelift.jitstatspyre/bench/synth/getframe_method_call_residual_body_once.dynasm.jitstatspyre/bench/synth/getframe_method_call_residual_body_once.pypyre/bench/synth/getframe_method_call_residual_body_once.wasm.jitstatspyre/pyre-interpreter/src/module/_random/mod.rspyre/pyre-interpreter/src/module/posix/interp_posix.rspyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/tests.rspyre/pyre-jit-trace/src/jitcode_dispatch/vstack_mirror.rspyre/pyre-jit/src/eval.rspyre/pyre-jit/tests/gc_stress.rs
| vstack_valid: false, | ||
| vstack_last_ref: OpRef::NONE, | ||
| vstack_reorder_ceiling: u32::MAX, | ||
| vstack_reorder_saved: None, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
Add a dedicated test for the vstack_reorder_saved restore path.
All changes in this file only initialize the new vstack_reorder_saved field to None in existing fixtures. No test drives the walk through an actual out-of-order excursion (the documented LOAD_ATTR + CALL reorder case) and asserts that the saved (py_pc, depth, boxes) triple is restored verbatim when the walk returns to the arm coordinate.
This restore logic sits on a JIT-correctness-critical path: a wrong restore silently produces a corrupted operand-stack mirror that only surfaces as a miscompiled bridge resume at runtime. Add a synthetic jitcode fixture that arms the reorder region (a non-cfg_successor boundary), then returns to the same (py_pc, depth), and assert vstack_boxes equals the pre-excursion snapshot.
Also applies to: 667-667, 1054-1054
🤖 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/jitcode_dispatch/tests.rs` at line 444, Add a focused
test in the existing jitcode-dispatch test helpers that exercises the
`vstack_reorder_saved` restore path: build a synthetic jitcode fixture with a
non-`cfg_successor` boundary that arms the reorder region, performs an
out-of-order excursion, then returns to the same `(py_pc, depth)`. Assert that
the restored `vstack_boxes` exactly matches the pre-excursion snapshot, while
initializing the new field to `None` remains unchanged in existing fixtures.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit be1f980). Files in the reviewed diffCodex did not produce a report (exit 1). Last log lines: |
… to its arm point `reconcile_vstack_at_boundary` armed `vstack_reorder_ceiling` on a boundary whose `new_pypc` is not a CFG successor of `prev_pypc`, and every following boundary was then forced to `ShadowReseed`, which clears the operand mirror and refills it from the virtualizable shadow. Mid-expression that shadow's stack region holds the NULLs the in-flight opcode's `popvalue_maybe_none` wrote, so the reseed dropped those slots and shifted the surviving boxes down: for a method-form `LOAD_ATTR` + `CALL` pair the callable landed in the `self_or_null` slot with the TOS left a hole, `flush_escape_state_with_latched_stack` declined, and the walk kept the legacy replay. Record `(py_pc, depth, boxes)` in the new `WalkContext::vstack_reorder_saved` when the region arms, and when a boundary returns to that exact `(py_pc, depth)` restore the boxes verbatim and close the region. Such an excursion retires no Python opcode, so the operand stack it left is the one it resumes with. Add `bench/synth/getframe_method_call_residual_body_once.py` with its three backend baselines (`fbw_blackhole_adopted_single_frame=10`, `loops_aborted=10`, `loops_compiled=0` on all three); it prints `(20000, 20000, 20005)` without this change. Re-record `exception_reused_object_tb_not_doubled.wasm.jitstats`, whose `fbw_blackhole_adopted_single_frame` rises 0 -> 3 with `loops_compiled`/`bridges_compiled`/`guard_failures` unchanged. Assisted-by: Claude
…stalled() `with_dynasm_active_gc` and the six pyre-object-facing allocation trampolines already skip the `DYNASM_ACTIVE_GC` thread-local when the process-global `GC_BOX_INSTALLED` flag is clear. `with_dynasm_active_gc_mut` and eleven further trampolines still read the thread-local unconditionally. That flag is set only by `install_gc_box`, which no production path calls — `pyre-jit`'s `eval.rs` installs each backend through `install_gc_standalone` — so the reads always miss, and on Mach-O each is a `_tlv_get_addr` call plus a `RefCell` borrow. Gate the remaining twelve sites the same way. The cranelift and wasm forwarders already carry the check; dynasm was the one backend without it. Reached through `with_dynasm_active_gc_mut`: the four JIT malloc slow paths and the three JIT write-barrier trampolines. Gated directly: `dynasm_gc_write_barrier`, `dynasm_gc_write_barrier_managed`, `dynasm_gc_add_root`, `dynasm_gc_remove_root`, `dynasm_heap_stats`, `dynasm_collect_full`, `dynasm_collect_oldgen_nonmoving`, `dynasm_get_objects`, `dynasm_get_referents`, `dynasm_is_tracked`, and the oldgen-freelist diagnostic. On a list-ref-store loop, `sample` over five interleaved runs per arm charges 410 of 3988 `dynasm_gc_write_barrier` samples to `_tlv_get_addr` before and 265 of 3768 after. The remainder is `gc_sync`'s own thread-local, which this does not touch. Assisted-by: Claude
…aptations The parity review surfaced three places where this branch has no upstream counterpart and the code did not record why. `DYNASM_ACTIVE_GC`: `gc.py:30` `GcLLDescription.__init__` holds `self.gcdescr` as a plain field on the backend descriptor, so the per-thread cell is scaffolding rather than a ported structure; note that only `install_gc_box` fills it, that only tests reach that path, and that every read is consequently gated on `majit_gc::gc_box_installed()`. `WalkContext::vstack_reorder_saved`: `pyjitpl.py:1892` `MIFrame.run_one_step` steps a live frame whose `registers_r` survive the step, so upstream has nothing to snapshot; the mirror is reconstructed from source pcs instead. The mirror-restore block in `reconcile_vstack_at_boundary`: `pyopcode.py:1037` `LOAD_ATTR` pops and pushes the live value stack in place, so the interpreter has no walk position to leave and return to. Assisted-by: Claude
`gc_box_installed()` gated every backend's box read at run time. Without the new `majit-gc/gc_box` feature it is a constant `false`, so each `gc_box_installed() && ...` branch and the `GC_BOX_INSTALLED` load become dead code; `note_gc_box_installed()` panics in such a build rather than leaving an installed box invisible to every probe. The three backend crates carry the feature in `dev-dependencies`, which is the only place a box is installed from: every `set_gc_allocator` / `with_gc_allocator` call site sits inside a `#[cfg(test)]` module or under `tests/`, and `JitDriverPair::set_gc_allocator` has no caller at all. Assisted-by: Claude
…zer runs Three comments said `driver_pair` reaches `set_gc_allocator`. It reaches `init_gc_subsystem` -> `install_gc_into_backend` -> `install_gc_standalone`, which registers the hooks against the `gc_sync` singleton and stores no per-thread box. Assisted-by: Claude
`reconcile_vstack_at_boundary` returned as soon as it replayed the saved boxes, which skipped the `reseed_vstack_from_shadow` hole-fill and the `vstack_valid` check that guards it. A hole the arm point already carried stayed NONE in the restored mirror, so `stack_sync` omitted that operand slot and the resumed frame read the slot below it. `bench/synth/getframe_stored_fback_walk.py` raised `TypeError: 'range_iterator' object is not subscriptable` at `box[0].f_back.f_locals['total']` on all three backends; it prints `199990000 0` again. Keep the restore as this boundary's reconcile (a new `_ if restored` match arm) and let the shared fill and position update at the end of the function run over it. Assisted-by: Claude
…backend Each trampoline opened with `majit_gc::gc_box_installed() && DYNASM_ACTIVE_GC.with(...)` (and the cranelift / wasm equivalents), repeating the same borrow-and-map preamble at 22, 14 and 6 sites. Move the `thread_local!` and that preamble into a private `gc_box` module per backend, exposing `with_ref` / `with_mut` / `with_reentrant_ref` / `with_mut_or_busy` / `present` / `store` / `clear`, and call those instead. The runtime `gc_box_installed()` gate stays where it was — inside the accessors. It cannot become a `#[cfg]` here: a Cargo feature is per-crate, so a backend crate cannot `#[cfg(feature = "gc_box")]` on a feature of `majit-gc` (`expected values for 'feature' are: 'dynasm'`). Without `majit-gc/gc_box` the call is a constant `false`, so each accessor folds to `None` and `nm` finds no `*_ACTIVE_GC` symbol in the release binary. Assisted-by: Claude
`os.fstat` on Windows built its result from `File::metadata`, which carries no file index and no volume serial, so `make_stat_result` filled st_ino and st_dev with zero and st_nlink with one. The same file reached by path answered with its real identity, and a character device or pipe answered with a disk file's format bits. Call `rustpython_host_env::fileutils::fstat` — the `StatStruct` producer the path forms already go through — and render it with `stat_fields_from_statstruct` / `stat_result_from_fields`. That function already reports an invalid fd as ERROR_INVALID_HANDLE, so the local `invalid_handle()` now only covers an error with no raw OS code. Not built locally: `cargo check --target x86_64-pc-windows-msvc` needs an MSVC toolchain for `stacker`'s C build. Assisted-by: Claude
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-backend-dynasm/src/runner.rs`:
- Around line 134-139: Update majit/majit-backend-dynasm/src/runner.rs:134-139
so with_ref delegates to the existing with_reentrant_ref accessor. In
majit/majit-backend-dynasm/src/runner.rs:204-206,
majit/majit-backend-cranelift/src/compiler.rs:1519-1521, and
majit/majit-backend-wasm/src/lib.rs:360-362, change each present accessor to use
try_borrow and return true when borrowing fails because a mutable borrow is
active.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/vstack_mirror.rs`:
- Around line 449-454: Clear vstack_reorder_saved and reset
vstack_reorder_ceiling when re-seeding in seed_callee_vstack_mirror,
seed_vstack_mirror, and the full-body vstack_enter_exception_handler path before
setting vstack_valid = true. Ensure stale reorder state cannot be restored
across unrelated seed boundaries.
🪄 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: f09369e9-f55a-4c2a-8a2b-b2de1c547764
📒 Files selected for processing (9)
majit/majit-backend-cranelift/src/compiler.rsmajit/majit-backend-dynasm/src/runner.rsmajit/majit-backend-wasm/src/lib.rspyre/pyre-interpreter/src/module/posix/interp_posix.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/tests.rspyre/pyre-jit-trace/src/jitcode_dispatch/vstack_mirror.rspyre/pyre-jit/src/eval.rspyre/pyre-jit/tests/gc_stress.rs
Comment on lines
+134
to
+139
| pub(super) fn with_ref<R>(f: impl FnOnce(&dyn majit_gc::GcAllocator) -> R) -> Option<R> { | ||
| if !majit_gc::gc_box_installed() { | ||
| return None; | ||
| } | ||
| DYNASM_ACTIVE_GC.with(|cell| cell.borrow().as_deref().map(f)) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Non-reentrant borrow() in the gc_box presence and read accessors. All three gc_box modules added reentrancy-tolerant accessors (with_reentrant_ref, with_mut_or_busy) because a GC query can fire while with_mut still holds the RefCell mutable borrow. The presence and plain-read accessors kept cell.borrow(), so those same reentrant paths panic with BorrowMutError instead of answering. Replace borrow() with try_borrow() and treat a held mutable borrow as "box present".
majit/majit-backend-dynasm/src/runner.rs#L134-L139: routewith_refthroughwith_reentrant_ref, and changepresentat Lines 204-206 totry_borrow, reportingtrueonErr.majit/majit-backend-cranelift/src/compiler.rs#L1519-L1521: changepresenttotry_borrow, reportingtrueonErr.majit/majit-backend-wasm/src/lib.rs#L360-L362: changepresenttotry_borrow, reportingtrueonErr.
📍 Affects 3 files
majit/majit-backend-dynasm/src/runner.rs#L134-L139(this comment)majit/majit-backend-cranelift/src/compiler.rs#L1519-L1521majit/majit-backend-wasm/src/lib.rs#L360-L362
🤖 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-backend-dynasm/src/runner.rs` around lines 134 - 139, Update
majit/majit-backend-dynasm/src/runner.rs:134-139 so with_ref delegates to the
existing with_reentrant_ref accessor. In
majit/majit-backend-dynasm/src/runner.rs:204-206,
majit/majit-backend-cranelift/src/compiler.rs:1519-1521, and
majit/majit-backend-wasm/src/lib.rs:360-362, change each present accessor to use
try_borrow and return true when borrowing fails because a mutable borrow is
active.
Comment on lines
+449
to
+454
| ctx.vstack_reorder_saved = None; | ||
| } | ||
| if !cfg_successor && ctx.vstack_reorder_ceiling == u32::MAX { | ||
| ctx.vstack_reorder_ceiling = (new_pypc as usize).max(prev_pypc) as u32; | ||
| ctx.vstack_reorder_saved = | ||
| Some((prev_pypc as u32, ctx.vstack_depth, ctx.vstack_boxes.clone())); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Expect every mirror re-seed and every path that sets vstack_valid=true
# to clear vstack_reorder_saved and reset vstack_reorder_ceiling.
rg -n -C 10 \
'vstack_valid\s*=|vstack_reorder_saved|vstack_reorder_ceiling|vstack_enter_exception_handler|seed_callee_vstack_mirror' \
pyre/pyre-jit-trace/src/jitcode_dispatchRepository: youknowone/pyre
Length of output: 50372
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
cd pyre/pyre-jit-trace/src/jitcode_dispatch
fd -a '.*\.rs$' . | sed 's#^\./##' | grep -E '(^|/)vstack_mirror\.rs$|(^|/)exception|handler|residual|mirror|walk|i' | head -200
echo '--- vstack_mirror size ---'
wc -l vstack_mirror.rs
echo '--- exact occurrences in relevant source only (excluding tests.rs) ---'
rg -n -C 3 \
'vstack_valid\s*=|vstack_reorder_saved|vstack_reorder_ceiling|vstack_enter_exception_handler|seed_callee_vstack_mirror|fbw_seed|invalid|invalidate|restart|resume|outer_jitcode_index|outer_resume_marker_jit_pc' \
$(fd -a '.*\.rs$' . | sed 's#^\./##' | grep -v '/tests.rs$' | tr '\n' ' ')
echo '--- vstack_mirror relevant ranges ---'
sed -n '1,180p' vstack_mirror.rs
sed -n '180,240p' vstack_mirror.rs
sed -n '430,560p' vstack_mirror.rsRepository: youknowone/pyre
Length of output: 50372
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
cd pyre/pyre-jit-trace/src/jitcode_dispatch
echo '--- vstack_mirror exact occurrences in file only ---'
rg -n -C 4 \
'vstack_valid\s*=|vstack_reorder_saved|vstack_reorder_ceiling|vstack_enter_exception_handler|fbw_seed|invalid|invalidate|restart|resume|outer_jitcode_index|outer_resume_marker_jit_pc|reconcile_vstack_at_boundary' \
vstack_mirror.rs || true
echo '--- walk seed/restart helper occurrences in non-test core files ---'
rg -n -C 3 \
'fn fbw_seed|fbw_seed\(|fn .*seed|seed_callee_vstack_mirror|vstack_valid\s*=\s*(true|false|!)|outer_jitcode_index|outer_resume_marker_jit_pc|valid.*mirror|mirror' \
$(fd -a '.*\.rs$' . | sed 's#^\./##' | grep -v '/tests.rs$' | tr '\n' ' ') | head -n 300
echo '--- vstack_mirror key ranges ---'
sed -n '1,230p' vstack_mirror.rs
echo '--- exit points ---'
sed -n '420,560p' vstack_mirror.rsRepository: youknowone/pyre
Length of output: 50371
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
cd pyre/pyre-jit-trace/src/jitcode_dispatch
echo '--- seed callee vstack mirror ---'
sed -n '1003,1021p' vstack_mirror.rs
echo '--- seed full-body vstack mirror ---'
sed -n '1023,1131p' vstack_mirror.rs
echo '--- full-body exception handler vstack enter ---'
sed -n '1190,1293p' vstack_mirror.rs
echo '--- callee exception handler vstack enter ---'
sed -n '1295,1365p' vstack_mirror.rs
echo '--- other vstack_valid assignment sites excluding vstack_mirror.rs ---'
rg -n -C 3 \
'\bvstack_valid\s*=|seed_callee_vstack_mirror\(|seed_vstack_mirror\(|vstack_enter_exception_handler\(' \
$(git ls-files 'pyre/pyre-jit-trace/src/jitcode_dispatch/*.rs' | grep -v 'vstack_mirror.rs' | tr '\n' ' ')
echo '--- vstack_mirror remaining vstack_valid assignment sites ---'
rg -n -C 3 '\bvstack_valid\s*=' vstack_mirror.rsRepository: youknowone/pyre
Length of output: 13590
Clear the saved reorder state in re-seed paths.
seed_callee_vstack_mirror, seed_vstack_mirror, and full-body vstack_enter_exception_handler all set vstack_valid = true without clearing vstack_reorder_saved and vstack_reorder_ceiling. Clear both fields in these re-seed paths, or guard the reorder restore so it cannot apply across unrelated seed boundaries.
🤖 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/jitcode_dispatch/vstack_mirror.rs` around lines 449 -
454, Clear vstack_reorder_saved and reset vstack_reorder_ceiling when re-seeding
in seed_callee_vstack_mirror, seed_vstack_mirror, and the full-body
vstack_enter_exception_handler path before setting vstack_valid = true. Ensure
stale reorder state cannot be restored across unrelated seed boundaries.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rebased onto
d936eb4be42(#1102). Two of the original commits — the Windowsst_ino/st_devpath fix and the_randomMersenne Twister allocation — weredropped: both landed under us in #1085, and re-applying them produced an empty
diff.
What is here
GC box (the epic's #396 R4 endpoint). Every backend trampoline used to open
with
majit_gc::gc_box_installed() && DYNASM_ACTIVE_GC.with(|c| …)— the sameborrow-and-map preamble at 22 dynasm, 14 cranelift and 6 wasm sites. Each
backend now reaches the box through one private
gc_boxmodule exposingwith_ref/with_mut/with_reentrant_ref/with_mut_or_busy/present/store/clear.The runtime gate stays inside those accessors rather than becoming a
#[cfg].That is not a preference: a Cargo feature is per-crate, so a backend crate
cannot
#[cfg(feature = "gc_box")]on a feature ofmajit-gc— rustc answersexpected values for 'feature' are: 'dynasm'. Withoutmajit-gc/gc_boxthe callis a constant
false, so each accessor folds toNoneandnm -a target/release/pyre-dynasm | rg ACTIVE_GCfinds only the threemajit_gc::ACTIVE_GC_*globals — noDYNASM_ACTIVE_GC. The box is eliminated bythe optimizer instead of by conditional compilation, and the R4 endpoint holds
either way.
vstack mirror.
reconcile_vstack_at_boundarynow restores the saved operandboxes when a layout excursion returns to the
(py_pc, depth)its reorder regionarmed at, instead of reseeding from the virtualizable shadow — mid-expression
that shadow's stack region holds the NULLs the in-flight opcode's
popvalue_maybe_nonewrote. New fixturebench/synth/getframe_method_call_residual_body_once.pywith its three backendbaselines; it prints
(20000, 20000, 20005)without the change.Windows
os.fstat. It answered fromstd::fs::Metadata, which carries nofile index or volume serial, so
st_ino/st_devcame back zero andst_nlinkone while the same file reached by path reported its real identity. Now goes
through
rustpython_host_env::fileutils::fstat— theStatStructproducer thepath forms already use.
A defect this branch introduced, and fixed
The vstack restore above originally
returned as soon as it replayed the savedboxes. That skipped the function's shared tail, which is not only the three
position-update lines it duplicated but also the
reseed_vstack_from_shadowhole-fill. A slot the arm point already carried as
NONEstayedNONE,stack_syncomitted that operand, and the resumed frame read the slot below it:bench/synth/getframe_stored_fback_walk.pyraisedTypeError: 'range_iterator' object is not subscriptableon all three backends.Fixed by keeping the restore as the boundary's reconcile (
_ if restored) andletting the shared fill run over it.
Verification
cargo test --all --no-default-features --features dynasm— 7504 passed, 0 failedcargo fmt --all -- --check— cleanpyre/check.py— dynasm 405/406, cranelift 405/406, wasm 401/402The one remaining row is
synth/pypy_type_surface(
bridges_compiled 5 -> 102,guard_failures 1011 -> 20497), and it is notthis branch's:
dad2a722907); thisbranch touches neither,
origin/mainand re-extractingthe LLBC reproduces the row with byte-identical numbers,
It is an all-platform base regression, already root-caused elsewhere:
origin/main'sown CI run 31179140700 at
d936eb4be42reds it on ubuntu-24.04, windows-latestand macos-latest alike, and #999's own run is already red with the same numbers,
so the recorded
5/1011reproduced on no host including the one that wroteit. The cause is the
Cls.__name__fold guarding a null metaclass slot; the fixis
95ec7f2d474(PR #1106), which is not onmainyet. This row goes green whenthat lands. Not re-recorded —
5 -> 102bridges is a regression signature, notbenign drift.
— authored by Claude
Summary by CodeRabbit
Bug Fixes
fstat, including file identity, timestamps, and file-type information.Tests