Re: [gdbserver/patch] Z packet support

[Daniel Jacobowitz <drow@false.org>]

On Wed, Dec 01, 2004 at 04:12:43PM +0100, Orjan Friberg wrote:
> A while ago there was a brief discussion concerning Z packet support in the 
> gdbserver (starting at 
> http://sources.redhat.com/ml/gdb-patches/2004-05/msg00706.html) where it 
> was suggested that maybe the watchpoint code should be shared with GDB.
> 
> Well, I implemented Z packet support in the most straight-forward way ever: 
> separate from GDB (and I have an upcoming CRISv32 port which would use it). 
> I don't know if the various watchpoint functions need to change to 
> accommodate other architectures (if memory serves me correctly there were 
> some issues with the s390 on the host side regarding "stopped data 
> address").

I'm fine with the approach.  The implementation needs a bit of work,
though.

First of all, your mailer has mangled the patch.  A lot of leading
whitespace is messed up.  Could you fix that?  I'd like to be able to
play with this for i386 before approving it.

Secondly (presumably a separate problem), you've lost a lot of
whitespace before open parentheses.

> +  /* Watchpoint related functions.  */
> +  int (*insert_watchpoint)(char type, CORE_ADDR addr, int len);
> +  int (*remove_watchpoint)(char type, CORE_ADDR addr, int len);
> +  int (*stopped_by_watchpoint) (void);
> +  CORE_ADDR (*stopped_data_address) (void);

More detailed comments, please - at least for the target.h copy.  For
instance, what are the return values?

> +static int linux_insert_watchpoint (char type, CORE_ADDR addr, int len)
> +{
> +  if (the_low_target.insert_watchpoint != NULL)
> +    return the_low_target.insert_watchpoint (type, addr, len);
> +  else
> +    return -1;
> +}

Formatting; function names start in column 0.  Also, this block of
wrapper functions could use at least one comment.

> Index: remote-utils.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/remote-utils.c,v
> retrieving revision 1.22
> diff -u -p -r1.22 remote-utils.c
> --- remote-utils.c	16 Oct 2004 17:42:00 -0000	1.22
> +++ remote-utils.c	1 Dec 2004 14:49:02 -0000
> @@ -639,6 +639,30 @@ prepare_resume_reply (char *buf, char st
>    if (status == 'T')
>      {
>        const char **regp = gdbserver_expedite_regs;
> +
> +      if (*the_target->stopped_by_watchpoint != NULL

I'm surprised that compiles.... the * there is spurious.

> +	  && (*the_target->stopped_by_watchpoint) ())
> +	{
> +	  CORE_ADDR addr;
> +
> +	  strncpy (buf, "watch:", 6);
> +	  buf += 6;
> +
> +	  addr = (*the_target->stopped_data_address) ();
> +
> +	  *buf++ = tohex ((addr >> 28) & 0xf);
> +	  *buf++ = tohex ((addr >> 24) & 0xf);
> +	  *buf++ = tohex ((addr >> 20) & 0xf);
> +	  *buf++ = tohex ((addr >> 16) & 0xf);
> +
> +	  *buf++ = tohex ((addr >> 12) & 0xf);
> +	  *buf++ = tohex ((addr >> 8) & 0xf);
> +	  *buf++ = tohex ((addr >> 4) & 0xf);
> +	  *buf++ = tohex (addr & 0xf);
> +
> +	  *buf++ = ';';
> +	}

This is broken on 64-bit targets.  This will require a certain amount
of shuffling to figure out the proper size of an address.

> +	    case 'Z':
> +	      {
> +		char *lenptr;
> +		char *dataptr;
> +		CORE_ADDR addr = strtoul(&own_buf[3], &lenptr, 16);
> +		int len = strtol(lenptr + 1, &dataptr, 16);
> +		char type = own_buf[1];
> +
> +		set_desired_inferior (0);
> +		if (the_target->insert_watchpoint != NULL
> +		    && ((*the_target->insert_watchpoint) (type, addr, len)
> +			== 0))
> +		  write_ok (own_buf);
> +		else
> +		  own_buf[0] = '\0';
> +		break;
> +	      }

There's two problems here:

  - Some of the z/Z commands are breakpoints, not watchpoints.  Please
    only invoke watchpoint methods for watchpoint commands.

  - An empty string is supposed to represent an unrecognized "type". 
    We should distinguish error from unsupported.

Why the call to set_desired_inferior?  This should presumably affect
all threads, since that's how GDB behaves (or is supposed to). 
Most likely that should be handled in the linux-low wrappers.

-- 
Daniel Jacobowitz