From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30682 invoked by alias); 19 May 2014 08:26:52 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 30659 invoked by uid 89); 19 May 2014 08:26:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,MIME_BASE64_BLANKS,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mga02.intel.com Received: from mga02.intel.com (HELO mga02.intel.com) (134.134.136.20) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 19 May 2014 08:26:48 +0000 Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga101.jf.intel.com with ESMTP; 19 May 2014 01:26:46 -0700 X-ExtLoop1: 1 Received: from irsmsx104.ger.corp.intel.com ([163.33.3.159]) by orsmga001.jf.intel.com with ESMTP; 19 May 2014 01:26:44 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.70]) by IRSMSX104.ger.corp.intel.com ([169.254.5.98]) with mapi id 14.03.0123.003; Mon, 19 May 2014 09:26:43 +0100 From: "Agovic, Sanimir" To: 'Yao Qi' , Jan Kratochvil CC: Tom Tromey , "gdb-patches@sourceware.org" Subject: RE: [PATCH 07/14] add infcall_mmap and gcc_target_options gdbarch methods Date: Mon, 19 May 2014 08:26:00 -0000 Message-ID: <0377C58828D86C4588AEEC42FC3B85A71D86F725@IRSMSX105.ger.corp.intel.com> References: <1400253995-12333-1-git-send-email-tromey@redhat.com> <1400253995-12333-8-git-send-email-tromey@redhat.com> <5379A051.9040209@codesourcery.com> <20140519064019.GA19564@host2.jankratochvil.net> <5379B528.5040607@codesourcery.com> In-Reply-To: <5379B528.5040607@codesourcery.com> Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 X-IsSubscribed: yes X-SW-Source: 2014-05/txt/msg00333.txt.bz2 PiBPbiAwNS8xOS8yMDE0IDAyOjQwIFBNLCBKYW4gS3JhdG9jaHZpbCB3cm90 ZToNCj4gPj4gSSBkb3VidCB0aGUgaW50ZXJmYWNlIGxpa2UgdGhpcyBpcyBz dWZmaWNpZW50IGZvciBvdGhlciBhcmNocywgbGlrZQ0KPiA+PiA+IGFybSBh bmQgbWlwcywgd2hpY2ggaGF2ZSBtdWx0aXBsZSBtdWx0aWxpYnMsIHN1Y2gg YXMgLW1hcm0vdGh1bWIsDQo+ID4+ID4gLW1mbG9hdC1hYmk9e2hhcmQsc29m dGZwfSwgZXRjLiAgVGhpcyBob29rIGluIEdEQiBoYXMgdG8gdGFrZSBzb21l dGhpbmcNCj4gPj4gPiBpbnRvIGFjY291bnQsIHN1Y2ggYXMgZ2RiYXJjaCwg Y3VycmVudCBmcmFtZSwgdGhlIHJlbGF0ZWQgYmZkLCBldGMsIGluDQo+ID4+ ID4gb3JkZXIgdG8gcmV0dXJuIGEgY29ycmVjdCBvciBjb21wYXRpYmxlIG9w dGlvbnMgZm9yIGdjYyB0byBjb21waWxlDQo+ID4+ID4gc291cmNlLg0KPiA+ IEl0IGFscmVhZHkgYWxyZWFkeSB0YWtlcyAnZ2RiYXJjaCcgYXMgaXRzIHBh cmFtZXRlci4gIElmIGl0IGlzIG5vdCBlbm91Z2ggc29tZQ0KPiA+IG1vcmUg cGFyYW1ldGVycyBjYW4gYmUgYWRkZWQuICBCdXQgSU1PIHRob3NlIHNob3Vs ZCBiZSBhZGRlZCBvbmx5IHdoZW4gdGhpcw0KPiA+IG1ldGhvZCBnZXRzIGlt cGxlbWVudGVkIGZvciBhcmNoIHdoaWNoIG5lZWRzIHN1Y2ggcGFyYW1ldGVy Lg0KPiANCj4gV2UgY2FuIGFkZCB0aGVzZSBwYXJhbWV0ZXJzIHdoZW4gd2Ug cmVhbGx5IG5lZWQgdGhlbS4gIFRoYXQgaXMgZmluZS4NCj4gSG93ZXZlciwg SSBzdGlsbCBkb3VidCB3aGV0aGVyIEdEQiBpcyBhYmxlIHRvIHJldHVybiB0 aGUgY29ycmVjdCBnY2MNCj4gb3B0aW9ucyBieSBtZWFucyBvZiBhbmFseXNp bmcgZXhlY3V0YWJsZSBvbmx5LiAgU3VwcG9zaW5nIHRoZSBleGVjdXRhYmxl DQo+IGlzIGNvbXBpbGVkIHdpdGggIi1tYXJjaD1hcm12Ny1hIC1tdGh1bWIg LW1mbG9hdC1hYmk9aGFyZCAtbWZwdT1uZW9uIiwNCj4gR0RCIHNob3VsZCBr bm93IHRoZSBjb2RlIGlzIHRodW1iIGFuZCBmbG9hdC1hYmkgaXMgaGFyZC4g IEhvdyBjYW4gR0RCDQo+IHRlbGwgIi1tYXJjaD1hcm12Ny1hIC1tZnB1PW5l b24iIGlzIHVzZWQgdG9vPyAgSWYgR0RCIGRvZXNuJ3Qga25vdywNCj4gd2hh dCBvcHRpb25zIHRoaXMgaG9vayBzaG91bGQgcmV0dXJuPyBhbmQgaXMgdGhl IG9iamVjdCBjb2RlDQo+IGNvbXBpbGVkICJvbiB0aGUgZmx5IiBzdGlsbCBj b21wYXRpYmxlIHRvIHRoZSBpbmZlcmlvciBjb2RlIGFuZCB0YXJnZXQNCj4g cnVudGltZT8NCj4gDQpUaGlzIHdpbGwgcHJvYmFibHkgb25seSB3b3JrIGZv ciBob3N0ID09IHRhcmdldCB3aXRoIHRoZSBkZWZhdWx0IGNvbXBpbGVyDQpv cHRpb25zLiBBbnkgb3RoZXIgY2FzZXMgYXJlIGRvb21lZCB0byBmYWlsLiBT byB5b3VyIGV4YW1wbGUgaXMgb25lIG91dCANCm9mIG1hbnkuDQoNCldlIG1h eSBjb25zaWRlciBwaWNraW5nIHRoZSBjdSBkaWUgZm9yIHRoZSBjdXJyZW50 ICRwYyBhbmQgZXh0cmFjdCB0aGUgY29tcGlsZXINCm9wdGlvbnMgZnJvbSBE V19BVF9wcm9kdWNlclsxXS4gQnV0IHRoaXMgcmVxdWlyZXMgcGFyc2luZyB0 aGUgbmVjZXNzYXJ5IGJpdHMgb3V0DQpvZiBhIHN0cmluZy4gQWRkaW5nIHNv bWV0aGluZyBsaWtlIERXX0FUX3Byb2R1Y2VyX29wdGlvbnMgdG8gZHdhcmYg d291bGQgbWFrZQ0KdGhpbmdzIG1vcmUgc3RyYWlnaHQgZm9yd2FyZC4NCg0K WzFdIERXX0FUX3Byb2R1Y2VyICAgIDogWy4uLl0gLW10dW5lPWdlbmVyaWMg LW1hcmNoPXg4Ni02NCAtZw0KDQotU2FuaW1pcg0KDQpJbnRlbCBHbWJICkRv cm5hY2hlciBTdHJhc3NlIDEKODU2MjIgRmVsZGtpcmNoZW4vTXVlbmNoZW4s IERldXRzY2hsYW5kClNpdHogZGVyIEdlc2VsbHNjaGFmdDogRmVsZGtpcmNo ZW4gYmVpIE11ZW5jaGVuCkdlc2NoYWVmdHNmdWVocmVyOiBDaHJpc3RpYW4g TGFtcHJlY2h0ZXIsIEhhbm5lcyBTY2h3YWRlcmVyLCBEb3VnbGFzIEx1c2sK UmVnaXN0ZXJnZXJpY2h0OiBNdWVuY2hlbiBIUkIgNDc0NTYKVXN0Li1JZE5y Li9WQVQgUmVnaXN0cmF0aW9uIE5vLjogREUxMjkzODU4OTUKQ2l0aWJhbmsg RnJhbmtmdXJ0IGEuTS4gKEJMWiA1MDIgMTA5IDAwKSA2MDAxMTkwNTIK >From gdb-patches-return-112496-listarch-gdb-patches=sources.redhat.com@sourceware.org Mon May 19 09:48:43 2014 Return-Path: Delivered-To: listarch-gdb-patches@sources.redhat.com Received: (qmail 16302 invoked by alias); 19 May 2014 09:48:43 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 16266 invoked by uid 89); 19 May 2014 09:48:40 -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-wi0-f169.google.com Received: from mail-wi0-f169.google.com (HELO mail-wi0-f169.google.com) (209.85.212.169) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Mon, 19 May 2014 09:48:38 +0000 Received: by mail-wi0-f169.google.com with SMTP id hi2so4901010wib.2 for ; Mon, 19 May 2014 02:48:35 -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:references:in-reply-to:content-type :content-transfer-encoding; bh=LLrKJOkqb5QbjNoe7HPalNLQZ5gtvG0pkSXIH6Q8rTQ=; b=KJKk5bd88eR0/kt0e1CUVRLwUqesvqQWvKrUkVjLvfo8QrxuYzfiE6VExJ/T5yv9nE H3okMw0dwENfe8bBcob0MP9nxiM04guv3UXuu+bzNmPkiC/7WjjFcQhNFk20dzw4p3AC BHKpcY2BNcWwUbsvpgcUZN2SA2QYUbXjQn4nErUiLSOeWAi32J/Rqmln7C2VsP9Q5tVT Z3i44Xq8I1tiJqEIHJaCpS26GadjifDDPlD5H84zsCG2nt6XfQm5i+BRCCmViUBh8Aax raM0kRO6LAIRQCz/2U3QHIoVzZHLzE+k+3BsWINw5JYdKT2fK1ldXGEJ1CKMq/+x7BEn +tbA== X-Gm-Message-State: ALoCoQnl4CtVvYXzE2DagSB0TzQ6Nr7qk2f5A/kxiIIFVyCYF3z0RoWBGDQxqOPiRP8szCUVPtnP X-Received: by 10.194.189.116 with SMTP id gh20mr28583409wjc.41.1400492915653; Mon, 19 May 2014 02:48:35 -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 w6sm13459894wjq.29.2014.05.19.02.48.34 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Mon, 19 May 2014 02:48:34 -0700 (PDT) Message-ID: <5379D371.1090005@embecosm.com> Date: Mon, 19 May 2014 09:48:00 -0000 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: [PING][RFC][PATCH][PR remote/16896] Invalidate a register in cache when a remote target failed to write it. References: <536BC1A3.7010705@embecosm.com> In-Reply-To: <536BC1A3.7010705@embecosm.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2014-05/txt/msg00334.txt.bz2 Content-length: 8868 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 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; > + } > } > > >