From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26115 invoked by alias); 29 Apr 2014 02:10:16 -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 22820 invoked by uid 89); 29 Apr 2014 02:09:05 -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 autolearn=ham version=3.3.2 X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 29 Apr 2014 02:09:01 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1WexTc-0002LZ-Iu from Hui_Zhu@mentor.com for gdb-patches@sourceware.org; Mon, 28 Apr 2014 19:08:56 -0700 Received: from SVR-ORW-FEM-05.mgc.mentorg.com ([147.34.97.43]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Mon, 28 Apr 2014 19:08:56 -0700 Received: from localhost.localdomain (147.34.91.1) by svr-orw-fem-05.mgc.mentorg.com (147.34.97.43) with Microsoft SMTP Server id 14.2.247.3; Mon, 28 Apr 2014 19:07:41 -0700 Message-ID: <535F09B6.1050104@mentor.com> Date: Tue, 29 Apr 2014 02:10:00 -0000 From: Hui Zhu User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.4.0 MIME-Version: 1.0 To: Subject: Re: [PATCH v2] Fix several "set remote foo-packet on/off" commands. References: <1396307414-2053-1-git-send-email-palves@redhat.com> <533A7E83.4070200@codesourcery.com> <533AABE1.8040101@redhat.com> <533AB01E.4060003@redhat.com> <20140428191608.GA9089@adacore.com> <535EF9DC.4050706@redhat.com> In-Reply-To: <535EF9DC.4050706@redhat.com> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2014-04/txt/msg00605.txt.bz2 On 04/29/14 09:01, Pedro Alves wrote: > Hi Joel, > > Bummer, sorry for the trouble. > > On 04/28/2014 08:16 PM, Joel Brobecker wrote: > >> I worked around the issue by making this package a configurable >> packet (diff attached) but I have a feeling that there is either >> something I am not getting, or perhaps a hole in the current >> design. Or perhaps something else? I am not quite clear on what >> should be done, yet. > > I think the design is sound. See more info in the patch below. > > I'd be fine with either: > > - restoring things to how they've "always" been immediately. > That is, push the patch below. We can then incrementally add the > missing associated commands, along with corresponding manual and > possibly testsuite changes/additions, as a non-priority task. This patch fixed my issue with qemu that donn't support qnonstop. Thanks, Hui > > - or, adding all the missing commands now, and add an assertion just > like in the patch below, but with no exception list, of course. > (but TBC, I can't offer to work on that myself now.) > > Let me know what you think. > > ------------- > From 37a80ecbfd5cab03a3a88f73d7d06bc6a4f757b9 Mon Sep 17 00:00:00 2001 > From: Pedro Alves > Date: Tue, 29 Apr 2014 01:14:22 +0100 > Subject: [PATCH] Fix remote connection to targets that don't support the > QNonStop packet. > > ... and others. The recent patch that fixed several "set remote > foo-packet on/off" commands introduced a regression, observable when > connecting GDB to QEMU. For instance: > > (gdb) set debug remote 1 > (gdb) tar rem :4444 > Remote debugging using :4444 > Sending packet: $qSupported:multiprocess+;qRelocInsn+#2a...Ack > Packet received: PacketSize=1000;qXfer:features:read+ > Packet qSupported (supported-packets) is supported > Sending packet: $Hgp0.0#ad...Ack > Packet received: OK > Sending packet: $qXfer:features:read:target.xml:0,ffb#79...Ack > Packet received: [...] > Sending packet: $qXfer:features:read:arm-core.xml:0,ffb#08...Ack > Packet received: [...] > !!! -> Sending packet: $QNonStop:0#8c...Ack > Packet received: > Remote refused setting all-stop mode with: > > The "QNonStop" feature is associated with the PACKET_QNonStop packet, > with a default of PACKET_DISABLE, so GDB should not be sending the > packet at all. > > The patch that introduced the regression decoupled packet_config's > 'detect' and 'support' fields, making the former (an auto_boolean) > purely the associated "set remote foo-packet" command's variable. In > the example above, the packet config's 'supported' field does end up > correctly set to PACKET_DISABLE. However, nothing is presently > initializing packet configs that don't actually have a command > associated. Those configs's 'detect' field then ends up set to > AUTO_BOOLEAN_TRUE, simply because that happens to be 0. This forces > GDB to assume the packet is supported, irrespective of what the target > claims it supports, just like if the user had done "set remote > foo-packet on" (this being the associated command, if there was one). > > Ideally, all packet configs would have a command associated. While > that isn't true, make sure all packet configs are initialized, even if > no command is associated, and add an assertion that prevents adding > more packets/features without an associated command. > > Tested on x86_64 Fedora 17, against pristine gdbserver, and against a > gdbserver with the QNonStop packet/feature disabled with a local hack. > > gdb/ > 2014-04-29 Pedro Alves > > * remote.c (struct packet_config) : Extend comment. > (add_packet_config_cmd): Don't set the config's detect or support > fields here. > (init_all_packet_configs): Also initialize the config's 'detect' > field. > (reset_all_packet_configs_support): New function. > (remote_open_1): Call reset_all_packet_configs_support instead of > init_all_packet_configs. > (_initialize_remote): Initialize all packet configs. Assert that > all packets have an associated command, except a few known > outliers. > --- > gdb/remote.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 54 insertions(+), 6 deletions(-) > > diff --git a/gdb/remote.c b/gdb/remote.c > index dcd3cdd..4177b39 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -1067,7 +1067,8 @@ struct packet_config > /* If auto, GDB auto-detects support for this packet or feature, > either through qSupported, or by trying the packet and looking > at the response. If true, GDB assumes the target supports this > - packet. If false, the packet is disabled. */ > + packet. If false, the packet is disabled. Configs that don't > + have an associated command always have this set to auto. */ > enum auto_boolean detect; > > /* Does the target support this packet? */ > @@ -1129,8 +1130,6 @@ add_packet_config_cmd (struct packet_config *config, const char *name, > > config->name = name; > config->title = title; > - config->detect = AUTO_BOOLEAN_AUTO; > - config->support = PACKET_SUPPORT_UNKNOWN; > set_doc = xstrprintf ("Set use of remote protocol `%s' (%s) packet", > name, title); > show_doc = xstrprintf ("Show current use of remote " > @@ -3632,10 +3631,11 @@ extended_remote_open (char *name, int from_tty) > remote_open_1 (name, from_tty, &extended_remote_ops, 1 /*extended_p */); > } > > -/* Generic code for opening a connection to a remote target. */ > +/* Reset all packets back to "unknown support". Called when opening a > + new connection to a remote target. */ > > static void > -init_all_packet_configs (void) > +reset_all_packet_configs_support (void) > { > int i; > > @@ -3643,6 +3643,20 @@ init_all_packet_configs (void) > remote_protocol_packets[i].support = PACKET_SUPPORT_UNKNOWN; > } > > +/* Initialize all packet configs. */ > + > +static void > +init_all_packet_configs (void) > +{ > + int i; > + > + for (i = 0; i < PACKET_MAX; i++) > + { > + remote_protocol_packets[i].detect = AUTO_BOOLEAN_AUTO; > + remote_protocol_packets[i].support = PACKET_SUPPORT_UNKNOWN; > + } > +} > + > /* Symbol look-up. */ > > static void > @@ -4192,7 +4206,7 @@ remote_open_1 (char *name, int from_tty, > > /* Reset the target state; these things will be queried either by > remote_query_supported or as they are needed. */ > - init_all_packet_configs (); > + reset_all_packet_configs_support (); > rs->cached_wait_status = 0; > rs->explicit_packet_size = 0; > rs->noack_mode = 0; > @@ -11869,6 +11883,8 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL, > NULL, /* FIXME: i18n: */ > &setlist, &showlist); > > + init_all_packet_configs (); > + > add_packet_config_cmd (&remote_protocol_packets[PACKET_X], > "X", "binary-download", 1); > > @@ -12052,6 +12068,38 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL, > add_packet_config_cmd (&remote_protocol_packets[PACKET_qXfer_btrace], > "qXfer:btrace", "read-btrace", 0); > > + /* Assert that we've registered commands for all packet configs. */ > + { > + int i; > + > + for (i = 0; i < PACKET_MAX; i++) > + { > + /* Ideally all configs would have a command associated. Some > + still don't though. */ > + int excepted; > + > + switch (i) > + { > + case PACKET_QNonStop: > + case PACKET_multiprocess_feature: > + case PACKET_EnableDisableTracepoints_feature: > + case PACKET_tracenz_feature: > + case PACKET_DisconnectedTracing_feature: > + case PACKET_augmented_libraries_svr4_read_feature: > + /* Additions to this list need to be well justified. */ > + excepted = 1; > + break; > + default: > + excepted = 0; > + break; > + } > + > + /* This catches both forgetting to add a config command, and > + forgetting to remove a packet from the exception list. */ > + gdb_assert (excepted == (remote_protocol_packets[i].name == NULL)); > + } > + } > + > /* Keep the old ``set remote Z-packet ...'' working. Each individual > Z sub-packet has its own set and show commands, but users may > have sets to this variable in their .gdbinit files (or in their