Update bdb from v3.14.3 and fix trace_event#7255
Conversation
📝 WalkthroughWalkthroughUpdated the trace event protocol in the VM to return the trace function's non-None return value as Changes
Sequence DiagramsequenceDiagram
participant Caller
participant VM as VM Frame Lifecycle
participant TraceEvent as trace_event()
participant TraceFunc as Trace Function
participant Frame as Per-Frame State
rect rgba(200, 150, 100, 0.5)
Note over Caller,Frame: New Behavior: Trace Return Value Propagation
Caller->>VM: call_event (entering frame)
VM->>TraceEvent: trace_event(CallEvent, arg)
TraceEvent->>TraceFunc: invoke trace function
TraceFunc-->>TraceEvent: returns Some(new_trace_fn) or None
TraceEvent-->>VM: Ok(Some(new_trace_fn)) or Ok(None)
alt Has return value
VM->>Frame: update frame.trace = new_trace_fn
end
Caller->>VM: return_event (exiting frame)
alt frame.trace OR profiling active
VM->>TraceEvent: trace_event(ReturnEvent, return_value)
TraceEvent->>TraceFunc: invoke trace function
TraceFunc-->>TraceEvent: returns result
TraceEvent-->>VM: Ok(Some(...)) or Ok(None)
else skip return event
Note over VM: Return event not fired
end
end
rect rgba(100, 150, 200, 0.5)
Note over Caller,Frame: Error Handling with State Cleanup
TraceEvent->>TraceFunc: invoke trace function
TraceFunc-->>TraceEvent: error
TraceEvent->>Frame: clear f_trace (cleanup)
TraceEvent-->>VM: Err(exception)
VM-->>Caller: propagate error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 |
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin bdb |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: Error: 'frozenset' object has no attribute 'discard' Legend:
|
- Return trace function's return value from trace_event() to support per-frame f_trace assignment - Match CPython's trace_trampoline: set f_trace from call event return value, clear on error - Fire return event only when frame is traced or profiled - Remove expectedFailure from passing bdb/settrace tests
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/vm/src/vm/mod.rs (1)
1163-1181: Consider extracting duplicate trace dispatch logic.The trace handling in
resume_gen_frame(lines 1163-1181) is nearly identical towith_frame_exc(lines 1094-1111). Per coding guidelines, consider extracting the common trace dispatch logic into a helper to reduce duplication.♻️ Sketch of potential helper extraction
// Helper to handle trace dispatch around frame execution fn dispatch_traced_frame<R>( &self, frame: &FrameRef, f: impl FnOnce() -> PyResult<R>, ) -> PyResult<R> { match self.trace_event(TraceEvent::Call, None) { Ok(trace_result) => { if let Some(local_trace) = trace_result { *frame.trace.lock() = local_trace; } let result = f(); if result.is_ok() && self.use_tracing.get() && (!self.is_none(&frame.trace.lock()) || !self.is_none(&self.profile_func.borrow())) { let _ = self.trace_event(TraceEvent::Return, None); } result } Err(e) => Err(e), } }Then both
with_frame_excandresume_gen_framecan call this helper.🤖 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 - 1181, Extract the duplicated trace dispatch logic into a helper method (e.g., dispatch_traced_frame) and replace the in-place logic in both resume_gen_frame and with_frame_exc with calls to that helper; the helper should call self.trace_event(TraceEvent::Call, None), apply any returned local trace into frame.trace.lock(), invoke the provided closure to execute the frame body, and if the closure returns Ok and tracing/profile is enabled (check self.use_tracing.get(), frame.trace.lock(), and self.profile_func.borrow()), call self.trace_event(TraceEvent::Return, None) before returning the closure result; ensure the helper accepts &self, frame: &FrameRef, and a closure FnOnce() -> PyResult<T> so both resume_gen_frame and with_frame_exc can pass their existing frame-execution closures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/vm/src/vm/mod.rs`:
- Around line 1163-1181: Extract the duplicated trace dispatch logic into a
helper method (e.g., dispatch_traced_frame) and replace the in-place logic in
both resume_gen_frame and with_frame_exc with calls to that helper; the helper
should call self.trace_event(TraceEvent::Call, None), apply any returned local
trace into frame.trace.lock(), invoke the provided closure to execute the frame
body, and if the closure returns Ok and tracing/profile is enabled (check
self.use_tracing.get(), frame.trace.lock(), and self.profile_func.borrow()),
call self.trace_event(TraceEvent::Return, None) before returning the closure
result; ensure the helper accepts &self, frame: &FrameRef, and a closure
FnOnce() -> PyResult<T> so both resume_gen_frame and with_frame_exc can pass
their existing frame-execution closures.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Lib/bdb.pyis excluded by!Lib/**Lib/test/test_bdb.pyis excluded by!Lib/**Lib/test/test_sys_settrace.pyis excluded by!Lib/**
📒 Files selected for processing (2)
crates/vm/src/protocol/callable.rscrates/vm/src/vm/mod.rs
Summary by CodeRabbit