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]

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


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


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