Skip to content

Conversation

@3rdit
Copy link
Contributor

@3rdit 3rdit commented Dec 23, 2025

PR Overview

This PR adds conditional breakpoint support to the debugger, allowing a user to specify expressions that must evaluate to true for the breakpoint to actually trigger. This is implemented at the debugger core level (not adapter level), so it works the same across all adapters.

As discussed with @xusheng6:

The conditions for breakpoints are written using the expression parser (i.e. $rax == 0) making it the exact same for each debugger. The API now has:

  • C++: SetBreakpointCondition(), GetBreakpointCondition() on DebuggerController
  • Python: set_breakpoint_condition(), get_breakpoint_condition()

For example:

controller->SetBreakpointCondition(address, "$rax == 0");
std::string cond = controller->GetBreakpointCondition(address);
controller.set_breakpoint_condition(addr, "$rbx > 100")
cond = controller.get_breakpoint_condition(addr)

In the breakpoint widget, there's a new "Condition" column that can be set by right-clicking and pressing "Edit Condition...", this works in the widget and disassembly view.

image

The condition is also persistent across sessions (saved to the binary view metadata).

Upcoming Changes

  • Condition validation on the input dialog; the idea being to stop users from entering invalid expressions (this is tricky to add due to the way the expression parser works, not going to do this)
  • Possibility of adding hit counts, e.g., break after N hits
  • It makes sense to have some sort of unit testing for this. I reviewed the unit testing and noticed how breakpoints get tested in debugger_test.py, it would be trivial to add a quick unit test for conditions.
  • Adding callbacks for conditional breakpoints being hit (Support callback when a breakpoint is hit #422). That may be out of scope for this PR albeit and @xusheng6 will need to advise.

This PR will close #93 and maybe close #422 (if the feature above is approved to be added in this PR).

@3rdit 3rdit marked this pull request as draft December 24, 2025 00:40
@xusheng6
Copy link
Member

Hi @3rdit I will review this ASAP and let you know what I think

For context, I am working on a different PR that adds hardware breakpoint #53. And in light of the hardware breakpoint, I plan to make another large refactor on the breakpoint handling, e.g., the added breakpoints will be referred to by their index, not their address, etc.

This would mean some non-trivial merge conflict in the future. However, I hope to minimize the frictions for you as you are doing these all for free. I will most likely review and merge your PR first, and brace for the merge conflicts on my branch. Hopefully that will be manageable

DebuggerState* m_state;
std::vector<ModuleNameAndOffset> m_breakpoints;
std::map<ModuleNameAndOffset, bool> m_enabledState;
std::map<ModuleNameAndOffset, std::string> m_conditions;
Copy link
Member

@xusheng6 xusheng6 Dec 25, 2025

Choose a reason for hiding this comment

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

I am not a big fan of m_conditions here. I know you are copying the m_enabledState pattern, but that is really just a quick and dirty way to add "enable/disable" breakpoint into the product without a full refactor (a refactor on the breakpoint is already planned, but we wanted to slip that feature into the debugger before the refactor lands).

Ideally we would want something like this

  struct Breakpoint {
      ModuleNameAndOffset address;
      bool enabled = true;
      std::string condition;
  };

which is apparently better in terms of how to keep things more organized. I will make a decision on whether or not to integrate the refactor with this PR

Copy link
Contributor Author

@3rdit 3rdit Dec 25, 2025

Choose a reason for hiding this comment

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

I like this idea. I've followed your code patterns to keep it all consistent.

I've refactored the three separate variables into a BreakpointEntry struct (named to avoid a conflict with the enum) stored in a single vector. FindBreakpoint will now return an iterator directly to avoid redundant lookups when modifying or erasing entries. This reduces complexity in the lookups/logic and should make your planned refactor easier to do.

See: 9eb418b and b322922

@xusheng6 xusheng6 marked this pull request as ready for review December 25, 2025 10:55
@xusheng6
Copy link
Member

@3rdit I have a look at the code today and it looks good to me in general. I also did functionality tests and it works like a charm. I will see how the unit test goes

Most likely I will take over this PR and make some changes to it, and then merge it. I will keep you posted. Thanks again for the contribution!

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds conditional breakpoint support to the debugger, enabling users to specify expressions (e.g., $rax == 0x1234) that must evaluate to true for a breakpoint to trigger. The implementation is at the core debugger level and works consistently across all debug adapters. Conditions are evaluated using Binary Ninja's expression parser and persist across sessions through metadata storage.

Key changes:

  • Core breakpoint data structure extended with condition field and evaluation logic
  • C++, C FFI, and Python APIs provide get/set methods for breakpoint conditions
  • UI enhancements include a new "Condition" column in the breakpoints widget and "Edit Condition..." context menu action
  • Comprehensive unit tests validate condition setting, getting, and clearing functionality

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
core/debuggerstate.h Introduces BreakpointEntry struct with condition field and declares condition management methods
core/debuggerstate.cpp Implements condition storage, retrieval, FindBreakpoint helper, metadata serialization/deserialization, and refactors breakpoint operations to use new structure
core/debuggercontroller.h Declares SetBreakpointCondition, GetBreakpointCondition, and EvaluateBreakpointCondition methods
core/debuggercontroller.cpp Implements condition evaluation logic that checks expressions when breakpoints are hit and continues execution if conditions are false
core/ffi.cpp Updates FFI layer to expose condition field in BNDebugBreakpoint and adds condition get/set functions
api/ffi.h Adds condition field to BNDebugBreakpoint struct and declares new condition-related FFI functions
api/debuggerapi.h Adds condition field to DebugBreakpoint struct and declares C++ API methods for condition management
api/debuggercontroller.cpp Implements C++ API wrappers for condition get/set operations
api/python/debuggercontroller.py Adds Python bindings for set_breakpoint_condition and get_breakpoint_condition with comprehensive docstrings
ui/uinotification.cpp Registers "Edit Condition..." menu action in debugger context menu
ui/ui.cpp Implements BreakpointEditConditionCallback and binds it to UI actions, handles new condition changed events
ui/breakpointswidget.h Adds m_condition field to BreakpointItem and ConditionColumn to widget model
ui/breakpointswidget.cpp Implements UI rendering for condition column, editCondition dialog, and updates BreakpointItem constructor and equality operator
ui/debuggerwidget.cpp Handles new breakpoint condition changed events to trigger UI updates
test/debugger_test.py Adds test_breakpoint_condition to verify condition setting, updating, clearing, and error cases

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 43 to 44
// Condition is not part of identity - same location = same breakpoint
return (m_enabled == other.enabled()) && (m_location == other.location()) && (m_address == other.address());
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

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

The comparison operator includes the enabled state in the equality check, which means two breakpoints at the same location with different enabled states would be considered different. This contradicts the comment on line 43 which states "Condition is not part of identity - same location = same breakpoint". The enabled state should not be part of the identity comparison either, similar to how condition is excluded.

Suggested change
// Condition is not part of identity - same location = same breakpoint
return (m_enabled == other.enabled()) && (m_location == other.location()) && (m_address == other.address());
// Condition and enabled state are not part of identity - same location/address = same breakpoint
return (m_location == other.location()) && (m_address == other.address());

Copilot uses AI. Check for mistakes.
@xusheng6 xusheng6 merged commit a0ec8c5 into Vector35:dev Dec 26, 2025
1 check passed
@xusheng6
Copy link
Member

I have merged the PR, thanks a lot for the contribution! @3rdit

@xusheng6
Copy link
Member

Reopening this due to a few issues I just identified:

  1. It does not work when we step onto an instruciton with conditions on it. Normally we would expect it to stop, but it will only stop when the condition is true. This is largely because LLDB (incorrectly) reports the stop reason as Breakpoint than SingleStep. We need to find a way to workaround it
  2. This may behave badly when the user is stepping on the ILs
  3. We should probably ship it along side: Remove the $ before the register and module names added into the expression parser #942

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.

Support callback when a breakpoint is hit Conditional breakpoints

2 participants