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.
