Re: remote watchpoint support

[Andrew Cagney <ac131313 at cygnus dot com>]

Mark Salter wrote:
> 
> Ok. Here's a patch that allows a remote target to pass watchpoint
> information back to gdb using the T packet. It also fixes the T
> packet handling to ignore other optional info which may not be
> recognized. This changes the behavior to match the documentation.
> 
> While testing this, I discovered that GDB tries to read data from
> the watched address before the watchpoints are removed. This causes
> another watchpoint exception so the stub returns an error response
> to the read memory request.
> 
> --Mark
> 
> Index: ChangeLog
> ===================================================================
> RCS file: /cvs/src/src/gdb/ChangeLog,v
> retrieving revision 1.744
> diff -p -r1.744 ChangeLog
> *** ChangeLog   2000/10/31 19:35:03     1.744
> --- ChangeLog   2000/11/01 21:06:26
> ***************
> *** 1,3 ****
> --- 1,10 ----
> + 2000-11-01  Mark Salter  <msalter@redhat.com>
> +
> +       * remote.c (remote_wait): Add support to extract optional
> +       watchpoint information from T-packet. Ignore unrecognized
> +       optional info in T-packet.
> +       (remote_async_wait): Ditto.
> +


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

> 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?

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

	Andrew


> *************** 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:

> --- 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.

	Andrew