From patchwork Thu Jun 12 17:35:37 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pierre Langlois X-Patchwork-Id: 1471 Received: (qmail 2973 invoked by alias); 12 Jun 2014 17:35:54 -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 2943 invoked by uid 89); 12 Jun 2014 17:35:52 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, UNSUBSCRIBE_BODY autolearn=no version=3.3.2 X-HELO: mail-wg0-f44.google.com Received: from mail-wg0-f44.google.com (HELO mail-wg0-f44.google.com) (74.125.82.44) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Thu, 12 Jun 2014 17:35:51 +0000 Received: by mail-wg0-f44.google.com with SMTP id x13so1621528wgg.27 for ; Thu, 12 Jun 2014 10:35:47 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=nRSdf+Tu01x6j3y3hTCmrhmCUBX3uITrAJVmixTqK0Y=; b=R9z6eHCPnvoHPb4wIy4VCJvqteCZsKOdsziid9M6vfBrTbcwpuXVV3fsglRYtAGP4F Xl9TUr3TSM+MeRy+5gLIFEq7LAOypg6WVnSwliskQSHzORaC/Lr3heSA/5O1pG2Kh+a6 Z5OiLo6iNsxb7gDlXCI96AI0hrhYHnQBQb9AMY3fvL+3yXhaSnWO6PUyNPiJ+kNARaXt lfhQLo9hDMd+1iQJPeZXrGcRJeP7TcgznWjobXcES005akZNz+QagZVPO2r6k9DHuDjH 5hHKFykkZxcFWlfWYpHoi2fCDY161JmaTcaD+fvv1gSTHyw3yJDkh/jFrTlAaKF39MYk 3akA== X-Gm-Message-State: ALoCoQnJpKLKUGlboB/AeKEgKoSVm96NCLEGpcmiC+WDBl0ib+QGU+3mLDffuRhFhoT1KUpBK4zn X-Received: by 10.180.93.234 with SMTP id cx10mr8689068wib.18.1402594547511; Thu, 12 Jun 2014 10:35:47 -0700 (PDT) Received: from localhost.localdomain (cust64-dsl91-135-5.idnet.net. [91.135.5.64]) by mx.google.com with ESMTPSA id do5sm34539353wib.16.2014.06.12.10.35.45 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 12 Jun 2014 10:35:46 -0700 (PDT) From: Pierre Langlois To: gdb-patches@sourceware.org Cc: Pierre Langlois Subject: [PATCH v3][PR remote/16896] Invalidate a register in cache when a remote target failed to write it. Date: Thu, 12 Jun 2014 18:35:37 +0100 Message-Id: <1402594537-8033-1-git-send-email-pierre.langlois@embecosm.com> X-IsSubscribed: yes 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 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. 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 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