-
Notifications
You must be signed in to change notification settings - Fork 598
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Debugger: Add conditional data breakpoints, and a few minor bug fixes #2473
base: master
Are you sure you want to change the base?
Conversation
Thanks! I'll get around to reviewing this soon, super busy with work this week :( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only looked at the CPU side for now, I'm a little concerned about the performance impact given the fact that this code can potentially execute up to 33.8 million times per second.
src/core/cpu_core.cpp
Outdated
{ | ||
const u32 pc = g_state.regs.pc; | ||
Instruction inst = g_state.current_instruction; | ||
inst.bits = inst_bits; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant assignment
if (inst.bits == 0) | ||
return false; | ||
|
||
switch (inst.op) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just read g_state.current_instruction
src/core/cpu_core.cpp
Outdated
// continue is measurably faster than break on msvc for some reason | ||
continue; | ||
} | ||
if (ExecutionBreakpointCheck()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (ExecutionBreakpointCheck() || ConditionalBreakpointLookAhead())
src/core/cpu_core.cpp
Outdated
case InstructionOp::lw: | ||
{ | ||
const VirtualMemoryAddress addr = ReadReg(inst.i.rs) + inst.i.imm_sext32(); | ||
DataBreakpointCheck<MemoryAccessType::Read>(addr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to get very slow. It would probably be better to align the test address down, and do a single range check in DataBreakpointCheck
.
@@ -17,6 +17,21 @@ enum class MemoryAccessSize : u32 | |||
Word | |||
}; | |||
|
|||
struct DebugAddress |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than combining the breakpoints, it'd be better to store the execution and data breakpoints in separate arrays (fewer loops/comparisons for every guest instruction).
dbbf234
to
e3d9ba4
Compare
5bb1e94
to
f993f09
Compare
8fb8259
to
5ef2ba0
Compare
1f89203
to
084a76a
Compare
2005150
to
8867bb1
Compare
b26a4d7
to
7ea7d3b
Compare
349fcfa
to
6d3f01c
Compare
0cb0e34
to
55d96f8
Compare
I've implemented conditional data breakpoints for variable memory sizes. Duckstation was storing the address of each breakpoint as the information to check for when an execution breakpoint is valid. I expanded this idea by creating the struct
DebugAddress
, which encapsulates anaddress
, asize
and adebug_mode
. The user can create aDebugAddress
by adding a breakpoint using the UI, and select multiple conditions for the breakpoint: read, write, changed value or execution.The execution breakpoint that was implemented on duck had to be modified by adding an extra check for the
debug_mode
, so only execution breakpoints can trigger that event. The data breakpoint was implemented by adding a function that looks ahead of what the next CPU instruction is going to be, and if the next instruction matches the conditions of anyDebugAddress
, then the emulation is paused before the CPU executes that instruction. The UI had to be changed in order for the user to create such breakpoints.A few minor bug fixes unrelated to that:
sll zero, zero
tonop
.This PR addresses some of the issues mentioned at #2179.