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

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

[Aleksandar Ristovski]

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

Hello,

This adds support for Z0 and Z1 (and z0 and z1) packets.

Thanks,

-- 
Aleksandar Ristovski
QNX Software Systems

ChangeLog:


* target.h (insert_breakpoint, remove_breakpoint): New 
target functions.
* server.c (handle_query): '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-20090616.diff --]
[-- Type: text/x-patch, Size: 6111 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	17 Jun 2009 01:11:04 -0000
@@ -228,6 +228,14 @@ struct target_ops
 
   int (*stopped_by_watchpoint) (void);
 
+  /* 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);
+
   /* Returns the address associated with the watchpoint that hit, if any;
      returns 0 otherwise.  */
 
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	17 Jun 2009 01:11:05 -0000
@@ -1106,7 +1106,8 @@ handle_query (char *own_buf, int packet_
       if (the_target->qxfer_osdata != NULL)
 	strcat (own_buf, ";qXfer:osdata:read+");
 
-      strcat (own_buf, ";multiprocess+");
+      if (multi_process)
+	strcat (own_buf, ";multiprocess+");
 
       if (target_supports_non_stop ())
 	strcat (own_buf, ";QNonStop+");
@@ -2373,28 +2374,48 @@ process_serial_event (void)
 	CORE_ADDR addr = strtoul (&own_buf[3], &lenptr, 16);
 	int len = strtol (lenptr + 1, &dataptr, 16);
 	char type = own_buf[1];
+	int res = -1;
+
+	/* 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'))
+	if (type < '2' && type >= '0')
 	  {
-	    /* No watchpoint support or not a watchpoint command;
-	       unrecognized either way.  */
-	    own_buf[0] = '\0';
+	    /* Insert breakpoint.  */
+	    if (the_target->insert_breakpoint == NULL)
+	      {
+		/* Not supported.  */
+		own_buf[0] = '\0';
+		res = 1;
+	      }
+	    else
+	      res = (*the_target->insert_breakpoint) (type, addr);
 	  }
 	else
 	  {
-	    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 (the_target->insert_watchpoint == NULL)
+	      {
+		/* No watchpoint support or not a watchpoint command;
+		   unrecognized either way.  */
+		own_buf[0] = '\0';
+		res = 1;
+	      }
 	    else
-	      write_enn (own_buf);
+	      {
+		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';
+	else
+	  write_enn (own_buf);
 	break;
       }
     case 'z':
@@ -2404,28 +2425,48 @@ process_serial_event (void)
 	CORE_ADDR addr = strtoul (&own_buf[3], &lenptr, 16);
 	int len = strtol (lenptr + 1, &dataptr, 16);
 	char type = own_buf[1];
+	int res = -1;
 
-	if (the_target->remove_watchpoint == NULL
-	    || (type < '2' || type > '4'))
+	/* Type: '0' - software-breakpoint
+		 '1' - hardware-breakpoint
+		 '2' - write watchpoint
+		 '3' - read watchpoint
+		 '4' - access watchpoint  */
+
+	if (type < '2' && type >= '0')
 	  {
-	    /* No watchpoint support or not a watchpoint command;
-	       unrecognized either way.  */
-	    own_buf[0] = '\0';
+	    /* Remove breakpoint.  */
+	    if (the_target->remove_breakpoint == NULL)
+	      {
+		/* Not supported.  */
+		own_buf[0] = '\0';
+		res = 1;
+	      }
+	    else
+	      res = (*the_target->remove_breakpoint) (type, addr);
 	  }
 	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';
+	    if (the_target->remove_watchpoint == NULL)
+	      {
+		/* No watchpoint support or not a watchpoint command;
+		   unrecognized either way.  */
+		own_buf[0] = '\0';
+		res = 1;
+	      }
 	    else
-	      write_enn (own_buf);
+	      {
+		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);
 	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	17 Jun 2009 01:11:05 -0000
@@ -3027,6 +3027,8 @@ static struct target_ops linux_target_op
   linux_insert_watchpoint,
   linux_remove_watchpoint,
   linux_stopped_by_watchpoint,
+  NULL,  /* insert_breakpoint */
+  NULL,  /* remove_breakpoint */
   linux_stopped_data_address,
 #if defined(__UCLIBC__) && defined(HAS_NOMMU)
   linux_read_offsets,
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	17 Jun 2009 01:11:05 -0000
@@ -615,6 +615,8 @@ static struct target_ops spu_target_ops 
   NULL,
   NULL,
   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	17 Jun 2009 01:11:05 -0000
@@ -1700,6 +1700,8 @@ static struct target_ops win32_target_op
   NULL,
   NULL,
   NULL,
+  NULL, /* insert_breakpoint */
+  NULL, /* remove_breakpoint */
   NULL,
   NULL,
   NULL,

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

[Aleksandar Ristovski]

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

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.

[-- Attachment #2: gdbserver-Z0Z1support-20090616.diff --]
[-- Type: text/x-patch, Size: 5775 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	17 Jun 2009 01:11:04 -0000
@@ -228,6 +228,14 @@ struct target_ops
 
   int (*stopped_by_watchpoint) (void);
 
+  /* 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);
+
   /* Returns the address associated with the watchpoint that hit, if any;
      returns 0 otherwise.  */
 
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	17 Jun 2009 01:11:05 -0000
@@ -2373,28 +2374,48 @@ process_serial_event (void)
 	CORE_ADDR addr = strtoul (&own_buf[3], &lenptr, 16);
 	int len = strtol (lenptr + 1, &dataptr, 16);
 	char type = own_buf[1];
+	int res = -1;
+
+	/* 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'))
+	if (type < '2' && type >= '0')
 	  {
-	    /* No watchpoint support or not a watchpoint command;
-	       unrecognized either way.  */
-	    own_buf[0] = '\0';
+	    /* Insert breakpoint.  */
+	    if (the_target->insert_breakpoint == NULL)
+	      {
+		/* Not supported.  */
+		own_buf[0] = '\0';
+		res = 1;
+	      }
+	    else
+	      res = (*the_target->insert_breakpoint) (type, addr);
 	  }
 	else
 	  {
-	    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 (the_target->insert_watchpoint == NULL)
+	      {
+		/* No watchpoint support or not a watchpoint command;
+		   unrecognized either way.  */
+		own_buf[0] = '\0';
+		res = 1;
+	      }
 	    else
-	      write_enn (own_buf);
+	      {
+		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';
+	else
+	  write_enn (own_buf);
 	break;
       }
     case 'z':
@@ -2404,28 +2425,48 @@ process_serial_event (void)
 	CORE_ADDR addr = strtoul (&own_buf[3], &lenptr, 16);
 	int len = strtol (lenptr + 1, &dataptr, 16);
 	char type = own_buf[1];
+	int res = -1;
 
-	if (the_target->remove_watchpoint == NULL
-	    || (type < '2' || type > '4'))
+	/* Type: '0' - software-breakpoint
+		 '1' - hardware-breakpoint
+		 '2' - write watchpoint
+		 '3' - read watchpoint
+		 '4' - access watchpoint  */
+
+	if (type < '2' && type >= '0')
 	  {
-	    /* No watchpoint support or not a watchpoint command;
-	       unrecognized either way.  */
-	    own_buf[0] = '\0';
+	    /* Remove breakpoint.  */
+	    if (the_target->remove_breakpoint == NULL)
+	      {
+		/* Not supported.  */
+		own_buf[0] = '\0';
+		res = 1;
+	      }
+	    else
+	      res = (*the_target->remove_breakpoint) (type, addr);
 	  }
 	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';
+	    if (the_target->remove_watchpoint == NULL)
+	      {
+		/* No watchpoint support or not a watchpoint command;
+		   unrecognized either way.  */
+		own_buf[0] = '\0';
+		res = 1;
+	      }
 	    else
-	      write_enn (own_buf);
+	      {
+		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);
 	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	17 Jun 2009 01:11:05 -0000
@@ -3027,6 +3027,8 @@ static struct target_ops linux_target_op
   linux_insert_watchpoint,
   linux_remove_watchpoint,
   linux_stopped_by_watchpoint,
+  NULL,  /* insert_breakpoint */
+  NULL,  /* remove_breakpoint */
   linux_stopped_data_address,
 #if defined(__UCLIBC__) && defined(HAS_NOMMU)
   linux_read_offsets,
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	17 Jun 2009 01:11:05 -0000
@@ -615,6 +615,8 @@ static struct target_ops spu_target_ops 
   NULL,
   NULL,
   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	17 Jun 2009 01:11:05 -0000
@@ -1700,6 +1700,8 @@ static struct target_ops win32_target_op
   NULL,
   NULL,
   NULL,
+  NULL, /* insert_breakpoint */
+  NULL, /* remove_breakpoint */
   NULL,
   NULL,
   NULL,

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

[Doug Evans]

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

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

[Aleksandar Ristovski]

[-- 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,

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

[Pedro Alves]

On Friday 19 June 2009 14:55:56, Aleksandar Ristovski wrote:

> 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 

                                   ^ spurious space

> +     Returns 0 if successful, 1 otherwise.  */
> +
> +  int (*insert_breakpoint) (char type, CORE_ADDR addr);
> +  int (*remove_breakpoint) (char type, CORE_ADDR addr);

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:

	if (the_target->insert_watchpoint == NULL
	    || (type < '2' || type > '4'))
	  {

... to also allow type == '0' or '1' ?

Looking at your nto-low.c file in the original patch, you were
doing this:

+static int
+nto_insert_breakpoint (char type, CORE_ADDR addr)
            ^^^^^^^^^^
+{
+  TRACE ("%s\n", __func__);
+
+  return nto_insert_watchpoint (type, addr, 1);
                     ^^^^^^^^^^
+}

In the latest patch, you're doing this:

+static int
+nto_insert_breakpoint (char type, CORE_ADDR addr)
+{
+  TRACE ("%s\n", __func__);
+  switch (type)
+    {
+    case '0': /* software-breakpoint */
+      return nto_breakpoint (addr, _DEBUG_BREAK_EXEC, 0);
+    case '1': /* hardware-breakpoint */
+      return nto_breakpoint (addr, _DEBUG_BREAK_EXEC | _DEBUG_BREAK_HW, 0);
+    }
+  return 1; /* Not supported.  */
+}

+static int
+nto_insert_watchpoint (char type, CORE_ADDR addr, int len)
+{
+  int wtype = _DEBUG_BREAK_HW; /* Always request HW.  */
+
+  TRACE ("%s type:%c\n", __func__, type);
+  switch (type)
+    {
+    case '2':  /* write watchpoint */
+      wtype |= _DEBUG_BREAK_RW;
+      break;
+    case '3':  /* read watchpoint */
+      wtype |= _DEBUG_BREAK_RD
+      break;
+    case '4':  /* access watchpoint */
+      wtype |= _DEBUG_BREAK_RW;
+      break;
+    default:
+      return 1; /* Not supported.  */
+    }
+  return nto_breakpoint (addr, wtype, 0);
+}

That is, still always deferring to a single function
(nto_breakpoint).

This suggests that the insert_breakpoint and insert_watchpoint
interface distintion is unnecessary.

-- 
Pedro Alves

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

[Aleksandar Ristovski]

Pedro Alves wrote:
> On Friday 19 June 2009 14:55:56, Aleksandar Ristovski wrote:
> 
>> 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 
> 
>                                    ^ spurious space
> 
>> +     Returns 0 if successful, 1 otherwise.  */
>> +
>> +  int (*insert_breakpoint) (char type, CORE_ADDR addr);
>> +  int (*remove_breakpoint) (char type, CORE_ADDR addr);
> 
> 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:

That was my initial implementation, prior to proposing the 
change. Then I looked at target ops in gdb; there we have 
two different functions for breakpoint and watchpoint so I 
followed that logic (even though the logic there seems to be 
incomplete: there is a pair for hw and non-hw breakponts but 
only one pair for watchpoints).

But either way is fine with me - just let me know.

Thanks,

-- 
Aleksandar Ristovski
QNX Software Systems

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

[Pedro Alves]

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:
> 
> That was my initial implementation, prior to proposing the 
> change. Then I looked at target ops in gdb; there we have 
> two different functions for breakpoint and watchpoint so I 
> followed that logic (even though the logic there seems to be 
> incomplete: there is a pair for hw and non-hw breakponts but 
> only one pair for watchpoints).

That's because software watchpoints aren't "inserted".  Instead,
GDB forces the target to single-step all the way (see
bpstat_should_step calls in infrun.c), and evaluates the
watchpoints expressions for changes at each step.

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

-- 
Pedro Alves

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

[Aleksandar Ristovski]

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

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:
>> That was my initial implementation, prior to proposing the 
>> change. Then I looked at target ops in gdb; there we have 
>> two different functions for breakpoint and watchpoint so I 
>> followed that logic (even though the logic there seems to be 
>> incomplete: there is a pair for hw and non-hw breakponts but 
>> only one pair for watchpoints).
> 
> That's because software watchpoints aren't "inserted".

Yes, silly me.

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

I attached what I committed.

ChangeLog:

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



Thanks,

-- 
Aleksandar Ristovski
QNX Software Systems


[-- Attachment #2: gdbserver-Z0Z1support-20090623.diff --]
[-- Type: text/x-patch, Size: 3699 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	23 Jun 2009 15:06:21 -0000
@@ -2371,66 +2371,56 @@ 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';
+
+	/* 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';
-	  }
+	    || the_target->remove_watchpoint == NULL)
+	  res = 1;  /* Not supported.  */
 	else
-	  {
-	    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';
-	    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];
+	  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.  */
+	    }
 
-	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: target.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/target.h,v
retrieving revision 1.37
diff -u -p -r1.37 target.h
--- target.h	19 Jun 2009 13:35:35 -0000	1.37
+++ target.h	23 Jun 2009 15:06:21 -0000
@@ -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);

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

[Pedro Alves]

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, and you bypassed
the "rename" suggestion...

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

> I attached what I committed.
> 
> ChangeLog:
> 
> * 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'.



-- 
Pedro Alves

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

[Aleksandar Ristovski]

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, and you bypassed
> the "rename" suggestion...

This version is the previous one with the changes you 
suggested (minus renaming).

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

Hmm... you mean we should probably guard all? You are 
probably right.

In any case - advise on next step:
-revert
-rename
-rename + guard

Thanks,

Aleksandar

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

[Aleksandar Ristovski]

[-- 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);

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

[Doug Evans]

On Wed, Jun 24, 2009 at 11:50 AM, Aleksandar
Ristovski<aristovski@qnx.com> wrote:
> 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)

fwiw, I think the naming is important.  People come at the source from
various angles, starting points, and contexts.  I can imagine someone
getting tripped up (i.e. spending time doing something that they
otherwise wouldn't have) by reading "insert_watchpoint" and not
knowing it also applied to breakpoints.  Good names are important. (or
at least not obviously problematic names; picking good names is
sometimes hard, for me anyway).

My $0.02.

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

[Aleksandar Ristovski]

Doug Evans wrote:
> On Wed, Jun 24, 2009 at 11:50 AM, Aleksandar
> Ristovski<aristovski@qnx.com> wrote:
>> 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)
> 
> fwiw, I think the naming is important.  People come at the source from
> various angles, starting points, and contexts.  I can imagine someone
> getting tripped up (i.e. spending time doing something that they
> otherwise wouldn't have) by reading "insert_watchpoint" and not
> knowing it also applied to breakpoints.  Good names are important. (or
> at least not obviously problematic names; picking good names is
> sometimes hard, for me anyway).
> 
> My $0.02.
> 

Never ment to say it isn't important, just that 
"insert_point" didn't look more clear to me.  But that's 
just me, of course.

-- 
Aleksandar Ristovski
QNX Software Systems

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

[Doug Evans]

On Wed, Jun 24, 2009 at 12:04 PM, Aleksandar
Ristovski<aristovski@qnx.com> wrote:
> Never ment to say it isn't important, just that "insert_point" didn't look
> more clear to me.  But that's just me, of course.

Righto.

insert_point has the feature that it doesn't suggest solely one or the other.
Maybe insert_bw_point?  [b = break, w = watch]  Dunno.
I don't have too strong an opinion on the replacement (naming is
hard), but I do feel the status quo is going to trip people up.

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

[Pedro Alves]

On Wednesday 24 June 2009 19:50:44, Aleksandar Ristovski wrote:
> 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)


Urgh, I was just about to press the send button when I saw
this message of yours.  This version corrects a few troubles
with the previous commit (see ChangeLog) (one of them I still
see in your new patch) and I've tested it on x86_64-linux.
Aleksandar, please, please, do run the testsuite (and state
that you have) when posting patches.  E.g., we could have caught
the vKill issue that Pierre fixed when we made only linux
report multi-process (the testsuite runs in "target remote"
mode, hence with multi-process off most of the way).

-- 
Pedro Alves

2009-06-24  Pedro Alves  <pedro@codesourcery.com>

	* server.c (process_serial_event): Re-return unsupported, not
	error, if the type isn't recognized.  Re-allow supporting only
	insert or remove packets.  Also call require_running for
	breakpoints.  Add missing break statement to default case.  Tidy.
	* target.h (struct target_ops): Rename insert_watchpoint to
	insert_point, and remove_watchpoint to remove_point.

	* linux-low.h (struct linux_target_ops): Likewise.
	* linux-low.c (linux_insert_watchpoint): Rename to ...
	(linux_insert_point): ... this.  Adjust.
	(linux_remove_watchpoint): Rename to ...
	(linux_remove_point): ... this.  Adjust.
	(linux_target_ops): Adjust.
	* linux-crisv32-low.c (cris_insert_watchpoint): Rename to ...
	(cris_insert_point): ... this.
	(cris_remove_watchpoint): Rename to ...
	(cris_remove_point): ... this.
	(the_low_target): Adjust.

---
 gdb/gdbserver/linux-crisv32-low.c |    8 +++---
 gdb/gdbserver/linux-low.c         |   21 ++++++++-------
 gdb/gdbserver/linux-low.h         |    7 +++--
 gdb/gdbserver/server.c            |   50 ++++++++++++++------------------------
 gdb/gdbserver/target.h            |    6 ++--
 5 files changed, 41 insertions(+), 51 deletions(-)

Index: src/gdb/gdbserver/server.c
===================================================================
--- src.orig/gdb/gdbserver/server.c	2009-06-24 19:48:36.000000000 +0100
+++ src/gdb/gdbserver/server.c	2009-06-24 19:58:34.000000000 +0100
@@ -2383,38 +2383,26 @@ process_serial_event (void)
 	int len = strtol (lenptr + 1, &dataptr, 16);
 	char type = own_buf[1];
 	int res;
-	const int insert_ = ch == 'Z';
+	const int insert = ch == 'Z';
 
-	/* Type: '0' - software-breakpoint
-		 '1' - hardware-breakpoint
-		 '2' - write watchpoint
-		 '3' - read watchpoint
-		 '4' - access watchpoint  */
-
-	if (the_target->insert_watchpoint == NULL
-	    || the_target->remove_watchpoint == NULL)
-	  res = 1;  /* Not supported.  */
-	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.  */
-	    }
+	/* Default to unrecognized/unsupported.  */
+	res = 1;
+	switch (type)
+	  {
+	  case '0': /* software-breakpoint */
+	  case '1': /* hardware-breakpoint */
+	  case '2': /* write watchpoint */
+	  case '3': /* read watchpoint */
+	  case '4': /* access watchpoint */
+	    require_running (own_buf);
+	    if (insert && the_target->insert_point != NULL)
+	      res = (*the_target->insert_point) (type, addr, len);
+	    else if (!insert && the_target->remove_point != NULL)
+	      res = (*the_target->remove_point) (type, addr, len);
+	    break;
+	  default:
+	    break;
+	  }
 
 	if (res == 0)
 	  write_ok (own_buf);
Index: src/gdb/gdbserver/target.h
===================================================================
--- src.orig/gdb/gdbserver/target.h	2009-06-24 19:48:36.000000000 +0100
+++ src/gdb/gdbserver/target.h	2009-06-24 19:48:37.000000000 +0100
@@ -213,7 +213,7 @@ struct target_ops
   int (*read_auxv) (CORE_ADDR offset, unsigned char *myaddr,
 		    unsigned int len);
 
-  /* Insert and remove a hardware watchpoint.
+  /* Insert and remove a break or watchpoint.
      Returns 0 on success, -1 on failure and 1 on unsupported.
      The type is coded as follows:
        '0' - software-breakpoint
@@ -222,8 +222,8 @@ struct target_ops
        '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);
+  int (*insert_point) (char type, CORE_ADDR addr, int len);
+  int (*remove_point) (char type, CORE_ADDR addr, int len);
 
   /* Returns 1 if target was stopped due to a watchpoint hit, 0 otherwise.  */
 
Index: src/gdb/gdbserver/linux-low.h
===================================================================
--- src.orig/gdb/gdbserver/linux-low.h	2009-06-24 19:48:36.000000000 +0100
+++ src/gdb/gdbserver/linux-low.h	2009-06-24 19:48:37.000000000 +0100
@@ -80,9 +80,10 @@ struct linux_target_ops
   int decr_pc_after_break;
   int (*breakpoint_at) (CORE_ADDR pc);
 
-  /* Watchpoint related functions.  See target.h for comments.  */
-  int (*insert_watchpoint) (char type, CORE_ADDR addr, int len);
-  int (*remove_watchpoint) (char type, CORE_ADDR addr, int len);
+  /* Breakpoint and watchpoint related functions.  See target.h for
+     comments.  */
+  int (*insert_point) (char type, CORE_ADDR addr, int len);
+  int (*remove_point) (char type, CORE_ADDR addr, int len);
   int (*stopped_by_watchpoint) (void);
   CORE_ADDR (*stopped_data_address) (void);
 
Index: src/gdb/gdbserver/linux-low.c
===================================================================
--- src.orig/gdb/gdbserver/linux-low.c	2009-06-24 19:48:36.000000000 +0100
+++ src/gdb/gdbserver/linux-low.c	2009-06-24 19:48:37.000000000 +0100
@@ -2671,24 +2671,25 @@ linux_read_auxv (CORE_ADDR offset, unsig
   return n;
 }
 
-/* These watchpoint related wrapper functions simply pass on the function call
-   if the target has registered a corresponding function.  */
+/* These breakpoint and watchpoint related wrapper functions simply
+   pass on the function call if the target has registered a
+   corresponding function.  */
 
 static int
-linux_insert_watchpoint (char type, CORE_ADDR addr, int len)
+linux_insert_point (char type, CORE_ADDR addr, int len)
 {
-  if (the_low_target.insert_watchpoint != NULL)
-    return the_low_target.insert_watchpoint (type, addr, len);
+  if (the_low_target.insert_point != NULL)
+    return the_low_target.insert_point (type, addr, len);
   else
     /* Unsupported (see target.h).  */
     return 1;
 }
 
 static int
-linux_remove_watchpoint (char type, CORE_ADDR addr, int len)
+linux_remove_point (char type, CORE_ADDR addr, int len)
 {
-  if (the_low_target.remove_watchpoint != NULL)
-    return the_low_target.remove_watchpoint (type, addr, len);
+  if (the_low_target.remove_point != NULL)
+    return the_low_target.remove_point (type, addr, len);
   else
     /* Unsupported (see target.h).  */
     return 1;
@@ -3030,8 +3031,8 @@ static struct target_ops linux_target_op
   linux_look_up_symbols,
   linux_request_interrupt,
   linux_read_auxv,
-  linux_insert_watchpoint,
-  linux_remove_watchpoint,
+  linux_insert_point,
+  linux_remove_point,
   linux_stopped_by_watchpoint,
   linux_stopped_data_address,
 #if defined(__UCLIBC__) && defined(HAS_NOMMU)
Index: src/gdb/gdbserver/linux-crisv32-low.c
===================================================================
--- src.orig/gdb/gdbserver/linux-crisv32-low.c	2009-06-24 19:48:35.000000000 +0100
+++ src/gdb/gdbserver/linux-crisv32-low.c	2009-06-24 19:48:37.000000000 +0100
@@ -137,7 +137,7 @@ cris_write_data_breakpoint (int bp, unsi
 }
 
 static int
-cris_insert_watchpoint (char type, CORE_ADDR addr, int len)
+cris_insert_point (char type, CORE_ADDR addr, int len)
 {
   int bp;
   unsigned long bp_ctrl;
@@ -220,7 +220,7 @@ cris_insert_watchpoint (char type, CORE_
 }
 
 static int
-cris_remove_watchpoint (char type, CORE_ADDR addr, int len)
+cris_remove_point (char type, CORE_ADDR addr, int len)
 {
   int bp;
   unsigned long bp_ctrl;
@@ -375,8 +375,8 @@ struct linux_target_ops the_low_target =
   cris_reinsert_addr,
   0,
   cris_breakpoint_at,
-  cris_insert_watchpoint,
-  cris_remove_watchpoint,
+  cris_insert_point,
+  cris_remove_point,
   cris_stopped_by_watchpoint,
   cris_stopped_data_address,
 };

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

[Aleksandar Ristovski]

Pedro Alves wrote:
> On Wednesday 24 June 2009 19:50:44, Aleksandar Ristovski wrote:
>> 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)
> 
> 
> Urgh, I was just about to press the send button when I saw
> this message of yours.  This version corrects a few troubles
> with the previous commit (see ChangeLog) (one of them I still
> see in your new patch) and I've tested it on x86_64-linux.
> Aleksandar, please, please, do run the testsuite (and state
> that you have) when posting patches.  E.g., we could have caught
> the vKill issue that Pierre fixed when we made only linux
> report multi-process (the testsuite runs in "target remote"
> mode, hence with multi-process off most of the way).
> 


Your patch: Wasn't it more desireable to distinguish between 
(1) -unsupported and
(-1)-error?

i.e. in "default", shouldn't it be "res=-1" so that it makes 
it clear that gdb passed invalid argument?


Testsuite: I do run it but I admit sometimes not due to 
issues my system is giving me lately; I have to run tests 
"manually" i.e. one by one (I'll have to take some time to 
figure out why is the test run, after some time, just 
starting to ERROR, unable to properly spawn new gdb instance).



-- 
Aleksandar Ristovski
QNX Software Systems

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

[Pedro Alves]

On Wednesday 24 June 2009 20:19:55, Aleksandar Ristovski wrote:
> Your patch: Wasn't it more desireable to distinguish between 
> (1) -unsupported and
> (-1)-error?
> 
> i.e. in "default", shouldn't it be "res=-1" so that it makes 
> it clear that gdb passed invalid argument?

Z0, Z1, Z2, etc. are different packets, the "type" is not
an argument to a Z packet.  If we ever came up with a
Z8 packet, a gdbserver not recognizing it should reply
"unsupported", not error.

> 
> Testsuite: I do run it but I admit sometimes not due to 
> issues my system is giving me lately; I have to run tests 
> "manually" i.e. one by one (I'll have to take some time to 
> figure out why is the test run, after some time, just 
> starting to ERROR, unable to properly spawn new gdb instance).

Okay, but there's always test with local gdbserver on linux or Windows
otherwise.  It's really easy:

 http://sourceware.org/gdb/wiki/Native_gdbserver_testing

-- 
Pedro Alves

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

[Aleksandar Ristovski]

Pedro Alves wrote:
> On Wednesday 24 June 2009 19:50:44, Aleksandar Ristovski wrote:
>> 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)
> 
> 
> Urgh, I was just about to press the send button when I saw
> this message of yours.  This version corrects a few troubles
> with the previous commit (see ChangeLog) (one of them I still
> see in your new patch) and I've tested it on x86_64-linux.
> Aleksandar, please, please, do run the testsuite (and state
> that you have) when posting patches.  E.g., we could have caught
> the vKill issue that Pierre fixed when we made only linux
> report multi-process (the testsuite runs in "target remote"
> mode, hence with multi-process off most of the way).
> 

And just another small note:

+	    require_running (own_buf);
+	    if (insert && the_target->insert_point != NULL)
+	      res = (*the_target->insert_point) (type, addr, len);
+	    else if (!insert && the_target->remove_point != NULL)
+	      res = (*the_target->remove_point) (type, addr, len);
+	    break;

They should either both be present or none. In the gdb 
document, there is implementation note that reads:

Implementation notes: A remote target shall return an empty 
string for an un-recognized breakpoint or watchpoint packet 
type. A remote target shall support either both or neither 
of a given `Ztype...' and `ztype...' packet pair. To
avoid potential problems with duplicate packets, the 
operations should be imple-mented in an idempotent way.

So, I would make it something like I proposed (if either is 
NULL, it's unsupported - also makes a clear statement to new 
target implementors).


Thanks
-- 
Aleksandar Ristovski
QNX Software Systems

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

[Pedro Alves]

On Wednesday 24 June 2009 20:25:35, Aleksandar Ristovski wrote:
> So, I would make it something like I proposed (if either is 
> NULL, it's unsupported - also makes a clear statement to new 
> target implementors).

IMO, hardly a real problem.  Even if one implements both callbacks
one needs to be sure to handle e.g., one of z2 and Z2 on each
of them.  It just looks unnecessary cautiousness to me.  We've not
cared for that for a long while in server.c, and don't see much of
a point we have to now --- it's not like someone adding support
for inserting HW breakpoint wouldn't notice that she missed adding
support for removing it.  OTOH, it makes the code a bit more tidy
to not care about it --- it's just mostly about avoiding a NULL
deference as is.

I've checked the patch in.  If we need to split this up, it's
really a trivial change, so let's worry about it then, and move
on.

As for naming, thinking about the GDB side, GDB is a bit stuck to
calling everything "breakpoints" --- breakpoints, watchpoints, catchpoints,
and now tracepoints are all subsets of "breakpoints".  The HPD spec,
Frysk and TotalView (well both of these are HPD influenced) at least,
call all these "actionpoints".  Personally, I wish we'd call
them actionpoints too, and have "info actionpoints" show all of
those, "info breakpoints" show only breakpoints, "info watchpoints"
show only watchpoints, etc.  But I'm sure it is not a practical
change at this point ( is it ?? :-) )

-- 
Pedro Alves