sys: root the audit emit's livevars, and land the two _getframe commits #1112 merged without - #1132
Conversation
WalkthroughThe interpreter now roots frames, audit events, arguments, and hooks across collection points. Frame lookup delays forcing until globals are read. JIT diagnostic counting and benchmark documentation also receive targeted updates. ChangesFrame and audit rooting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PythonProgram
participant sys_audit
participant AuditDispatch
participant AuditHook
PythonProgram->>sys_audit: submit event and arguments
sys_audit->>AuditDispatch: create rooted event and arguments
AuditDispatch->>AuditHook: invoke rooted hook
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 |
![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
🤖 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-interpreter/src/module/sys/vm.rs`:
- Around line 2584-2599: Run and report both cargo check and cargo test with the
dynasm feature, plus all eight benchmarks because the JIT files changed. Include
the results in the PR description and explain any benchmark regressions without
reverting parity-correct changes.
- Around line 2603-2604: Change the audit-hook function’s holder parameter from
&AuditHolder to *const AuditHolder so no shared reference remains live during
raw writes. Within the function, dereference the pointer only as needed to call
publish_roots on hooks_w and copy hook_count before any hook can mutate the
holder through sys.addaudithook. Update affected call sites to pass the raw
pointer while preserving existing hook behavior.
- Around line 2736-2740: Update sys_audit to call split_builtin_kwargs(args)
before pinning or constructing args_w, and reject any real keyword arguments
while retaining the positional arguments only. Build args_w from the stripped
argument slice so the trailing __pyre_kw__ marker is never forwarded as an audit
argument.
🪄 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: c85b0c89-23ed-4670-8c3c-6feb63330026
📒 Files selected for processing (6)
pyre/bench/synth/getframe_inline_subwalk_multiframe.pypyre/bench/synth/getframe_while_escaping_read_frame_identity.pypyre/bench/synth/sys_audit_hooks.pypyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/state.rs
| // `pin_root` copies the pointer into a shadow-stack slot and the collector | ||
| // rewrites THAT slot, never the local it was copied from. So every value | ||
| // still needed after one of the app-level calls below is reached through | ||
| // its slot index, the way `baseobjspace::isinstance` does it. | ||
| // | ||
| // The event, the arguments and the hook set are one livevar set spanning | ||
| // three slices, so they are published together and normalized once | ||
| // (`gc_roots::pin_roots`): a per-value pin queries the collector after the | ||
| // first write, which would let a foreign collection run while the values | ||
| // behind it were still invisible to it. | ||
| // | ||
| // A hook may install another hook, and upstream's list is replaced rather | ||
| // than appended to, so an in-flight trigger keeps iterating the set it | ||
| // started with. Snapshotting into pinned roots reproduces that and keeps | ||
| // every callable forwarded across the calls below. | ||
| let hooks_w = holder.hooks_w.clone(); | ||
| for &w_hook in &hooks_w { | ||
| pyre_object::gc_roots::pin_root(w_hook); | ||
| } | ||
| // started with. The published slots ARE that snapshot — they keep every | ||
| // callable forwarded across the calls below and are unaffected by a | ||
| // replacement of `holder.hooks_w`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Report the required build and benchmark runs.
The PR description reports cargo fmt --check and the wasm synthetic suite. It does not report cargo check and cargo test with --features dynasm. The stack also changes pyre/pyre-jit-trace/src/state.rs and pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs, so the eight-benchmark run applies. Add both results, and explain any regression instead of reverting parity-correct code.
As per coding guidelines: "Before committing, run cargo check and cargo test with --features dynasm; after JIT changes, run all eight benchmarks and explain regressions rather than automatically reverting parity-correct code."
🤖 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-interpreter/src/module/sys/vm.rs` around lines 2584 - 2599, Run and
report both cargo check and cargo test with the dynasm feature, plus all eight
benchmarks because the JIT files changed. Include the results in the PR
description and explain any benchmark regressions without reverting
parity-correct changes.
Source: Coding guidelines
| let hooks_slot = pyre_object::gc_roots::publish_roots(&holder.hooks_w); | ||
| let hook_count = holder.hooks_w.len(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Consider taking the holder as a raw pointer instead of &AuditHolder.
The holder shared reference stays live for the whole function. An audit hook can call sys.addaudithook, which writes (*holder).hooks_w = next through a raw pointer at line 2789 while this shared reference is alive. The changed code no longer reads holder after line 2604, so behavior is correct, but the overlapping shared reference and raw write is an aliasing violation under Stacked Borrows. Copy hook_count and publish the hooks, then drop the reference by taking *const AuditHolder in the signature.
🤖 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-interpreter/src/module/sys/vm.rs` around lines 2603 - 2604, Change
the audit-hook function’s holder parameter from &AuditHolder to *const
AuditHolder so no shared reference remains live during raw writes. Within the
function, dereference the pointer only as needed to call publish_roots on
hooks_w and copy hook_count before any hook can mutate the holder through
sys.addaudithook. Update affected call sites to pass the raw pointer while
preserving existing hook behavior.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 9f2df15). 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
|
… top `_getframemodulename` forced `topframeref` before walking and then read `w_globals` off the frame the walk ends at, unforced. `w_globals` is one of the six fields `interp_jit.py:25-30` declares virtualizable, so the read is the consumer that needs the force; forcing at the walk instead escapes the traced virtualizable and `vable_after_residual_call` aborts the trace, which is the shape `executioncontext.rs force_frame` documents. Measured on a three-deep call chain over 100000 iterations: `abort: vable escape` goes 5 -> 0 at depths 1, 2 and 3, and stays 5 at depth 0, where the frame being read is the live portal virtualizable. Reported module names are unchanged at depths 0-4 and match a `PYRE_NO_JIT=1` run. Assisted-by: Claude
… with None
`pin_root` writes a shadow-stack slot and a moving collection rewrites that slot,
not the local it was copied from, so every value the audit path still needs after
an allocation is now read back through its slot index:
* `trigger_audit_events` publishes the event, the caller's arguments and the
hook set with `publish_roots` and normalizes the whole range once, rather
than calling `pin_root` per value — a per-value pin queries the collector
after the first write, leaving the values behind it invisible to a foreign
collection (`gc_roots::pin_roots`). The tuple, the event and each hook then
come out of `shadow_stack_get` at every use.
* `audit` roots `args_w` before `w_str_new` and emits from the reloaded
values; `sys_audit` does the same around `w_str_from_wtf8`. The slice a
builtin receives is `call_function_impl_result`'s reloaded copy on the Rust
stack, which no collection rewrites.
* `getframe` roots the frame it is about to return across the emit and reads
it back, and enters that bracket only when `audit_hooks_armed()`.
* `sys_addaudithook` pins before the `sys.addaudithook` event instead of
after it, and takes the hook out of its slot when appending; the borrow of
`holder.hooks_w` is scoped so none is live across the `Vec` allocation.
The `__cantrace__` lookup moves from `baseobjspace::findattr` to
`findattr_result`. `space.findattr` (`baseobjspace.py:881-888`) returns `None`
for any non-async error out of the lookup, so a hook whose `__cantrace__`
descriptor raises is treated as not having one; the bare `findattr` panics on
those instead. SystemExit is the arm that travels out.
Assisted-by: Claude
The note still claimed "the undo stays armed so the legacy replay re-enters the pre-flush frame" after the deferred arm was withdrawn. The capture does stay in `ESCAPE_FLUSH_UNDO`, but nothing consumes it from this arm: the committed-pc walk-end leg is gated on a pc this arm never sets and the deferred leg on a flag it never arms, so `capture_escape_flush_undo` supersedes it on the next force or the walk-start reset drops it. Assisted-by: Claude
`note_inline_subwalk_end` bumped 59 before `try_driver_pair`, so a driverless call counted a close that never pushed an entry. It now bumps where `note_inline_subwalk_start` bumps 58 — both counters measure an appended entry, and an unclosed entry reads as `ptp_push != ptp_pop`. Assisted-by: Claude
`sys_audit_hooks` records why the non-RuntimeError and RuntimeError-subclass arms of `error_is_runtime_error` are not exercised and cannot be from app level: a hook is installed for the life of the interpreter and the first one that raises masks every hook behind it, and the upstream facility for clearing the set between cases (`__pypy__._testing_clear_audithooks`, `interp_magic.py:292`) refuses to run once translated. `getframe_while_escaping_read_frame_identity` states the two counts separately — the multi-frame adoption count is unmoved at 10, and it is the single-frame count that went 5 -> 0 alongside `part_a`'s loop compiling. `getframe_inline_subwalk_multiframe` carries `# noqa: B018` on the standalone `sys._getframe(0).f_locals`, whose read IS the force under test. Assisted-by: Claude
The refusal predicate widened from `RuntimeError` to `Exception` and was renamed, so the header named a function and a class that no longer decide anything here. It now states which arm the app-level refusal below takes — a raise from app code carries an exception object, so the isinstance arm answers and the `PyErrorKind` fallback behind it is for errors that never materialised one. Assisted-by: Claude
adfe74d to
9f2df15
Compare
![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/9f2df156936ee52b79db965fbb8bd9645e04fd36/pyre-interpreter/src/module/sys/vm.rs#L2704
When an audit hook's __cantrace__ descriptor raises KeyboardInterrupt, PyError::from_exc_object represents it with PyErrorKind::RuntimeError while retaining the actual exception object, so the SystemExit-only guard misses it and this catch-all silently treats the attribute as absent. PyPy's space.findattr re-raises both SystemExit and KeyboardInterrupt, meaning sys.audit should propagate the interruption rather than proceeding to invoke the hook; check the materialized exception object with the shared async-exception logic before swallowing the error.
AGENTS.md reference: AGENTS.md:L231-L233
ℹ️ 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".
|
CI attribution for this PR — every failing row is inherited from main or a measurement artifact, verified against
The whole host was ~20% slower and dynasm startup went 0.056s -> 0.080s, while the fixture's total stayed 0.07-0.08s on both. No jitstats or ceiling has been re-recorded in this PR. — commented by Claude |
1 participant
Follow-up to #1112, rebased onto current main.
#1112 was squash-merged at
d2f4ce1522e, which predates part of the branch, soone commit here is work that PR was supposed to carry and did not. The rest
answer the review comments the merged PR collected.
Not in main despite #1112
sys: force the frame_getframemodulenamereads instead of the stack top—main still forces
topframerefat the walk START and readsw_globalsoff theframe the walk ENDS at without forcing it.
w_globalsis one of the six fieldsinterp_jit.py:25-30declares virtualizable, so the force belongs at theconsumer; forcing the walk escapes the traced virtualizable and
vable_after_residual_callaborts with ABORT_ESCAPE. Measured on thegetframe_*corpus: vable-escape aborts 5 → 0 for depth ≥ 1.The branch's second unmerged commit — unwrap and rebuild
sys.audit's eventrather than forwarding the caller's object — has been dropped: #1113 landed
the same round trip meanwhile (
str_utf8_w+w_str_new). See the note at thebottom for the one thing that commit did differently.
Review answers
sys: root the audit emit's livevars ...— Codex P1 (reload pinned auditobjects after collection points) and CodeRabbit's two
vm.rsfindings. Everyvalue the audit path needs after an allocation now comes back through its
shadow-stack slot;
trigger_audit_eventspublishes the event, the argumentsand the hook set in one phase and normalizes once, rather than
pin_rootpervalue (
gc_roots::pin_rootsdocuments why the per-value form reopens thewindow it exists to close).
auditandsys_auditroot their arguments infront of the event wrap,
getframeroots the frame it returns across theemit, and
sys_addaudithookpins before thesys.addaudithookevent ratherthan after. The commit also moves the
__cantrace__lookup tofindattr_result:space.findattr(baseobjspace.py:881-888) answersNonefor any non-async error, where the bare
findattrpanicked.jit: bump the inline-subwalk close counter below the driver lookup—CodeRabbit on
state.rs.bench: note which audit-hook arms are unreachable ...— CodeRabbit's threefixture comments. The requested extra coverage for the propagating arm of the
refusal predicate is not added, and the header now records why it cannot
be: a hook is installed for the life of the interpreter and the first one that
raises masks every hook behind it, and upstream's own facility for clearing the
set between cases (
__pypy__._testing_clear_audithooks,interp_magic.py:292) refuses to run once translated.bench: retarget the audit-hook note aterror_is_exception`` — the sameheader cited
error_is_runtime_errorand `RuntimeError`, both of which coroutine, generator, io, exceptions and CPython allocation/layout metadata; five jit fixes #1126replaced (`Exception`, renamed predicate).
One review finding deliberately not acted on
Codex P1 on
residual_call.rs:1356asks formark_escape_flush_undo_pending()back in the locals-only escape-flush arm. That arm was removed by
jit: drop the locals-region leg's deferred escape-flush undo armon measurement:the
value-stack underflowit was added for came from an A/B run on stale LLBC,and re-running the reproducing arm on artefacts that pass
PYRE_LLBC_STRICT=1gives 0/10 panics on cranelift with the whole synthetic suite and every recorded
counter unmoved. Restoring the call would revert an executed result on a static
argument, so this PR fixes the comment instead — it still claimed the capture
was consumed by a legacy replay, which is what stopped being true.
One open question, left as main has it
sys_auditunwraps withstr_utf8_w, and #1113's comment states that choice isdeliberate: "an event name holding a lone surrogate is a
UnicodeEncodeErroratthe call". The dropped commit used the surrogate-tolerant read instead. Measured:
Upstream's spec is
@unwrap_spec(event="text"), i.e.space.text_w, and thePyPy oracle accepts the name. This PR does not change it — reversing a
documented, deliberate decision is not something a conflict resolution should
do silently. Raising it here so the call is made on the evidence rather than by
whichever branch merged last.
Verification
PYRE_LLBC_STRICT=1 python3 pyre/check.py --synthetic-only:The single failure is
synth/list_pop_append, an inherited base red: the CI runfor #1119's own merge commit (
1a63e36e37a, run31313268003,headShaequal)already reads
FAIL dynasm 59.8x > gate 22x/FAIL cranelift 68.9x > gate 22xon the fixture that same commit introduced the 22x ceiling for, and every main
run since carries it.
Output parity against
pypy3re-checked forsys_audit_hooks,getframe_audit_event,getframe_inline_subwalk_multiframeandgetframe_while_escaping_read_frame_identity.cargo fmt --checkclean.🤖 Generated with Claude Code