Re: [rfa] hardware breakpoints -- remote targets

[Andrew Cagney <ac131313@ges.redhat.com>]

> I revised the patch to include Mark Salter's earlier patch, as well as
> his documentation changes.

Lets see, Mark's doco patch was previously approved vis:
http://sources.redhat.com/ml/gdb-patches/2000-11/msg00020.html
(Someone (me) has even gone through the table and has a patch to fix the 
formatting pending :-)

Hold back on the commit, though, until we've got the below ok.



> -char *unpack_varlen_hex (char *buff, int *result);
> +char *unpack_varlen_hex (char *buff, ULONGEST *result);

I didn't see a changelog entry for this.  Anyway, this change (and the 
corresponding int->ULONGEST tweaks) should be separated out committed 
independantly.  This part is approved.



> +/* This is set to the data address of the access causing the target
> + * to stop for a watchpoint. */
(NB: no leading ``*'' on the second comment line.)

> +static CORE_ADDR remote_watch_data_address;
> +
> +/* This is non-zero if taregt stopped for a watchpoint. */
> +static int remote_stopped_by_watchpoint_p;
> +
> +

For the above can you please add a ``FIXME: graces/YYYY-MM-DD:'' style 
comment noteing that, like gdbarch_tdep() for ``struct gdbarch'', these 
static variables should be bound to an instance of the target object. 
Only  there isn't such a thing :-(



> @@ -3025,6 +3035,8 @@
>        if (target_wait_loop_hook)
>  	(*target_wait_loop_hook) ();
>  
> +      remote_stopped_by_watchpoint_p = 0;
> +
>        switch (buf[0])
>  	{
>  	case 'E':		/* Error of some sort */
> @@ -3048,10 +3060,19 @@
>  		unsigned char *p1;
>  		char *p_temp;
>  		int fieldsize;
> +		LONGEST pnum = 0;
>  
> -		/* Read the ``P'' register number.  */
> -		LONGEST pnum = strtol ((const char *) p, &p_temp, 16);
> -		p1 = (unsigned char *) p_temp;
> +		/* If this packet is an awatch packet, don't parse the 'a'
> +		   as a register number.  */

Add a comment mentioning that ``p1'' is left pointing to ``p'' if there 
wasn't a register number.



> +		if (!strstr ((const char *) p, "awatch"))

Use ``strncmp (p, "awatch", strlen ("awatch")) != 0''.

This is because the string compare needs to be anchored on the first 
character (otherwize it could miss-parse ``00=a123;awatch''.  BTW, it 
should be possible to safely remove most of those ``(const char*)'' and 
``(char*)'' casts.


> +		  {
> +		    /* Read the ``P'' register number.  */
> +		    pnum = strtol ((const char *) p, &p_temp, 16);
> +		    p1 = (unsigned char *) p_temp;
> +		  }
> +		else 
> +		  p1=p;

Don't forget the spaces:
	p1 = p;



>  		if (p1 == p)	/* No register number present here */
>  		  {
> @@ -3066,6 +3087,21 @@
>  			record_currthread (thread_num);
>  			p = (unsigned char *) p_temp;
>  		      }
> +		    else if ((strncmp ((const char *) p, "watch", p1 - p) == 0)
> +			     || (strncmp ((const char *) p, "rwatch", p1 - p) == 0)
> +			     || (strncmp ((const char *) p, "awatch", p1 - p) == 0))
> +		      {
> +			remote_stopped_by_watchpoint_p = 1;
> +			p = unpack_varlen_hex (++p1, &addr);
> +			remote_watch_data_address = (CORE_ADDR)addr;
> +		      }
> +		    else
> + 		      {
> + 			/* silently skip unknown optional info */
> + 			p_temp = (unsigned char *) strchr ((const char *) p1+1, ';');

Spaces, ``p1 + 1'', no cast.  The comment should be a sentence (hi MarkK 
;-) with period and two spaces after it :-)


> @@ -3268,10 +3307,19 @@
>  		unsigned char *p1;
>  		char *p_temp;
>  		int fieldsize;
> +		long pnum = 0;
>  
> -		/* Read the register number */
> -		long pnum = strtol ((const char *) p, &p_temp, 16);
> -		p1 = (unsigned char *) p_temp;
> +		/* If this packet is an awatch packet, don't parse the 'a'

see the comments above ...



> +static void
> +set_hardware_watchpoint_limit (char *args, int from_tty)
> +{
> +  if (args)
> +    {
> +      char *arg_end;
> +      int val = strtoul (args, &arg_end, 10);
> +      if (*args && *arg_end == '\0')
> +        {
> +          remote_hw_watchpoint_limit = val;
> +	  return;
> +	}
> +    }
> +  error ("Illegal argument for \"set remote hardware-watchpoint-limit\" command");
> +}

Hmm, can you please split out the new commands from the other parts of 
the patch so that they can be considered separatly.  I'm wondering if a 
better way of doing this is to have GDB automatically detect if another 
watchpoint is available.

Anyway new commands would need doco.

Andrew

(And thanks for finally creating a patch that adds watchpoint support to 
the target vector where it belongs!)