This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 08/10] Handle "show remote memory-write-packet-size" when not connected


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 <palves@redhat.com>
> 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
> <https://sourceware.org/ml/gdb-patches/2015-08/msg00743.html>, 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  <palves@redhat.com>
>
> 	* 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  <palves@redhat.com>
>
> 	* 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/


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]