From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cagney To: GDB Patches Subject: Re: generic_load: Add ``set remote {read,write}-packet-size NNNN'' Date: Wed, 03 Nov 1999 21:23:00 -0000 Message-id: <38211813.946E03E3@cygnus.com> References: <381FE36C.EA79E935@cygnus.com> X-SW-Source: 1999-q4/msg00169.html 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 > > * 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 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: 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 To: Pierre Muller 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: <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 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 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