From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 87197 invoked by alias); 22 May 2018 21:43:35 -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 87143 invoked by uid 89); 22 May 2018 21:43:34 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.6 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=sk:memory_, sk:actual_, tweaked, sk:get_mem 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; Tue, 22 May 2018 21:43:31 +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 0772E40122BA; Tue, 22 May 2018 21:43:30 +0000 (UTC) Received: from localhost (unused-10-15-17-196.yyz.redhat.com [10.15.17.196]) by smtp.corp.redhat.com (Postfix) with ESMTP id C53E62166BB3; Tue, 22 May 2018 21:43:29 +0000 (UTC) From: Sergio Durigan Junior To: Pedro Alves Cc: Simon Marchi , gdb-patches@sourceware.org Subject: Re: [PATCH 08/10] Handle "show remote memory-write-packet-size" when not connected References: <20180516141830.16859-1-palves@redhat.com> <20180516141830.16859-9-palves@redhat.com> <28ff1dac-fa38-6e19-fd0e-b4aed7e1832f@ericsson.com> Date: Tue, 22 May 2018 21:55:00 -0000 In-Reply-To: (Pedro Alves's message of "Mon, 21 May 2018 20:47:46 +0100") Message-ID: <871se31i4e.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2018-05/txt/msg00568.txt.bz2 On Monday, May 21 2018, Pedro Alves wrote: > 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? I'm seeing a few failures on Debian-s390x-native-extended-gdbserver-m64: PASS -> FAIL: gdb.base/remote.exp: write-packet default PASS -> FAIL: gdb.base/remote.exp: set write-packet - small PASS -> FAIL: gdb.base/remote.exp: set write-packet - very-small new FAIL: gdb.base/remote.exp: write-packet default again new FAIL: gdb.base/remote.exp: write-packet default limit again PASS -> FAIL: gdb.reverse/s390-mvcle.exp: check MVCLE record 2 PASS -> FAIL: gdb.reverse/s390-mvcle.exp: check MVCLE record 3 PASS -> KFAIL: gdb.threads/process-dies-while-handling-bp.exp: non_stop=off: cond_bp_target=0: inferior 1 exited gdb.log shows: ... show remote memory-write-packet-size The memory-write-packet-size is 0 (default). Packets are limited to 16383 bytes. (gdb) FAIL: gdb.base/remote.exp: write-packet default set remote memory-write-packet-size Argument required (integer, `fixed' or `limited'). (gdb) PASS: gdb.base/remote.exp: set write-packet - NULL set remote memory-write-packet-size 20 (gdb) PASS: gdb.base/remote.exp: set remote memory-write-packet-size 20 show remote memory-write-packet-size The memory-write-packet-size is 20. Packets are limited to 20 bytes. (gdb) FAIL: gdb.base/remote.exp: set write-packet - small set remote memory-write-packet-size 1 (gdb) PASS: gdb.base/remote.exp: set remote memory-write-packet-size 1 show remote memory-write-packet-size The memory-write-packet-size is 1. Packets are limited to 20 bytes. (gdb) FAIL: gdb.base/remote.exp: set write-packet - very-small set remote memory-write-packet-size 0 (gdb) PASS: gdb.base/remote.exp: set remote memory-write-packet-size 0 show remote memory-write-packet-size The memory-write-packet-size is 0 (default). Packets are limited to 16383 bytes. (gdb) FAIL: gdb.base/remote.exp: write-packet default again set remote memory-write-packet-size fixed The target may not be able to correctly handle a memory-write-packet-size of 16384 bytes. Change the packet size? (y or n) y (gdb) PASS: gdb.base/remote.exp: set remote memory-write-packet-size fixed show remote memory-write-packet-size The memory-write-packet-size is 0 (default). Packets are fixed at 16384 bytes. (gdb) PASS: gdb.base/remote.exp: write-packet default fixed set remote memory-write-packet-size limit (gdb) PASS: gdb.base/remote.exp: set remote memory-write-packet-size limit show remote memory-write-packet-size The memory-write-packet-size is 0 (default). Packets are limited to 16383 bytes. (gdb) FAIL: gdb.base/remote.exp: write-packet default limit again set remote memory-write-packet-size 398 (gdb) PASS: gdb.base/remote.exp: timed download `remote' - limit, 398 - set packet size set remote memory-write-packet-size limit ... This is on a try build; unfortunately our BuildBot has a very big queue right now and the "official" commit hasn't been tested yet. Please let me know if you need more info. I haven't actually tested to see if this was the exact commit that caused the regression, but it seems to be the one. > ------------------ > 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 -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/