Support GDB stub for remote debugging #74
Conversation
Code Review Agent Run #c506b3Actionable Suggestions - 5
Review Details
AI Code Review powered by |
Changelist by BitoThis pull request implements the following key changes.
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
| emu->mtimer.mtimecmp = calloc(vm->n_hart, sizeof(uint64_t)); | ||
| emu->mswi.msip = calloc(vm->n_hart, sizeof(uint32_t)); | ||
| emu->sswi.ssip = calloc(vm->n_hart, sizeof(uint32_t)); |
There was a problem hiding this comment.
Consider checking if memory allocation for mtimecmp, msip, and ssip arrays was successful to handle out-of-memory conditions gracefully.
Code suggestion
Check the AI-generated fix before applying
| emu->mtimer.mtimecmp = calloc(vm->n_hart, sizeof(uint64_t)); | |
| emu->mswi.msip = calloc(vm->n_hart, sizeof(uint32_t)); | |
| emu->sswi.ssip = calloc(vm->n_hart, sizeof(uint32_t)); | |
| emu->mtimer.mtimecmp = calloc(vm->n_hart, sizeof(uint64_t)); | |
| if (!emu->mtimer.mtimecmp) { | |
| fprintf(stderr, "Failed to allocate mtimecmp array\n"); | |
| return -1; | |
| } | |
| emu->mswi.msip = calloc(vm->n_hart, sizeof(uint32_t)); | |
| if (!emu->mswi.msip) { | |
| fprintf(stderr, "Failed to allocate msip array\n"); | |
| return -1; | |
| } | |
| emu->sswi.ssip = calloc(vm->n_hart, sizeof(uint32_t)); | |
| if (!emu->sswi.ssip) { | |
| fprintf(stderr, "Failed to allocate ssip array\n"); | |
| return -1; | |
| } |
Code Review Run #c506b3
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| virtio_net_refresh_queue(&emu->vnet); | ||
| if (emu->vnet.InterruptStatus) | ||
| emu_update_vnet_interrupts(vm); |
There was a problem hiding this comment.
The code is accessing vnet member without checking if VIRTIONET is enabled. Consider adding proper #ifdef guards.
Code suggestion
Check the AI-generated fix before applying
| virtio_net_refresh_queue(&emu->vnet); | |
| if (emu->vnet.InterruptStatus) | |
| emu_update_vnet_interrupts(vm); | |
| #if SEMU_HAS(VIRTIONET) | |
| virtio_net_refresh_queue(&emu->vnet); | |
| if (emu->vnet.InterruptStatus) | |
| emu_update_vnet_interrupts(vm); | |
| #endif |
Code Review Run #c506b3
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| vm_error_report(vm->hart[i]); | ||
| return 2; |
There was a problem hiding this comment.
Consider adding more detailed error reporting. The current vm_error_report() call could be enhanced with additional context about which hart failed and what operation was being performed.
Code suggestion
Check the AI-generated fix before applying
| vm_error_report(vm->hart[i]); | |
| return 2; | |
| fprintf(stderr, "Error in hart %d: ", i); | |
| vm_error_report(vm->hart[i]); | |
| return 2; |
Code Review Run #c506b3
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| if (regno > 32) | ||
| return EFAULT; |
There was a problem hiding this comment.
The regno parameter in semu_read_reg() is compared against 32 but the function accepts it as signed int. Consider using unsigned comparison to prevent potential issues with negative values.
Code suggestion
Check the AI-generated fix before applying
| if (regno > 32) | |
| return EFAULT; | |
| if (regno < 0 || regno > 32) | |
| return EFAULT; |
Code Review Run #c506b3
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
main.c
Outdated
| { | ||
| semu_t *semu = (semu_t *) args; | ||
| hart_t *hart = semu->vm.hart[semu->curr_cpuid]; | ||
| mem_load(hart, addr, len, val); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Consider adding error handling for mem_load() call in semu_read_mem(). The function may fail but the error is not propagated to the caller.
Code suggestion
Check the AI-generated fix before applying
| { | |
| semu_t *semu = (semu_t *) args; | |
| hart_t *hart = semu->vm.hart[semu->curr_cpuid]; | |
| mem_load(hart, addr, len, val); | |
| return 0; | |
| } | |
| { | |
| semu_t *semu = (semu_t *) args; | |
| hart_t *hart = semu->vm.hart[semu->curr_cpuid]; | |
| mem_load(hart, addr, len, val); | |
| if (hart->error) { | |
| hart->error = ERR_NONE; | |
| return EFAULT; | |
| } | |
| return 0; | |
| } |
Code Review Run #c506b3
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
|
|
||
| /* Initialize the emulator */ | ||
| emu_state_t emu; | ||
| memset(&emu, 0, sizeof(emu)); | ||
| memset(emu, 0, sizeof(*emu)); |
There was a problem hiding this comment.
Check the comments in riscv.h carefully:
/* To use the emulator, start by initializing a hart_t object with zero values,
* invoke vm_init(), and set the required environment-supplied callbacks. You
* may also set other necessary fields such as argument registers and s_mode,
* ensuring that all field restrictions are met to avoid undefined behavior.
*
* Once the emulator is set up, execute the emulation loop by calling
* "vm_step()" repeatedly. Each call attempts to execute a single instruction.
*
* If the execution completes successfully, the "vm->error" field will be set
* to ERR_NONE. However, if an error occurs during execution, the emulator will
* halt and the "vm->error" field will provide information about the error. It
* is important to handle the emulation error before calling "vm_step()" again;
* otherwise, it will not execute any instructions. The possible errors are
* described above for reference.
*/Do follow the manner unless you are about to refine the usage as the major purpose.
There was a problem hiding this comment.
The purpose of abstracting semu_init() here is to share the same logic between debug & normal mode. The behavior in normal mode should not be changed here. We follow these APIs in the same way.
|
I think the change still respects the riscv.h manner, as no behavior changes for the normal mode. Also, I don't want to fix the reviews by BOT here since they just follow the original implementation. The commit just separates the initialization of semu to reuse it for debug mode. Please let me know if you still need any changes from me. |
main.c
Outdated
| { | ||
| char *kernel_file; | ||
| char *dtb_file; | ||
| char *initrd_file; | ||
| char *disk_file; | ||
| char *netdev; | ||
| int hart_count = 1; | ||
| bool debug = false; | ||
| emu_state_t *emu = &semu->emu; |
There was a problem hiding this comment.
The statement emu_state_t *emu = &semu->emu; does not look elegant since adjacent semu and emu cause confusion. Can you refine it?
There was a problem hiding this comment.
The variable name semu for semu_t is very intuitive, and I don't want to change emu to other names as it used to define like. The solution I can come up with here is to change semu_t to other names. Any suggestion?
There was a problem hiding this comment.
How about consolidating the existing semu structure instead?
Code Review Agent Run #fb547cActionable Suggestions - 2
Review Details
AI Code Review powered by |
| vm_step(vm->hart[i]); | ||
| if (likely(!vm->hart[i]->error)) | ||
| continue; |
There was a problem hiding this comment.
Consider moving the error check for vm_step() before updating timer and SWI interrupts. If vm_step() fails, the interrupt state may be inconsistent.
Code suggestion
Check the AI-generated fix before applying
- emu_update_timer_interrupt(vm->hart[i]);
- emu_update_swi_interrupt(vm->hart[i]);
-
- vm_step(vm->hart[i]);
- if (likely(!vm->hart[i]->error))
- continue;
+ vm_step(vm->hart[i]);
+ if (likely(!vm->hart[i]->error)) {
+ emu_update_timer_interrupt(vm->hart[i]);
+ emu_update_swi_interrupt(vm->hart[i]);
+ continue;
+ }
Code Review Run #fb547c
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| vm_step(vm->hart[i]); | ||
| if (likely(!vm->hart[i]->error)) | ||
| continue; |
There was a problem hiding this comment.
Consider adding error handling for vm_step() return value. The function may return an error state that should be handled before continuing execution.
Code suggestion
Check the AI-generated fix before applying
| vm_step(vm->hart[i]); | |
| if (likely(!vm->hart[i]->error)) | |
| continue; | |
| int step_ret = vm_step(vm->hart[i]); | |
| if (step_ret != 0) | |
| return step_ret; | |
| if (likely(!vm->hart[i]->error)) | |
| continue; |
Code Review Run #fb547c
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
jserv
changed the title
Code Review Agent Run #8e516eActionable Suggestions - 1
Additional Suggestions - 2
Review Details
AI Code Review powered by |
|
Integrate all fields include |
| @@ -355,9 +355,11 @@ bool virtio_snd_init(virtio_snd_state_t *vsnd); | |||
|
|
|||
| /* memory mapping */ | |||
| typedef struct { | |||
| bool debug; | |||
There was a problem hiding this comment.
Consider initializing the debug flag to a default value since it's a boolean field in the emu_state_t struct. This helps prevent undefined behavior if the field is accessed before being explicitly set.
Code suggestion
Check the AI-generated fix before applying
| bool debug; | |
| bool debug = false; |
Code Review Run #8e516e
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Enable the emulator to act as gdbstub. This feature helps to dignose and debug issue of emulator more simply.
Code Review Agent Run #73b0faActionable Suggestions - 0Additional Suggestions - 1
Review Details
AI Code Review powered by |
|
Thank @RinHizakura for contributing! |
Enable the emulator to act as gdbstub. This feature helps to dignose and debug issue of emulator more simply.
Summary by Bito
This PR implements GDB remote debugging functionality by integrating mini-gdbstub library, adding debug mode with -g option, and restructuring the main emulation loop. The changes enable the emulator to function as a GDB remote target on port 1234, supporting interactive debugging through register/memory access and execution control features.Unit tests added: False
Estimated effort to review (1-5, lower is better): 4