GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Fix trace/profile events for generators and exceptions by youknowone · Pull Request #7268 · RustPython/RustPython Conversation
|
📝 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 |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin bdb |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
github-actions
Bot
commented
Feb 28, 2026
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
📦 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:
|
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
- 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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| // 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?; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 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=20
Repository: 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 5
Repository: 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?).
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
…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)
- 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)
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
1 participant
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.