From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 49D113858D20 for ; Fri, 4 Feb 2022 18:03:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 49D113858D20 Received: from mail-ej1-f71.google.com (mail-ej1-f71.google.com [209.85.218.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-654-h2u1WzZhOxC9MX7kREQrIQ-1; Fri, 04 Feb 2022 13:03:23 -0500 X-MC-Unique: h2u1WzZhOxC9MX7kREQrIQ-1 Received: by mail-ej1-f71.google.com with SMTP id i21-20020a1709063c5500b006b4c7308c19so2902908ejg.14 for ; Fri, 04 Feb 2022 10:03:23 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=0V4m4AeuGCCVt9ca6zLYSsdlN0UKQhW6EBXUKgc+jpg=; b=ljOrENjwGeb8dUB0gXnQNBJvtJ0iWf+8nJ0s47feiFUW7CquWJM/jvBkQvJNU4A+/o RZzxP308xI3DxGfZippzdCuAUk3M7ImRxrojGPY9bHYiLusu/SV3L3Chmx+bRjEeX8Jq shMKWcDnRHbgqI92xGvD0Xncdz54+BzadDUxYbjyPtX2qCO+2UbDozpwAi5NaRFABt0w 5Ip8rGfl9mxQonSArcraZ5PJrLYZJzOgwPToYa/5oI2wdu3ilY6apLKKdC+ZJLeq7mx8 s8GcNsTJzXrkuVJG66hJRI+1EvdX2b+yUgHrd+ZBJDbVsWfomPO3Ay1JIdye3RnyO+LV d0Og== X-Gm-Message-State: AOAM531591znCnMjizSklJ3hWUOIyCdthkWxKWboTQ/j5pWv9gV/1FD2 ubpjbMjVcCqOtquaCaQGnoiqiGwjII4t5xYm6BHkUFc76w9CNCiC5ULG2zS/7R1BchDHiYLPVSC JKhuYo1jLebGqiHHyJMCd+w== X-Received: by 2002:a05:6402:1290:: with SMTP id w16mr280305edv.331.1643997802294; Fri, 04 Feb 2022 10:03:22 -0800 (PST) X-Google-Smtp-Source: ABdhPJy8iuMlgd6Fbmo+uqa2u++WdKLZ2jU5XW6F5tFGfW8ODvUk/pwhZ4HpnCu0UvYqmslpPE3X5g== X-Received: by 2002:a05:6402:1290:: with SMTP id w16mr280288edv.331.1643997802058; Fri, 04 Feb 2022 10:03:22 -0800 (PST) Received: from localhost (92.40.178.205.threembb.co.uk. [92.40.178.205]) by smtp.gmail.com with ESMTPSA id lu44sm884798ejb.145.2022.02.04.10.03.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 04 Feb 2022 10:03:21 -0800 (PST) Date: Fri, 4 Feb 2022 18:03:20 +0000 From: Andrew Burgess To: "Schimpe, Christina" Cc: "gdb-patches@sourceware.org" Subject: Re: [PATCH 2/3] gdb: Add per-remote target variables for memory read and write config Message-ID: <20220204180320.GD1917497@redhat.com> References: <20220113152118.1465255-1-christina.schimpe@intel.com> <20220113152118.1465255-3-christina.schimpe@intel.com> <20220118113904.GE622389@redhat.com> MIME-Version: 1.0 In-Reply-To: X-Operating-System: Linux/5.8.18-100.fc31.x86_64 (x86_64) X-Uptime: 18:01:55 up 11 days, 8:48, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-3.0 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_ABUSEAT, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Fri, 04 Feb 2022 18:03:27 -0000 * Schimpe, Christina via Gdb-patches [2022-01-24 15:06:31 +0000]: > Hi Andrew, > > Thanks a lot for your feedback. > Before sending a V2 for this patch, there is still one point I would like to discuss > (refer to my answer to your comment below). > > I also noticed that I missed an adaption in the function > static void > show_memory_packet_size (struct memory_packet_config *config) > > at > if (remote != NULL) > printf_filtered (_("Packets are limited to %ld bytes.\n"), > remote->get_memory_packet_size (config)); > > In the function call get_memory_packet_size the global configuration is passed, > but the current target's configuration should be used. > I will correct this with the next version of this patch. > > > > The former global variables for that configuration are still available > > > to allow the command line configuration for all future remote > > > connections. Similar to the command line configuration of the per- > > > remote target feature array, the commands > > > > > > - set remotewritesize (deprecated) > > > - set remote memory-read-packet-size > > > - set remote memory-write-packet-size > > > > > > will configure the current target (if available) and future remote > > > connections. The show command will display the global configuration > > > and the packet size of the current remote target, if available. > > > > > > It is required to adapt the test gdb.base/remote.exp which is failing > > > for --target_board=native-extended-gdbserver. With that board GDB > > > connects to gdbserver at gdb start time. Due to this patch two > > > loggings "The target may not be able to.." are shown if the command > > > 'set remote memory-write-packet-size fixed' is executed while a target > > > is connected for the current inferior. To fix this, the clean_restart > > > command is moved to a later time point of the test. It is sufficient > > > to be connected to the server when "runto_main" is executed. Now the > > > connection time is similar to a testrun with > > > --target_board=native-gdbserver. > > > > > > To allow the user to distinguish between the packet-size configuration > > > for future connections and for the currently selected target, the > > > logging of the command 'set remote memory-write-packet-size fixed' is > > > adapted. > > > --- > > > gdb/remote.c | 94 +++++++++++++++++++------------ > > > gdb/testsuite/gdb.base/remote.exp | 7 ++- > > > 2 files changed, 61 insertions(+), 40 deletions(-) > > > > This needs a NEWS entry and a docs update. > > > > Like with the previous patch, I think we should consider having the set/show > > commands print more informative messages about which targets are being > > changed or displayed. > > > > Yes, I agree, this could be improved. I was wondering if we should then adapt > the show command such that it behaves similar to the previous patch e.g. > "The show command always displays the current remote target's > configuration. If no remote target is selected the default > configuration for future connections is shown.". That sounds reasonable to me, and inline with the previous patch. > For the GDB user it might be less confusing if we keep the behaviour of the > "show remote" commands consistent. I agree that being consistent across commands, especially related commands is super important. Thanks, Andrew > > > > > > > + memory_packet_config m_memory_read_packet_config; > > > + memory_packet_config m_memory_write_packet_config; > > > > Comment new fields please. > > Yes, I will adapt it. > > > > @@ -1956,9 +1971,16 @@ set_memory_packet_size (const char *args, > > struct memory_packet_config *config) > > > ? DEFAULT_MAX_MEMORY_PACKET_SIZE_FIXED > > > : size); > > > > > > - if (! query (_("The target may not be able to correctly handle a %s\n" > > > - "of %ld bytes. Change the packet size? "), > > > - config->name, query_size)) > > > + if (target_connected > > > + && ! query (_("The target may not be able to correctly handle a > > %s\n" > > > > The space after '!' is not gdb style, could you remove it please. > > Also the two below please. > > > > Thanks, > > Andrew > > Thank you, I will adapt it. > > Best Regards, > Christina > Intel Deutschland GmbH > Registered Address: Am Campeon 10, 85579 Neubiberg, Germany > Tel: +49 89 99 8853-0, www.intel.de > Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva > Chairperson of the Supervisory Board: Nicole Lau > Registered Office: Munich > Commercial Register: Amtsgericht Muenchen HRB 186928 >