This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[RFC][PATCH][PR remote/16896] Invalidate a register in cache when a remote target failed to write it.


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]