This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
[RFC][PATCH][PR remote/16896] Invalidate a register in cache when a remote target failed to write it.
- From: Pierre Langlois <pierre dot langlois at embecosm dot com>
- To: gdb-patches at sourceware dot org
- Date: Thu, 08 May 2014 18:40:51 +0100
- Subject: [RFC][PATCH][PR remote/16896] Invalidate a register in cache when a remote target failed to write it.
- Authentication-results: sourceware.org; auth=none
Hello all,
As shown by the bug report, GDB crashes when the remote target was unable to
write to a register (the program counter) with the 'P' packet. This was reported
for AVR but can be reproduced on any architecture with a gdbserver that fails to
handle a 'P' packet.
This patch makes the functions sending 'P' and 'G' packets return
a packet_result enum instead of dealing with the error themselves. This allows
remote_store_registers to handle the error and invalidate a register in cache if
it was not written to the hardware.
I have tagged this as [RFC] because I would like some input about the changes
I've made for better error handling. And maybe start off a discussion about how
GDB should handle errors returned by a remote target.
Issue
=====
This GDB session was done with a custom gdbserver patched to send an error
packet when trying to set the program counter with a 'P' packet:
~~~
(gdb) file Debug/ATMega2560-simple-program.elf
Reading symbols from Debug/ATMega2560-simple-program.elf...done.
(gdb) target remote :51000
Remote debugging using :51000
0x00000000 in __vectors ()
(gdb) load
Loading section .text, size 0x1fc lma 0x0
Start address 0x0, load size 508
Transfer rate: 248 KB/sec, 169 bytes/write.
(gdb) b main
Breakpoint 1 at 0x164: file .././ATMega2560-simple-program.c, line 39.
(gdb) c
Continuing.
Program received signal SIGTRAP, Trace/breakpoint trap.
main () at .././ATMega2560-simple-program.c:42
42 DDRD |= LED0_MASK;// | LED1_MASK;
(gdb) info line 43
Line 43 of ".././ATMega2560-simple-program.c" is at address 0x178 <main+40> but contains no code.
(gdb) set $pc=0x178
Could not write register "PC2"; remote failure reply 'E00'
(gdb) info registers pc
pc 0x178 0x178 <main+40>
(gdb) s
../../unisrc-mainline/gdb/infrun.c:1978: internal-error: resume: Assertion `pc_in_thread_step_range (pc, tp)' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n)
../../unisrc-mainline/gdb/infrun.c:1978: internal-error: resume: Assertion `pc_in_thread_step_range (pc, tp)' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Create a core file of GDB? (y or n)
~~~
We can see that even though GDB reports that writing to the register failed, the
register cache was updated:
~~~
(gdb) set $pc=0x178
Could not write register "PC2"; remote failure reply 'E00'
(gdb) info registers pc
pc 0x178 0x178 <main+40>
~~~
The root of the problem is of course in the gdbserver but I thought GDB should
keep a register cache consistent with the hardware even in case of a failure.
Testing
=======
For now, I have tested this with a patched gdbserver, as well as running the
regression test suite. I was wondering how to go about adding a test for this in
the GDB testsuite. Could we include faulty dummy servers to test each packets?
Best,
Pierre
2014-05-08 Pierre Langlois <pierre.langlois@embecosm.com>
PR remote/16896
* remote.c (store_register_using_P): Return packet_result for
error handling.
(store_register_using_G): Likewise.
(remote_store_registers): Handle errors for store_register_using_P
and store_register_using_G. If the remote target sends an error
packet, we should invalidate the register in cache before throwing
an error.
---
gdb/remote.c | 78 ++++++++++++++++++++++++++++++++++++++----------------------
1 file changed, 49 insertions(+), 29 deletions(-)
diff --git a/gdb/remote.c b/gdb/remote.c
index ba04d0c..d5a0b5f 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -6236,10 +6236,10 @@ remote_prepare_to_store (struct target_ops *self, struct regcache *regcache)
}
}
-/* Helper: Attempt to store REGNUM using the P packet. Return fail IFF
- packet was not recognized. */
+/* Helper: Attempt to store REGNUM using the P packet. Return the packet's
+ return value. */
-static int
+static enum packet_result
store_register_using_P (const struct regcache *regcache,
struct packet_reg *reg)
{
@@ -6251,10 +6251,10 @@ store_register_using_P (const struct regcache *regcache,
char *p;
if (packet_support (PACKET_P) == PACKET_DISABLE)
- return 0;
+ return PACKET_UNKNOWN;
if (reg->pnum == -1)
- return 0;
+ return PACKET_UNKNOWN;
xsnprintf (buf, get_remote_packet_size (), "P%s=", phex_nz (reg->pnum, 0));
p = buf + strlen (buf);
@@ -6263,24 +6263,13 @@ store_register_using_P (const struct regcache *regcache,
putpkt (rs->buf);
getpkt (&rs->buf, &rs->buf_size, 0);
- switch (packet_ok (rs->buf, &remote_protocol_packets[PACKET_P]))
- {
- case PACKET_OK:
- return 1;
- case PACKET_ERROR:
- error (_("Could not write register \"%s\"; remote failure reply '%s'"),
- gdbarch_register_name (gdbarch, reg->regnum), rs->buf);
- case PACKET_UNKNOWN:
- return 0;
- default:
- internal_error (__FILE__, __LINE__, _("Bad result from packet_ok"));
- }
+ return packet_ok (rs->buf, &remote_protocol_packets[PACKET_P]);
}
/* Store register REGNUM, or all registers if REGNUM == -1, from the
- contents of the register cache buffer. FIXME: ignores errors. */
+ contents of the register cache buffer. Return the packet's return value. */
-static void
+static enum packet_result
store_registers_using_G (const struct regcache *regcache)
{
struct remote_state *rs = get_remote_state ();
@@ -6313,9 +6302,7 @@ store_registers_using_G (const struct regcache *regcache)
bin2hex (regs, p, rsa->sizeof_g_packet);
putpkt (rs->buf);
getpkt (&rs->buf, &rs->buf_size, 0);
- if (packet_check_result (rs->buf) == PACKET_ERROR)
- error (_("Could not write registers; remote failure reply '%s'"),
- rs->buf);
+ return packet_check_result (rs->buf);
}
/* Store register REGNUM, or all registers if REGNUM == -1, from the contents
@@ -6325,6 +6312,8 @@ static void
remote_store_registers (struct target_ops *ops,
struct regcache *regcache, int regnum)
{
+ struct gdbarch *gdbarch = get_regcache_arch (regcache);
+ struct remote_state *rs = get_remote_state ();
struct remote_arch_state *rsa = get_remote_arch_state ();
int i;
@@ -6341,8 +6330,23 @@ remote_store_registers (struct target_ops *ops,
possible; we often change only a small number of registers.
Sometimes we change a larger number; we'd need help from a
higher layer to know to use 'G'. */
- if (store_register_using_P (regcache, reg))
- return;
+ switch (store_register_using_P (regcache, reg))
+ {
+ case PACKET_OK:
+ return;
+ case PACKET_ERROR:
+ {
+ regcache_invalidate (regcache, regnum);
+ error (_("Could not write register \"%s\"; remote failure reply '%s'"),
+ gdbarch_register_name (gdbarch, reg->regnum), rs->buf);
+ }
+ case PACKET_UNKNOWN:
+ /* Fall back to using a 'G' packet. */
+ break;
+ default:
+ internal_error (__FILE__, __LINE__,
+ _("Bad result from store_register_using_P"));
+ }
/* For now, don't complain if we have no way to write the
register. GDB loses track of unavailable registers too
@@ -6351,17 +6355,33 @@ remote_store_registers (struct target_ops *ops,
if (!reg->in_g_packet)
return;
- store_registers_using_G (regcache);
+ if (store_registers_using_G (regcache) != PACKET_OK)
+ {
+ regcache_invalidate (regcache, regnum);
+ error (_("Could not write register \"%s\"; remote failure reply '%s'"),
+ gdbarch_register_name (gdbarch, reg->regnum), rs->buf);
+ }
return;
}
- store_registers_using_G (regcache);
+ if (store_registers_using_G (regcache) != PACKET_OK)
+ error (_("Could not write registers; remote failure reply '%s'"),
+ rs->buf);
for (i = 0; i < gdbarch_num_regs (get_regcache_arch (regcache)); i++)
if (!rsa->regs[i].in_g_packet)
- if (!store_register_using_P (regcache, &rsa->regs[i]))
- /* See above for why we do not issue an error here. */
- continue;
+ {
+ if (store_register_using_P (regcache, &rsa->regs[i]) == PACKET_ERROR)
+ {
+ regcache_invalidate (regcache, i);
+ error (_("Could not write register \"%s\"; remote failure reply '%s'"),
+ gdbarch_register_name (gdbarch, i), rs->buf);
+ }
+ else
+ /* See above for why we do not issue an error here when the result is
+ PACKET_UNKNOWN. */
+ continue;
+ }
}
--
1.9.0