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

[Aleksandar Ristovski <aristovski@qnx.com>]

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

Doug Evans wrote:
> On Tue, Jun 16, 2009 at 6:31 PM, Aleksandar Ristovski<aristovski@qnx.com> wrote:
>> Aleksandar Ristovski wrote:
>>> Hello,
>>>
>>> This adds support for Z0 and Z1 (and z0 and z1) packets.
>>>
>> I apologize, the patch should be this one. Change log corrected as well.
>>
>> --
>> Aleksandar Ristovski
>> QNX Software Systems
>>
>>
>> ChangeLog:
>>
>> * target.h (insert_breakpoint, remove_breakpoint): New target functions.
>> * server.c (process_serial_event): 'Z' and 'z' packet - add support
>> for [Zz]0 and [Zz]1 - software and hardware breakpoints.
>> * linux-low.c (linux_target_ops): Add new fields.
>> * spu-low.c (spu_target_ops): Likewise.
>> * win32-low.c (win32_target_ops): Likewise.
> 
> Hi.
> 
> Minor nits (fwiw):
> 
> Inserting the insert_breakpoint/remove_breakpoint fields *between* the
> stopped_by_watchpoint/stopped_data_address fields is odd.
> Can they go before insert_watchpoint?
> 
> I think the code in server.c that watches for 0/1/2/3/4 should use a switch.
> 
> The code previously caught invalid values (e.g. type == '5'), but you
> accidentally removed that (IIUC).
> 
> The code in server.c does this:
> 
> +           if (the_target->insert_watchpoint == NULL)
> +             {
> +               /* No watchpoint support or not a watchpoint command;
> +                  unrecognized either way.  */
> +               own_buf[0] = '\0';
> +               res = 1;
> +             }
> 
> and later does this:
> 
> +       if (res == 0)
> +         write_ok (own_buf);
> +       else if (res == 1)
> +         /* Unsupported.  */
> +         own_buf[0] = '\0';
> +       else
> +         write_enn (own_buf);
> 
> How about just setting res = 1 in the first code snippet above?
> [This appears for both 'z' and 'Z' packets.]
> 

Ok, I made changes accordingly. Note that since the code in 
'Z' and 'z' differs only in whether "insert_" or "remove_" 
version of function is called, I merged the two with the 
distinction being made at the calling point.


-- 
Aleksandar Ristovski
QNX Software Systems


ChangeLog:

* target.h (insert_breakpoint, remove_breakpoint): New 
target functions.
* server.c (process_serial_event): 'Z' and 'z' packet - add 
support
for [Zz]0 and [Zz]1 - software and hardware breakpoints.
* linux-low.c (linux_target_ops): Add new fields.
* spu-low.c (spu_target_ops): Likewise.
* win32-low.c (win32_target_ops): Likewise.

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

Index: target.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/target.h,v
retrieving revision 1.36
diff -u -p -r1.36 target.h
--- target.h	1 Apr 2009 22:50:24 -0000	1.36
+++ target.h	19 Jun 2009 13:47:44 -0000
@@ -213,6 +213,14 @@ struct target_ops
   int (*read_auxv) (CORE_ADDR offset, unsigned char *myaddr,
 		    unsigned int len);
 
+  /* Insert and remove breakpoint.  Argument TYPE can be:
+       '0' = software-breakpoint
+       '1' = hardware-breakpoint 
+     Returns 0 if successful, 1 otherwise.  */
+
+  int (*insert_breakpoint) (char type, CORE_ADDR addr);
+  int (*remove_breakpoint) (char type, CORE_ADDR addr);
+
   /* Insert and remove a hardware watchpoint.
      Returns 0 on success, -1 on failure and 1 on unsupported.
      The type is coded as follows:
Index: server.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/server.c,v
retrieving revision 1.97
diff -u -p -r1.97 server.c
--- server.c	24 May 2009 21:06:53 -0000	1.97
+++ server.c	19 Jun 2009 13:47:44 -0000
@@ -2366,66 +2370,64 @@ 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;
+	const int insert_ = ch == 'Z';
 
-	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
-	  {
-	    int res;
+	/* Type: '0' - software-breakpoint
+		 '1' - hardware-breakpoint
+		 '2' - write watchpoint
+		 '3' - read watchpoint
+		 '4' - access watchpoint  */
 
-	    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';
+	switch (type)
+	  {
+	  case '0':
+	    /* Fallthrough.  */
+	  case '1':
+	    if ((insert_ ? the_target->insert_breakpoint
+			 : the_target->remove_breakpoint) == NULL)
+	      res = 1; /* Not supported.  */
 	    else
-	      write_enn (own_buf);
+	      res = insert_ ? (*the_target->insert_breakpoint) (type, addr)
+			    : (*the_target->remove_breakpoint) (type, addr);
+	    break;
+	  case '2':
+	    /* Fallthrough.  */
+	  case '3':
+	    /* Fallthrough.  */
+	  case '4':
+	    if ((insert_ ? the_target->insert_watchpoint
+			 : the_target->remove_watchpoint) == NULL)
+	      res = 1;  /* Not supported.  */
+	    else
+	      {
+		require_running (own_buf);
+		res = insert_ ? (*the_target->insert_watchpoint) (type, addr,
+								  len)
+			      : (*the_target->remove_watchpoint) (type, addr,
+								  len);
+	      }
+	    break;
+	  default:
+	    res = -1; /* Unrecognized type.  */
 	  }
-	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';
-	  }
+	if (res == 0)
+	  write_ok (own_buf);
+	else if (res == 1)
+	  /* Unsupported.  */
+	  own_buf[0] = '\0';
 	else
-	  {
-	    int res;
-
-	    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);
-	  }
+	  write_enn (own_buf);
 	break;
       }
     case 'k':
Index: linux-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-low.c,v
retrieving revision 1.105
diff -u -p -r1.105 linux-low.c
--- linux-low.c	24 May 2009 17:44:19 -0000	1.105
+++ linux-low.c	19 Jun 2009 13:47:45 -0000
@@ -3024,6 +3030,8 @@ static struct target_ops linux_target_op
   linux_look_up_symbols,
   linux_request_interrupt,
   linux_read_auxv,
+  NULL,  /* insert_breakpoint */
+  NULL,  /* remove_breakpoint */
   linux_insert_watchpoint,
   linux_remove_watchpoint,
   linux_stopped_by_watchpoint,
Index: spu-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/spu-low.c,v
retrieving revision 1.24
diff -u -p -r1.24 spu-low.c
--- spu-low.c	3 Apr 2009 14:38:39 -0000	1.24
+++ spu-low.c	19 Jun 2009 13:47:45 -0000
@@ -612,6 +612,8 @@ static struct target_ops spu_target_ops 
   spu_look_up_symbols,
   spu_request_interrupt,
   NULL,
+  NULL, /* insert_breakpoint */
+  NULL, /* remove_breakpoint */
   NULL,
   NULL,
   NULL,
Index: win32-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/win32-low.c,v
retrieving revision 1.35
diff -u -p -r1.35 win32-low.c
--- win32-low.c	1 Apr 2009 22:50:24 -0000	1.35
+++ win32-low.c	19 Jun 2009 13:47:45 -0000
@@ -1697,6 +1697,8 @@ static struct target_ops win32_target_op
   NULL,
   win32_request_interrupt,
   NULL,
+  NULL, /* insert_breakpoint */
+  NULL, /* remove_breakpoint */
   NULL,
   NULL,
   NULL,