From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f54.google.com (mail-wr1-f54.google.com [209.85.221.54]) by sourceware.org (Postfix) with ESMTPS id A4FED3858C00 for ; Tue, 4 Oct 2022 13:48:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A4FED3858C00 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wr1-f54.google.com with SMTP id bu30so1525420wrb.8 for ; Tue, 04 Oct 2022 06:48:12 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:content-language:in-reply-to:mime-version :user-agent:date:message-id:from:references:cc:to:subject :x-gm-message-state:from:to:cc:subject:date; bh=jHhQj+yWPJySX1WiSt0dOfLeDdTGUWw+CDJdEHrFI5k=; b=Xf+Z9bi8vqcH5X9Iqq4FyaDeMC3HmOrA22ASrEKkhy1cvoARSLQPMc6ufarfVKIcN/ afKFgqOwHnN+6swWApzIJmyFvpyYAyu2a6Px7LWuVlskPUiC7n8wy6p5l4/s58XZpMCG a7JS41R41ByR6sfVoBJeI1IIpfiZIOoe217zU+q10qEU4sLnAZozCI8guABHURxAPiaL R34kHX45TD+vmqEi4hBMV2EEz4lN8jKk4Fw9lv69G+eBZnEs8Ux8PMXT0zj1qxrRiaj/ q/uf937UVZjMFzmnopvZYqwMq7K4eSstH71pr286bbLTIMv1B6WrS0yamxwsZEnjVikO xl0A== X-Gm-Message-State: ACrzQf3Q1nnQhlHzyKAChS8KekX354ieGqKsIFhViLY7GkMrW2WR25UU L+lUPrSf5WVD77tm1xhi4mk= X-Google-Smtp-Source: AMsMyM7S4LG6xzY89VadRhEdWqhf4BBTRiFlxBjQogZRD8mF0thvK6a7MbUPlFlIjwVcd/fnPUb3cA== X-Received: by 2002:a5d:500a:0:b0:22e:34bd:c907 with SMTP id e10-20020a5d500a000000b0022e34bdc907mr7794619wrt.548.1664891291370; Tue, 04 Oct 2022 06:48:11 -0700 (PDT) Received: from ?IPv6:2001:8a0:f93a:3b00:e038:5cdc:b8bf:4653? ([2001:8a0:f93a:3b00:e038:5cdc:b8bf:4653]) by smtp.gmail.com with ESMTPSA id ay26-20020a05600c1e1a00b003b4868eb6bbsm19408741wmb.23.2022.10.04.06.48.09 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 04 Oct 2022 06:48:10 -0700 (PDT) Subject: Re: [PATCH v3 1/3] gdb: Make global feature array a per-remote target array To: Christina Schimpe , gdb-patches@sourceware.org Cc: aburgess@redhat.com, eliz@gnu.org, tom@tromey.com References: <20220909153709.3687178-1-christina.schimpe@intel.com> <20220909153709.3687178-2-christina.schimpe@intel.com> From: Pedro Alves Message-ID: <7f3a8e9c-0c75-e8ac-cf01-14b12e5eb96e@palves.net> Date: Tue, 4 Oct 2022 14:48:09 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: <20220909153709.3687178-2-christina.schimpe@intel.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-5.4 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 04 Oct 2022 13:48:20 -0000 Hi Christina, Other than a couple tiny nits, just one issue with the data structures below. I think once this is resolved, it'll be ready to go. Thanks for the patience. On 2022-09-09 4:37 p.m., Christina Schimpe wrote: > > +/* Convert the packet support auto_boolean to a name used for gdb printing. */ > + > +static const char * > +get_packet_support_name (auto_boolean support) > +{ > + switch (support) > + { > + case AUTO_BOOLEAN_TRUE: > + return _("on"); > + case AUTO_BOOLEAN_FALSE: > + return _("off"); > + case AUTO_BOOLEAN_AUTO: > + return _("auto"); These shouldn't actually be translated, as GDB only accepts these in English. Like, in (gdb) set foo off "off" is always "off". > + default: > + return _("internal-error"); I'd just remove this one. > + > struct threads_listing_context; > > /* Stub vCont actions support. > @@ -395,6 +574,90 @@ static const target_info remote_target_info = { > remote_doc > }; > > +/* Description and configuration of a remote packet. */ > + > +struct packet_config > + { > + /* Name of the packet used for gdb output. */ > + const char *name; > + > + /* Title of the packet, used by the set/show remote name-packet > + commands to identify the individual packages and gdb output. */ > + const char *title; > + > + /* 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. Configs that don't > + have an associated command always have this set to auto. */ > + enum auto_boolean detect; > + > + /* The "show remote foo-packet" command created for this packet. */ > + cmd_list_element *show_cmd; > + > + /* The "set remote foo-packet" command created for this packet. */ > + cmd_list_element *set_cmd; > + > + /* Does the target support this packet? */ > + enum packet_support support; > + }; So this structure is now a bit odd. It holds both packet description stuff, like the name/title, which is shared between all the remote connections; and configuration stuff, which is per-connection. I think we should split this in two structs / arrays, so we only have one copy of the description stuff. In fact, aren't show_cmd/set_cmd above essentially unused after this patch? I can only see places writing to those fields, I see nowhere actually reading from those fields. I think you can just drop them entirely. > + > +/* This global array contains the default configuration for every new > + per-remote target array. */ > +static packet_config remote_protocol_packets[PACKET_MAX]; > @@ -1967,10 +2256,13 @@ add_packet_config_cmd (struct packet_config *config, const char *name, > = add_setshow_auto_boolean_cmd (cmd_name.release (), class_obscure, > &config->detect, set_doc.get (), > show_doc.get (), NULL, /* help_doc */ > - NULL, > + set_remote_protocol_packet_cmd, > show_remote_protocol_packet_cmd, > &remote_set_cmdlist, &remote_show_cmdlist); > config->show_cmd = cmds.show; > + config->set_cmd = cmds.set; These two above, are the only references to config->show_cmd / config->set_cmd AFAICT. > + cmds.show->set_context(config); > + cmds.set->set_context(config); Missing space before parens in those two set_context calls. > @@ -2273,19 +2401,24 @@ show_remote_protocol_packet_cmd (struct ui_file *file, int from_tty, > struct cmd_list_element *c, > const char *value) > { > - struct packet_config *packet; > + remote_target *remote = get_current_remote_target (); > gdb_assert (c->var.has_value ()); > > - for (packet = remote_protocol_packets; > - packet < &remote_protocol_packets[PACKET_MAX]; > - packet++) > + auto *default_config = static_cast (c->context ()); > + const int packet_idx = std::distance (remote_protocol_packets, > + default_config); > + > + if (packet_idx >= 0 && packet_idx < PACKET_MAX) Spurious double space in "packet_idx <". > { > - if (c == packet->show_cmd) > - { > - show_packet_config_cmd (file, packet); > - return; > - } > + if (remote != nullptr) > + show_packet_config_cmd > + (file, &remote->m_features.m_protocol_packets[packet_idx], true); > + else > + show_packet_config_cmd > + (file, &remote_protocol_packets[packet_idx], false); > + return; > } > + > internal_error (__FILE__, __LINE__, _("Could not find config for %s"), > c->name); > }