)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"9b9f7f05cc68754b08b7b55c2afc38896f9bc22f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"9e5afcd4_3b29717d","updated":"2025-11-11 17:21:25.000000000","message":"Thanks!","commit_id":"81b199629f658423d15fb422d9b2a33bbdb32e2b"}],"src/target/armv4_5.c":[{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"67caae3ee365b03815140cd196a0877c36c9cf94","unresolved":true,"context_lines":[{"line_number":703,"context_line":"\t\treg_list[i].arch_info \u003d \u0026reg_arch_info[i];"},{"line_number":704,"context_line":"\t\treg_list[i].exist \u003d true;"},{"line_number":705,"context_line":""},{"line_number":706,"context_line":"\t\t/* This really depends on the calling convention in use."},{"line_number":707,"context_line":"\t\t * Set to loosely reflect AAPCS and checked with gdb function"},{"line_number":708,"context_line":"\t\t * call command not to change the registers of the debugged program. */"},{"line_number":709,"context_line":"\t\treg_list[i].caller_save \u003d i \u003c 4 || i \u003d\u003d 9"}],"source_content_type":"text/x-csrc","patch_set":1,"id":"ba1da8ce_345c38b3","line":706,"updated":"2025-11-09 23:59:23.000000000","message":"Actually this has nothing to do with the target\u0027s calling convention!\nI think we have completely missed the point.\nThis flag is used by OpenOCD in\nhttps://review.openocd.org/c/openocd/+/9208/1/src/server/gdb_server.c#2531\nto send to GDB the register attribute\n`save-restore\u003d\"yes\"`\nor\n`save-restore\u003d\"no\"`\n\nhttps://sourceware.org/gdb/current/onlinedocs/gdb.html/Target-Description-Format.html#Registers-2\nreports:\n```\nsave-restore\n  Whether the register should be preserved across inferior function calls;\n  this must be either yes or no. The default is yes, which is appropriate\n  for most registers except for some system control registers; this is not\n  related to the target’s ABI.\n```\n\nAnd in GDB source code \nhttps://sourceware.org/git/?p\u003dbinutils-gdb.git;a\u003dblob;f\u003dgdbsupport/tdesc.h;h\u003deafa95da955e70e64253229ebfc5b3424f4f7465#l89\n```\n/* If this flag is set, GDB should save and restore this register\n   around calls to an inferior function.  */\nint save_restore;\n```\n\nSo, I read this as related to the \"calling\" chapter in\nhttps://sourceware.org/gdb/current/onlinedocs/gdb.html/Altering.html\n\nWhen GDB calls a function in the program, it needs to know if some CPU register has to be saved and restored to prevent destroying the current context of execution.\nI think GDB saves them as local data in the GDB memory.\n\nDigging in GDB code, when it creates an inferior of a certain architecture, it already has default equivalent to `save-restore\u003d\"yes\"`.\nOnly some system register has \"no\" in architectures s390 and rs6000, plus the register \"tag_ctl\" in aarch64 with MTE (Memory Tagging Extension)\nThen the XML target description from the remote gdb-server (OpenOCD) can override such defaults.\n\nI think we should put `true` everywhere, apart some specific case as in GDB.\n\nMaybe we should even revert the logic, having this flag named `no_save_restore`.\nBy default it would be `false`, thus sending `save-restore\u003d\"yes\"`.\nOnly when we want to send `save-restore\u003d\"no\"` we set it to `true`.","commit_id":"b8b6f3ec67c381fa094e31d5a59f2fd50aaca69d"},{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"4449c63a708248951f431f4c7707927df90c9feb","unresolved":true,"context_lines":[{"line_number":703,"context_line":"\t\treg_list[i].arch_info \u003d \u0026reg_arch_info[i];"},{"line_number":704,"context_line":"\t\treg_list[i].exist \u003d true;"},{"line_number":705,"context_line":""},{"line_number":706,"context_line":"\t\t/* This really depends on the calling convention in use."},{"line_number":707,"context_line":"\t\t * Set to loosely reflect AAPCS and checked with gdb function"},{"line_number":708,"context_line":"\t\t * call command not to change the registers of the debugged program. */"},{"line_number":709,"context_line":"\t\treg_list[i].caller_save \u003d i \u003c 4 || i \u003d\u003d 9"}],"source_content_type":"text/x-csrc","patch_set":1,"id":"cd44e78c_b057b5a7","line":706,"in_reply_to":"7816ab27_95c920c2","updated":"2025-11-10 14:12:04.000000000","message":"Maybe more valuable having the flag changed as three-state, to keep more flexibility:\n```\nenum gdb_save_restore {\n    GDB_DEFAULT_SAVE_RESTORE,\n    GDB_NO_SAVE_RESTORE,\n    GDB_SAVE_RESTORE,\n};\n\nswitch (reg_list[i]-\u003esave_restore) {\nGDB_NO_SAVE_RESTORE:\n    xml_printf(\u0026retval, \u0026tdesc, \u0026pos, \u0026size, \" save-restore\u003d\\\"no\\\"\");\n    break;\nGDB_SAVE_RESTORE:\n    xml_printf(\u0026retval, \u0026tdesc, \u0026pos, \u0026size, \" save-restore\u003d\\\"yes\\\"\");\n    break;\nGDB_DEFAULT_SAVE_RESTORE:\ndefault:\n    // send nothing, keep GDB default\n    break;\n}\n```\nBut this is not urgent.\n\nInstead, I run some test on cortex-a and I tracked the registers changed by GDB while calling the `fib()` function copied from riscv-tests. They are: r0-r3, sp, lr, pc, cpsr, fpscr.\nGDB runs the following, to execute the function:\n- set \u0027lr\u0027 to ELF entry point;\n- set \u0027r0\u0027 to the parameter of the function;\n- decrement \u0027sp\u0027 by 8 (I don\u0027t know why. Maybe based on stack frame size of function at current PC);\n- set \u0027pc\u0027 at the function to execute;\n- set BP at the ELF entry point (now the return address of the function);\n- continue.\n\nWith all registers in save-restore (with exception of FPU regs), I\u0027m able to continue running my firmware after GDB runs the function.\nBut if I set save-restore also the FPU regs, then GDB crashes for SIGSEGV while sending\n`[remote] Sending packet: $P33\u003d2074ff2f00000000#a4`\nand OpenOCD say:\n`Error: gdb sent 64 bits for a 32-bit register (sp_hyp)`\nI believe we have a huge mess in the list of cortex-a registers... to be investigated asap.\n\nOn your side, why you say the test doesn\u0027t pass? What effect you see or what\u0027s operation that doesn\u0027t work?","commit_id":"b8b6f3ec67c381fa094e31d5a59f2fd50aaca69d"},{"author":{"_account_id":1000687,"name":"Tomas Vanek","display_name":"Tomas Vanek","email":"vanekt@fbl.cz","username":"vanekt"},"change_message_id":"93db4d0cd568139df6ca1cef85efe46eb8e72c7c","unresolved":true,"context_lines":[{"line_number":703,"context_line":"\t\treg_list[i].arch_info \u003d \u0026reg_arch_info[i];"},{"line_number":704,"context_line":"\t\treg_list[i].exist \u003d true;"},{"line_number":705,"context_line":""},{"line_number":706,"context_line":"\t\t/* This really depends on the calling convention in use."},{"line_number":707,"context_line":"\t\t * Set to loosely reflect AAPCS and checked with gdb function"},{"line_number":708,"context_line":"\t\t * call command not to change the registers of the debugged program. */"},{"line_number":709,"context_line":"\t\treg_list[i].caller_save \u003d i \u003c 4 || i \u003d\u003d 9"}],"source_content_type":"text/x-csrc","patch_set":1,"id":"7816ab27_95c920c2","line":706,"in_reply_to":"ba1da8ce_345c38b3","updated":"2025-11-10 06:46:08.000000000","message":"\u003e Actually this has nothing to do with the target\u0027s calling convention!\n\u003e I think we have completely missed the point.\n\nI don\u0027t think so except the really misleading `caller_save` variable name.\nIf the target\u0027s calling convention requests procedures to save some register then gdb will save and restore it unnecessarily. But doing so should not impose a problem, it could be the safer side.\n\n\u003e I think we should put `true` everywhere, apart some specific case as in GDB.\n\nUnfortunately not as easy as that.\nriscv code sets `caller_save \u003d true` and `DebugFunctionCall` from riscv-tests passes.\nIt works also for Cortex-M, where `caller_save \u003d true` for all regs and `DebugFunctionCall` from ported part of riscv-tests passed.\nAlso lakemont, armv8, mips32/64, stm8 and arc targets set `caller_save \u003d true` usually with pointless copypasta comment `/* gdb defaults to true */` but no idea how they works under gdb.\n\nSo logically when I saw fails in `DebugFunctionCall` from ported riscv-tests I tried to set `caller_save \u003d true` for all regs first. But it breaks again!\nThen I suspected that restoring the core-state-banked regs could make some problem. However excluding them (`caller_save \u003d false`) didn\u0027t help too.\nI ended up with the minimal set of registers to save and the test passed. Sure we can add r4..r8, r10, r11, which are according to AAPCS saved by called procedure.\nAnd I see I left out r12, which should be saved.\nBut saving one of LR, SP causes the test fails, I would bet something is screwed in gdb arm-tdep code. Or a bug in cortex_a target code?\n\n\u003e Maybe we should even revert the logic, having this flag named `no_save_restore`.\n\u003e By default it would be `false`, thus sending `save-restore\u003d\"yes\"`.\n\u003e Only when we want to send `save-restore\u003d\"no\"` we set it to `true`.\n\nNot a bad idea. Until we met the register which gdb defaults to `save-restore\u003d\"no\"` and we need to force saving it.","commit_id":"b8b6f3ec67c381fa094e31d5a59f2fd50aaca69d"},{"author":{"_account_id":1000687,"name":"Tomas Vanek","display_name":"Tomas Vanek","email":"vanekt@fbl.cz","username":"vanekt"},"change_message_id":"2c9baa953b21f6ce8a9f8aacd2772370bdc41e16","unresolved":true,"context_lines":[{"line_number":703,"context_line":"\t\treg_list[i].arch_info \u003d \u0026reg_arch_info[i];"},{"line_number":704,"context_line":"\t\treg_list[i].exist \u003d true;"},{"line_number":705,"context_line":""},{"line_number":706,"context_line":"\t\t/* This really depends on the calling convention in use."},{"line_number":707,"context_line":"\t\t * Set to loosely reflect AAPCS and checked with gdb function"},{"line_number":708,"context_line":"\t\t * call command not to change the registers of the debugged program. */"},{"line_number":709,"context_line":"\t\treg_list[i].caller_save \u003d i \u003c 4 || i \u003d\u003d 9"}],"source_content_type":"text/x-csrc","patch_set":1,"id":"cd153e09_ed002828","line":706,"in_reply_to":"c7651a1f_86cd3170","updated":"2025-11-10 17:03:28.000000000","message":"I see one problem: gdb restores registers after the first and before the second call and stores different values to the same register (base and the banked alias):\n```\nDebug: 2488 8505 arm_dpm.c:351 dpm_write_reg(): [stm32mp15x.cpu0] WRITE: sp, 1005bfb8\nDebug: 2489 8505 cortex_a.c:336 cortex_a_write_dcc(): write DCC 0x10000000\nDebug: 2490 8506 arm_dpm.c:351 dpm_write_reg(): [stm32mp15x.cpu0] WRITE: lr, 10000000\nDebug: 2491 8506 cortex_a.c:336 cortex_a_write_dcc(): write DCC 0x00000010\nDebug: 2492 8508 cortex_a.c:336 cortex_a_write_dcc(): write DCC 0x1005bfc0\nDebug: 2493 8509 arm_dpm.c:351 dpm_write_reg(): [stm32mp15x.cpu0] WRITE: sp_usr, 1005bfc0\nDebug: 2494 8509 cortex_a.c:336 cortex_a_write_dcc(): write DCC 0x10000aac\nDebug: 2495 8510 arm_dpm.c:351 dpm_write_reg(): [stm32mp15x.cpu0] WRITE: lr_usr, 10000aac\n```\nAt least lr should be 0x10000000 as the catch return breakpoint is at 0x10000000\nExactly as I expected.\nLet\u0027s try mark `caller_save \u003d true` just the non banked regs.","commit_id":"b8b6f3ec67c381fa094e31d5a59f2fd50aaca69d"},{"author":{"_account_id":1000687,"name":"Tomas Vanek","display_name":"Tomas Vanek","email":"vanekt@fbl.cz","username":"vanekt"},"change_message_id":"ecbf6cbd982549b97f0f640dd66fb24c8c3a80fb","unresolved":true,"context_lines":[{"line_number":703,"context_line":"\t\treg_list[i].arch_info \u003d \u0026reg_arch_info[i];"},{"line_number":704,"context_line":"\t\treg_list[i].exist \u003d true;"},{"line_number":705,"context_line":""},{"line_number":706,"context_line":"\t\t/* This really depends on the calling convention in use."},{"line_number":707,"context_line":"\t\t * Set to loosely reflect AAPCS and checked with gdb function"},{"line_number":708,"context_line":"\t\t * call command not to change the registers of the debugged program. */"},{"line_number":709,"context_line":"\t\treg_list[i].caller_save \u003d i \u003c 4 || i \u003d\u003d 9"}],"source_content_type":"text/x-csrc","patch_set":1,"id":"f3560750_635779a8","line":706,"in_reply_to":"cd153e09_ed002828","updated":"2025-11-10 17:16:25.000000000","message":"Previously I missed that not banked LR is @ index 38 and used lr_usr @ 14 instead - that was why I wrote that saving one of LR, SP breaks the test.\nWith\n```\nreg_list[i].caller_save \u003d i \u003c\u003d 12\n                          || i \u003d\u003d 15  // PC\n                          || i \u003d\u003d 37  // SP\n                          || i \u003d\u003d 38; // LR\n```\nthe DebugFunctionCall passes.\nSeems better. Do you agree?\n\nAnd what about fp regs? Save all?","commit_id":"b8b6f3ec67c381fa094e31d5a59f2fd50aaca69d"},{"author":{"_account_id":1000687,"name":"Tomas Vanek","display_name":"Tomas Vanek","email":"vanekt@fbl.cz","username":"vanekt"},"change_message_id":"48c21e279c0b839cb757dd37f2d8e97e3e8e84f9","unresolved":true,"context_lines":[{"line_number":703,"context_line":"\t\treg_list[i].arch_info \u003d \u0026reg_arch_info[i];"},{"line_number":704,"context_line":"\t\treg_list[i].exist \u003d true;"},{"line_number":705,"context_line":""},{"line_number":706,"context_line":"\t\t/* This really depends on the calling convention in use."},{"line_number":707,"context_line":"\t\t * Set to loosely reflect AAPCS and checked with gdb function"},{"line_number":708,"context_line":"\t\t * call command not to change the registers of the debugged program. */"},{"line_number":709,"context_line":"\t\treg_list[i].caller_save \u003d i \u003c 4 || i \u003d\u003d 9"}],"source_content_type":"text/x-csrc","patch_set":1,"id":"c7651a1f_86cd3170","line":706,"in_reply_to":"cd44e78c_b057b5a7","updated":"2025-11-10 16:51:34.000000000","message":"I re-run DebugFunctionCall with all non fp regs `caller_save \u003d true` and fp regs not saved. It is also important to say that I used gdb vanilla 16.2 built from the source on aarch64-unknown-linux-gnu and configured as multiarch\n`../configure --enable-targets\u003darm-none-eabi,riscv64-unknown-elf,avr-unknown-elf --disable-sim`\n\nThe test fails on the second call to fib():\n```\n(gdb) p/x fib(6)\np/x fib(6)\n$1 \u003d 0x8\n(gdb) p/x fib(7)\np/x fib(7)\n \nBreakpoint 1, _exit (status\u003d13) at programs/arm-a/init.c:9\n9           volatile int i \u003d 42;\nThe program being debugged stopped while in a function called from GDB.\nEvaluation of the expression containing the function\n(fib) will be abandoned.\n```\nI\u0027ll look what happens on the register level.","commit_id":"b8b6f3ec67c381fa094e31d5a59f2fd50aaca69d"},{"author":{"_account_id":1000687,"name":"Tomas Vanek","display_name":"Tomas Vanek","email":"vanekt@fbl.cz","username":"vanekt"},"change_message_id":"297e445a1a3fdd6558f3f7d2bbfd4e710cca6147","unresolved":false,"context_lines":[{"line_number":703,"context_line":"\t\treg_list[i].arch_info \u003d \u0026reg_arch_info[i];"},{"line_number":704,"context_line":"\t\treg_list[i].exist \u003d true;"},{"line_number":705,"context_line":""},{"line_number":706,"context_line":"\t\t/* This really depends on the calling convention in use."},{"line_number":707,"context_line":"\t\t * Set to loosely reflect AAPCS and checked with gdb function"},{"line_number":708,"context_line":"\t\t * call command not to change the registers of the debugged program. */"},{"line_number":709,"context_line":"\t\treg_list[i].caller_save \u003d i \u003c 4 || i \u003d\u003d 9"}],"source_content_type":"text/x-csrc","patch_set":1,"id":"bfdb4218_4249b8b9","line":706,"in_reply_to":"e29cd1bd_27be7643","updated":"2025-11-11 12:29:40.000000000","message":"Yes, both are included in the new patchset.","commit_id":"b8b6f3ec67c381fa094e31d5a59f2fd50aaca69d"},{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"c98f7dcda25426383ef84b3e07d2779868f7b6e9","unresolved":true,"context_lines":[{"line_number":703,"context_line":"\t\treg_list[i].arch_info \u003d \u0026reg_arch_info[i];"},{"line_number":704,"context_line":"\t\treg_list[i].exist \u003d true;"},{"line_number":705,"context_line":""},{"line_number":706,"context_line":"\t\t/* This really depends on the calling convention in use."},{"line_number":707,"context_line":"\t\t * Set to loosely reflect AAPCS and checked with gdb function"},{"line_number":708,"context_line":"\t\t * call command not to change the registers of the debugged program. */"},{"line_number":709,"context_line":"\t\treg_list[i].caller_save \u003d i \u003c 4 || i \u003d\u003d 9"}],"source_content_type":"text/x-csrc","patch_set":1,"id":"e29cd1bd_27be7643","line":706,"in_reply_to":"f3560750_635779a8","updated":"2025-11-10 18:20:29.000000000","message":"I think that also CPSR (31) should be saved.\nImagine we halt in the middle of a function and in GDB we call a new function.\nThe condition flags were already in use when halt, but now get polluted by the GDB execution. They should be restored.\nYes, FP should be saved, as we don\u0027t know if the function called by GDB will use them. And also FPSCR should be saved, as CPSR.","commit_id":"b8b6f3ec67c381fa094e31d5a59f2fd50aaca69d"}]}