Re: generic_load: Add ``set remote {read,write}-packet-size NNNN''

[Andrew Cagney <ac131313@cygnus.com>]

Well,

After much feedback :-), I've reviewed the behavour of the commands and
consequently changed it to:

	set remote memory-{read,write}-packet-size {VALUE,fixed,limit}

The VALUE is either a ``limit'' or ``fixed''.  A ``limit'' is further
reduced by GDB being paranoid about overrunning the targets buffer.  A
fixed value bypasses that mechanism.  GDB's existing behavour is to
``limit'' the packet size and that is made the default.

The main motivation for the change is that my initial implementaton
would change the packet size whenever the architecture changed :-(.  The
new implementation is more transparent (?).  

For instance:

Initially the packet size is not set so the target default is used and
that can be changed to anything:

(gdb) show remote memory-read-packet-size
The memory-read-packet-size is 0.  Packets are limited to 400 bytes.
(gdb) set remote memory-read-packet-size 200
(gdb) show remote memory-read-packet-size
The memory-read-packet-size is 200.  Packets are limited to 200 bytes.
(gdb) set remote memory-read-packet-size 10000
(gdb) show remote memory-read-packet-size
The memory-read-packet-size is 10000.  Packets are limited to 400 bytes.

Fixing the packet size attracts a warning and a different show output:

(gdb) set remote memory-read-packet-size fixed
The target may not be able to correctly handle a memory-read packet size
of 200 bytes.
Change the packet size? (y or n) y
(gdb) show remote memory-read-packet-size 
The memory-read-packet-size is fixed at 200 bytes.
(gdb) set remote memory-read-packet-size limit
(gdb) show remote memory-read-packet-size
The memory-read-packet-size is 200.  Packets are limited to 200 bytes.
(gdb) set remote memory-read-packet-size 0
(gdb) show remote memory-read-packet-size
The memory-read-packet-size is 0.  Packets are limited to 400 bytes.

Changing the architecture leads to fairly consistent behavour (say there
are two architectures byte50 and byte60):

(gdb) set remote memory-read-packet-size fixed
...
(gdb) set remote memory-read-packet-size 100
(gdb) set architecture byte50
(gdb) show remote memory-read-packet-size
The memory-read-packet-size is fixed at 100 bytes.
(gdb) set architecture byte60
(gdb) show remote memory-read-packet-size
The memory-read-packet-size is fixed at 100 bytes.
(gdb) set remote memory-read-packet-size limit
(gdb) show remote memory-read-packet-size
The memory-read-packet-size is 100.  Packets are limited to 60 bytes.
(gdb) set architecture byte50
(gdb) show remote memory-read-packet-size
The memory-read-packet-size is 100.  Packets are limited to 50 bytes.

More importantly the sequence:

	set remote .... NNN
	file ZYX

and

	file ZYX
	set remote .... NNN

now have identical behavour.

Thoughts.  Better names?

	Andrew


Andrew Cagney wrote:
> 
> Hello,
> 
> The attatched patch to remote.c adds support for the two commands:
> 
>         set remote read-packet-size VALUE
>         set remote write-packet-size VALUE
> 
> (and deprecates the old badly named ``set remotepacketsize''). The
> command has the following behavour:
> 
>         <=0     The default packet size (as determined
>                 by playing around with REGISTER_RAW_SIZE
>                 is restored.
> 
>         >=16k   Illegal until the alloca() are removed.
> 
>         >= ~REGISTER_RAW_SIZE/2
>                 The user is prompted for confirmation that
>                 they really really want to change the buffer
>                 size.
> 
>         other   The specified size is used but also capped
>                 by the size of the ``g'' packet returned
>                 by the target (consistent with the current
>                 behavour)!
> 
> Of course, if someone has better names (or other comments) please post
> :-)
> 
>         Andrew
> 
> (gdb) set remote read-packet-size 0
> Packet size set to the default of 400 bytes.
> (gdb) set remote read-packet-size 200
> (gdb) show remote read-packet-size
> The maximum number of characters per memory-read packet is 200.
> (gdb) set remote read-packet-size 401
> The read packet size of 401 is larger then the
> default of 400 for this target.
> Do you really want to make the change? (y or n) n
> (gdb) show remote read-packet-size
> The maximum number of characters per memory-read packet is 200.
> (gdb) set remote read-packet-size 401
> The read packet size of 401 is larger then the
> default of 400 for this target.
> Do you really want to make the change? (y or n) y
> (gdb) show remote read-packet-size
> The maximum number of characters per memory-read packet is 401.
> (gdb)
> 
>   ------------------------------------------------------------------------
> Wed Nov  3 17:14:39 1999  Andrew Cagney  <cagney@b1.cygnus.com>
> 
>         * remote.c (get_memory_packet_size, set_memory_packet_size): New
>         functions. Set/compute the size of a memory read/write packet.
>         (set_memory_read_packet_size, set_memory_write_packet_size): New.
>         Verify changes to the memory read/write packet size.
>         (memory_read_packet_size, memory_write_packet_size):
>         New. Determine the current memory read/write packet size.  A
>         function is needed as ``actual_register_packet_size'', a variable
>         is used in the calculation.
>         (register_remote_packet_sizes, build_remote_packet_sizes):
>         Initialize packet sizes according the current architecture.
>         (prefered_write_packet_size, actual_write_packet_size,
>         prefered_read_packet_size, actual_read_packet_size): New
>         variables.
>         (remote_fetch_registers, remote_fetch_registers,
>         remote_write_bytes, remote_read_bytes, build_remote_gdbarch_data):
>         Update.
>         (_initialize_remote): Add the commands ``set remote
>         read-packet-size'' and ``set remote write-packet-size''.
>         Deprecate ``set remotepacketsize''.
>
From muller@cerbere.u-strasbg.fr Thu Nov 04 02:43:00 1999
From: Pierre Muller <muller@cerbere.u-strasbg.fr>
To: gdb-patches@sourceware.cygnus.com
Subject: Watching complex expressions patch
Date: Thu, 04 Nov 1999 02:43:00 -0000
Message-id: <199911041058.LAA04309@cerbere.u-strasbg.fr>
References: <Pine.LNX.4.10.9911031800380.19860-100000@hpcll168.cup.hp.com>
X-SW-Source: 1999-q4/msg00170.html
Content-length: 5912

  I think that the approach of VALUE_LAZY for the watchpoint is completely
wrong !

  I propose here a patch that completely change the mecanism of which
memory must be 
watched :

   if you have a struct  t { int a,b,c;}
and you set a watch to t.c
you only want to get stopped if t.c changes not if t.a or t.b changes
but currently using the VALUE_LAZY seems to be quite unsure 
  On  go32 target,
the code wanted to watch first t.c and then this entire t struct and that
is wrong of course !

  So my mecanism remembers the last memory that has been set a watch 
and after reject all watches of bigger memory regions including that memory !

  For more complex watch, with intermediate value that can change
like if p is a pointer to the above struct t
   watch p->c
  will the watch both current location of t.c (if p is set to &t)
and location of p as it is not inside the memory region of t.c itself !
(Even if you imagine that you have complicated case where the same memory is 
fetched several times it still should work correctly !)


Index: breakpoint.c
===================================================================
RCS file: /cvs/gdb/gdb/gdb/breakpoint.c,v
retrieving revision 1.1.1.16
diff -b -c -r1.1.1.16 breakpoint.c
*** breakpoint.c	1999/11/02 04:44:13	1.1.1.16
--- breakpoint.c	1999/11/04 10:30:09
***************
*** 951,956 ****
--- 951,959 ----
  
  	if (within_current_scope)
  	  {
+          CORE_ADDR last_lval_address = 0;
+          int last_lval_size = 0;
+          int last_type = 0;
  	    /* Evaluate the expression and cut the chain of values
  	       produced off from the value chain.
  
***************
*** 967,977 ****
  	    /* Look at each value on the value chain.  */
  	    for (; v; v = v->next)
  	      {
! 		/* If it's a memory location, and GDB actually needed
!                    its contents to evaluate the expression, then we
!                    must watch it.  */
! 		if (VALUE_LVAL (v) == lval_memory
! 		    && ! VALUE_LAZY (v))
  		  {
  		    CORE_ADDR addr;
  		    int len, type;
--- 970,982 ----
  	    /* Look at each value on the value chain.  */
  	    for (; v; v = v->next)
  	      {
! 		  /* If it's a memory location, then we must watch it.  */
!         if ((VALUE_LVAL (v) == lval_memory) &&
!         /* Unless its a bigger part from a small part we want to
!            watch */
!             !((VALUE_ADDRESS (v) + VALUE_OFFSET (v) <= last_lval_address) &&
!               (VALUE_ADDRESS (v) + VALUE_OFFSET (v) + TYPE_LENGTH
(VALUE_TYPE (v))
!                 >= last_lval_address + last_lval_size)))
  		  {
  		    CORE_ADDR addr;
  		    int len, type;
***************
*** 983,988 ****
--- 988,997 ----
  		      type = hw_read;
  		    else if (b->type == bp_access_watchpoint)
  		      type = hw_access;
+           /* Non terminal are only important if
+              their value changes */
+           if (last_type)
+             type = hw_write;
  
  		    val = target_insert_watchpoint (addr, len, type);
  		    if (val == -1)
***************
*** 991,996 ****
--- 1000,1008 ----
  			break;
  		      }
  		    val = 0;
+           last_lval_size = len;
+           last_lval_address = VALUE_ADDRESS (v) + VALUE_OFFSET (v);
+           last_type = type;
  		  }
  	      }
  	    /* Failure to insert a watchpoint on any memory value in the
***************
*** 1309,1323 ****
  	   && !b->duplicate)
      {
        value_ptr v, n;
  
        b->inserted = (is == mark_inserted);
        /* Walk down the saved value chain.  */
        for (v = b->val_chain; v; v = v->next)
  	{
  	  /* For each memory reference remove the watchpoint
  	     at that address.  */
! 	  if (VALUE_LVAL (v) == lval_memory
! 	      && ! VALUE_LAZY (v))
  	    {
  	      CORE_ADDR addr;
  	      int len, type;
--- 1321,1352 ----
  	   && !b->duplicate)
      {
        value_ptr v, n;
+       CORE_ADDR last_lval_address = 0;
+       int last_lval_size = 0;
+       int last_type = 0;
  
        b->inserted = (is == mark_inserted);
        /* Walk down the saved value chain.  */
        for (v = b->val_chain; v; v = v->next)
  	{
  	  /* For each memory reference remove the watchpoint
+ 	     at that address.  */
+ 		  /* If it's a memory location, then we must watch it.  */
+         if ((VALUE_LVAL (v) == lval_memory) &&
+         /* Unless its a bigger part from a small part we want to
+            watch */
+             !((VALUE_ADDRESS (v) + VALUE_OFFSET (v) <= last_lval_address) &&
+               (VALUE_ADDRESS (v) + VALUE_OFFSET (v) + TYPE_LENGTH
(VALUE_TYPE (v))
+                 >= last_lval_address + last_lval_size)))
+ 	  /* For each memory reference remove the watchpoint
  	     at that address.  */
! 		  /* If it's a memory location, then we must watch it.  */
!         if ((VALUE_LVAL (v) == lval_memory) &&
!         /* Unless its a bigger part from a small part we want to
!            watch */
!             !((VALUE_ADDRESS (v) + VALUE_OFFSET (v) <= last_lval_address) &&
!               (VALUE_ADDRESS (v) + VALUE_OFFSET (v) + TYPE_LENGTH
(VALUE_TYPE (v))
!                 >= last_lval_address + last_lval_size)))
  	    {
  	      CORE_ADDR addr;
  	      int len, type;
***************
*** 1329,1339 ****
--- 1358,1373 ----
  		type = hw_read;
  	      else if (b->type == bp_access_watchpoint)
  		type = hw_access;
+          if (last_type)
+            type = hw_write;
  
  	      val = target_remove_watchpoint (addr, len, type);
  	      if (val == -1)
  		b->inserted = 1;
  	      val = 0;
+          last_lval_size = len;
+          last_lval_address = VALUE_ADDRESS (v) + VALUE_OFFSET (v);
+          last_type = type;
  	    }
  	}
        /* Failure to remove any of the hardware watchpoints comes here.  */




Pierre Muller
Institut Charles Sadron
6,rue Boussingault
F 67083 STRASBOURG CEDEX (France)
mailto:muller@ics.u-strasbg.fr
Phone : (33)-3-88-41-40-07  Fax : (33)-3-88-41-40-99
From msnyder@cygnus.com Thu Nov 04 11:31:00 1999
From: Michael Snyder <msnyder@cygnus.com>
To: Pierre Muller <muller@cerbere.u-strasbg.fr>
Cc: gdb-patches@sourceware.cygnus.com
Subject: Re: Watching complex expressions patch
Date: Thu, 04 Nov 1999 11:31:00 -0000
Message-id: <3821DF12.7A84@cygnus.com>
References: <Pine.LNX.4.10.9911031800380.19860-100000@hpcll168.cup.hp.com> <199911041058.LAA04309@cerbere.u-strasbg.fr>
X-SW-Source: 1999-q4/msg00171.html
Content-length: 1046

Pierre Muller wrote:
> 
>   I think that the approach of VALUE_LAZY for the watchpoint is
> completely wrong !
> 
>   I propose here a patch that completely change the mecanism of
> which memory must be watched :
> 
>    if you have a struct  t { int a,b,c;}
> and you set a watch to t.c
> you only want to get stopped if t.c changes not if t.a or t.b
> changes but currently using the VALUE_LAZY seems to be quite unsure
>   On  go32 target,
> the code wanted to watch first t.c and then this entire t struct
> and that is wrong of course !

You are quite right!
This problem is not limited to go32, I think it is general.
I will review your patch, and if it really fixes this problem
I will be very happy.

>   So my mecanism remembers the last memory that has been set a watch
> and after reject all watches of bigger memory regions including that
> memory !

Hmmm... I'm not sure that's really a valid approach.
What if the user deliberately says:
	watch t.a
	watch t

You might argue that is unnecessary, but we have
no reason to forbid it.
From shebs@cygnus.com Thu Nov 04 11:50:00 1999
From: Stan Shebs <shebs@cygnus.com>
To: muller@cerbere.u-strasbg.fr
Cc: gdb-patches@sourceware.cygnus.com
Subject: Re: Watching complex expressions patch
Date: Thu, 04 Nov 1999 11:50:00 -0000
Message-id: <199911041950.LAA23185@andros.cygnus.com>
References: <199911041058.LAA04309@cerbere.u-strasbg.fr>
X-SW-Source: 1999-q4/msg00172.html
Content-length: 748

   Date: Thu, 04 Nov 1999 11:46:19 +0100
   From: Pierre Muller <muller@cerbere.u-strasbg.fr>

     So my mecanism remembers the last memory that has been set a watch 
   and after reject all watches of bigger memory regions including that memory !

I'm not clear on what you're getting at here, but it sounds wrong.
GDB should allow the user to set any combination of watchpoints on the
same or related pieces of data.  The reason is the same as for
breakpoints; it's possible to attach conditions to watchpoints, and
you may have reason to attach one condition to "watch t.c", but a
different condition to a "watch t".  GDB should evaluate both
conditions whenever t.c changes, but only one condition each time t.a
or t.b changes.

								Stan