Re: [patch] gdbserver: Add support for Z0/Z1 packets

[Aleksandar Ristovski <aristovski@qnx.com>]

[-- Attachment #1: Type: text/plain, Size: 1969 bytes --]

Pedro Alves wrote:
> On Tuesday 23 June 2009 16:17:58, Aleksandar Ristovski wrote:
>> Pedro Alves wrote:
>>> On Monday 22 June 2009 20:38:50, Aleksandar Ristovski wrote:
>>>
>>>>> Z0 and Z1 breakpoints also take a 'len' argument, just
>>>>> like Z2-Z4.  You should also pass those down.
>>>>>
>>>>> But, Let's take a step back --- why not just rename the
>>>>> insert_watchpoint|remove_watchpoint functions to insert_point,remove_point,
>>>>> and relax the type checks in server.c:
> 
>>>> But either way is fine with me - just let me know.
>>> I'd prefer the approach I suggested, and worry about splitting
>>> the breakpoints from watchpoints API if/when we actually need it.
>>>
>> Ok, then that version is committed.
> 
> Well, we had never seen "that" version

Ok, to rectify this I am attaching two versions: one if I 
revert the changes I committed and the other is diff to what 
is in now.

 > ... and you bypassed the "rename" suggestion...

I did not do any renaming - I think it is not terribly 
confusing since both in target.h comment and server.c 'Z' 
case it is made very clear that it handles both breakpoints 
and watchpoints (i.e. I don't find it any clearer if it was 
called "insert_point"... it would still require reading the 
comment in target.h)

> 
> Would you care to explain why are watchpoints guarded on
> require_running and breakpoints aren't?  It's not super
> obvious to me.

Both proposed versions now check for require_running for any 
kind of breakpoint.

ChangeLog - diff to what is in now.
* server.c (process_serial_event): Treat all types of
break/watch-points the same.

ChangeLog - diff to what was before my commit:
* server.c (process_serial_event): Add support for Z0 and Z1 
packet.
* target.h: Comment for *_watchpoint to make it clear the 
functions can get types '0' and '1'.

(attached first diff to what is in already and then diff to 
what was before commit).


Thanks,

-- 
Aleksandar Ristovski
QNX Software Systems

[-- Attachment #2: gdbserver-Z0Z1support-20090624.diff --]
[-- Type: text/x-patch, Size: 1520 bytes --]

Index: server.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/server.c,v
retrieving revision 1.99
diff -u -p -r1.99 server.c
--- server.c	23 Jun 2009 15:12:44 -0000	1.99
+++ server.c	24 Jun 2009 15:36:11 -0000
@@ -2381,7 +2381,6 @@ process_serial_event (void)
 	int len = strtol (lenptr + 1, &dataptr, 16);
 	char type = own_buf[1];
 	int res;
-	const int insert_ = ch == 'Z';
 
 	/* Type: '0' - software-breakpoint
 		 '1' - hardware-breakpoint
@@ -2392,27 +2391,16 @@ process_serial_event (void)
 	if (the_target->insert_watchpoint == NULL
 	    || the_target->remove_watchpoint == NULL)
 	  res = 1;  /* Not supported.  */
+	else if (type >= '0' && type <= '4')
+	  {
+	    const int insert_ = ch == 'Z';
+
+	    require_running (own_buf);
+	    res = insert_ ? (*the_target->insert_watchpoint) (type, addr, len)
+			  : (*the_target->remove_watchpoint) (type, addr, len);
+	  }
 	else
-	  switch (type)
-	    {
-	    case '2':
-	      /* Fallthrough.  */
-	    case '3':
-	      /* Fallthrough.  */
-	    case '4':
-	      require_running (own_buf);
-	      /* Fallthrough.  */
-	    case '0':
-	      /* Fallthrough.  */
-	    case '1':
-	      res = insert_ ? (*the_target->insert_watchpoint) (type, addr,
-								len)
-			    : (*the_target->remove_watchpoint) (type, addr,
-								len);
-	      break;
-	    default:
-	      res = -1; /* Unrecognized type.  */
-	    }
+	  res = -1; /* Unrecognized type.  */
 
 	if (res == 0)
 	  write_ok (own_buf);

[-- Attachment #3: gdbserver-Z0Z1support-20090624-1.diff --]
[-- Type: text/x-patch, Size: 3443 bytes --]

Index: server.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/server.c,v
retrieving revision 1.98
diff -u -p -r1.98 server.c
--- server.c	19 Jun 2009 13:35:35 -0000	1.98
+++ server.c	24 Jun 2009 17:13:04 -0000
@@ -2371,66 +2371,44 @@ process_serial_event (void)
       signal = 0;
       myresume (own_buf, 1, signal);
       break;
-    case 'Z':
+    case 'Z':  /* insert_ ... */
+      /* Fallthrough.  */
+    case 'z':  /* remove_ ... */
       {
 	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];
+	int res;
+
+	/* Type: '0' - software-breakpoint
+		 '1' - hardware-breakpoint
+		 '2' - write watchpoint
+		 '3' - read watchpoint
+		 '4' - access watchpoint  */
 
 	if (the_target->insert_watchpoint == NULL
-	    || (type < '2' || type > '4'))
-	  {
-	    /* No watchpoint support or not a watchpoint command;
-	       unrecognized either way.  */
-	    own_buf[0] = '\0';
-	  }
-	else
+	    || the_target->remove_watchpoint == NULL)
+	  res = 1;  /* Not supported.  */
+	else if (type >= '0' && type <= '4')
 	  {
-	    int res;
-
 	    require_running (own_buf);
-	    res = (*the_target->insert_watchpoint) (type, addr, len);
-	    if (res == 0)
-	      write_ok (own_buf);
-	    else if (res == 1)
-	      /* Unsupported.  */
-	      own_buf[0] = '\0';
+	    if (ch == 'Z')
+	      res = (*the_target->insert_watchpoint) (type, addr, len);
 	    else
-	      write_enn (own_buf);
-	  }
-	break;
-      }
-    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];
-
-	if (the_target->remove_watchpoint == NULL
-	    || (type < '2' || type > '4'))
-	  {
-	    /* No watchpoint support or not a watchpoint command;
-	       unrecognized either way.  */
-	    own_buf[0] = '\0';
+	      res = (*the_target->remove_watchpoint) (type, addr, len);
 	  }
 	else
-	  {
-	    int res;
+	  res = -1; /* Unrecognized type.  */
 
-	    require_running (own_buf);
-	    res = (*the_target->remove_watchpoint) (type, addr, len);
-	    if (res == 0)
-	      write_ok (own_buf);
-	    else if (res == 1)
-	      /* Unsupported.  */
-	      own_buf[0] = '\0';
-	    else
-	      write_enn (own_buf);
-	  }
+	if (res == 0)
+	  write_ok (own_buf);
+	else if (res == 1)
+	  /* Unsupported.  */
+	  own_buf[0] = '\0';
+	else
+	  write_enn (own_buf);
 	break;
       }
     case 'k':
Index: target.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/target.h,v
retrieving revision 1.37
retrieving revision 1.38
diff -u -p -r1.37 -r1.38
--- target.h	19 Jun 2009 13:35:35 -0000	1.37
+++ target.h	23 Jun 2009 15:12:44 -0000	1.38
@@ -216,10 +216,11 @@ struct target_ops
   /* Insert and remove a hardware watchpoint.
      Returns 0 on success, -1 on failure and 1 on unsupported.
      The type is coded as follows:
-       2 = write watchpoint
-       3 = read watchpoint
-       4 = access watchpoint
-  */
+       '0' - software-breakpoint
+       '1' - hardware-breakpoint
+       '2' - write watchpoint
+       '3' - read watchpoint
+       '4' - access watchpoint  */
 
   int (*insert_watchpoint) (char type, CORE_ADDR addr, int len);
   int (*remove_watchpoint) (char type, CORE_ADDR addr, int len);