[RFC] checking the Z-packet suppport on gdbserver

[RFC] checking the Z-packet suppport on gdbserver

[Emi SUZUKI]

[-- Attachment #1: Type: Text/Plain, Size: 4687 bytes --]

Hello members,

Now I've noticed that GDB always tries to use hardware watchpoints
while remote debugging, even the target doesn't have the hardware
watchpoint support.  

-----
$ ./gdb hello_target
GNU gdb 6.6.50.20070904-cvs
Copyright (C) 2007 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "--host=i686-pc-linux-gnu --target=powerpc64-linux"...
(gdb) set solib-absolute-prefix /target/sysroot
(gdb) target remote remote_server:2015
Remote debugging using remote_server:2015
0x0ffd6bf4 in _start () from /target/sysroot/lib/ld.so.1
(gdb) set debug remote 1
(gdb) watch glob
Sending packet: $m10010b30,4#84...Ack
Packet received: 00000003
Hardware watchpoint 1: glob
(gdb) c
Continuing.
Sending packet: $Z0,ffcf654,4#4a...Ack
Packet received:
Packet Z0 (software-breakpoint) is NOT supported
Sending packet: $mffcf654,4#01...Ack
Packet received: 893a0000
Sending packet: $Xffcf654,0:#22...Ack
Packet received: OK
binary downloading suppported by target
Sending packet: $Xffcf654,4:}]\202\020\b#9a...Ack
Packet received: OK
Sending packet: $m10010b30,4#84...Ack
Packet received: 00000003
Sending packet: $Z2,10010b30,4#cf...Ack
Packet received:
Packet Z2 (write-watchpoint) is NOT supported
warning: Could not remove hardware watchpoint 1.
Warning:
Could not insert hardware watchpoint 1.
Could not insert hardware breakpoints:
You may have requested too many hardware breakpoints/watchpoints.

(gdb)
------

GDB decides which type of watchpoints should be set when giving the
command like "watch foo".  And hardware watchpoints can be used when
gdbserver has the Z-packet support.  However, as the session log above
has shown, GDB does not check the Z-packet support on gdbserver when
deciding the type of the watchpoint but when actually setting the
watchpoint to the target.  

It has a simple workaround for this problem: delete the watchpoint and
setting "remote hardware-watchpoint-limit" to 0, and set it again.
But I thought that the timing when checking the Z-packet support
should be fxied and made a fix for this as attached.  
The concept is:

- gdbserver: Allows "Z#" packets (not "Z#,addr,length").  
             Responds "OK" if hardware breakpoints/watchpoints are
             supported or responds "".  

- gdb:       Sends a "Z#" packet for checking if hardware
             breakpoints/watchpoints can be handled on the target.  


And two things which I feel indecisive for this fix are there:

- I defined a new function pointer "watchpoint_support" in target_ops
  for gdbserver, because I thought we cannot decide the common address
  value for all the architectures supported by gdbserver at which we
  should not set a hardware watchpoint.  If we can, we can use the
  existing format of Z-packet for this purpose.  

- The newly defined packet format might cause problems like exceptions
  or setting a hardware watchpoint at the unexpected location when
  working with older versions of gdbserver, because in which the
  Z-packet format is not checked at all.  The codes in CVS head are
  below:


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

            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;
      }


Would anyone give me any comments how it should be treated as a whole?
Defines another packet for it?  Applies as proposed and notes "it
might not work with older versions of gdbserver" ?


By the way, now I have *finally* got the copyright assignment for FSF.
But the problem I have addressed for the first time here has already
fixed by Luis Machado and many active members in the list.  I would
like to thank all of them, and apologize for not giving enough
comments for it.  


Best regards,
-- 
Emi SUZUKI / emi-suzuki at tjsys.co.jp

[-- Attachment #2: zpacket-support.diff --]
[-- Type: Text/Plain, Size: 8937 bytes --]

Index: gdb/gdbserver/server.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/server.c,v
retrieving revision 1.56
diff -u -r1.56 server.c
--- gdb/gdbserver/server.c	23 Aug 2007 18:08:48 -0000	1.56
+++ gdb/gdbserver/server.c	13 Sep 2007 10:51:07 -0000
@@ -1097,21 +1097,30 @@
 	      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->insert_watchpoint == NULL
 		    || (type < '2' || type > '4'))
 		  {
 		    /* No watchpoint support or not a watchpoint command;
 		       unrecognized either way.  */
 		    own_buf[0] = '\0';
+		    break;
+		  }
+
+		if (own_buf[2] == '\0')
+		  {
+		    if ((*the_target->watchpoint_support) (type))
+		      write_ok (own_buf);
+		    else
+		      /* Unsupported.  */
+		      own_buf[0] = '\0';
 		  }
 		else
 		  {
+		    char *lenptr;
+		    char *dataptr;
+		    CORE_ADDR addr = strtoul (&own_buf[3], &lenptr, 16);
+		    int len = strtol (lenptr + 1, &dataptr, 16);
 		    int res;
 
 		    res = (*the_target->insert_watchpoint) (type, addr, len);
Index: gdb/gdbserver/linux-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-low.c,v
retrieving revision 1.61
diff -u -r1.61 linux-low.c
--- gdb/gdbserver/linux-low.c	4 Sep 2007 21:30:23 -0000	1.61
+++ gdb/gdbserver/linux-low.c	13 Sep 2007 10:50:59 -0000
@@ -1635,6 +1635,16 @@
 }
 
 static int
+linux_watchpoint_support (char type)
+{
+  if (the_low_target.watchpoint_support != NULL)
+    return the_low_target.watchpoint_support (type);
+  else
+    /* Unsupported.  */
+    return 0;
+}
+
+static int
 linux_stopped_by_watchpoint (void)
 {
   if (the_low_target.stopped_by_watchpoint != NULL)
@@ -1721,6 +1731,7 @@
   linux_read_auxv,
   linux_insert_watchpoint,
   linux_remove_watchpoint,
+  linux_watchpoint_support,
   linux_stopped_by_watchpoint,
   linux_stopped_data_address,
 #if defined(__UCLIBC__) && defined(HAS_NOMMU)
Index: gdb/gdbserver/linux-low.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-low.h,v
retrieving revision 1.18
diff -u -r1.18 linux-low.h
--- gdb/gdbserver/linux-low.h	23 Aug 2007 18:08:48 -0000	1.18
+++ gdb/gdbserver/linux-low.h	13 Sep 2007 10:50:59 -0000
@@ -63,6 +63,7 @@
   /* 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);
+  int (*watchpoint_support) (char type);
   int (*stopped_by_watchpoint) (void);
   CORE_ADDR (*stopped_data_address) (void);
 
Index: gdb/gdbserver/target.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/target.h,v
retrieving revision 1.25
diff -u -r1.25 target.h
--- gdb/gdbserver/target.h	23 Aug 2007 18:08:48 -0000	1.25
+++ gdb/gdbserver/target.h	13 Sep 2007 10:51:11 -0000
@@ -155,6 +155,11 @@
 
   int (*insert_watchpoint) (char type, CORE_ADDR addr, int len);
   int (*remove_watchpoint) (char type, CORE_ADDR addr, int len);
+  
+  /* Check if hardware watchpoints are supported.  
+     Returns 0 on unsupported, else on supported.  
+     The type is coded as mentioned above.  */
+  int (*watchpoint_support) (char type);
 
   /* Returns 1 if target was stopped due to a watchpoint hit, 0 otherwise.  */
 
Index: gdb/gdbserver/win32-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/win32-low.c,v
retrieving revision 1.15
diff -u -r1.15 win32-low.c
--- gdb/gdbserver/win32-low.c	3 Sep 2007 22:17:27 -0000	1.15
+++ gdb/gdbserver/win32-low.c	13 Sep 2007 10:51:16 -0000
@@ -1522,6 +1522,7 @@
   NULL,
   NULL,
   NULL,
+  NULL,
   win32_arch_string
 };
 
Index: gdb/gdbserver/linux-x86-64-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-x86-64-low.c,v
retrieving revision 1.16
diff -u -r1.16 linux-x86-64-low.c
--- gdb/gdbserver/linux-x86-64-low.c	23 Aug 2007 18:08:48 -0000	1.16
+++ gdb/gdbserver/linux-x86-64-low.c	13 Sep 2007 10:51:00 -0000
@@ -174,6 +174,7 @@
   NULL,
   NULL,
   NULL,
+  NULL,
   0,
   "i386:x86-64",
 };
Index: gdb/gdbserver/linux-crisv32-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-crisv32-low.c,v
retrieving revision 1.5
diff -u -r1.5 linux-crisv32-low.c
--- gdb/gdbserver/linux-crisv32-low.c	23 Aug 2007 18:08:48 -0000	1.5
+++ gdb/gdbserver/linux-crisv32-low.c	13 Sep 2007 10:50:51 -0000
@@ -307,6 +307,21 @@
 }
 
 static int
+cris_watchpoint_support (char type)
+{
+  /* Breakpoint/watchpoint types is described in the comment for 
+     cris_insert_watchpoint.  */
+  
+  if (type < '2' || type > '4') 
+    {
+      /* Unsupported.  */
+      return 0;
+    }
+  /* Supported.  */
+  return 1;
+}
+
+static int
 cris_stopped_by_watchpoint (void)
 {
   unsigned long exs;
@@ -373,6 +388,7 @@
   cris_breakpoint_at,
   cris_insert_watchpoint,
   cris_remove_watchpoint,
+  cris_watchpoint_support,
   cris_stopped_by_watchpoint,
   cris_stopped_data_address,
 };
Index: gdb/gdbserver/linux-i386-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-i386-low.c,v
retrieving revision 1.13
diff -u -r1.13 linux-i386-low.c
--- gdb/gdbserver/linux-i386-low.c	23 Aug 2007 18:08:48 -0000	1.13
+++ gdb/gdbserver/linux-i386-low.c	13 Sep 2007 10:50:51 -0000
@@ -200,6 +200,7 @@
   NULL,
   NULL,
   NULL,
+  NULL,
   0,
   "i386"
 };
Index: gdb/gdbserver/linux-ppc64-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-ppc64-low.c,v
retrieving revision 1.8
diff -u -r1.8 linux-ppc64-low.c
--- gdb/gdbserver/linux-ppc64-low.c	23 Aug 2007 18:08:48 -0000	1.8
+++ gdb/gdbserver/linux-ppc64-low.c	13 Sep 2007 10:51:00 -0000
@@ -130,5 +130,6 @@
   NULL,
   NULL,
   NULL,
+  NULL,
   1
 };
Index: gdb/gdbserver/spu-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/spu-low.c,v
retrieving revision 1.12
diff -u -r1.12 spu-low.c
--- gdb/gdbserver/spu-low.c	23 Aug 2007 18:08:48 -0000	1.12
+++ gdb/gdbserver/spu-low.c	13 Sep 2007 10:51:09 -0000
@@ -596,6 +596,7 @@
   NULL,
   NULL,
   NULL,
+  NULL,
   spu_arch_string,
   spu_proc_xfer_spu,
 };
Index: gdb/gdbserver/linux-cris-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-cris-low.c,v
retrieving revision 1.5
diff -u -r1.5 linux-cris-low.c
--- gdb/gdbserver/linux-cris-low.c	23 Aug 2007 18:08:48 -0000	1.5
+++ gdb/gdbserver/linux-cris-low.c	13 Sep 2007 10:50:49 -0000
@@ -116,8 +116,4 @@
   cris_reinsert_addr,
   0,
   cris_breakpoint_at,
-  0,
-  0,
-  0,
-  0,
 };
Index: gdb/remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.266
diff -u -r1.266 remote.c
--- gdb/remote.c	23 Aug 2007 18:08:36 -0000	1.266
+++ gdb/remote.c	13 Sep 2007 10:50:49 -0000
@@ -5374,6 +5374,53 @@
 		  _("remote_remove_watchpoint: reached end of function"));
 }
 
+static int
+hardware_resouce_to_Z_packet (int type)
+{
+  switch (type)
+    {
+    case bp_hardware_breakpoint:
+      return Z_PACKET_HARDWARE_BP;
+      break;
+    case bp_hardware_watchpoint:
+      return Z_PACKET_WRITE_WP;
+      break;
+    case bp_read_watchpoint:
+      return Z_PACKET_READ_WP;
+      break;
+    case bp_access_watchpoint:
+      return Z_PACKET_ACCESS_WP;
+      break;
+    default:
+      internal_error (__FILE__, __LINE__,
+		      _("hardware_resouce_to_Z_packet: bad watchpoint type %d"), type);
+    }
+}
+
+static int
+remote_check_Zpacket_support (int type)
+{
+  struct remote_state *rs = get_remote_state ();
+  enum Z_packet_type packet = hardware_resouce_to_Z_packet (type);
+
+  if (remote_protocol_packets[PACKET_Z0 + packet].support == PACKET_DISABLE)
+    return 0;
+
+  sprintf (rs->buf, "Z%x", packet);
+  putpkt (rs->buf);
+  getpkt (&rs->buf, &rs->buf_size, 0);
+
+  switch (packet_ok (rs->buf, &remote_protocol_packets[PACKET_Z0 + packet]))
+    {
+    case PACKET_ERROR:
+    case PACKET_UNKNOWN:
+      return 0;
+    case PACKET_OK:
+      return 1;
+    }
+  internal_error (__FILE__, __LINE__,
+		  _("remote_check_Zpacket_supported: reached end of function"));
+}
 
 int remote_hw_watchpoint_limit = -1;
 int remote_hw_breakpoint_limit = -1;
@@ -5381,6 +5428,9 @@
 static int
 remote_check_watch_resources (int type, int cnt, int ot)
 {
+  if (! remote_check_Zpacket_support (type))
+    return 0;
+
   if (type == bp_hardware_breakpoint)
     {
       if (remote_hw_breakpoint_limit == 0)

Re: [RFC] checking the Z-packet suppport on gdbserver

[Jim Blandy]

Emi SUZUKI <emi-suzuki@tjsys.co.jp> writes:
> GDB decides which type of watchpoints should be set when giving the
> command like "watch foo".  And hardware watchpoints can be used when
> gdbserver has the Z-packet support.  However, as the session log above
> has shown, GDB does not check the Z-packet support on gdbserver when
> deciding the type of the watchpoint but when actually setting the
> watchpoint to the target.  

Yeah, this is pretty losing behavior on GDB's part.

> Would anyone give me any comments how it should be treated as a whole?
> Defines another packet for it?  Applies as proposed and notes "it
> might not work with older versions of gdbserver" ?

I wonder, would it make sense to have GDB assume that hardware
watchpoints are *not* available on remote targets, and then have
gdbserver send a 'qSupported' packet stubfeature that tells GDB that
hardware watchpoints are okay?

Re: [RFC] checking the Z-packet support on gdbserver

[Emi SUZUKI]

[-- Attachment #1: Type: Text/Plain, Size: 1650 bytes --]

Hello Jim,

Thank you for your comment.  

From: Jim Blandy <jimb at codesourcery.com>
Subject: Re: [RFC] checking the Z-packet support on gdbserver
Date: Thu, 13 Sep 2007 10:05:50 -0700

> Emi SUZUKI <emi-suzuki@tjsys.co.jp> writes:
> > Would anyone give me any comments how it should be treated as a whole?
> > Defines another packet for it?  Applies as proposed and notes "it
> > might not work with older versions of gdbserver" ?
> 
> I wonder, would it make sense to have GDB assume that hardware
> watchpoints are *not* available on remote targets, and then have
> gdbserver send a 'qSupported' packet stubfeature that tells GDB that
> hardware watchpoints are okay?

Well, I understood casually that the answer of 'qSupported' packet
would tell the support for the other 'q' packets from the current
implementaion...  According to the description in the info, definitely
it would make sense.  How about the attached?


Meanwhile, I've found the definition below in gdb/config/nm-i386.h.  

  #define TARGET_CAN_USE_HARDWARE_WATCHPOINT(type, cnt, ot) 1

TARGET_CAN_USE_HARDWARE_WATCHPOINT is used in watch_command_1 for
checking if the support of hardware watchpoints are available on the
target.  If it is not defined, the target vector function
'to_can_use_hw_breakpoint' would be called.  So, the above implies
that GDB built for the x86 native target is not supposed to debug the
remote target.  But it should be, to debug a target running on a
remote machine which is the same architecture to the local host,
shouldn't it?
(Although it would not be much necessary in practice...)

Best regards,
-- 
Emi SUZUKI / emi-suzuki at tjsys.co.jp


[-- Attachment #2: zpacket-redefined.diff --]
[-- Type: Text/Plain, Size: 8675 bytes --]

Index: gdb/gdbserver/server.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/server.c,v
retrieving revision 1.56
diff -u -r1.56 server.c
--- gdb/gdbserver/server.c	23 Aug 2007 18:08:48 -0000	1.56
+++ gdb/gdbserver/server.c	14 Sep 2007 09:30:25 -0000
@@ -540,8 +540,21 @@
 	strcat (own_buf, ";qXfer:spu:read+;qXfer:spu:write+");
 
       if (get_features_xml ("target.xml") != NULL)
-	strcat (own_buf, ";qXfer:features:read+");
+ 	strcat (own_buf, ";qXfer:features:read+");
 
+      char type;
+      char buf[8];
+      /* Watchpoints support: the watchpoint type codes are 
+	 described in target.h. */
+      for (type = 2; type < 5; type++)
+	{
+	  if (the_target->watchpoint_support != NULL
+	      && the_target->watchpoint_support (type))
+	    {
+	      sprintf (buf, ";Z%d+", type);
+	      strcat (own_buf, buf);
+	    }
+	}
       return;
     }
 
Index: gdb/gdbserver/linux-low.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-low.h,v
retrieving revision 1.18
diff -u -r1.18 linux-low.h
--- gdb/gdbserver/linux-low.h	23 Aug 2007 18:08:48 -0000	1.18
+++ gdb/gdbserver/linux-low.h	14 Sep 2007 09:30:19 -0000
@@ -63,6 +63,7 @@
   /* 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);
+  int (*watchpoint_support) (char type);
   int (*stopped_by_watchpoint) (void);
   CORE_ADDR (*stopped_data_address) (void);
 
Index: gdb/gdbserver/linux-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-low.c,v
retrieving revision 1.61
diff -u -r1.61 linux-low.c
--- gdb/gdbserver/linux-low.c	4 Sep 2007 21:30:23 -0000	1.61
+++ gdb/gdbserver/linux-low.c	14 Sep 2007 09:30:19 -0000
@@ -1635,6 +1635,16 @@
 }
 
 static int
+linux_watchpoint_support (char type)
+{
+  if (the_low_target.watchpoint_support != NULL)
+    return the_low_target.watchpoint_support (type);
+  else
+    /* Unsupported.  */
+    return 0;
+}
+
+static int
 linux_stopped_by_watchpoint (void)
 {
   if (the_low_target.stopped_by_watchpoint != NULL)
@@ -1721,6 +1731,7 @@
   linux_read_auxv,
   linux_insert_watchpoint,
   linux_remove_watchpoint,
+  linux_watchpoint_support,
   linux_stopped_by_watchpoint,
   linux_stopped_data_address,
 #if defined(__UCLIBC__) && defined(HAS_NOMMU)
Index: gdb/gdbserver/target.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/target.h,v
retrieving revision 1.25
diff -u -r1.25 target.h
--- gdb/gdbserver/target.h	23 Aug 2007 18:08:48 -0000	1.25
+++ gdb/gdbserver/target.h	14 Sep 2007 09:30:29 -0000
@@ -155,6 +155,11 @@
 
   int (*insert_watchpoint) (char type, CORE_ADDR addr, int len);
   int (*remove_watchpoint) (char type, CORE_ADDR addr, int len);
+  
+  /* Check if hardware watchpoints are supported.  
+     Returns 0 on unsupported, else on supported.  
+     The type is coded as mentioned above.  */
+  int (*watchpoint_support) (char type);
 
   /* Returns 1 if target was stopped due to a watchpoint hit, 0 otherwise.  */
 
Index: gdb/gdbserver/linux-x86-64-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-x86-64-low.c,v
retrieving revision 1.16
diff -u -r1.16 linux-x86-64-low.c
--- gdb/gdbserver/linux-x86-64-low.c	23 Aug 2007 18:08:48 -0000	1.16
+++ gdb/gdbserver/linux-x86-64-low.c	14 Sep 2007 09:30:21 -0000
@@ -174,6 +174,7 @@
   NULL,
   NULL,
   NULL,
+  NULL,
   0,
   "i386:x86-64",
 };
Index: gdb/gdbserver/win32-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/win32-low.c,v
retrieving revision 1.15
diff -u -r1.15 win32-low.c
--- gdb/gdbserver/win32-low.c	3 Sep 2007 22:17:27 -0000	1.15
+++ gdb/gdbserver/win32-low.c	14 Sep 2007 09:30:34 -0000
@@ -1522,6 +1522,7 @@
   NULL,
   NULL,
   NULL,
+  NULL,
   win32_arch_string
 };
 
Index: gdb/gdbserver/linux-i386-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-i386-low.c,v
retrieving revision 1.13
diff -u -r1.13 linux-i386-low.c
--- gdb/gdbserver/linux-i386-low.c	23 Aug 2007 18:08:48 -0000	1.13
+++ gdb/gdbserver/linux-i386-low.c	14 Sep 2007 09:30:10 -0000
@@ -200,6 +200,7 @@
   NULL,
   NULL,
   NULL,
+  NULL,
   0,
   "i386"
 };
Index: gdb/gdbserver/linux-crisv32-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-crisv32-low.c,v
retrieving revision 1.5
diff -u -r1.5 linux-crisv32-low.c
--- gdb/gdbserver/linux-crisv32-low.c	23 Aug 2007 18:08:48 -0000	1.5
+++ gdb/gdbserver/linux-crisv32-low.c	14 Sep 2007 09:30:10 -0000
@@ -307,6 +307,21 @@
 }
 
 static int
+cris_watchpoint_support (char type)
+{
+  /* Breakpoint/watchpoint types is described in the comment for 
+     cris_insert_watchpoint.  */
+  
+  if (type < '2' || type > '4') 
+    {
+      /* Unsupported.  */
+      return 0;
+    }
+  /* Supported.  */
+  return 1;
+}
+
+static int
 cris_stopped_by_watchpoint (void)
 {
   unsigned long exs;
@@ -373,6 +388,7 @@
   cris_breakpoint_at,
   cris_insert_watchpoint,
   cris_remove_watchpoint,
+  cris_watchpoint_support,
   cris_stopped_by_watchpoint,
   cris_stopped_data_address,
 };
Index: gdb/gdbserver/linux-ppc64-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-ppc64-low.c,v
retrieving revision 1.8
diff -u -r1.8 linux-ppc64-low.c
--- gdb/gdbserver/linux-ppc64-low.c	23 Aug 2007 18:08:48 -0000	1.8
+++ gdb/gdbserver/linux-ppc64-low.c	14 Sep 2007 09:30:21 -0000
@@ -130,5 +130,6 @@
   NULL,
   NULL,
   NULL,
+  NULL,
   1
 };
Index: gdb/gdbserver/spu-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/spu-low.c,v
retrieving revision 1.12
diff -u -r1.12 spu-low.c
--- gdb/gdbserver/spu-low.c	23 Aug 2007 18:08:48 -0000	1.12
+++ gdb/gdbserver/spu-low.c	14 Sep 2007 09:30:27 -0000
@@ -596,6 +596,7 @@
   NULL,
   NULL,
   NULL,
+  NULL,
   spu_arch_string,
   spu_proc_xfer_spu,
 };
Index: gdb/gdbserver/linux-cris-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-cris-low.c,v
retrieving revision 1.5
diff -u -r1.5 linux-cris-low.c
--- gdb/gdbserver/linux-cris-low.c	23 Aug 2007 18:08:48 -0000	1.5
+++ gdb/gdbserver/linux-cris-low.c	14 Sep 2007 09:30:07 -0000
@@ -116,8 +116,4 @@
   cris_reinsert_addr,
   0,
   cris_breakpoint_at,
-  0,
-  0,
-  0,
-  0,
 };
Index: gdb/remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.266
diff -u -r1.266 remote.c
--- gdb/remote.c	23 Aug 2007 18:08:36 -0000	1.266
+++ gdb/remote.c	14 Sep 2007 09:30:06 -0000
@@ -2395,6 +2395,11 @@
     PACKET_qXfer_spu_write },
   { "QPassSignals", PACKET_DISABLE, remote_supported_packet,
     PACKET_QPassSignals },
+  { "Z0", PACKET_DISABLE, remote_supported_packet, PACKET_Z0 },
+  { "Z1", PACKET_DISABLE, remote_supported_packet, PACKET_Z1 },
+  { "Z2", PACKET_DISABLE, remote_supported_packet, PACKET_Z2 },
+  { "Z3", PACKET_DISABLE, remote_supported_packet, PACKET_Z3 },
+  { "Z4", PACKET_DISABLE, remote_supported_packet, PACKET_Z4 },
 };
 
 static void
@@ -5374,6 +5379,39 @@
 		  _("remote_remove_watchpoint: reached end of function"));
 }
 
+static int
+hardware_resouce_to_Z_packet (int type)
+{
+  switch (type)
+    {
+    case bp_hardware_breakpoint:
+      return Z_PACKET_HARDWARE_BP;
+      break;
+    case bp_hardware_watchpoint:
+      return Z_PACKET_WRITE_WP;
+      break;
+    case bp_read_watchpoint:
+      return Z_PACKET_READ_WP;
+      break;
+    case bp_access_watchpoint:
+      return Z_PACKET_ACCESS_WP;
+      break;
+    default:
+      internal_error (__FILE__, __LINE__,
+		      _("hardware_resouce_to_Z_packet: bad watchpoint type %d"), type);
+    }
+}
+
+static int
+remote_check_Zpacket_support (int type)
+{
+  enum Z_packet_type packet = hardware_resouce_to_Z_packet (type);
+
+  if (remote_protocol_packets[PACKET_Z0 + packet].support == PACKET_DISABLE)
+    return 0;
+
+  return 1;
+}
 
 int remote_hw_watchpoint_limit = -1;
 int remote_hw_breakpoint_limit = -1;
@@ -5381,6 +5419,9 @@
 static int
 remote_check_watch_resources (int type, int cnt, int ot)
 {
+  if (! remote_check_Zpacket_support (type))
+    return 0;
+
   if (type == bp_hardware_breakpoint)
     {
       if (remote_hw_breakpoint_limit == 0)

Re: [RFC] checking the Z-packet suppport on gdbserver

[Daniel Jacobowitz]

On Thu, Sep 13, 2007 at 09:08:22PM +0900, Emi SUZUKI wrote:
> GDB decides which type of watchpoints should be set when giving the
> command like "watch foo".  And hardware watchpoints can be used when
> gdbserver has the Z-packet support.  However, as the session log above
> has shown, GDB does not check the Z-packet support on gdbserver when
> deciding the type of the watchpoint but when actually setting the
> watchpoint to the target.  

This is an interesting problem.  There are a couple of parts to it.

Watchpoints are stored in a table, along with breakpoints.  The table
entries last the entire GDB session, unless they are deleted by the
user.  In that time, the user can disconnect from the remote target
and connect to a different one, or to a native process.  One of these
might support hardware watchpoints; another might not.  That's one
reason we don't check when the watchpoint is created.  But it isn't
a very good reason; you can't set hardware watchpoints before
"run" either, and no one complains about that.  We could solve
this by setting just a "watchpoint", and deciding whether it was a
"software watchpoint" or "hardware watchpoint" later on.  Or
we could just not worry about it.  Not worrying about it is
probably OK.

So on to the more relevant part.  Native targets define several
methods for dealing with watchpoints:

    int (*to_can_use_hw_breakpoint) (int, int, int);
    int (*to_remove_watchpoint) (CORE_ADDR, int, int);
    int (*to_insert_watchpoint) (CORE_ADDR, int, int);
    int (*to_stopped_by_watchpoint) (void);
    int to_have_steppable_watchpoint;
    int to_have_continuable_watchpoint;
    int (*to_stopped_data_address) (struct target_ops *, CORE_ADDR *);
    int (*to_region_ok_for_hw_watchpoint) (CORE_ADDR, int);

And, inconsistently, the gdbarch method
gdbarch_have_nonsteppable_watchpoint.

The steppable / nonsteppable / continable distinction describes what
happens after a watchpoint is hit; whether it can be single stepped
while inserted, must be removed and stepped past, or is already past.
This varies by architecture but also by target, e.g. a simulator might
implement nonsteppable watchpoints for i386 (which normally has
continuable watchpoints).

to_region_ok_for_hw_watchpoint decides whether a region is too big
or misaligned to be watched.  to_can_use_hw_breakpoint decides whether
too many hardware watchpoints have been requested.  The remote target
can't implement either of these.  Figuring out the answer to either
is a bit complicated.  Every architecture's watchpoints work a
bit differently.

Before I think about how to do it, the question is: should we bother?
Or is the any watchpoints / no watchpoints distinction good enough?
We can use qSupported to convey any information we like from the
target to GDB but once we get the data we have to figure out what
to do with it.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] checking the Z-packet support on gdbserver

[Daniel Jacobowitz]

On Fri, Sep 14, 2007 at 06:39:13PM +0900, Emi SUZUKI wrote:
> Meanwhile, I've found the definition below in gdb/config/nm-i386.h.  
> 
>   #define TARGET_CAN_USE_HARDWARE_WATCHPOINT(type, cnt, ot) 1
> 
> TARGET_CAN_USE_HARDWARE_WATCHPOINT is used in watch_command_1 for
> checking if the support of hardware watchpoints are available on the
> target.  If it is not defined, the target vector function
> 'to_can_use_hw_breakpoint' would be called.  So, the above implies
> that GDB built for the x86 native target is not supposed to debug the
> remote target.  But it should be, to debug a target running on a
> remote machine which is the same architecture to the local host,
> shouldn't it?
> (Although it would not be much necessary in practice...)

Yes, this is a bug.  It's a little tricky to fix because there's a
variety of different i386 targets sharing that header file, but it's
certainly fixable.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] checking the Z-packet support on gdbserver

[Jim Blandy]

Emi SUZUKI <emi-suzuki@tjsys.co.jp> writes:
> From: Jim Blandy <jimb at codesourcery.com>
> Subject: Re: [RFC] checking the Z-packet support on gdbserver
> Date: Thu, 13 Sep 2007 10:05:50 -0700
>
>> Emi SUZUKI <emi-suzuki@tjsys.co.jp> writes:
>> > Would anyone give me any comments how it should be treated as a whole?
>> > Defines another packet for it?  Applies as proposed and notes "it
>> > might not work with older versions of gdbserver" ?
>> 
>> I wonder, would it make sense to have GDB assume that hardware
>> watchpoints are *not* available on remote targets, and then have
>> gdbserver send a 'qSupported' packet stubfeature that tells GDB that
>> hardware watchpoints are okay?
>
> Well, I understood casually that the answer of 'qSupported' packet
> would tell the support for the other 'q' packets from the current
> implementaion...  According to the description in the info, definitely
> it would make sense.  How about the attached?

This looks good to me.  Dan should probably approve the gdbserver
side, but that looks appropriately done to me, too.

In remote.c, I noticed two things:

- You want 'resource', not 'resouce'.

- It seems to me that hardware_resource_to_Z_packet should just return
  one of the PACKET_Z[0-4], ... constants directly, instead of
  returning a Z_PACKET_* constant and letting
  remote_check_Zpacket_support convert that to a PACKET_Z? enum value.

Re: [RFC] checking the Z-packet support on gdbserver

[Daniel Jacobowitz]

On Tue, Sep 18, 2007 at 02:17:11PM -0700, Jim Blandy wrote:
> This looks good to me.  Dan should probably approve the gdbserver
> side, but that looks appropriately done to me, too.

Please see my earlier response.  The question is what information we
should ask the remote target for, before we set a protocol in stone
to ask it.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] checking the Z-packet support on gdbserver

[Jim Blandy]

Emi SUZUKI <emi-suzuki@tjsys.co.jp> writes:
> Well, I understood casually that the answer of 'qSupported' packet
> would tell the support for the other 'q' packets from the current
> implementaion...  According to the description in the info, definitely
> it would make sense.  How about the attached?

Actually, thinking more carefully: what effect will this change have
on existing stubs that do support hardware watchpoints, but don't
include 'Z0+', etc. in their qSupported responses?  Won't it mean that
new GDBs will decline to use hardware watchpoints with them?

It seems to me that, when GDB has no information about 'Zx'
availability, it should continue to assume that they are available and
then get errors if not, as it does now, but it should also work with
smarter stubs, like your patched GDB server, to provide a better user
experience.

So, would it make more sense for the initial state of the Z packets in
remote_protocol_features to be PACKET_SUPPORT_UNKNOWN, and for
gdbserver to transmit either Zx- or Zx+ as appropriate?  Thus, in your
case, as soon as GDB connected to the target it would know that
hardware watchpoints weren't available, but on connection to some
older stub which said nothing about the Z packets in its qSupported
response (if it gave such a response at all), GDB would continue to
try hardware watchpoints.

Re: [RFC] checking the Z-packet support on gdbserver

[Emi SUZUKI]

Hello Jim, 

Thank you for your review and sorry for a bit later response.  

From: Jim Blandy <jimb at codesourcery.com>
Subject: Re: [RFC] checking the Z-packet support on gdbserver
Date: Tue, 18 Sep 2007 14:45:27 -0700

> So, would it make more sense for the initial state of the Z packets in
> remote_protocol_features to be PACKET_SUPPORT_UNKNOWN, and for
> gdbserver to transmit either Zx- or Zx+ as appropriate?  Thus, in your
> case, as soon as GDB connected to the target it would know that
> hardware watchpoints weren't available, but on connection to some
> older stub which said nothing about the Z packets in its qSupported
> response (if it gave such a response at all), GDB would continue to
> try hardware watchpoints.

Hmmm.  But my primal worry for the current behavior is that working
with the stubs which do not support hardware watchpoints (including
the older ones) cause errors.  Although I know it would be a passive
attitude towards using hardware watchpoints, using software ones
instead of hardware ones should not cause errors.  And if the older
stubs have Z-packet supports, you can activate them by setting "set
remote Z-packet on" or something, no matter how the initial state of
Z-packets in remote_protocol_features are.  

Actually, I couldn't find any reason why the initial value of
remote_hw_watchpoint_limit defined in remote.c is -1, and it stands
for that hardware watchpoints are available...

My best regards, 
-- 
Emi SUZUKI / emi-suzuki at tjsys.co.jp