Changes to gdb_server.c break debugging of some or all non RISC-V targets #115
Comments
|
I don't have time to really dig into this right now, but I can provide a little background. The change in question causes errors encountered while reading/writing registers to be propagated to gdb. Previously if a register could not be read (eg. because it doesn't exist) gdb would think the read was successful, and we'd have to provide some value that is hopefully recognized as bogus. In the log you provided, the new code tells you what register couldn't be accessed. It returns an error to gdb, which in turn decides to abort the session. (I don't know why gdb decides to do so.) I've highlighted that part below: The 'g' packet is gdb requesting to see "all" registers, so it's not even a case of gdb requesting a register that doesn't exist. Maybe the target is running and that is why the request fails. It would need more debugging to figure out. |
|
@TM1234 Can you provide the log file on your non RISC-V target without this change applied? Do you still get the error message that @timsifive identified, but it is silently ignored? At any rate, I suspected that this might cause issues with upstream OpenOCD, since it is returning errors in places that it used to ignore them. I think we should file an issue upstream, since it seems like bad behavior to drop error messages. |
|
Hi Megan - attached is the openocd -d log for a debug session with a Cortex-M3 target with the aforementioned gdb_server.c mods disabled. I don't see any error being ignored - do you? Tim - I wonder if the "Couldn't get register r0" happens because the target is not in debug halt at that stage but the mods to gdb_server.c don't take account of this being a possible reason for not being able to read the regs? Hope this helps. For now I have to build openocd with these mods disabled because they break non RISC-V debugging - or possibly debugging of any target that is not halted when the attempt is made to read the regs. I'm still not clear on what adverse impact this might have on RISC-V debugging? |
|
Sorry - I missed that Tim had posted this earlier:
In this specific case I think that this is the problem alright but the point is that the mods mentioned are assuming that the regs can be read and/or that the target is halted when this may not be the case - hence the mods are not really safe. They work for RISC-V because in many cases the openocd debug config script will have a halt but not all target scripts will have this - nor should they. In fact we have a halt in our RISC-V debug script only because our RISC-V target does not provide the debugger with the ability to do a proper initial reset/halt via JTAG. |
|
It sounds like we should revert this change to gdb_server.c, and understand why it was not fixed in upstream openOCD. This issue that this change was initially set up to handle has since been handled by a workaround in GDB (you can now explicitly set whether your core supports 'C' extension, even if MISA is not in the 1.9.1 location). |
|
This is a more general issue, though. When a user asks to read a CSR, there really isn't a good alternative besides having the debugger try to do the read, and return either success or error. Without this change, the best we can do is return some value like 0 or ~0 and hope the user realizes the CSR isn't present. |
|
I agree that this is a problem. I don't understand why it works for non RISC-V Targets. |
|
Does it work for non-RISC-V targets? Eg. MIPS has a coprocessor register space much like CSRs. How is the problem solved there? A user should be able to request to view an arbitrary one, and get back either its value or some message indicating it doesn't exist. And that message should be true (ie. allowing for cores which implement unknown-to-the-debugger extensions). |
Hi Megan - can you clarify what you mean please? Seems to me that the mods to gdb_server.c are assuming that the registers are always readable. Hope this helps. |
|
The mods to gdb_server.c simply return the error that OpenOCD has taken the time to report. If it is not an error for the registers to be read when a given target is not halted, then OpenOCD wouldn't return an error, and the gdb_server.c mods would have no effect. If it IS an error for the registers to be read when the hart is halted, well, then the upstream tool/script/GDB should be notified and handle it properly. It sounds like the real issue is the latter : for other targets, the upstream tools or scripts (I don't know which) are doing bad things and never realized it before, so they haven't properly handled the error. This particular gdb_server code makes no assumptions about whether the registers can be read or not. If an error is returned, it simply passes it on. |
|
OK - I don't really understand why these changes break Cortex-M debugging but it works without them. |
|
I looked at the two logs. In the "passing" log, the 'g' packet which fails on the "failing" log seems to be completely ignored. GDB closes down the session when the error from the |
|
for the records: |
|
Is there a way to ask about this to the OpenOCD maintainers? Not sure their issue/ Q&A mechanism. |
Any use? |
is this the question you want to ask? isn't this more of a GDB question? on the other side, if the discussion is only about the 4 patches that I just disabled, I think that we are 'fighting against the wind mills', the chances of pushing something like this upstream are not that high. |
|
Tim, do you have a plan how to fix this? Did you identify the reasons why these patches were applied? Personally I would reverse the patches completely, since both myself and Microsemi use OpenOCD like this without any problems. If you have serious reasons to maintain these patches, in order to make them harmless for other platforms, and make them upstreamable, I suggest you add a new flag to the target structure (like is_error_strict, or whatever), and do the extra processing only when this flag is set, otherwise revert to the original behaviour. |
|
I fully agree with Liviu - I had been meaning to revisit this issue but this has nudged me into action. As stated simply earlier several times the changes break at least Cortex-M (and maybe other CPU) debugging but RISC-V debugging (so far) works fine with the changes disabled so they seem to me to be redundant. And they will definitely not be upstreamable as they stand. I vote for their removal too. But if they have to be retained (and the case for retention is very weak as far as I can see) then they should be made optional/configurable as Liviu suggests. |
|
On Fri, Jan 19, 2018 at 1:09 AM, Liviu Ionescu ***@***.***> wrote:
If you have serious reasons to maintain these patches, in order to make
them harmless for other platforms, and make them upstreamable, I suggest
you add a new flag to the target structure (like is_error_strict, or
whatever), and do the extra processing only when this flag is set,
otherwise revert to the original behaviour.
That's my plan.
|
Without this change, connecting to ARM targets is impossible. Fixes #115. Change-Id: Ie33c7e15ac1bed8c9cbd8e6a78de92d5498c5999
|
thank you, Tim. I'll include it in my next release. it would be great to upstream the patch too. |
|
I'm afraid this patch will not be accepted upstream: http://openocd.zylin.com/#/c/4452/ I suggest you either upstream the RISC-V code and later retry to plead for your patch, or simply reverse these changes in your fork, since they seem not necessary anyway. |
|
This is basically what I said a good while back:
I have never understood why ANY changes were required to generic/non target specific code to accommodate RISC-V changes - and unfortunately commit comments never really explained either. |
|
The modified patch was cherry picked on Apr 7. Tim, can you confirm that after the latest merge from master, everything is ok? |
|
I don't have an ARM target so I can't reproduce this problem. As far as I'm aware both mainline and the RISC-V branch now have the |
|
The question was "after the latest merge from master, everything is ok for RISC-V targets?" Do you automatically initialise the flag related to |
|
Just like |
|
So, by default, both are disabled, for all targets. |
|
That's right. Both options default to disabled. |
The changes to gdb_get_registers_packet(), gdb_set_registers_packet(), gdb_get_register_packet() and gdb_set_register_packet() in this commit:
2ae0078#diff-1f3fc53de9d7ac5aa6603362f46a669d
break debugging of some (or maybe all?) non RISC-V targets. The rationale for these changes is also not clear.
For example before these changes (or if these changes are disabled in the latest version of gdb_server.c) debugging of our Cortex-M targets worked fine.
But with these changes enabled Cortex-M (e.g. Cortex-M3 in this case) debugging fails with the following Eclipse error dialog:
and the following OpenOCD log output:
In case it helps the verbose (-d) log output is:
The text was updated successfully, but these errors were encountered: