Fix trace/profile events for generators and exceptions#7268
Merged
youknowone merged 1 commit intoRustPython:mainfrom Mar 1, 2026
Merged
Fix trace/profile events for generators and exceptions#7268youknowone merged 1 commit intoRustPython:mainfrom
youknowone merged 1 commit intoRustPython:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughIntroduces 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. Comment |
Contributor
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin bdb |
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:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify 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
⛔ Files ignored due to path filters (2)
Lib/test/test_bdb.pyis excluded by!Lib/**Lib/test/test_sys_setprofile.pyis excluded by!Lib/**
📒 Files selected for processing (5)
.cspell.dict/cpython.txtcrates/vm/src/frame.rscrates/vm/src/protocol/callable.rscrates/vm/src/stdlib/sys/monitoring.rscrates/vm/src/vm/mod.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)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/vm/src/frame.rs (1)
2217-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
ResumeandInstrumentedResume.♻️ 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 Agents
Verify 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.
🤖 Prompt for all review comments with AI agents
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
⛔ Files ignored due to path filters (2)
Lib/test/test_bdb.pyis excluded by!Lib/**Lib/test/test_sys_setprofile.pyis excluded by!Lib/**
📒 Files selected for processing (5)
.cspell.dict/cpython.txtcrates/vm/src/frame.rscrates/vm/src/protocol/callable.rscrates/vm/src/stdlib/sys/monitoring.rscrates/vm/src/vm/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- .cspell.dict/cpython.txt
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?; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 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":argis the value the frame is returning, orNoneif 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":argis 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 Agents
Verify 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?).
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.
Summary by CodeRabbit
New Features
Improvements / Bug Fixes
Refactor