Fix trace/profile events for generators and exceptions - #7268
Merged
youknowone merged 1 commit intoMar 1, 2026
Merged
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
📝 Walkthrough
WalkthroughIntroduces exception and opcode trace events, moves per-frame line tracking into FrameState, adds per-code event masking, centralizes call/return trace dispatch into a helper, and updates frame execution paths to use the new tracing and monitoring flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant VM
participant Frame
participant MonitoringState
participant TraceFunc
Caller->>VM: invoke frame execution
VM->>MonitoringState: events_for_code(code_id)
VM->>TraceFunc: call TraceFunc(frame, "call", ...)
TraceFunc-->>VM: optional per-frame trace object
VM->>Frame: execute body (may yield/raise)
Frame->>MonitoringState: consult per-frame events (opcode/line)
alt exception raised
Frame->>TraceFunc: call TraceFunc(frame, "exception", (type,value,traceback))
else normal return
VM->>TraceFunc: call TraceFunc(frame, "return", result)
end
VM-->>Caller: return/resume result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem🚥 Pre-merge checks | ✅ 3 ✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches 🧪 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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin bdb |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/bdb.py dependencies:
dependent tests: (1 tests)
[x] test: cpython/Lib/test/test_sys.py (TODO: 8) dependencies: dependent tests: (225 tests)
Legend:
|
Sorry, something went wrong.
youknowone
marked this pull request as ready for review
March 1, 2026 02:42
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agentsVerify each finding against the current code and only fix it if needed. Inline comments: In `@crates/vm/src/frame.rs`: - Around line 599-612: The opcode trace block fires without checking the frame's f_trace_opcodes flag; update the conditional that currently checks vm.use_tracing and self.object.trace.lock() to also require the frame's trace-opcode flag (e.g. self.object.trace_opcodes or equivalent field on the frame object) is true before calling vm.trace_event; specifically modify the if that excludes Instruction::Resume/InstrumentedResume/ExtendedArg so it additionally checks the frame-level f_trace_opcodes flag and only calls vm.trace_event(crate::protocol::TraceEvent::Opcode, None)? when that flag is set.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between b05e74b and 0a6c7c0.
⛔ Files ignored due to path filters (2)- Lib/test/test_bdb.py is excluded by !Lib/**
- Lib/test/test_sys_setprofile.py is excluded by !Lib/**
- .cspell.dict/cpython.txt
- crates/vm/src/frame.rs
- crates/vm/src/protocol/callable.rs
- crates/vm/src/stdlib/sys/monitoring.rs
- crates/vm/src/vm/mod.rs
Sorry, something went wrong.
Comment thread
crates/vm/src/frame.rs
- Add TraceEvent::Exception and Opcode variants with profile filtering - Extract dispatch_traced_frame helper for Call/Return trace events - Fire exception trace on new raises, SEND StopIteration, FOR_ITER StopIteration - Fire opcode trace events gated by f_trace_opcodes - Move prev_line to FrameState for persistence across generator suspend/resume - Reset prev_line in gen_throw for correct LINE monitoring after yield - Add per-code event mask (events_for_code) to prevent unrelated code instrumentation - Remove expectedFailure markers from test_bdb (5) and test_sys_setprofile (14)
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)crates/vm/src/frame.rs (1)🤖 Prompt for all review comments with AI agents2217-2221: Extract duplicated stale-instrumentation refresh logic.
Line 2217 and Line 2597 duplicate the same re-instrumentation/version-sync block. Pulling that into one helper will reduce drift risk between Resume and InstrumentedResume.
♻️ Proposed refactor+ #[inline] + fn refresh_instrumentation_if_stale(&mut self, vm: &VirtualMachine) -> bool { + let global_ver = vm + .state + .instrumentation_version + .load(atomic::Ordering::Acquire); + let code_ver = self + .code + .instrumentation_version + .load(atomic::Ordering::Acquire); + if code_ver != global_ver { + let events = { + let state = vm.state.monitoring.lock(); + state.events_for_code(self.code.get_id()) + }; + monitoring::instrument_code(self.code, events); + self.code + .instrumentation_version + .store(global_ver, atomic::Ordering::Release); + self.update_lasti(|i| *i -= 1); + return true; + } + false + }- if code_ver != global_ver { - let events = { - let state = vm.state.monitoring.lock(); - state.events_for_code(self.code.get_id()) - }; - monitoring::instrument_code(self.code, events); - self.code - .instrumentation_version - .store(global_ver, atomic::Ordering::Release); - // Re-execute this instruction (it may now be INSTRUMENTED_RESUME) - self.update_lasti(|i| *i -= 1); - } + if self.refresh_instrumentation_if_stale(vm) { + // Re-execute this instruction (it may now be INSTRUMENTED_RESUME) + }- if code_ver != global_ver { - let events = { - let state = vm.state.monitoring.lock(); - state.events_for_code(self.code.get_id()) - }; - monitoring::instrument_code(self.code, events); - self.code - .instrumentation_version - .store(global_ver, atomic::Ordering::Release); - // Re-execute (may have been de-instrumented to base Resume) - self.update_lasti(|i| *i -= 1); - return Ok(None); - } + if self.refresh_instrumentation_if_stale(vm) { + // Re-execute (may have been de-instrumented to base Resume) + return Ok(None); + }As per coding guidelines, "When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code."
Also applies to: 2597-2600
🤖 Prompt for AI AgentsVerify each finding against the current code and only fix it if needed. In `@crates/vm/src/frame.rs` around lines 2217 - 2221, The duplicated re-instrumentation logic in Resume and InstrumentedResume (the block that locks vm.state.monitoring, calls state.events_for_code(self.code.get_id()), then calls monitoring::instrument_code(self.code, events)) should be extracted into a single helper (e.g., refresh_instrumentation or sync_instrumentation_for_code) to avoid drift; create a private function that takes &self (or &Code) and &VmState/monitoring handle as needed, move the lock+events_for_code call and the monitoring::instrument_code call into it, and replace the duplicated blocks in Resume and InstrumentedResume with a single call to this helper.
Verify each finding against the current code and only fix it if needed. Inline comments: In `@crates/vm/src/vm/mod.rs`: - Around line 1163-1173: The return trace currently always passes None to self.trace_event for TraceEvent::Return; instead, inspect the function outcome (the local variable result that holds Ok(R) or Err): when result is Ok(value) extract that return value and pass it as the arg to self.trace_event (so trace_event receives the actual return object), and when result is Err pass None as before; update the call site around the existing self.use_tracing check (references: self.use_tracing.get(), frame.trace.lock(), self.profile_func.borrow(), trace_event / TraceEvent::Return, ret_result) to build the correct arg from result before calling trace_event and then propagate ret_result? using the same error-handling pattern (ret_result?). --- Nitpick comments: In `@crates/vm/src/frame.rs`: - Around line 2217-2221: The duplicated re-instrumentation logic in Resume and InstrumentedResume (the block that locks vm.state.monitoring, calls state.events_for_code(self.code.get_id()), then calls monitoring::instrument_code(self.code, events)) should be extracted into a single helper (e.g., refresh_instrumentation or sync_instrumentation_for_code) to avoid drift; create a private function that takes &self (or &Code) and &VmState/monitoring handle as needed, move the lock+events_for_code call and the monitoring::instrument_code call into it, and replace the duplicated blocks in Resume and InstrumentedResume with a single call to this helper.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 0a6c7c0 and dbd7210.
⛔ Files ignored due to path filters (2)- Lib/test/test_bdb.py is excluded by !Lib/**
- Lib/test/test_sys_setprofile.py is excluded by !Lib/**
- .cspell.dict/cpython.txt
- crates/vm/src/frame.rs
- crates/vm/src/protocol/callable.rs
- crates/vm/src/stdlib/sys/monitoring.rs
- crates/vm/src/vm/mod.rs
- .cspell.dict/cpython.txt
Sorry, something went wrong.
Comment thread
crates/vm/src/vm/mod.rs
Comment on lines
+1163
to
+1173
| // Fire 'return' event if frame is being traced or profiled. | ||
| // PY_UNWIND fires PyTrace_RETURN with arg=None — so we fire for | ||
| // both Ok and Err, matching `call_trace_protected` behavior. | ||
| if self.use_tracing.get() | ||
| && (!self.is_none(&frame.trace.lock()) || !self.is_none(&self.profile_func.borrow())) | ||
| { | ||
| let ret_result = self.trace_event(TraceEvent::Return, None); | ||
| // call_trace_protected: if trace function raises, its error | ||
| // replaces the original exception. | ||
| ret_result?; | ||
| } |
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
🧩 Analysis chain🌐 Web query:
CPython sys.settrace protocol return event arg value successful return exception
💡 Result:
In CPython, a sys.settrace callback is called as tracefunc(frame, event, arg) where arg depends on event:
- event == "return": arg is the value the frame is returning, or None if the frame is being exited because an exception is being raised. Also, the trace function’s return value is ignored for "return" events. [1]
- event == "exception": arg is a 3-tuple (exception, value, traceback), and the trace function’s return value does control the subsequent local trace function. [1]
(You may see "exception" events at multiple stack levels as the exception propagates.) [1]
How to tell “successful return” vs “exceptional exit”
Because "return" uses arg == None both for (a) a real return None and (b) exiting due to an exception, you generally distinguish them by tracking whether you previously received an "exception" event for that same frame before its "return" event. The docs explicitly state the "return"-arg rule but do not provide a separate “success/failure” flag. [1]
Source
[1] Python sys.settrace documentation (event meanings and arg values). (docs.python.org)
Citations:
🏁 Script executed:
# First, let's examine the code at the specified location
cat -n crates/vm/src/vm/mod.rs | sed -n '1150,1180p'Repository: RustPython/RustPython
Length of output: 1452
🏁 Script executed:
# Let's also check the TraceEvent type definition and all usages of TraceEvent::Return
rg "TraceEvent::" -A 3 -B 3 --max-count=20Repository: RustPython/RustPython
Length of output: 4802
🏁 Script executed:
# Check the dispatch_traced_frame function to understand the full context
rg "dispatch_traced_frame" -A 25 -B 5Repository: RustPython/RustPython
Length of output: 4903
Return trace event must pass the actual return value on successful function return, not always None.
According to CPython's sys.settrace protocol, the "return" event receives arg as either the returned value (on normal return) or None (if exiting due to exception). The current code at line 1169 always passes None regardless of success or failure.
This breaks compatibility with debuggers and profilers that expect to inspect the return value. Fix by extracting the return value from successful results (when result is Ok(R)) and passing it to trace_event, while reserving None for exception cases (when result is Err).
🤖 Prompt for AI AgentsVerify each finding against the current code and only fix it if needed. In `@crates/vm/src/vm/mod.rs` around lines 1163 - 1173, The return trace currently always passes None to self.trace_event for TraceEvent::Return; instead, inspect the function outcome (the local variable result that holds Ok(R) or Err): when result is Ok(value) extract that return value and pass it as the arg to self.trace_event (so trace_event receives the actual return object), and when result is Err pass None as before; update the call site around the existing self.use_tracing check (references: self.use_tracing.get(), frame.trace.lock(), self.profile_func.borrow(), trace_event / TraceEvent::Return, ret_result) to build the correct arg from result before calling trace_event and then propagate ret_result? using the same error-handling pattern (ret_result?).
Sorry, something went wrong.
youknowone
added a commit
to youknowone/RustPython
that referenced
this pull request
Mar 22, 2026
…tPython#7268) - Add TraceEvent::Exception and Opcode variants with profile filtering - Extract dispatch_traced_frame helper for Call/Return trace events - Fire exception trace on new raises, SEND StopIteration, FOR_ITER StopIteration - Fire opcode trace events gated by f_trace_opcodes - Move prev_line to FrameState for persistence across generator suspend/resume - Reset prev_line in gen_throw for correct LINE monitoring after yield - Add per-code event mask (events_for_code) to prevent unrelated code instrumentation - Remove expectedFailure markers from test_bdb (5) and test_sys_setprofile (14)
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
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.
Summary by CodeRabbit
New Features
Improvements / Bug Fixes
Refactor