Support hardware breakpoint and watchpoint - #22
Conversation
This adds GDB RSP support for hardware breakpoints (Z1) and watchpoints (Z2-Z4), enabling memory access debugging in the emulator. Protocol changes: - Extend bp_type_t with BP_HARDWARE, WP_WRITE, WP_READ, WP_ACCESS - Handle Z1 (hw breakpoint), Z2 (write), Z3 (read), Z4 (access) packets - Add 'len' parameter to set_bp/del_bp for memory region size - Advertise hwbreak+ and swbreak+ capabilities in qSupported
|
One serious problem is that the commit mixes so many irrelevant changes to the PR topic...... From a collaboration perspective, I don't encourage this approach. Is it possible to make only the changes corresponding to the commit and split it into several commits/PRs? |
| break; | ||
| snprintf(packet_str, sizeof(packet_str), "E%02x", ret & 0xff); | ||
| conn_send_pktstr(&gdbstub->priv->conn, packet_str); | ||
| return; |
There was a problem hiding this comment.
Shouldn't we modify lines 204-205 to a single break?
| if (regno < 0 || regno >= gdbstub->arch.reg_num) { | ||
| SEND_EINVAL(gdbstub); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Remove this piece of code. Such logic should be handled in read_reg by the user themselves.
| if (regno < 0 || regno >= gdbstub->arch.reg_num) { | ||
| SEND_EINVAL(gdbstub); | ||
| return; | ||
| } |
| if (payload_length > remaining) | ||
| payload_length = remaining; | ||
| if (payload_length > max_payload) | ||
| payload_length = max_payload; |
There was a problem hiding this comment.
payload_length = length > remaining ? remaining : length ;
payload_length = payload_length > max_payload ? max_payload : payload_length;
| /* Enforce SMP limit to prevent buffer overflow */ | ||
| if (smp >= MAX_SMP_COUNT) { | ||
| smp = MAX_SMP_COUNT - 1; | ||
| } |
There was a problem hiding this comment.
Handle error like this just gives us a new issue.
Let's check the smp in the initialization and fail early to hint to the user that this is not supported currently.
| int smp; | ||
| int reg_num; | ||
| const char *target_desc; /* XML target description (may be NULL) */ | ||
| int smp; /* Number of CPUs (0 or 1 for single-core) */ |
There was a problem hiding this comment.
This won't be a 0 or 1 value for sure.
| * | ||
| * For watchpoints (WP_WRITE, WP_READ, WP_ACCESS): | ||
| * - 'len' parameter is the memory region size in bytes to watch | ||
| * - Must be validated by implementation (typical max: 8 bytes) |
There was a problem hiding this comment.
Why do we explain the len here? This is really confusing.
| /** | ||
| * Set a breakpoint or watchpoint. | ||
| * @param addr Target address | ||
| * @param len For BP_*: instruction size; for WP_*: memory region size |
There was a problem hiding this comment.
In the implementation, BP_* doesn't utilize the len, but here it says it is the instruction size.
| /** | ||
| * Delete a breakpoint or watchpoint. | ||
| * @param addr Target address (must match set_bp call) | ||
| * @param len Must match the len used in set_bp |
There was a problem hiding this comment.
No. I think the delete breakpoint doesn't need to "match" for GDBstub. The user can do anything they want, and gdbstub just needs to report the error if it really doesn't match.
|
@jserv I appreciate having a more complete implementation for the mini-gdbstub, but I believe we have code/comments that were generated by AI, but some are incorrect or misleading. As a reviewer, I am really disappointed when I keep finding these AI-generated wrong things. I am not sure if you have reviewed these codes carefully, but could you please separate the part of the breakpoint supported and the others first? At least don't mess everything in one commit, and I can approach reviewing these codes easily. |
b40c643 to
dac98cc
Compare
2 participants
This adds GDB RSP support for hardware breakpoints (Z1) and watchpoints (Z2-Z4), enabling memory access debugging in the emulator.
Protocol changes: