This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
[PING][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: Mon, 19 May 2014 10:48:33 +0100
- Subject: [PING][RFC][PATCH][PR remote/16896] Invalidate a register in cache when a remote target failed to write it.
- Authentication-results: sourceware.org; auth=none
- References: <536BC1A3 dot 7010705 at embecosm dot com>
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;
> + }
> }
>
>
>