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

login
register
mail settings
Submitter Pierre Langlois
Date May 8, 2014, 5:40 p.m.
Message ID <536BC1A3.7010705@embecosm.com>
Download mbox | patch
Permalink /patch/838/
State Changes Requested
Headers show

Comments

Pierre Langlois - May 8, 2014, 5:40 p.m.
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
Pierre Langlois - May 19, 2014, 9:48 a.m.
Ping.

On 08/05/14 18:40, Pierre Langlois wrote:
> 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;
> +      }
>  }
>  
>  
>
Pedro Alves - May 19, 2014, 3:22 p.m.
On 05/19/2014 10:48 AM, Pierre Langlois wrote:

>> +      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);
>> +	  }

Other targets can just as well throw errors.  I think it'd be better to
handle this generically in regcache_raw_read/write instead, around the
target_store_registers call.  Did you consider that?  Note we already
consider errors there, with a cleanup to restore inferior_ptid.

Patch

=====

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;
+      }
 }