Breakpoint improvements #1809
Breakpoint improvements #1809
Conversation
rocketz
commented
Nov 24, 2024
rocketz
commented
- Cleaned up UI. New breakpoints are now handled in a pop-up dialog
- Breakpoint are now shown in a table.
- You can easily see and edit breakpoint names in the table
- Clicking on the address will take you there (in asm or memory view)
- Enable all/disable all/delete all buttons
- Context menu in assembly view to quickly create read/write breakpoints on the address the instruction reads/writes
- New breakpoint types:
- On changes. WIll only break when the value is written to with a change. The new value written is the new value tested against
- Lower than
- Higher than
- Equal
- Range
- When a code breakpoint is hit it is highlighted in red in breakpoint table
- Cleaned up UI. New breakpoints are now handled in a pop-up dialog - Breakpoint are now shown in a table. - You can easily see and edit breakpoint names in the table - Clicking on the address will take you there (in asm or memory view) - Enable all/disable all/delete all buttons - Context menu in assembly view to quickly create read/write breakpoints on the address the instruction reads/writes - New breakpoint types: * On changes. WIll only break when the value is written to with a change. The new value written is the new value tested against * Lower than * Higher than * Equal * Range - When a code breakpoint is hit it is highlighted in red in breakpoint table
| #include "fmt/format.h" | ||
| #include "imgui.h" | ||
| #include "support/imgui-helpers.h" | ||
|
|
||
| static ImVec4 s_currentColor = ImColor(0xff, 0xeb, 0x3b); | ||
| // Note: We ignore SWL and SWR |
There was a problem hiding this comment.
This is a bit of a blind spot overall. It feels like we can properly support swl/swr. I'll try and work this out a bit.
| MemVal final = {}; | ||
| switch (width) { | ||
| case 1: { | ||
| uint8_t val = PCSX::g_emulator->m_mem->read8(addr); |
There was a problem hiding this comment.
We shouldn't use read8/read16/read32 for the purpose of the debugger, as it may advance internal state machines on things like the cd-rom controller or the pads. This one's a bit difficult.
There was a problem hiding this comment.
oh I see.. maybe we need a more direct access?
src/gui/widgets/breakpoints.cc
Outdated
| doBreak = curVal != self->conditionData(); | ||
| if (doBreak) | ||
| { | ||
| // TODO: can't update since 'self' is const |
There was a problem hiding this comment.
Could you fill in what'd need to be updated still? It's probably doable to make everything work properly still. It may be fine to also change the upper API generally speaking, but I'd rather we discuss this a bit. We could also use mutable within reason.
There was a problem hiding this comment.
I actually have this working, but I messed up with some stashes. Will bring those changes in.
src/gui/widgets/breakpoints.cc
Outdated
| // than that they want to use the same label twice | ||
| m_bpLabelString[0] = 0; | ||
|
|
||
| static int breakCondition2 = 0; |
There was a problem hiding this comment.
Could we name this breakConditionImGuiValue instead or something? The 2 suffix doesn't really explain why it's there in the first place.
My pleasure!
No worries. You shouldn't waste valuable turkey-time on this :)
Cool. I'll fix a few things though. |
|
Updating from main as there was some CI breakages, and we can see better where we're at here. |
|
Right so there's still some breakages on Linux: src/gui/widgets/breakpoints.cc:242:32: error: format not a string literal and no format arguments [-Werror=format-security]Needs to use |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1809 +/- ##
==========================================
- Coverage 9.73% 9.19% -0.54%
==========================================
Files 451 467 +16
Lines 136014 144109 +8095
==========================================
+ Hits 13238 13250 +12
- Misses 122776 130859 +8083 ☔ View full report in Codecov by Sentry. |
|
Alright, let's try and get that in... |
WalkthroughThe pull request introduces enhancements to the debugging system in the PCSX emulator. The changes focus on expanding breakpoint functionality by adding a new Changes
Sequence DiagramsequenceDiagram
participant User
participant GUI
participant Debug
participant Emulator
User->>GUI: Select Memory Address
GUI->>User: Show Breakpoint Options
User->>GUI: Choose Breakpoint Condition
GUI->>Debug: addBreakpoint(address, type, size)
Debug->>Emulator: Set Breakpoint
Emulator-->>Debug: Breakpoint Registered
Debug-->>GUI: Confirm Breakpoint
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/gui/widgets/assembly.cc (1)
343-348: Creating memory breakpoints directly from the Assembly view.
This is a helpful addition for quick debugging. Consider eventually offering a condition selection (similar to code breakpoints) to match the newBreakpointConditionfeatures. Additionally, watch the complexity of this function as it grows.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 343-348: ❌ New issue: Complex Method
PCSX::Widgets::Assembly::addMemoryEditorContext has a cyclomatic complexity of 9, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/core/debug.h(5 hunks)src/gui/widgets/assembly.cc(1 hunks)src/gui/widgets/breakpoints.cc(1 hunks)src/gui/widgets/breakpoints.h(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
src/gui/widgets/assembly.cc
[warning] 343-348: ❌ New issue: Complex Method
PCSX::Widgets::Assembly::addMemoryEditorContext has a cyclomatic complexity of 9, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
src/gui/widgets/breakpoints.cc
[warning] 94-425: ❌ Getting worse: Complex Method
PCSX::Widgets::Breakpoints::draw increases in cyclomatic complexity from 25 to 70, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[warning] 94-425: ❌ Getting worse: Bumpy Road Ahead
PCSX::Widgets::Breakpoints::draw increases from 5 to 12 logical blocks with deeply nested code, threshold is one single block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 94-425: ❌ New issue: Deep, Nested Complexity
PCSX::Widgets::Breakpoints::draw has a nested complexity depth of 6, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
🔇 Additional comments (10)
src/gui/widgets/breakpoints.cc (5)
21-22: No issues found in the new includes.
Everything looks good and appears necessary for the features implemented.
27-34: Revisit partial store instructions (SWL/SWR).
This code continues to skip partial store instructions like SWL and SWR. As noted in a past review, proper handling of these instructions would be beneficial to accurately capture the value about to be written.
36-50: Function for condition names looks good.
The switch adequately covers all known breakpoint conditions, returning translated strings.
52-85: Use of read8/read16/read32 in debugging context may cause side effects.
Prior comments warned that these read methods can advance certain emulator internal states (e.g., CD-ROM, controllers). Consider adding a safer, direct-read mechanism if these side effects are undesirable in debugging scenarios.
88-89: Color constants defined.
No issues found. The naming is clear, and the colors are easily distinguishable.src/gui/widgets/breakpoints.h (1)
44-44: Verify the reduced buffer size for breakpoint addresses.
Changing from 20 to 9 bytes exactly accommodates up to 8 hex characters plus the null terminator. If the user ever attempts a prefix (e.g., "0x") or inputs more digits, it may cause partial or invalid reads. Confirm the UI’s text fields and error handling align with this constraint.src/core/debug.h (4)
68-68: Allowing a non-const Breakpoint pointer.
Switching to a non-const pointer is consistent with updatingconditionData()or other state within the invoker. Looks correct.
80-83: New getters and setters for condition and conditionData.
These are straightforward and align with the new breakpoint condition functionality.
101-102: Defaulting toBreakpointCondition::Always.
No issues here. Initializing the condition to Always is a sensible default.
165-165: Mass removal of breakpoints.
TheremoveAllBreakpoints()method is a logical addition to cleanly clear all breakpoints in one step.
| default: | ||
| case Debug::BreakpointCondition::Always: | ||
| break; | ||
| case Debug::BreakpointCondition::Equal: | ||
| conditionData = conditionVal; | ||
| break; | ||
| case Debug::BreakpointCondition::Less: | ||
| case Debug::BreakpointCondition::Greater: | ||
| case Debug::BreakpointCondition::Change: | ||
| conditionData = conditionVal; | ||
| break; | ||
| } | ||
|
|
||
| Debug::Breakpoint* bp = debugger->addBreakpoint(breakpointAddress, bpType, | ||
| (bpType == Debug::BreakpointType::Exec) ? 4 : actualWidth, _("GUI"), | ||
| m_bpLabelString, invoker); | ||
|
|
||
| bp->setCondition(breakCondition); | ||
| bp->setConditionData(conditionData); | ||
|
|
||
| // we clear the label string because it seems more likely that the user would forget to clear the | ||
| // field than that they want to use the same label twice | ||
| m_bpLabelString[0] = 0; | ||
| ImGui::CloseCurrentPopup(); | ||
| } | ||
| } | ||
|
|
||
| ImGui::EndPopup(); | ||
| } | ||
| ImGui::End(); | ||
|
|
||
| if (!editorToOpen.empty()) { | ||
| ImGui::OpenPopup(editorToOpen.c_str()); | ||
| if (!tree.empty()) { | ||
| ImGui::SameLine(); | ||
| if (ImGui::Button(_("Activate All"))) { | ||
| for (auto bp = tree.begin(); bp != tree.end(); bp++) { | ||
| bp->enable(); | ||
| } | ||
| } | ||
|
|
||
| ImGui::SameLine(); | ||
| if (ImGui::Button(_("Deactivate All"))) { | ||
| for (auto bp = tree.begin(); bp != tree.end(); bp++) { | ||
| bp->disable(); | ||
| } | ||
| } | ||
|
|
||
| ImGui::SameLine(); | ||
| if (ImGui::Button(_("Delete All"))) { | ||
| ImGui::OpenPopup("delbp_popup"); | ||
| } | ||
| if (ImGui::BeginPopup("delbp_popup")) { | ||
| ImGui::TextUnformatted(_("Delete all Breakpoints?")); | ||
| if (ImGui::Button(_("Delete"))) { | ||
| g_emulator->m_debug->removeAllBreakpoints(); | ||
| } | ||
| ImGui::EndPopup(); | ||
| } | ||
| } | ||
| counter = 0; | ||
| for (auto bp = tree.begin(); bp != tree.end(); bp++, counter++) { | ||
| showEditLabelPopup(&*bp, counter); | ||
|
|
||
| ImGui::Separator(); | ||
| if (ImGui::TreeNode(_("Execution Map"))) { | ||
| if (ImGui::Button(_("Clear maps"))) { | ||
| debugger->clearMaps(); | ||
| } | ||
| ImGuiHelpers::ShowHelpMarker( | ||
| _("The mapping feature is a simple concept, but requires some amount of explanation. See the documentation " | ||
| "website for more details, in the Misc Features subsection of the Debugging section.")); | ||
| ImGui::Checkbox(_("Map execution"), &debugger->m_mapping_e); | ||
| ImGui::Checkbox(_("Map byte reads "), &debugger->m_mapping_r8); | ||
| ImGui::SameLine(); | ||
| ImGui::Checkbox(_("Map half reads "), &debugger->m_mapping_r16); | ||
| ImGui::SameLine(); | ||
| ImGui::Checkbox(_("Map word reads "), &debugger->m_mapping_r32); | ||
| ImGui::Checkbox(_("Map byte writes "), &debugger->m_mapping_w8); | ||
| ImGui::SameLine(); | ||
| ImGui::Checkbox(_("Map half writes "), &debugger->m_mapping_w16); | ||
| ImGui::SameLine(); | ||
| ImGui::Checkbox(_("Map word writes "), &debugger->m_mapping_w32); | ||
| ImGui::Separator(); | ||
| ImGui::Checkbox(_("Break on execution map"), &debugger->m_breakmp_e); | ||
| ImGui::Checkbox(_("Break on byte read map "), &debugger->m_breakmp_r8); | ||
| ImGui::SameLine(); | ||
| ImGui::Checkbox(_("Break on half read map "), &debugger->m_breakmp_r16); | ||
| ImGui::SameLine(); | ||
| ImGui::Checkbox(_("Break on word read map "), &debugger->m_breakmp_r32); | ||
| ImGui::Checkbox(_("Break on byte write map"), &debugger->m_breakmp_w8); | ||
| ImGui::SameLine(); | ||
| ImGui::Checkbox(_("Break on half write map"), &debugger->m_breakmp_w16); | ||
| ImGui::SameLine(); | ||
| ImGui::Checkbox(_("Break on word write map"), &debugger->m_breakmp_w32); | ||
| ImGui::TreePop(); | ||
| } | ||
|
|
||
| if (toErase) g_emulator->m_debug->removeBreakpoint(toErase); | ||
| ImGui::End(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
High cyclomatic complexity in Breakpoints::draw.
Static analysis indicates the method’s complexity has grown significantly, making it challenging to maintain and extend. Consider refactoring into smaller sub-functions (e.g., table creation, row rendering, condition popups, etc.) for improved readability and maintainability.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 94-425: ❌ Getting worse: Complex Method
PCSX::Widgets::Breakpoints::draw increases in cyclomatic complexity from 25 to 70, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[warning] 94-425: ❌ Getting worse: Bumpy Road Ahead
PCSX::Widgets::Breakpoints::draw increases from 5 to 12 logical blocks with deeply nested code, threshold is one single block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 94-425: ❌ New issue: Deep, Nested Complexity
PCSX::Widgets::Breakpoints::draw has a nested complexity depth of 6, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/debug.h(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: toolchain
- GitHub Check: asan
- GitHub Check: cross-arm64
- GitHub Check: aur-build
- GitHub Check: build-openbios
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: build
- GitHub Check: coverage
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (2)
src/core/debug.h (2)
Line range hint
40-168: Verify condition checking implementationThe header defines the structure for conditional breakpoints, but the actual condition checking logic needs to be implemented in the source file. Please ensure:
- The
triggerBPmethod checks conditions before triggering- For
Changecondition, previous values are tracked- For
Rangecondition, both bounds are properly handledLet's verify the implementation:
70-71:⚠️ Potential issueUpdate default lambda to match non-const Breakpoint parameter
The
BreakpointInvokertype now uses non-constBreakpoint*, but the default lambda inaddBreakpointstill usesconst Breakpoint*. This inconsistency should be fixed.Let's verify all usages of the invoker:
| BreakpointCondition m_condition = BreakpointCondition::Always; | ||
| uint32_t m_conditionData; |
There was a problem hiding this comment.
Initialize m_conditionData member
The m_conditionData member is not initialized, which could lead to undefined behavior. Initialize it to 0 since it's unused for the default Always condition.
BreakpointCondition m_condition = BreakpointCondition::Always;
- uint32_t m_conditionData;
+ uint32_t m_conditionData = 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| BreakpointCondition m_condition = BreakpointCondition::Always; | |
| uint32_t m_conditionData; | |
| BreakpointCondition m_condition = BreakpointCondition::Always; | |
| uint32_t m_conditionData = 0; |
| @@ -158,6 +165,7 @@ | |||
| if (m_lastBP == bp) m_lastBP = nullptr; | |||
| delete const_cast<Breakpoint*>(bp); | |||
| } | |||
| void removeAllBreakpoints() { m_breakpoints.clear(); } | |||
There was a problem hiding this comment.
Reset m_lastBP in removeAllBreakpoints
The removeAllBreakpoints method should reset m_lastBP to maintain consistency with removeBreakpoint.
- void removeAllBreakpoints() { m_breakpoints.clear(); }
+ void removeAllBreakpoints() {
+ m_lastBP = nullptr;
+ m_breakpoints.clear();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void removeAllBreakpoints() { m_breakpoints.clear(); } | |
| void removeAllBreakpoints() { | |
| m_lastBP = nullptr; | |
| m_breakpoints.clear(); | |
| } |
| @@ -37,6 +37,7 @@ | |||
| static bool isInKernel(uint32_t address, bool biosIsKernel = true); | |||
| static inline std::function<const char*()> s_breakpoint_type_names[] = {l_("Exec"), l_("Read"), l_("Write")}; | |||
| enum class BreakpointType { Exec, Read, Write }; | |||
| enum class BreakpointCondition { Always, Change, Greater, Less, Equal }; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation and Range condition
The new BreakpointCondition enum should include:
- Documentation explaining the behavior of each condition
- The
Rangecondition mentioned in the PR objectives
+ /// Breakpoint conditions that determine when a breakpoint should trigger
enum class BreakpointCondition {
+ /// Always trigger the breakpoint
Always,
+ /// Trigger when the value changes
Change,
+ /// Trigger when the value is greater than conditionData
Greater,
+ /// Trigger when the value is less than conditionData
Less,
+ /// Trigger when the value equals conditionData
Equal,
+ /// Trigger when the value is within a range (requires two values in conditionData)
+ Range
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| enum class BreakpointCondition { Always, Change, Greater, Less, Equal }; | |
| /// Breakpoint conditions that determine when a breakpoint should trigger | |
| enum class BreakpointCondition { | |
| /// Always trigger the breakpoint | |
| Always, | |
| /// Trigger when the value changes | |
| Change, | |
| /// Trigger when the value is greater than conditionData | |
| Greater, | |
| /// Trigger when the value is less than conditionData | |
| Less, | |
| /// Trigger when the value equals conditionData | |
| Equal, | |
| /// Trigger when the value is within a range (requires two values in conditionData) | |
| Range | |
| }; |
| BreakpointCondition condition() const { return m_condition; } | ||
| void setCondition(BreakpointCondition condition) { m_condition = condition; } | ||
| uint32_t conditionData() const { return m_conditionData; } | ||
| void setConditionData(uint32_t data) { m_conditionData = data; } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and documentation for condition methods
The condition methods need:
- Documentation explaining how
conditionDatais used for each condition type - Validation in
setConditionDatabased on the current condition
+ /// @returns The current condition for this breakpoint
BreakpointCondition condition() const { return m_condition; }
+ /// @param condition The new condition to set
void setCondition(BreakpointCondition condition) { m_condition = condition; }
+ /// @returns The condition-specific data:
+ /// - For Greater/Less/Equal: The value to compare against
+ /// - For Range: The upper bound (lower bound stored internally)
+ /// - For Always/Change: Unused
uint32_t conditionData() const { return m_conditionData; }
+ /// @param data The condition-specific data to set
+ /// @throws std::invalid_argument if data is invalid for current condition
void setConditionData(uint32_t data) {
+ switch (m_condition) {
+ case BreakpointCondition::Range:
+ if (data <= getLow()) throw std::invalid_argument("Range upper bound must be greater than lower bound");
+ break;
+ case BreakpointCondition::Always:
+ case BreakpointCondition::Change:
+ if (data != 0) throw std::invalid_argument("Condition data must be 0 for Always/Change conditions");
+ break;
+ }
m_conditionData = data;
}Committable suggestion skipped: line range outside the PR's diff.
