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
src/gui/widgets/breakpoints.cc
Outdated
| #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.
src/gui/widgets/breakpoints.cc
Outdated
| 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
Possibly related PRs
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
![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.
src/gui/widgets/breakpoints.cc
Outdated
| 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:
src/core/debug.h
Outdated
| 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; |
src/core/debug.h
Outdated
| @@ -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.
| Debug::BreakpointType type = (Debug::BreakpointType)m_breakpointType; | ||
| if (type != Debug::BreakpointType::Exec) { | ||
| ImGui::Combo(_("Break Condition"), &breakConditionImguiValue, _("Always\0Change\0Greater\0Less\0Equal\0")); | ||
| breakCondition = (Debug::BreakpointCondition)breakConditionImguiValue; |
There was a problem hiding this comment.
if the type is Exec, this path will not be executed, leaving breakCondition with an undefined value. It should be defined when declared above.
src/gui/widgets/breakpoints.cc
Outdated
| ImGui::TextColored(((bp->address() | bp->base()) == pc) ? s_hitColor : s_normalColor, "%d", row); | ||
|
|
||
| ImGui::TableNextColumn(); | ||
| std::string buttonStr = fmt::format("{:08x}", bp->address() | bp->base()); |
There was a problem hiding this comment.
to prevent conflicting imgui IDs we should append something like:
std::string buttonStr = fmt::format("{:08x}##{:d}", bp->address() | bp->base(), row);
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/gui/widgets/breakpoints.cc (2)
94-425: 🛠️ Refactor suggestionRefactor the complex draw method into smaller functions.
The
drawmethod has grown significantly in complexity (from 25 to 70 cyclomatic complexity), making it harder to maintain and understand. Consider breaking it down into smaller, focused functions for different aspects like table creation, row rendering, condition popups, etc.For example, you could extract these logical sections into separate methods:
renderBreakpointTable()- Lines ~104-180renderBreakpointAddDialog()- Lines ~187-360renderBreakpointControls()- Lines ~362-387renderExecutionMapOptions()- Lines ~391-422This would make the code more maintainable and easier to extend in the future.
🧰 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.
220-224:⚠️ Potential issueInitialize breakCondition with default value at declaration.
If the breakpoint type is Exec, this path will not be executed, leaving breakCondition potentially with an undefined value.
This is consistent with notdodgeball's previous comment: "if the type is Exec, this path will not be executed, leaving breakCondition with an undefined value."
Although you initialize it at line 220, it's better to be explicit about the behavior for Exec breakpoints:
Debug::BreakpointCondition breakCondition = Debug::BreakpointCondition::Always; Debug::BreakpointType type = (Debug::BreakpointType)m_breakpointType; if (type != Debug::BreakpointType::Exec) { ImGui::Combo(_("Break Condition"), &breakConditionImguiValue, _("Always\0Change\0Greater\0Less\0Equal\0")); breakCondition = (Debug::BreakpointCondition)breakConditionImguiValue; +} else { + // Execution breakpoints always use the "Always" condition }src/core/debug.h (1)
40-40: 🛠️ Refactor suggestionAdd Range condition to the BreakpointCondition enum.
The PR objectives mention a "Range" condition type, but it's not implemented in the enum.
enum class BreakpointCondition { Always, Change, Greater, Less, Equal, + Range };Also consider adding documentation comments to explain each condition's behavior, as suggested in previous review comments.
🧹 Nitpick comments (1)
src/gui/widgets/breakpoints.cc (1)
36-50: Add default case to handle future enum values.The switch statement doesn't have a default case, which could lead to unexpected behavior if new condition types are added to the enum in the future.
static const char* getBreakpointConditionName(PCSX::Debug::BreakpointCondition condition) { switch (condition) { case PCSX::Debug::BreakpointCondition::Always: return _("Always"); case PCSX::Debug::BreakpointCondition::Greater: return _("Greater"); case PCSX::Debug::BreakpointCondition::Less: return _("Less"); case PCSX::Debug::BreakpointCondition::Change: return _("Change"); case PCSX::Debug::BreakpointCondition::Equal: return _("Equal"); + default: + return _("Unknown"); } - return _("Unknown"); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/core/debug.h(5 hunks)src/gui/widgets/assembly.cc(1 hunks)src/gui/widgets/breakpoints.cc(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
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.
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.
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: aur-build
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: coverage
- GitHub Check: build-openbios
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: build
- GitHub Check: asan
- GitHub Check: cross-arm64
- GitHub Check: toolchain
🔇 Additional comments (7)
src/gui/widgets/assembly.cc (1)
343-348: Great addition of memory breakpoint creation in the context menu.This addition allows users to easily create memory read and write breakpoints directly from the assembly view, enhancing the debugging workflow. The implementation is clean and integrates well with the existing context menu structure.
🧰 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.src/gui/widgets/breakpoints.cc (3)
27-34: Add support for SWL and SWR instructions.The function ignores SWL and SWR instructions as noted in the comment, which could lead to incorrect values for these specific store instructions.
This aligns with a known limitation previously noted by nicolasnoble: "This is a bit of a blind spot overall. It feels like we can properly support swl/swr."
52-86: Use safer memory access methods for debugging.The current implementation uses
read8/read16/read32which could potentially advance internal state machines on components like the CD-ROM controller or pads, as noted in previous feedback.This matches nicolasnoble's previous 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."
127-128: Add unique identifiers to imgui buttons.To prevent conflicting ImGui IDs, append a unique identifier to button labels.
This matches notdodgeball's previous comment about preventing conflicting imgui IDs. The current implementation does this correctly by appending the row number to the button string:
std::string buttonStr = fmt::format("{:08x}##{:d}", bp->address() | bp->base(), row);Good job on this implementation!
src/core/debug.h (3)
82-85: Add validation in setConditionData based on condition type.Currently, there's no validation to ensure that the condition data makes sense for the given condition type.
Consider adding validation to ensure the data is appropriate for the condition, especially for a future Range condition which might require checking that the upper bound is greater than the lower bound.
168-171: Good addition of removeAllBreakpoints method.This method properly clears all breakpoints and resets m_lastBP to avoid dangling pointers, which is important for maintaining consistency when removing breakpoints.
70-71:❓ Verification inconclusive
Breaking change in BreakpointInvoker signature.
Changing the first parameter from
const Breakpoint*toBreakpoint*is necessary for the "Change" condition, but it's a breaking change that could affect existing code.
🏁 Script executed:
#!/bin/bash # Find any existing uses of BreakpointInvoker that might be affected by the signature change rg -A 2 -B 2 "BreakpointInvoker" --glob "!src/core/debug.h" --glob "!src/gui/widgets/breakpoints.cc"Length of output: 1224
Action Required: Confirm Consistency for BreakpointInvoker Change
The typedef for BreakpointInvoker insrc/core/debug.hwas updated from taking aconst Breakpoint*to a non-constBreakpoint*as its first parameter—this was needed for the new “Change” condition. However, our search reveals that several lambda implementations (e.g., insrc/gui/widgets/typed_debugger.cc) still declare the parameter as aconst PCSX::Debug::Breakpoint*. Although passing a non-const pointer to a lambda expecting a const pointer is allowed by conversion rules, the inconsistency in const-qualification may lead to unintended side effects or miscommunication of intended mutability. Please review these call sites and decide whether to update the lambda signatures to match the non-const requirement or document the intended behavior clearly.
- Locations to review:
src/core/debug.h: typedef definition of BreakpointInvoker (lines 70-71).src/gui/widgets/typed_debugger.cc: lambda expressions capturing BreakpointInvoker that still useconst PCSX::Debug::Breakpoint*.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
tests/support/mips.cc (2)
1-18: Fix copyright year for new file.The copyright notice contains a future year (2025) which should be corrected to the current year.
-/*************************************************************************** - * Copyright (C) 2025 PCSX-Redux authors * +/*************************************************************************** + * Copyright (C) 2024 PCSX-Redux authors *
68-75: Consider breaking down branch instruction test to improve test diagnostics.This test has 5 assertions, exceeding the recommended limit of 4 assertions per test block. While functionally correct, breaking it into smaller tests would make it easier to identify specific issues when a test fails.
-TEST(MipsTest, BranchInstruction) { - Instruction beqInstr(0x112a0003); // beq $t1, $t2, 3 - EXPECT_EQ(beqInstr.mnemonic(), Instruction::BEQ); - EXPECT_EQ(beqInstr.rs(), 9); - EXPECT_EQ(beqInstr.rt(), 10); - EXPECT_EQ(beqInstr.imm(), 3); - EXPECT_EQ(beqInstr.getBranchAddress(0x1000), 0x1010); -} +TEST(MipsTest, BranchInstructionBasic) { + Instruction beqInstr(0x112a0003); // beq $t1, $t2, 3 + EXPECT_EQ(beqInstr.mnemonic(), Instruction::BEQ); + EXPECT_EQ(beqInstr.rs(), 9); + EXPECT_EQ(beqInstr.rt(), 10); +} + +TEST(MipsTest, BranchInstructionAddressing) { + Instruction beqInstr(0x112a0003); // beq $t1, $t2, 3 + EXPECT_EQ(beqInstr.imm(), 3); + EXPECT_EQ(beqInstr.getBranchAddress(0x1000), 0x1010); +}src/mips/common/util/decoder.hh (8)
89-239: Consider refactoring themnemonic()method to reduce cyclomatic complexity.This method contains an extensive switch block, triggering complexity warnings. Splitting it into smaller helper functions or employing a data-driven approach (e.g., lookup tables) can improve maintainability and readability.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 89-239: ❌ New issue: Complex Method
mnemonic has a cyclomatic complexity of 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.
241-256: High cyclomatic complexity inisLoad().Though relatively straightforward, the multiple-case switch statement contributes to overall complexity. Consider consolidating repeated patterns or abstracting to a data-driven approach.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 241-256: ❌ New issue: Code Duplication
The module contains 8 functions with similar structure: getBranchAddress,getLoadAddress,getLoadMask,getStoreAddress and 4 more functions. Avoid duplicated, aka copy-pasted, code inside the module. More duplication lowers the code health.
[warning] 241-256: ❌ New issue: Complex Method
isLoad has a cyclomatic complexity of 11, 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.
272-286: Repeated load address calculation logic.
getLoadAddress()is specialized to certain mnemonics, mirroring logic in other methods. Consider merging overlapping patterns or using a shared helper function to reduce duplication.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 272-286: ❌ New issue: Complex Method
getLoadAddress has a cyclomatic complexity of 10, 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.
288-300: Repeated store address calculation logic.
getStoreAddress()similarly duplicates the approach used ingetLoadAddress(). Consider extracting common computation steps to reduce code duplication.
302-341: High cyclomatic complexity ingetLoadMask().Its multiple nested switch statements trigger warnings. Consider reducing branching via unified logic or a table-based approach. For example, mapping offset bits to bitmasks in a constant array can streamline repeated cases.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 302-341: ❌ New issue: Complex Method
getLoadMask has a cyclomatic complexity of 16, 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.
343-382: Multiple nested switch blocks ingetValueToStore().This method has layers of switch logic for both the mnemonic and the address alignment. Refactoring or partial table lookups can reduce branching and make the code easier to maintain.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 343-382: ❌ New issue: Complex Method
getValueToStore has a cyclomatic complexity of 15, 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.
384-421: High cyclomatic complexity ingetStoreMask().Similar to
getLoadMask(), repeated case handling for alignment is increasing complexity. Use a consistent and compact approach to retrieving bitmasks.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 384-421: ❌ New issue: Complex Method
getStoreMask has a cyclomatic complexity of 14, 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.
423-437: Branch calculation logic ingetBranchAddress().Multiple branch instructions share similar offset logic. Streamlining or reusing reusable offset computations may lower complexity and reduce future maintenance overhead.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 423-437: ❌ New issue: Complex Method
getBranchAddress has a cyclomatic complexity of 10, 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 (2)
53-74: Unreachablebreak;statement afterreturn.Within the
case 4block (lines 72-73), thereturnstatement makes the subsequentbreak;unreachable code. Removing the trailingbreak;clarifies intent.case 4: { return mem->readAt<uint32_t>(addr); - break; }
85-417: High cyclomatic complexity inBreakpoints::draw().This function has grown significantly, nesting multiple loops, conditionals, and UI logic. Refactoring parts of the rendering (e.g., table creation, row rendering, breakpoints popup management) into smaller sub-functions can improve readability and maintainability.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 85-417: ❌ 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] 85-417: ❌ 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] 85-417: ❌ 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.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/core/r3000a.h(2 hunks)src/gui/widgets/breakpoints.cc(1 hunks)src/mips/common/util/decoder.hh(1 hunks)src/mips/common/util/encoder.hh(1 hunks)src/mips/common/util/mips.hh(1 hunks)tests/support/mips.cc(1 hunks)vsprojects/tests/support/testsupport.vcxproj(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/mips/common/util/mips.hh
🧰 Additional context used
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
tests/support/mips.cc
[warning] 83-90: ❌ New issue: Large Assertion Blocks
The test suite contains 5 assertion blocks with at least 4 assertions, threshold = 4. This test file has several blocks of large, consecutive assert statements. Avoid adding more.
src/mips/common/util/decoder.hh
[warning] 241-256: ❌ New issue: Code Duplication
The module contains 8 functions with similar structure: getBranchAddress,getLoadAddress,getLoadMask,getStoreAddress and 4 more functions. Avoid duplicated, aka copy-pasted, code inside the module. More duplication lowers the code health.
[warning] 89-239: ❌ New issue: Complex Method
mnemonic has a cyclomatic complexity of 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] 302-341: ❌ New issue: Complex Method
getLoadMask has a cyclomatic complexity of 16, 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] 343-382: ❌ New issue: Complex Method
getValueToStore has a cyclomatic complexity of 15, 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] 384-421: ❌ New issue: Complex Method
getStoreMask has a cyclomatic complexity of 14, 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] 241-256: ❌ New issue: Complex Method
isLoad has a cyclomatic complexity of 11, 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] 423-437: ❌ New issue: Complex Method
getBranchAddress has a cyclomatic complexity of 10, 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] 272-286: ❌ New issue: Complex Method
getLoadAddress has a cyclomatic complexity of 10, 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] 1-1: ❌ New issue: Overall Code Complexity
This module has a mean cyclomatic complexity of 7.91 across 23 functions. The mean complexity threshold is 4. This file has many conditional statements (e.g. if, for, while) across its implementation, leading to lower code health. Avoid adding more conditionals.
src/gui/widgets/breakpoints.cc
[warning] 85-417: ❌ 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] 85-417: ❌ 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] 85-417: ❌ 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.
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: macos-arm-build-and-test
- GitHub Check: macos-intel-build-and-test
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: coverage
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: cross-arm64
- GitHub Check: toolchain
- GitHub Check: aur-build
- GitHub Check: build
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: asan
🔇 Additional comments (11)
vsprojects/tests/support/testsupport.vcxproj (1)
86-86: Appropriate addition of the new MIPS tests source file.The inclusion of the new
mips.cctest file in the project is properly done and necessary for the MIPS testing functionality to be built with the project.src/mips/common/util/encoder.hh (2)
31-31: Well-structured include for the MIPS type definitions.Adding the include for
mips.hhis a good step toward centralizing MIPS-related types and definitions.
36-36: Good refactoring to use centralized MIPS register types.Replacing the local
Regenum with a type alias toMips::Reghelps centralize MIPS register definitions and reduces redundancy in the codebase.src/core/r3000a.h (2)
37-37: Appropriate addition of MIPS utility header.The inclusion of the MIPS header file supports the refactoring to centralize MIPS-related types and is required for the type alias on line 93.
93-93: Good simplification using the centralized MIPS register types.Replacing the detailed
psxGPRRegsunion definition with a type alias toMips::GPRRegssimplifies this file and promotes code reuse by leveraging the standardized MIPS types.tests/support/mips.cc (6)
20-28: Well-structured imports and namespace usage.The includes and namespace declarations are properly organized and make the test code clean and readable.
29-32: Good basic register enum verification test.This test correctly verifies the expected values for the first (R0) and last (RA) registers in the MIPS register enumeration.
39-45: Good instruction decoding test.The test for the ADD instruction decoding is well-structured and verifies all relevant properties of the decoded instruction.
47-50: Proper instruction encoding test.The encoding test correctly verifies that the encoder produces the expected machine code for an ADD instruction.
83-90: Comprehensive encoder-decoder integration test.The test properly verifies that an encoded instruction can be correctly decoded back to its original form, validating the round-trip integrity of the encoder and decoder.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 83-90: ❌ New issue: Large Assertion Blocks
The test suite contains 5 assertion blocks with at least 4 assertions, threshold = 4. This test file has several blocks of large, consecutive assert statements. Avoid adding more.
34-37:❓ Verification inconclusive
Verify GPRRegs size expectation.
The test expects that
GPRRegsis exactly 34 uint32_t elements in size. While MIPS typically has 32 general-purpose registers, the additional 2 might account for special registers like 'lo' and 'hi'. Verify this expectation is correct.
🏁 Script executed:
#!/bin/bash # Check how GPRRegs is defined in mips.hh fd "struct GPRRegs" --type f --exec cat {} | grep -A 30 "struct GPRRegs"Length of output: 76
Action Required: Manually verify the GPRRegs definition.
The initial automated search didn’t return any details for the structure declaration, so we couldn’t confirm whether it indeed comprises exactly 34uint32_telements. Please manually inspect the source (likely inmips.hhor an equivalent file) to ensure that the additional two registers beyond the standard 32 (presumably forloandhi) are intended and correctly implemented.
- Verify that the definition of
GPRRegsmatches the expectation of 34 elements.- Confirm the source file’s location and declaration of
GPRRegs(e.g., inmips.hh).#!/bin/bash # Re-run a recursive search for the declaration of GPRRegs to help locate it: rg -A 30 "struct GPRRegs" .
