Message ID | 20160324223241.GB2548@host1.jankratochvil.net |
---|---|
State | New, archived |
Headers |
Received: (qmail 80927 invoked by alias); 24 Mar 2016 22:32:51 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 80687 invoked by uid 89); 24 Mar 2016 22:32:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00, SPF_HELO_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 24 Mar 2016 22:32:46 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id EA247D7E6D for <gdb-patches@sourceware.org>; Thu, 24 Mar 2016 22:32:44 +0000 (UTC) Received: from host1.jankratochvil.net (ovpn-116-22.ams2.redhat.com [10.36.116.22]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u2OMWffR007109 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Thu, 24 Mar 2016 18:32:43 -0400 Date: Thu, 24 Mar 2016 23:32:41 +0100 From: Jan Kratochvil <jan.kratochvil@redhat.com> To: Pedro Alves <palves@redhat.com> Cc: gdb-patches@sourceware.org, Gary Benson <gbenson@redhat.com> Subject: [patchv2 2/2] Workaround gdbserver<7.7 for setfs Message-ID: <20160324223241.GB2548@host1.jankratochvil.net> References: <20160319201842.GA16540@host1.jankratochvil.net> <56F13963.9040204@redhat.com> <20160322131604.GA24312@host1.jankratochvil.net> <56F14F1E.5010606@redhat.com> <20160323211547.GA17400@host1.jankratochvil.net> <20160324220933.GA27445@host1.jankratochvil.net> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="uQr8t48UFsdbeI+V" Content-Disposition: inline In-Reply-To: <20160324220933.GA27445@host1.jankratochvil.net> User-Agent: Mutt/1.5.24 (2015-08-30) X-IsSubscribed: yes |
Commit Message
Jan Kratochvil
March 24, 2016, 10:32 p.m. UTC
There was a bug in patchv1. gdb/ChangeLog 2016-03-24 Jan Kratochvil <jan.kratochvil@redhat.com> * remote.c (packet_ok): Add workaround for PACKET_vFile_setfs.
Comments
On 03/24/2016 10:32 PM, Jan Kratochvil wrote: > There was a bug in patchv1. > > > move2.patch > Please include self-contained a commit/rationale along with patch posts (and reposts). You had context in your intro to v1 that was lost in v2. > > gdb/ChangeLog > 2016-03-24 Jan Kratochvil <jan.kratochvil@redhat.com> > > * remote.c (packet_ok): Add workaround for PACKET_vFile_setfs. > > diff --git a/gdb/remote.c b/gdb/remote.c > index bb027cf..f80fee8 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -1453,7 +1453,15 @@ packet_ok (const char *buf, struct packet_config *config) > internal_error (__FILE__, __LINE__, > _("packet_ok: attempt to use a disabled packet")); > > - result = packet_check_result (buf); > + if (config == &remote_protocol_packets[PACKET_vFile_setfs] > + && strcmp (buf, "OK") == 0) > + { > + /* Workaround gdbserver < 7.7 before its fix from 2013-12-11. */ > + result = PACKET_UNKNOWN; > + } This comment could use more detail. E.g., reading this I'm left wondering, did it always respond OK to unknown vFile packets, or to all unknown packets? I think it was actually the latter. AFAICS from the commit you pointed at in v1, the "OK" was gdbserver mistaking any unknown packet for a vStopped packet, with vStopped being the notification ack for the "%Stop" RSP async notification. So it could also happen that gdb sends the setfs packet while gdbserver had a pending notification, and then gdbserver replies back a stop reply instead of "OK"... We may need to guarantee an early enough setfs is attempted. Is that already the case? If I'm right and gdbserver mishandled _any_ unknown packet, then I wonder whether you fix this one, but will trip on another when you get past initial connection and actually do any serious debugging? If not, this may be sufficient. Otherwise, we may need to come up with a different workaround, maybe based on sending an early probe packet, like "MustReplyEmpty", to which well behaved stubs reply empty, just because that's not a known packet to them. If a stub replies something other than empty to that one, then maybe we should disable all other auto-probed packets... That may force-disable too much functionality though... So in sum: - Expand comment a bit / include commit log with rationale in the patch. - Give assurance that this is sufficient and that we won't trip on the same thing with other packets anyway. Otherwise we may need to think of something else. Thanks, Pedro Alves
On Wed, 30 Mar 2016 16:17:11 +0200, Pedro Alves wrote: > E.g., reading this I'm left wondering, did it always respond OK to > unknown vFile packets, or to all unknown packets? I think it was > actually the latter. Yes. > AFAICS from the commit you pointed at in v1, the "OK" was > gdbserver mistaking any unknown packet for a vStopped packet, > with vStopped being the notification ack for the "%Stop" RSP async > notification. So it could also happen that gdb sends the setfs > packet while gdbserver had a pending notification, and then > gdbserver replies back a stop reply instead of "OK"... OK, I did not realize this possible regression. > We may need to guarantee an early enough setfs is attempted. > Is that already the case? For linux targets it is because they read: /proc/29202/smaps Although you are right that does not need to be the case for non-linux targets. setfs packet seems to be implemented linux-independently. > If I'm right and gdbserver mishandled _any_ unknown packet, > then I wonder whether you fix this one, but will trip on another > when you get past initial connection and actually do any serious > debugging? That would mean gdbserver < 7.7 did not work for "any serious debugging". I have seen the regression only since "setfs" but I admit I did not do "any serious debugging". > If not, this may be sufficient. Otherwise, we may need to come up with > a different workaround, maybe based on sending an early probe packet, > like "MustReplyEmpty", to which well behaved stubs reply empty, just because > that's not a known packet to them. If a stub replies something other than > empty to that one, then maybe we should disable all other > auto-probed packets... That may force-disable too much functionality though... > > So in sum: The patch was fixing a common use case with RHEL<=7 targets. You have provided out a counterexample that it may hypothetically regress in a racy case of non-linux FSF gdbserver. Normally I would find this workaround applicable only for RHEL GDBs but now that everything needs to be upstream first the RHEL workaround needs to be implemented in FSF GDB first (where it does not belong much IMNSHO). I will therefore try to implement the "MustReplyEmpty" packet but I have no idea what effect will have your mentioned "disable all other auto-probed packets". Thanks, Jan
diff --git a/gdb/remote.c b/gdb/remote.c index bb027cf..f80fee8 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -1453,7 +1453,15 @@ packet_ok (const char *buf, struct packet_config *config) internal_error (__FILE__, __LINE__, _("packet_ok: attempt to use a disabled packet")); - result = packet_check_result (buf); + if (config == &remote_protocol_packets[PACKET_vFile_setfs] + && strcmp (buf, "OK") == 0) + { + /* Workaround gdbserver < 7.7 before its fix from 2013-12-11. */ + result = PACKET_UNKNOWN; + } + else + result = packet_check_result (buf); + switch (result) { case PACKET_OK: