From patchwork Thu May 8 17:40:51 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pierre Langlois X-Patchwork-Id: 838 Return-Path: X-Original-To: siddhesh@wilcox.dreamhost.com Delivered-To: siddhesh@wilcox.dreamhost.com Received: from homiemail-mx23.g.dreamhost.com (mx2.sub5.homie.mail.dreamhost.com [208.113.200.128]) by wilcox.dreamhost.com (Postfix) with ESMTP id 5549436007A for ; Thu, 8 May 2014 10:41:03 -0700 (PDT) Received: by homiemail-mx23.g.dreamhost.com (Postfix, from userid 14314964) id F0BF2639311A2; Thu, 8 May 2014 10:41:02 -0700 (PDT) X-Original-To: gdb@patchwork.siddhesh.in Delivered-To: x14314964@homiemail-mx23.g.dreamhost.com Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by homiemail-mx23.g.dreamhost.com (Postfix) with ESMTPS id B4BEA6393B84A for ; Thu, 8 May 2014 10:41:02 -0700 (PDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:message-id:date:from:mime-version:to:subject :content-type:content-transfer-encoding; q=dns; s=default; b=KiB V4arbtEo9rZPDg/OW0Ja1mpTinO/FJaLCvhESyjJBC+yfLGTHXyYdKqkT5Cl25uJ JP7GG2gBDy4yZua/V9lufU9nAVlBRBCI5nKugFU8JgD/v+nlPiN2bq7QYhdKQpEO Vyoh7V+t/6O9ZH0wSqwnY+Oyn88YiPieo+JcwcXo= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:message-id:date:from:mime-version:to:subject :content-type:content-transfer-encoding; s=default; bh=FXCDlmaHb rEytTBbOiJM6VPLphg=; b=GTN80XzWDITC77ztUoXcOf8Vo8foIDCJ3GuXxYN54 fSrRUc4sRaDQNeDPHvwjiOXNLRFLxvh0ArYohBD2hNxbSwQ3h7qSCcZ4ddlftjFZ CeRjUdT9NNdc9f8IDtC3VFXVvIQ0wsXqB+xRjrYIBsO4/tkEQMFYF+DlNe7wja0q 2s= Received: (qmail 26998 invoked by alias); 8 May 2014 17:41:00 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 26985 invoked by uid 89); 8 May 2014 17:40:58 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, UNSUBSCRIBE_BODY autolearn=no version=3.3.2 X-HELO: mail-wi0-f177.google.com Received: from mail-wi0-f177.google.com (HELO mail-wi0-f177.google.com) (209.85.212.177) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Thu, 08 May 2014 17:40:57 +0000 Received: by mail-wi0-f177.google.com with SMTP id f8so92899wiw.10 for ; Thu, 08 May 2014 10:40:54 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:date:from:user-agent:mime-version:to :subject:content-type:content-transfer-encoding; bh=b3uR/GqWMqAdZFBFs0WGodflewXO/FnHxuUbltKkovE=; b=jWpDUpWuNQT9XSHuh3Gd1aqmNAZydxZbeYDHHmrP7IZuYvEKkMnFBaAHtrUHHQ5bGs pqHGdkwsmXQ/qgsI921tOOl3ClBVVXsEvO0DVBai8Jq6URcMZ3mgkuhTLcRHMgJCBKQe l9/uOQVqvulbvXnsjshxWZIFFZ6e499byDydElP8q3wJEgHbVNTKHEIQdVrdNOapZY/f n4QSSKn/oziDxZKCWnHHrHgVJiZUqvmy8TK2B2mU1wtg9KgG52Zhql/WTZvXh7D3T0SM CDfytjhJU6WTfTU05RdvS942ytnfSbLC1pzFI7A18yh9pGq6BIOMqPHSSLEll9zjTqsH AlnA== X-Gm-Message-State: ALoCoQkS+CDrbFIkIPQ+xgGzF+jZsUdWxaK9OtkCMLPltrSoatftWmsyw2NB5YdPflDKZyhfkyfk X-Received: by 10.180.105.72 with SMTP id gk8mr14025419wib.32.1399570853902; Thu, 08 May 2014 10:40:53 -0700 (PDT) Received: from [192.168.0.134] (cust64-dsl91-135-5.idnet.net. [91.135.5.64]) by mx.google.com with ESMTPSA id km6sm2109669wjc.6.2014.05.08.10.40.52 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Thu, 08 May 2014 10:40:53 -0700 (PDT) Message-ID: <536BC1A3.7010705@embecosm.com> Date: Thu, 08 May 2014 18:40:51 +0100 From: Pierre Langlois User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 MIME-Version: 1.0 To: gdb-patches@sourceware.org Subject: [RFC][PATCH][PR remote/16896] Invalidate a register in cache when a remote target failed to write it. X-IsSubscribed: yes X-DH-Original-To: gdb@patchwork.siddhesh.in 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 but contains no code. (gdb) set $pc=0x178 Could not write register "PC2"; remote failure reply 'E00' (gdb) info registers pc pc 0x178 0x178 (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 ~~~ 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 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; + } }