X Tutup
Skip to content

Update bdb from v3.14.3 and fix trace_event#7255

Merged
youknowone merged 2 commits intoRustPython:mainfrom
youknowone:bdb
Feb 28, 2026
Merged

Update bdb from v3.14.3 and fix trace_event#7255
youknowone merged 2 commits intoRustPython:mainfrom
youknowone:bdb

Conversation

@youknowone
Copy link
Member

@youknowone youknowone commented Feb 28, 2026

Summary by CodeRabbit

  • Bug Fixes
    • Improved trace function return value handling to correctly propagate trace results to callers.
    • Enhanced trace event dispatch to properly update per-frame trace configuration based on function results.
    • Better alignment with CPython's trace protocol for call and return event handling.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 28, 2026

📝 Walkthrough

Walkthrough

Updated the trace event protocol in the VM to return the trace function's non-None return value as Option<PyObjectRef> instead of discarding it. Modified frame lifecycle trace dispatch to use these returned values to update per-frame trace references, and added error handling to clean up per-frame state on trace function failures. Lines changed: +65/-20 across two files.

Changes

Cohort / File(s) Summary
Trace Event Protocol Updates
crates/vm/src/protocol/callable.rs
Updated trace_event and _trace_event_inner to return PyResult<Option<PyObjectRef>> instead of PyResult<()>. Trace function return values are now captured and propagated; on trace errors, per-frame f_trace is cleared before error propagation; returns Ok(None) when tracing is inactive.
Frame Lifecycle Trace Dispatch
crates/vm/src/vm/mod.rs
Reworked trace dispatch in with_frame_exc and resume_gen_frame to update per-frame trace based on trace_event results. Return trace events now fire conditionally based on active tracing/profiling and frame trace state, aligning with CPython trace\_trampoline semantics.

Sequence Diagram

sequenceDiagram
    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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Poem

🐰 A trace returns, no longer lost in void,
The frame now listens to what it's been told,
Return events dance when tracing's alight,
Error cleanup springs—state set right,
Protocol whispers flow through the flight! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title partially relates to the changeset. It mentions 'fix trace_event' which directly corresponds to the trace_event method signature changes and behavioral fixes in the callable.rs and mod.rs files. However, the first part about 'Update bdb from v3.14.3' is not reflected in the provided file summaries, making the title somewhat incomplete in capturing the actual changes.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Contributor

Code has been automatically formatted

The code in this PR has been formatted using:

  • ruff format
    Please pull the latest changes before pushing again:
git pull origin bdb

@github-actions
Copy link
Contributor

github-actions bot commented Feb 28, 2026

📦 Library Dependencies

The following Lib/ modules were modified. Here are their dependencies:

Error: 'frozenset' object has no attribute 'discard'

Legend:

  • [+] path exists in CPython
  • [x] up-to-date, [ ] outdated

@youknowone youknowone marked this pull request as ready for review February 28, 2026 03:09
CPython Developers and others added 2 commits February 28, 2026 12:09
- 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
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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 to with_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_exc and resume_gen_frame can 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

📥 Commits

Reviewing files that changed from the base of the PR and between 81acb9b and 754fc85.

⛔ Files ignored due to path filters (3)
  • Lib/bdb.py is excluded by !Lib/**
  • Lib/test/test_bdb.py is excluded by !Lib/**
  • Lib/test/test_sys_settrace.py is excluded by !Lib/**
📒 Files selected for processing (2)
  • crates/vm/src/protocol/callable.rs
  • crates/vm/src/vm/mod.rs

@youknowone youknowone merged commit cc4a7bb into RustPython:main Feb 28, 2026
14 checks passed
@youknowone youknowone deleted the bdb branch February 28, 2026 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

X Tutup