From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 109663 invoked by alias); 21 May 2018 19:47:53 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 109638 invoked by uid 89); 21 May 2018 19:47:51 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.4 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=transfers, polish, Perhaps X-HELO: mx1.redhat.com Received: from mx3-rdu2.redhat.com (HELO mx1.redhat.com) (66.187.233.73) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 21 May 2018 19:47:49 +0000 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1C21C4201AE6; Mon, 21 May 2018 19:47:48 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 85760215CDA7; Mon, 21 May 2018 19:47:47 +0000 (UTC) Subject: Re: [PATCH 08/10] Handle "show remote memory-write-packet-size" when not connected To: Simon Marchi , gdb-patches@sourceware.org References: <20180516141830.16859-1-palves@redhat.com> <20180516141830.16859-9-palves@redhat.com> <28ff1dac-fa38-6e19-fd0e-b4aed7e1832f@ericsson.com> From: Pedro Alves Message-ID: Date: Mon, 21 May 2018 20:41:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <28ff1dac-fa38-6e19-fd0e-b4aed7e1832f@ericsson.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-05/txt/msg00505.txt.bz2 On 05/18/2018 10:26 PM, Simon Marchi wrote: > On 2018-05-16 10:18 AM, Pedro Alves wrote: >> Currently "show remote memory-write-packet-size" says that the packet >> size is limited to whatever is stored in the remote_state global, even >> if not connected to a target. >> >> When we get to support multiple instances of remote targets, there >> won't be a remote_state global anymore, so that must be replaced by >> something else. >> >> Since it doesn't make sense to print the limit of the packet size of a >> non-existing connection, this patch makes us say that the limit will >> be further reduced when we connect. >> >> The text is taken from the command's online help, which says: >> >> "The actual limit is further reduced dependent on the target." > > The result sounds a bit weird: > > (gdb) show remote memory-read-packet-size > The memory-read-packet-size is 0. The actual limit will be further reduced dependent on the target. > (gdb) show remote memory-write-packet-size > The memory-write-packet-size is 0. The actual limit will be further reduced dependent on the target. > > How can the limit be reduced if it is zero? I don't really know about > this code, is zero a special value that means no limit? Perhaps it should be > handled differently to make the message clearer. Yeah, I think the command itself it pretty obscure/weird. "0" means "default packet size". I.e., whatever packet size the remote can handle. And if you do "set remote memory-write-packet fixed", then GDB picks an arbitrary default... I've spent a few hours today staring at this, trying to come up with something reasonable-ish, and this is what I came up with. I even caught a bug. I don't think it's worth it to polish this much more, as this is a pretty obscure command that most probably nobody uses nowadays. WDYT? ------------------ >From dcca1d189d4e3bdd21af369fa8dc5b450d3effad Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Mon, 21 May 2018 19:46:43 +0100 Subject: [PATCH] Handle "show remote memory-write-packet-size" when not connected Currently "show remote memory-write-packet-size" says that the packet size is limited to whatever is stored in the remote_state global, even if not connected to a target. When we get to support multiple instances of remote targets, there won't be a remote_state global anymore, so that must be replaced by something else. Since it doesn't make sense to print the limit of the packet size of a non-existing connection, this patch makes us say that the limit will be further reduced when we connect. The text is taken from the command's online help, which says: "The actual limit is further reduced dependent on the target." Note that a value of "0" is special, as per "help set remote memory-write-packet-size": ~~~ Specify the number of bytes in a packet or 0 (zero) for the default packet size. ~~~ I've tweaked "show remote memory-write-packet-size" to include "(default)" in the output in that case, like this: (gdb) show remote memory-write-packet-size The memory-write-packet-size is 0 (default). The actual limit will be further reduced dependent on the target. While working on this, I noticed that an explicit "set remote write-packet-size 0" does not makes GDB go back to the exact same state as the default state when GDB starts up: (gdb) show remote memory-write-packet-size The memory-write-packet-size is 0. [...] ^^ (gdb) set remote memory-write-packet-size 0 (gdb) show remote memory-write-packet-size The memory-write-packet-size is 16384. [...] ^^^^^ The "16384" number comes from DEFAULT_MAX_MEMORY_PACKET_SIZE. I think this happens because git commit a5c0808e221c ("gdb: remove packet size limit") at , added this: /* So that the query shows the correct value. */ if (size <= 0) size = DEFAULT_MAX_MEMORY_PACKET_SIZE; to set_memory_packet_size, but despite what the comment suggests, that also has the side-effect of recording DEFAULT_MAX_MEMORY_PACKET_SIZE in config->size. Finally, DEFAULT_MAX_MEMORY_PACKET_SIZE only makes sense for "set remote memory-write-packet-size fixed", so I've renamed it accordingly, to make it a little bit clearer. gdb/ChangeLog: yyyy-mm-dd Pedro Alves * remote.c (DEFAULT_MAX_MEMORY_PACKET_SIZE): Rename to ... (DEFAULT_MAX_MEMORY_PACKET_SIZE_FIXED): ... this. (get_fixed_memory_packet_size): New. (get_memory_packet_size): Use it. (set_memory_packet_size): Don't override the config size with DEFAULT_MAX_MEMORY_PACKET_SIZE. (show_memory_packet_size): Use get_fixed_memory_packet_size. Don't refer to get_memory_packet_size if not connected to a remote target. Show "(default)" if configured size is 0. gdb/testsuite/ChangeLog: yyyy-mm-dd Pedro Alves * gdb.base/remote.exp: Adjust expected output of "show remote memory-write-packet-size". Add tests for "set remote memory-write-packet-size 0" and "set remote memory-write-packet-size fixed/limit". --- gdb/remote.c | 60 ++++++++++++++++++++++++++------------- gdb/testsuite/gdb.base/remote.exp | 30 ++++++++++++++++++-- 2 files changed, 67 insertions(+), 23 deletions(-) diff --git a/gdb/remote.c b/gdb/remote.c index 59880a93a8..72254dba31 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -1328,16 +1328,29 @@ struct memory_packet_config int fixed_p; }; -/* The default max memory-write-packet-size. The 16k is historical. - (It came from older GDB's using alloca for buffers and the - knowledge (folklore?) that some hosts don't cope very well with - large alloca calls.) */ -#define DEFAULT_MAX_MEMORY_PACKET_SIZE 16384 +/* 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 + don't cope very well with large alloca calls.) */ +#define DEFAULT_MAX_MEMORY_PACKET_SIZE_FIXED 16384 /* The minimum remote packet size for memory transfers. Ensures we can write at least one byte. */ #define MIN_MEMORY_PACKET_SIZE 20 +/* Get the memory packet size, assuming it is fixed. */ + +static long +get_fixed_memory_packet_size (struct memory_packet_config *config) +{ + gdb_assert (config->fixed_p); + + if (config->size <= 0) + return DEFAULT_MAX_MEMORY_PACKET_SIZE_FIXED; + else + return config->size; +} + /* Compute the current size of a read/write packet. Since this makes use of ``actual_register_packet_size'' the computation is dynamic. */ @@ -1349,12 +1362,7 @@ get_memory_packet_size (struct memory_packet_config *config) long what_they_get; if (config->fixed_p) - { - if (config->size <= 0) - what_they_get = DEFAULT_MAX_MEMORY_PACKET_SIZE; - else - what_they_get = config->size; - } + what_they_get = get_fixed_memory_packet_size (config); else { what_they_get = get_remote_packet_size (); @@ -1414,16 +1422,17 @@ set_memory_packet_size (const char *args, struct memory_packet_config *config) something arbitrarily large. */ } - /* So that the query shows the correct value. */ - if (size <= 0) - size = DEFAULT_MAX_MEMORY_PACKET_SIZE; - /* Extra checks? */ if (fixed_p && !config->fixed_p) { + /* So that the query shows the correct value. */ + long query_size = (size <= 0 + ? 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, size)) + config->name, query_size)) error (_("Packet size not changed.")); } /* Update the config. */ @@ -1434,13 +1443,24 @@ set_memory_packet_size (const char *args, struct memory_packet_config *config) static void show_memory_packet_size (struct memory_packet_config *config) { - printf_filtered (_("The %s is %ld. "), config->name, config->size); + if (config->size == 0) + printf_filtered (_("The %s is 0 (default). "), config->name); + else + printf_filtered (_("The %s is %ld. "), config->name, config->size); if (config->fixed_p) printf_filtered (_("Packets are fixed at %ld bytes.\n"), - get_memory_packet_size (config)); + get_fixed_memory_packet_size (config)); else - printf_filtered (_("Packets are limited to %ld bytes.\n"), - get_memory_packet_size (config)); + { + struct remote_state *rs = get_remote_state (); + + if (rs->remote_desc != NULL) + printf_filtered (_("Packets are limited to %ld bytes.\n"), + get_memory_packet_size (config)); + else + puts_filtered ("The actual limit will be further reduced " + "dependent on the target.\n"); + } } static struct memory_packet_config memory_write_packet_config = diff --git a/gdb/testsuite/gdb.base/remote.exp b/gdb/testsuite/gdb.base/remote.exp index 26361af9a5..ba34441af2 100644 --- a/gdb/testsuite/gdb.base/remote.exp +++ b/gdb/testsuite/gdb.base/remote.exp @@ -35,7 +35,7 @@ if {$result != "" } then { # gdb_test "show remote memory-write-packet-size" \ - "The memory-write-packet-size is 0. Packets are limited to \[0-9\]+ bytes." \ + "The memory-write-packet-size is 0 \\(default\\). The actual limit will be further reduced dependent on the target\." \ "write-packet default" gdb_test "set remote memory-write-packet-size" \ @@ -44,14 +44,38 @@ gdb_test "set remote memory-write-packet-size" \ gdb_test_no_output "set remote memory-write-packet-size 20" gdb_test "show remote memory-write-packet-size" \ - "The memory-write-packet-size is 20. Packets are limited to 20 bytes." \ + "The memory-write-packet-size is 20. The actual limit will be further reduced dependent on the target\." \ "set write-packet - small" gdb_test_no_output "set remote memory-write-packet-size 1" gdb_test "show remote memory-write-packet-size" \ - "The memory-write-packet-size is 1. Packets are limited to 20 bytes." \ + "The memory-write-packet-size is 1. The actual limit will be further reduced dependent on the target\." \ "set write-packet - very-small" +gdb_test_no_output "set remote memory-write-packet-size 0" +gdb_test "show remote memory-write-packet-size" \ + "The memory-write-packet-size is 0 \\(default\\). The actual limit will be further reduced dependent on the target\." \ + "write-packet default again" + +set test "set remote memory-write-packet-size fixed" +gdb_test_multiple $test $test { + -re "Change the packet size. .y or n. " { + gdb_test_multiple "y" $test { + -re "$gdb_prompt $" { + pass $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" + +gdb_test_no_output "set remote memory-write-packet-size limit" +gdb_test "show remote memory-write-packet-size" \ + "The memory-write-packet-size is 0 \\(default\\). The actual limit will be further reduced dependent on the target\." \ + "write-packet default limit again" + # # Part TWO: Check the download behavior. # -- 2.14.3