From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f49.google.com (mail-wm1-f49.google.com [209.85.128.49]) by sourceware.org (Postfix) with ESMTPS id C4666385B52A for ; Mon, 23 Jan 2023 17:46:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org C4666385B52A 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-wm1-f49.google.com with SMTP id l8so9667818wms.3 for ; Mon, 23 Jan 2023 09:46:09 -0800 (PST) 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:message-id:reply-to; bh=338k7JB5F6qjXiElhFdIHsURG5PLU1UZ9JY5o66f6jg=; b=RU/msfXHxGbh2/V/338xjOC6/A3XCGBtKb5GQ9m11Cuuq+fnGlHnSNbbsXiMXeSqpy THXrKyuPOMTI9VW6eQ39FPU7iAA1WHg+0JFKfckfOjB+kt5XB4P9wg1praAfBR9MTo9z KDPtvGZMdS9Gc6WQuYpUvq0OUryCfmQ3MPAhlHuM3FI+R2FjZfBqaxAyC9BgbPGm1jXP RtsuwBK1k+D8L8seIbFgpDlh/nNSagqFNFjJBDPwgKPZZgaSEfjbgCli4s0/8wJDqyFS en2/xLlAb9/CTTZJLC81rTnCTtiiY1cMsvDgpXhaaemYRxy+FN9wrQrTkNPESd9sqXpP /Rnw== X-Gm-Message-State: AFqh2koMNxTfdjjz011BuwYaK+hUfIhia0lcXUkcOr3P+yetZdiYOUAy 1Z1hIgl44VzqCjZmH52SmCY= X-Google-Smtp-Source: AMrXdXvYfbMX6c8Mb4sU0EC3JVTxCewOXaao0rmpvAmhSv0LIyiewXhgtJz2sdpYSM0oz+ASEr5/5Q== X-Received: by 2002:a05:600c:510d:b0:3da:f719:50cd with SMTP id o13-20020a05600c510d00b003daf71950cdmr23700724wms.18.1674495968663; Mon, 23 Jan 2023 09:46:08 -0800 (PST) Received: from ?IPv6:2001:8a0:f92b:9e00::1fe? ([2001:8a0:f92b:9e00::1fe]) by smtp.gmail.com with ESMTPSA id d7-20020a5d4f87000000b002bbddb89c71sm2431666wru.67.2023.01.23.09.46.08 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 23 Jan 2023 09:46:08 -0800 (PST) Subject: Re: [PATCH v4 3/3] gdb: Remove workaround for the vCont packet To: Christina Schimpe , gdb-patches@sourceware.org Cc: tom@tromey.com, aburgess@redhat.com, eliz@gnu.org References: <20221221133958.2111768-1-christina.schimpe@intel.com> <20221221133958.2111768-4-christina.schimpe@intel.com> From: Pedro Alves Message-ID: <32fd8757-bc08-07d1-bbdd-f6da30137bc8@palves.net> Date: Mon, 23 Jan 2023 17:46:07 +0000 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: <20221221133958.2111768-4-christina.schimpe@intel.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.4 required=5.0 tests=BAYES_00,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,GIT_PATCH_0,HEADER_FROM_DIFFERENT_DOMAINS,KAM_DMARC_STATUS,KAM_INFOUSMEBIZ,KAM_SHORT,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 List-Id: On 2022-12-21 1:39 p.m., Christina Schimpe wrote: > The workaround for the vCont packet is no longer required due to the > former commit "gdb: Make global feature array a per-remote target array". > The vCont packet is now checked once when the connection is started and > the supported vCont actions are set to the target's remote state > attribute. So this patch is actually doing two things: #1 - removing the supports_vCont_probed hack. #2 - moving the vCont probing earlier, to connection start. To eliminate the workaround, you don't need #2. Doing just #1 would simply get us back to how it worked before the multi-target support. #2 is an extra change. I don't think there's any particular reason for deferring probing for vCont support up until we first try to resume. Looking at the history, we find that vCont was designed in 2003, here: https://marc.info/?l=gdb-patches&w=2&r=30&s=vcont&q=b and particularly "vCont?" probing was discussed here: https://marc.info/?l=gdb-patches&m=158570765842635&w=2 while the qSupported generic probing feature (done at connection startup) was only invented later in 2006: https://sourceware.org/legacy-ml/gdb/2006-05/msg00372.html Even without qSupported (otherwise I bet we wouldn't have vCont? but instead the support would have been reported via qSupported), it's not clear why Daniel didn't do the probing at initial connection time, I didn't find any message specifically about that. Likely it was written that way just to keep all the related code together, I guess. Thus, the patch is OK. But I wanted to explicitly state the two unrelated changes here for the record, and dig a bit at the history to figure out whether we'd be breaking some subtle use case. Thanks for this. Happy to see it finally done. Pedro Alves > --- > gdb/remote.c | 23 ++++------------------- > 1 file changed, 4 insertions(+), 19 deletions(-) > > diff --git a/gdb/remote.c b/gdb/remote.c > index 8042446ac3d..71ba7da231a 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -455,9 +455,6 @@ class remote_state > > /* The status of the stub support for the various vCont actions. */ > vCont_action_support supports_vCont; > - /* Whether vCont support was probed already. This is a workaround > - until packet_support is per-connection. */ > - bool supports_vCont_probed; > > /* True if the user has pressed Ctrl-C, but the target hasn't > responded to that. */ > @@ -4951,6 +4948,10 @@ remote_target::start_remote_1 (int from_tty, int extended_p) > which later probes to skip. */ > remote_query_supported (); > > + /* Check vCont support and set the remote state's vCont_action_support > + attribute. */ > + remote_vcont_probe (); > + > /* If the stub wants to get a QAllow, compose one and send it. */ > if (m_features.packet_support (PACKET_QAllow) != PACKET_DISABLE) > set_permissions (); > @@ -6467,7 +6468,6 @@ remote_target::remote_vcont_probe () > } > > m_features.packet_ok (rs->buf, PACKET_vCont); > - rs->supports_vCont_probed = true; > } > > /* Helper function for building "vCont" resumptions. Write a > @@ -6653,9 +6653,6 @@ remote_target::remote_resume_with_vcont (ptid_t scope_ptid, int step, > if (::execution_direction == EXEC_REVERSE) > return 0; > > - if (m_features.packet_support (PACKET_vCont) == PACKET_SUPPORT_UNKNOWN) > - remote_vcont_probe (); > - > if (m_features.packet_support (PACKET_vCont) == PACKET_DISABLE) > return 0; > > @@ -7189,12 +7186,6 @@ remote_target::remote_stop_ns (ptid_t ptid) > } > } > > - /* FIXME: This supports_vCont_probed check is a workaround until > - packet_support is per-connection. */ > - if (m_features.packet_support (PACKET_vCont) == PACKET_SUPPORT_UNKNOWN > - || !rs->supports_vCont_probed) > - remote_vcont_probe (); > - > if (!rs->supports_vCont.t) > error (_("Remote server does not support stopping threads")); > > @@ -14516,9 +14507,6 @@ remote_target::can_do_single_step () > { > struct remote_state *rs = get_remote_state (); > > - if (m_features.packet_support (PACKET_vCont) == PACKET_SUPPORT_UNKNOWN) > - remote_vcont_probe (); > - > return rs->supports_vCont.s && rs->supports_vCont.S; > } > else > @@ -14827,9 +14815,6 @@ show_range_stepping (struct ui_file *file, int from_tty, > bool > remote_target::vcont_r_supported () > { > - if (m_features.packet_support (PACKET_vCont) == PACKET_SUPPORT_UNKNOWN) > - remote_vcont_probe (); > - > return (m_features.packet_support (PACKET_vCont) == PACKET_ENABLE > && get_remote_state ()->supports_vCont.r); > } >