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

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

[Pierre Langlois]

Hello all,

With this third version I have corrected some formatting and made
make_cleanup_regcache_invalidate private to regcache.c.

--

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.

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.

Changes
=======

This patch adds routines to add a regcache_invalidate cleanup to the current
chain.

We can then register one before calling target_store_registers. This way if the
target throws an error, the register we wanted to write to will be invalidated
in cache. If target_store_registers succeeds, we can discard the new cleanup.

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-06-12  Pierre Langlois  <pierre.langlois@embecosm.com>

	PR remote/16896
	* regcache.c (struct register_to_invalidate): New structure.
	(do_register_invalidate, make_cleanup_regcache_invalidate): New
	functions.
	(regcache_raw_write): Call make_cleanup_regcache_invalidate.
---
 gdb/regcache.c | 43 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/gdb/regcache.c b/gdb/regcache.c
index 8b588c6..db0f772 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -267,6 +267,32 @@ make_cleanup_regcache_xfree (struct regcache *regcache)
   return make_cleanup (do_regcache_xfree, regcache);
 }
 
+/* Cleanup routines for invalidating a register.  */
+
+struct register_to_invalidate
+{
+  struct regcache *regcache;
+  int regnum;
+};
+
+static void
+do_regcache_invalidate (void *data)
+{
+  struct register_to_invalidate *reg = data;
+
+  regcache_invalidate (reg->regcache, reg->regnum);
+}
+
+static struct cleanup *
+make_cleanup_regcache_invalidate (struct regcache *regcache, int regnum)
+{
+  struct register_to_invalidate* reg = XNEW (struct register_to_invalidate);
+
+  reg->regcache = regcache;
+  reg->regnum = regnum;
+  return make_cleanup_dtor (do_regcache_invalidate, (void *) reg, xfree);
+}
+
 /* Return REGCACHE's architecture.  */
 
 struct gdbarch *
@@ -846,7 +872,8 @@ void
 regcache_raw_write (struct regcache *regcache, int regnum,
 		    const gdb_byte *buf)
 {
-  struct cleanup *old_chain;
+  struct cleanup *chain_before_save_inferior;
+  struct cleanup *chain_before_invalidate_register;
 
   gdb_assert (regcache != NULL && buf != NULL);
   gdb_assert (regnum >= 0 && regnum < regcache->descr->nr_raw_registers);
@@ -864,16 +891,26 @@ regcache_raw_write (struct regcache *regcache, int regnum,
 		  regcache->descr->sizeof_register[regnum]) == 0))
     return;
 
-  old_chain = save_inferior_ptid ();
+  chain_before_save_inferior = save_inferior_ptid ();
   inferior_ptid = regcache->ptid;
 
   target_prepare_to_store (regcache);
   memcpy (register_buffer (regcache, regnum), buf,
 	  regcache->descr->sizeof_register[regnum]);
   regcache->register_status[regnum] = REG_VALID;
+
+  /* Register a cleanup function for invalidating the register after it is
+     written, in case of a failure.  */
+  chain_before_invalidate_register =
+    make_cleanup_regcache_invalidate (regcache, regnum);
+
   target_store_registers (regcache, regnum);
 
-  do_cleanups (old_chain);
+  /* The target did not throw an error so we can discard invalidating the
+     register and restore the cleanup chain to what it was.  */
+  discard_cleanups (chain_before_invalidate_register);
+
+  do_cleanups (chain_before_save_inferior);
 }
 
 void
-- 
1.9.3

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

[Pedro Alves]

Excellent, thanks.

On 06/12/2014 06:35 PM, Pierre Langlois wrote:

" 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?"

These two sentences shouldn't really be in the commit log.

But to answer, if we can make that work, I don't see why not.
I don't think that's a trivial effort though.

> +  chain_before_invalidate_register =
> +    make_cleanup_regcache_invalidate (regcache, regnum);

I missed this before, but '=' goes on the next line, like:

  chain_before_invalidate_register
	= make_cleanup_regcache_invalidate (regcache, regnum);

OK with these changes.

Thanks!

-- 
Pedro Alves

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

[Pierre Langlois]

>> 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?"
> 
> These two sentences shouldn't really be in the commit log.
> 

Yes, absolutely. So what would be the usual way of formatting an email with
adding questions to the mix? I realize this out of the scope of the review so
I'll ask on IRC.

>> +  chain_before_invalidate_register =
>> +    make_cleanup_regcache_invalidate (regcache, regnum);
> 
> I missed this before, but '=' goes on the next line, like:
> 
>   chain_before_invalidate_register
> 	= make_cleanup_regcache_invalidate (regcache, regnum);
> 
> OK with these changes.
> 
> Thanks!
> 

Thank you for your review! I'll address the last formatting issue and commit it.

Best,

Pierre