riscv-collab / riscv-openocd

Fork of OpenOCD that has RISC-V support
Other
438 stars 319 forks source link

stack pointer is incorrect for suspended FreeRTOS tasks (saved context size is wrong) #861

Open danc86 opened 1 year ago

danc86 commented 1 year ago

I have a target using FreeRTOS v10.2 and when I tell gdb to switch to a suspended task and print a stack trace (info threads, thread 2, where) the stack frames are garbled. I noticed that most registers looked correct but sp was wrong.

In rtos_standard_rv32_stacking the size of the saved context is declared as 34 words, but on my target it's actually only 30 words. This patch fixed it for my target:

--- a/src/rtos/rtos_standard_stackings.c
+++ b/src/rtos/rtos_standard_stackings.c
@@ -407,7 +407,7 @@ const struct rtos_register_stacking rtos_metal_rv32_stacking = {
 };

 const struct rtos_register_stacking rtos_standard_rv32_stacking = {
-       .stack_registers_size = (32 + 2) * 4,
+       .stack_registers_size = 30 * 4,
        .stack_growth_direction = -1,
        .num_output_registers = 33,
        .calculate_process_stack = rtos_generic_stack_align8,

Unfortunately, it's actually a lot messier than that...

This commit originally added RISC-V support in FreeRTOS v10.1: https://github.com/FreeRTOS/FreeRTOS-Kernel/commit/3d8d2f3cc81130af792d805ba5f292f291c7c0f2 and it used a context size of 32 words, and a register layout that seems to match the Sifive Metal fork's register layout.

This commit added a different RISC-V port in FreeRTOS v10.2: https://github.com/FreeRTOS/FreeRTOS-Kernel/commit/ab41d89285bb3df483973f9e2e7b57e6f6a5c36b using a context size of 30 words and a register layout matching the "standard" one in rtos_standard_rv32_stack_offsets. I'm not sure why this IAR port had a different implementation of register saving than the earlier one in the GCC subdirectory. This is the version that I have on my target.

Later this commit in FreeRTOS v10.5: https://github.com/FreeRTOS/FreeRTOS-Kernel/commit/9efca75d1ebfc6c02f9e004c199dffa327267a09 seems to have unified the two implementations and used a context size of 31 words :upside_down_face: with the "standard" register layout except that mstatus is now moved to index 30 rather than 29 (29 holds xCriticalNesting instead).

danc86 commented 1 year ago

It seems like there is no "standard" here so these register stacking configs should probably be moved out of rtos_standard_stackings.c into a FreeRTOS-specific file.

There is also an OpenOCD command to pick the right stacking config, riscv_freertos_stacking, which is great. I could send a PR to add stacking config for the different variations of FreeRTOS (mainline_v10_1, iar_v10_2, mainline = v10.5) so that the user can pick the right one for their target. In my case I would need to use iar_v10_2.

timsifive commented 1 year ago

I could send a PR to add stacking config for the different variations of FreeRTOS (mainline_v10_1, iar_v10_2, mainline = v10.5) so that the user can pick the right one for their target.

That sounds like a good plan. It would be even better if there was some symbol in the FreeRTOS binary that OpenOCD could look at to learn the correct value, but that's probably a bigger issue.