Re: remote watchpoint support

[Mark Salter <msalter@redhat.com>]

>>>>> Andrew Cagney writes:

> FYI, changelog entries should be separated out and not included in the
> diff.

Yup. I think I did that the last time, but had a relapse of expediency
this time.

>> Index: remote.c
>> ===================================================================
>> RCS file: /cvs/src/src/gdb/remote.c,v
>> retrieving revision 1.26
>> diff -p -r1.26 remote.c
>> *** remote.c    2000/10/23 22:49:28     1.26
>> --- remote.c    2000/11/01 21:06:29
>> *************** void _initialize_remote (void);
>> *** 204,209 ****
>> --- 204,216 ----
>> 
>> /* */
>> 
>> + /* This is set to the data address of the access causing the target
>> +  * to stop for a watchpoint. */
>> + static CORE_ADDR remote_watch_data_address;
>> +
>> + /* This is non-zero if taregt stopped for a watchpoint. */
>> + static int remote_stopped_by_watchpoint_p;
>> +

> I've strong reservations about the addition of these two ``global''
> variables.  Is it possible to modify things higher up so that their
> addition isn't required?

Without a doubt. It would just take time to sort everything out to
make sure existing support isn't broken.

> I'm also trying to understand how remote_stopped_data_address() et.al.
> gets called.

Good luck with that. ;-)

Its all part of bpstat_stop_status(). The inferior stops and gdb tries
to see if the user needs to informed, etc. bpstat_stop_status() walks
through the bp list and for write-only wathcpoints it reads the watched
memory to see if changed. For read-only and access watchpoints, it
calls target_stopped_data_address() which is a macro that usually gets
defined in a tm-xxx.h file. If we add the remote_stopped_by_watchpoint()
and remote_stopped_data_address functions, then the tm-xxx.h additions
for remote watchpoint suppport are pretty simple:

  /* Add HW watchpoint support */

  #define TARGET_HAS_HARDWARE_WATCHPOINTS

  #define TARGET_HW_WATCH_LIMIT 1

  #define TARGET_CAN_USE_HARDWARE_WATCHPOINT(type, cnt, ot) \
	xxx_check_watch_resources (type, cnt, ot)

  /* When a hardware watchpoint fires off the PC will be left at the
     instruction which caused the watchpoint.  It will be necessary for
     GDB to step over the watchpoint. */

  /* Comment out.  On this target, the memory access is issued before
     the watchpoint triggers.*/
  /*#define HAVE_STEPPABLE_WATCHPOINT 1*/

  #define STOPPED_BY_WATCHPOINT(W) \
     remote_stopped_by_watchpoint()

  /* Use these macros for watchpoint insertion/deletion.  */
  #define target_insert_watchpoint(addr, len, type) \
		remote_insert_watchpoint (addr, len, type)
  #define target_remove_watchpoint(addr, len, type) \
		remote_remove_watchpoint (addr, len, type)
  /* Use this to get the data address that triggered a watchpoint */
  #define target_stopped_data_address() \
		remote_stopped_data_address()

  extern int xxx_check_watch_resources (int type, int cnt, int ot);



>> *************** struct gdb_ext_thread_info
>> *** 963,969 ****
>> 
>> #define BUF_THREAD_ID_SIZE (OPAQUETHREADBYTES*2)
>> 
>> ! char *unpack_varlen_hex (char *buff, int *result);
>> 
>> static char *unpack_nibble (char *buf, int *val);
>> 
>> --- 970,976 ----
>> 
>> #define BUF_THREAD_ID_SIZE (OPAQUETHREADBYTES*2)
>> 
>> ! char *unpack_varlen_hex (char *buff, ULONGEST *result);
>> 
>> static char *unpack_nibble (char *buf, int *val);
>> 

> The ChangeLog doesn't mention this change?  Can I suggest this be
> submitted separatly with the tweek that follows:

Oops.

>> --- 2657,2682 ----
>> p, buf);
>> if (strncmp ((const char *) p, "thread", p1 - p) == 0)
>> {
> 		          ULONGEST thread_num;
>> !                       p_temp = unpack_varlen_hex (++p1, &ul);
>> !                       thread_num = ul;
>> record_currthread (thread_num);
>> p = (unsigned char *) p_temp;
>> }

> Suggest just making threadnum a LONGEST and being done with it.

Ok.

--Mark