From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f48.google.com (mail-wm1-f48.google.com [209.85.128.48]) by sourceware.org (Postfix) with ESMTPS id 3E03A3839423 for ; Wed, 25 May 2022 14:27:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3E03A3839423 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-f48.google.com with SMTP id f23-20020a7bcc17000000b003972dda143eso1209565wmh.3 for ; Wed, 25 May 2022 07:27:18 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:references:from:in-reply-to :content-transfer-encoding; bh=lGOrYjJfCO4u20b4eXv2F9rA49bTPyRQ/r2kDZKzGKU=; b=zhPoFCLyPzidEJ1x80MZjvZ5XytB+/UxEfcpMVD7sTfMraJ0GnEiaFzmsmTPWlTbP2 HFjarode37n5AU6Y3NlMGpfT1DN8ZOGeto9Hy/YldzcY3+Pk0qWNT/OeTe8TABcnD8ab DqrB+drf5rz9fCslcBaipGGITBjk7+nax+96cTk8wN7CjeBagX7iqgnuZvr6LGphlJJo n0vkwTn4tQdFuk08cNB+LD9hgKh1gn9shvKAUFe2I96zUYFDYAcE9Y7JxJWlWNeY9cab oGz5SMp/VsLug9IC1uQptoFDiQy8FkOy8CU0GWr5cDTwJ7tewjvHUYVdJ2TRt/xfLd66 vmww== X-Gm-Message-State: AOAM532Hjbsviw/Pu2HuLgISI66rOHYOgad1LvuXT0vnW+KVuwkvDlWi cYY8MJEKENhLDtQX4rmWpr8= X-Google-Smtp-Source: ABdhPJwle+HOWGB9ON2PYUWFabEgnlxkTEixZS9l4qoUCE7pR68X4tuaVmEqbiWA/oY+d0YzzdA7Qw== X-Received: by 2002:a7b:c4cc:0:b0:397:431f:eeb4 with SMTP id g12-20020a7bc4cc000000b00397431feeb4mr8271298wmk.123.1653488837112; Wed, 25 May 2022 07:27:17 -0700 (PDT) Received: from ?IPV6:2001:8a0:f924:2600:209d:85e2:409e:8726? ([2001:8a0:f924:2600:209d:85e2:409e:8726]) by smtp.gmail.com with ESMTPSA id t22-20020a05600c41d600b003942a244ebesm2275128wmh.3.2022.05.25.07.27.15 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 25 May 2022 07:27:16 -0700 (PDT) Message-ID: <169590f1-2f6f-651b-dd12-d00f123e5199@palves.net> Date: Wed, 25 May 2022 15:27:14 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.0 Subject: Re: [PATCH v2 1/3] gdb: Make global feature array a per-remote target array Content-Language: en-US To: "Schimpe, Christina" , Andrew Burgess , "gdb-patches@sourceware.org" References: <20220329131158.3970228-1-christina.schimpe@intel.com> <20220329131158.3970228-2-christina.schimpe@intel.com> <08fd8bbf-c44e-7313-d7b3-7b0770c2c7d4@palves.net> From: Pedro Alves In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-4.0 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, T_SCC_BODY_TEXT_LINE autolearn=no 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: Wed, 25 May 2022 14:27:20 -0000 Hi Christina, On 2022-04-27 14:55, Schimpe, Christina wrote: > Hi Pedro and Andrew, > > Thanks a lot for identifying and fixing the regression and thank you for the review. > > I added my comments for Pedro's review below. It would be great if you could briefly review > my new suggestion for the logging. > > >> I'm not 100% sure about the solution here. >> >> What is the reasoning for making "set remote foo" affect future connections >> in addition to the current connection? I didn't see a discussion about that, >> and it seems counterintuitive to me offhand. I would think that: >> >> - if connected, the set command affects the current connection. >> >> - if not connected, the set command affects future connections. >> >> ... would be the simplest solution. Thus, if the inferior you have selected is >> connected and you want to configure future connections, you'd first drop >> the connection, or switch to an inferior that is not connected. >> > > I think the reason why I did not investigate this approach further is because these "set remote" > commands applied to future connections before and I did not want to change that. > My initial patch did not add any logging for the set remote commands and the user would not > have noticed that the configuration does not apply to future targets anymore (if connected). > But with the appropriate logging it should be clear and the user should be warned. So I don't > have any preferences anymore and would go ahead to adapt the patch to Pedro's suggestion, > if there are no further arguments against it. > >> Also, I think it would be better if both the set and the show commands used >> the same wording. Currently you have, when not connected: >> >> (gdb) set remote X-packet off >> Use of the 'p' packet for future remote targets is set to "off". >> (gdb) show remote X-packet >> Support for the 'p' packet on newly created remote targets is "disabled". >> >> Note the >> "Use of" vs "Support for", >> and the >> "for future remote targets", vs "on newly created remote targets". >> >> Also note that "disabled" is not accepted by the "set command", while >> printing it in quotes suggests that it would. I mean, note: >> >> Current master: >> >> (gdb) show remote X-packet >> Support for the `p' packet is currently disabled. >> >> (no quotes around disabled) >> >> vs your patches: >> >> (gdb) show remote X-packet >> Support for the 'p' packet on newly created remote targets is "disabled". >> >> and of course: >> >> (gdb) set remote X-packet disabled >> "on", "off" or "auto" expected. >> > > Yes, you are right. > > So I now would suggest the following logging for the packet configuration commands > (based on Pedro's suggestion for the new packet configuration): > > ~~~ > (gdb) set remote kill-packet > "on", "off" or "auto" expected. > (gdb) show remote kill-packet > Support for the 'vKill' packet on future remote targets is "auto", currently unknown. > (gdb) set remote kill-packet off > Support for the 'vKill' packet on future remote targets is set to "off". > (gdb) show remote kill-packet > Support for the 'vKill' packet on future remote targets is "off". > (gdb) target extended-remote :1234 > Remote debugging using :1234 > (gdb) set remote kill-packet on > Support for the 'vKill' packet on the current remote target is set to "on". > (gdb) show remote kill-packet > Support for the 'vKill' packet on the current remote target is "on". > ~~~~ > So the only difference between the logging for the show and the set commands, is that > for set we log "is set to" instead of "is". > Seems fine. > > And for the memory read and write configuration of patch (2/3): > > ~~~~ > (gdb) set remote memory-read-packet-size > Argument required (integer, "fixed" or "limited'). > (gdb) show remote memory-read-packet-size > The memory-read-packet-size on future remote targets is 0 (default). The actual limit will be further reduced dependent on the target. > (gdb) set remote memory-read-packet-size fixed > Future targets may not be able to correctly handle a memory-read-packet-size You say "future remote targets" in the other messages, so I guess here you'd say "Future remote targets" instead of "Future targets" too. > of 16384 bytes. Change the packet size for future remote targets? (y or n) y > The memory-read-packet-size on future remote targets is set to "fixed". > (gdb) show remote memory-read-packet-size > The memory-read-packet-size on future remote targets is 0 (default). Packets are fixed at 16384 bytes. > (gdb) target extended-remote :1234 > Remote debugging using :1234 > (gdb) set remote memory-read-packet-size 16300 > The memory-read-packet-size on the current remote targets is set to 16300. "remote targets" -> "remote target" (singular). > (gdb) show remote memory-read-packet-size > The memory-read-packet-size on the current remote target is 16300. Packets are fixed at 16300 bytes. > ~~~~ > Seems all fine to me. > Note that the configuration options before were shown as > ~~~~ > (gdb) set remote memory-read-packet-size > Argument required (integer, `fixed' or `limited'). > ~~~~ > > I also noted a small issue in the configuration for the "limited" option: > ~~~~ > (gdb) set remote memory-read-packet-size > Argument required (integer, `fixed' or `limited'). > (gdb) set remote memory-read-packet-size limited > Invalid memory-read-packet-size (bad syntax). > (gdb) set remote memory-read-packet-size limit > (gdb) > ~~~~ > So currently you need to specify "limit" although "limited" is suggested. I would adapt it to "limited" in the v3. Sounds like that may break someone's scripts? I'd go for tweaking the suggestion to say "limit" instead. I went to look what does the manual say, but I wasn't able to find where this comment is documented in the manual... Odd. The online help does say "limit", though: (gdb) help set remote memory-read-packet-size Set the maximum number of bytes per memory-read packet. Specify the number of bytes in a packet or 0 (zero) for the default packet size. The actual limit is further reduced dependent on the target. Specify ``fixed'' to disable the further restriction and ``limit'' to enable that restriction. (gdb)