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.129.124]) by sourceware.org (Postfix) with ESMTPS id 7A973385801F for ; Tue, 18 Jan 2022 11:39:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7A973385801F Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-607-cE2jWEnyMbeEOOTukdjurw-1; Tue, 18 Jan 2022 06:39:07 -0500 X-MC-Unique: cE2jWEnyMbeEOOTukdjurw-1 Received: by mail-wm1-f71.google.com with SMTP id bg16-20020a05600c3c9000b0034bea12c043so1624100wmb.7 for ; Tue, 18 Jan 2022 03:39:07 -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=kF6B4VYZ5PipoP8CV6+baY98DjY+8z9Z1Ab9Mk0u9do=; b=S+GTg0Cs2gVPuFGoC/yCC9GvQep+QRd83Ntsb8CSPnahjf84NfbAWTpnRtL10Lh2s0 aC3nLebaXJmw9TYrq2FBQL8CX14nfZcWlm19R49R3quSnG8/b5+eP2XTY0CjZvU2bpnd 5OdtJJuUQphh5g0+UyIpVGQL/LMrp3U25TxaGCvYX5auT+XRXxP/YHUePEX5TAPTaFam s5Gw6OFAgnWjSseb8h2vaofGpJ63kbL5E3f/8BW7+32DwEI1oSCsWf7+HlAGA0Yg4YQJ Y7cB+xPHfk+fPvOanVsc87ogdxz3eZs/KjxQlXo6zzHXa8rEMNLC1Orc0ptQtXpCtmH9 MPBA== X-Gm-Message-State: AOAM530CRFHKE/h+wi5bKqsq53xGWotQVXh5xSa9bB4aZD9vmle+TbIM eVq6PFhSI+5936KBnyPtOE2dhLcQ96qiSq2JanFuzH5Hf7VjWlZMtOgSeBkq8KVeC/VFtRpEKE8 OrP6CwrFQSJEWHGTMvrNHDw== X-Received: by 2002:adf:e312:: with SMTP id b18mr18297612wrj.321.1642505945940; Tue, 18 Jan 2022 03:39:05 -0800 (PST) X-Google-Smtp-Source: ABdhPJzzfjMOsI6u71d2JRCAbYNz6jCanb/QMy2jQvXw1RVsRFHoi6BLyx97kpFdjsSNbBm3mjp2CA== X-Received: by 2002:adf:e312:: with SMTP id b18mr18297594wrj.321.1642505945567; Tue, 18 Jan 2022 03:39:05 -0800 (PST) Received: from localhost (host86-188-49-82.range86-188.btcentralplus.com. [86.188.49.82]) by smtp.gmail.com with ESMTPSA id b5sm2121703wmq.28.2022.01.18.03.39.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 18 Jan 2022 03:39:05 -0800 (PST) Date: Tue, 18 Jan 2022 11:39:04 +0000 From: Andrew Burgess To: Christina Schimpe Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 2/3] gdb: Add per-remote target variables for memory read and write config Message-ID: <20220118113904.GE622389@redhat.com> References: <20220113152118.1465255-1-christina.schimpe@intel.com> <20220113152118.1465255-3-christina.schimpe@intel.com> MIME-Version: 1.0 In-Reply-To: <20220113152118.1465255-3-christina.schimpe@intel.com> X-Operating-System: Linux/5.8.18-100.fc31.x86_64 (x86_64) X-Uptime: 11:31:00 up 16 days, 20:25, 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=-12.4 required=5.0 tests=BAYES_00, DKIM_INVALID, DKIM_SIGNED, GIT_PATCH_0, KAM_DMARC_NONE, KAM_DMARC_STATUS, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham 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: Tue, 18 Jan 2022 11:39:20 -0000 * Christina Schimpe via Gdb-patches [2022-01-13 16:21:17 +0100]: > This patch adds per-remote target variables for the configuration of > memory read- and write packet size. It is a further change in addition > to commit "gdb: Make global feature array a per-remote target array" > to apply the fixme notes described in commit 5b6d1e4. > > 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. > > diff --git a/gdb/remote.c b/gdb/remote.c > index 10f9226827..a270cb688c 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -563,8 +563,31 @@ struct packet_config > enum packet_support support; > }; > > -/* This global array contains the default configuration for every new > - per-remote target array. */ > +/* User configurable variables for the number of characters in a > + memory read/write packet. MIN (rsa->remote_packet_size, > + rsa->sizeof_g_packet) is the default. Some targets need smaller > + values (fifo overruns, et.al.) and some users need larger values > + (speed up transfers). The variables ``preferred_*'' (the user > + request), ``current_*'' (what was actually set) and ``forced_*'' > + (Positive - a soft limit, negative - a hard limit). */ > + > +struct memory_packet_config > +{ > + const char *name; > + long size; > + int fixed_p; > +}; > + > +/* These global variables contain the default configuration for every new > + remote_feature object. */ > +static memory_packet_config memory_read_packet_config = > +{ > + "memory-read-packet-size", > +}; > +static memory_packet_config memory_write_packet_config = > +{ > + "memory-write-packet-size", > +}; > static packet_config remote_protocol_packets[PACKET_MAX]; > > class remote_features > @@ -573,6 +596,9 @@ class remote_features > > remote_features () > { > + m_memory_read_packet_config = memory_read_packet_config; > + m_memory_write_packet_config = memory_write_packet_config; > + > std::copy (std::begin (remote_protocol_packets), > std::end (remote_protocol_packets), > std::begin (m_protocol_packets)); > @@ -591,6 +617,9 @@ class remote_features > > void reset_all_packet_configs_support (void); > > + memory_packet_config m_memory_read_packet_config; > + memory_packet_config m_memory_write_packet_config; Comment new fields please. > + > packet_config m_protocol_packets[PACKET_MAX]; > }; > > @@ -1842,21 +1871,6 @@ show_remotebreak (struct ui_file *file, int from_tty, > static unsigned int remote_address_size; > > > -/* User configurable variables for the number of characters in a > - memory read/write packet. MIN (rsa->remote_packet_size, > - rsa->sizeof_g_packet) is the default. Some targets need smaller > - values (fifo overruns, et.al.) and some users need larger values > - (speed up transfers). The variables ``preferred_*'' (the user > - request), ``current_*'' (what was actually set) and ``forced_*'' > - (Positive - a soft limit, negative - a hard limit). */ > - > -struct memory_packet_config > -{ > - const char *name; > - long size; > - int fixed_p; > -}; > - > /* The default max memory-write-packet-size, when the setting is > "fixed". The 16k is historical. (It came from older GDB's using > alloca for buffers and the knowledge (folklore?) that some hosts > @@ -1922,7 +1936,8 @@ remote_target::get_memory_packet_size (struct memory_packet_config *config) > something really big then do a sanity check. */ > > static void > -set_memory_packet_size (const char *args, struct memory_packet_config *config) > +set_memory_packet_size (const char *args, struct memory_packet_config *config, > + bool target_connected) > { > int fixed_p = config->fixed_p; > long size = config->size; > @@ -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 > + "of %ld bytes. Change the packet size? "), > + config->name, query_size)) > + error (_("Packet size not changed.")); > + else if (! target_connected > + && ! query (_("Future targets may not be able to correctly " > + "handle a %s\nof %ld bytes. Change the packet size " > + "for future remote targets? "), > + config->name, query_size)) > error (_("Packet size not changed.")); > } > /* Update the config. */ > @@ -1989,16 +2011,15 @@ show_memory_packet_size (struct memory_packet_config *config) > } > } > > -/* FIXME: needs to be per-remote-target. */ > -static struct memory_packet_config memory_write_packet_config = > -{ > - "memory-write-packet-size", > -}; > - > static void > set_memory_write_packet_size (const char *args, int from_tty) > { > - set_memory_packet_size (args, &memory_write_packet_config); > + remote_target *remote = get_current_remote_target (); > + if (remote != nullptr) > + set_memory_packet_size > + (args, &remote->m_features.m_memory_write_packet_config, true); > + > + set_memory_packet_size (args, &memory_write_packet_config, false); > } > > static void > @@ -2060,19 +2081,18 @@ show_remote_packet_max_chars (struct ui_file *file, int from_tty, > long > remote_target::get_memory_write_packet_size () > { > - return get_memory_packet_size (&memory_write_packet_config); > + return get_memory_packet_size (&m_features.m_memory_write_packet_config); > } > > -/* FIXME: needs to be per-remote-target. */ > -static struct memory_packet_config memory_read_packet_config = > -{ > - "memory-read-packet-size", > -}; > - > static void > set_memory_read_packet_size (const char *args, int from_tty) > { > - set_memory_packet_size (args, &memory_read_packet_config); > + remote_target *remote = get_current_remote_target (); > + if (remote != nullptr) > + set_memory_packet_size > + (args, &remote->m_features.m_memory_read_packet_config, true); > + > + set_memory_packet_size (args, &memory_read_packet_config, false); > } > > static void > @@ -2084,7 +2104,7 @@ show_memory_read_packet_size (const char *args, int from_tty) > long > remote_target::get_memory_read_packet_size () > { > - long size = get_memory_packet_size (&memory_read_packet_config); > + long size = get_memory_packet_size (&m_features.m_memory_read_packet_config); > > /* FIXME: cagney/1999-11-07: Functions like getpkt() need to get an > extra buffer size argument before the memory read size can be > diff --git a/gdb/testsuite/gdb.base/remote.exp b/gdb/testsuite/gdb.base/remote.exp > index 1f0869433f..31c5adfd0d 100644 > --- a/gdb/testsuite/gdb.base/remote.exp > +++ b/gdb/testsuite/gdb.base/remote.exp > @@ -62,7 +62,7 @@ gdb_test "show remote memory-write-packet-size" \ > > set test "set remote memory-write-packet-size fixed" > gdb_test_multiple $test $test { > - -re "Change the packet size. .y or n. " { > + -re "Change the packet size for future remote targets. .y or n. " { > gdb_test_multiple "y" $test { > -re "$gdb_prompt $" { > pass $test > @@ -70,6 +70,7 @@ gdb_test_multiple $test $test { > } > } > } > + > gdb_test "show remote memory-write-packet-size" \ > "The memory-write-packet-size is 0 \\(default\\). Packets are fixed at 16384 bytes\." \ > "write-packet default fixed" > @@ -129,8 +130,6 @@ proc gdb_load_timed {executable class writesize} { > pass $test > } > > -clean_restart $binfile > - > # These download tests won't actually download anything on !is_remote > # target boards, but we run them anyway because it's simpler, and > # harmless. > @@ -155,6 +154,8 @@ gdb_load_timed $binfile "limit" 0 > # Get the size of random_data table (defaults to 48K). > set sizeof_random_data [get_sizeof "random_data" 48*1024] > > +clean_restart $binfile > + > # > # Part THREE: Check the upload behavour > # > -- > 2.25.1 > > 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 >