[PATCH 0/3] Check for zpoint support when handling watchpoints

[PATCH 0/3] Check for zpoint support when handling watchpoints

[Mohamed Bouhaouel]

Hi all,

This set of patches improves the flow of inserting a watchpoint or a
hardware breakpoint.  It consists of checking whether the target
supports the z_point before doing the actual insertion.  Having such a
flow would avoid unnecessary too many warning messages, not
increment the breakpoint counter when not supported, and prevent
commands from being aborted upon resuming.

Below are the behaviors, with and without the patches, of trying to
insert a hardware watchpoint when the target does not support that.

* Original behavior

(gdb) awatch dim0
Hardware watchpoint 2: dim0
Warning:
Could not insert hardware watchpoint 2.
Could not insert hardware breakpoints:
You may have requested too many hardware breakpoints/watchpoint.

(gdb) continue
Hardware watchpoint 2: dim0
Warning:
Could not insert hardware watchpoint 2.
Could not insert hardware breakpoints:
You may have requested too many hardware breakpoints/watchpoint.

Command aborted.
(gdb)

The warning messages keep popping up on each resume command (`continue`,
`step`, ...) until the user manually deletes the watchpoint.

* With patches applied

(gdb) awatch dim0
Target does not support this type of hardware watchpoint.
(gdb)

Regards,
Mohamed

Mohamed Bouhaouel (3):
  gdb, gdbserver, zpoint: report z_point support
  gdb, breakpoint: add a breakpoint type converter
  gdb, zpoint: check for target hardware breakpoint support

 gdb/breakpoint.c       | 20 ++++++++++++++++++++
 gdb/breakpoint.h       |  3 +++
 gdb/remote.c           | 41 ++++++++++++++++++++++++++++++++++++++---
 gdbserver/mem-break.cc |  2 +-
 gdbserver/mem-break.h  |  4 ++++
 gdbserver/server.cc    |  7 +++++++
 6 files changed, 73 insertions(+), 4 deletions(-)

-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 1/3] gdb, gdbserver, zpoint: report z_point support

[Mohamed Bouhaouel]

Report if the target supports Z-points when exchanging features
between GDB and gdbserver.

Signed-off-by: Mohamed Bouhaouel <mohamed.bouhaouel@intel.com>
---
 gdb/remote.c           | 15 +++++++++++++++
 gdbserver/mem-break.cc |  2 +-
 gdbserver/mem-break.h  |  4 ++++
 gdbserver/server.cc    |  7 +++++++
 4 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 526df313ea7..1139ce3ddf7 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -755,6 +755,9 @@ class remote_target : public process_stratum_target
   int insert_watchpoint (CORE_ADDR, int, enum target_hw_bp_type,
 			 struct expression *) override;
 
+  /* Returns TRUE if GDB Z breakpoint type is supported, FALSE otherwise.  */
+  bool supports_z_point_type (int);
+
   int remove_watchpoint (CORE_ADDR, int, enum target_hw_bp_type,
 			 struct expression *) override;
 
@@ -5604,6 +5607,11 @@ static const struct protocol_feature remote_protocol_features[] = {
   { "no-resumed", PACKET_DISABLE, remote_supported_packet, PACKET_no_resumed },
   { "memory-tagging", PACKET_DISABLE, remote_supported_packet,
     PACKET_memory_tagging_feature },
+  { "Z0", PACKET_SUPPORT_UNKNOWN, remote_supported_packet, PACKET_Z0 },
+  { "Z1", PACKET_SUPPORT_UNKNOWN, remote_supported_packet, PACKET_Z1 },
+  { "Z2", PACKET_SUPPORT_UNKNOWN, remote_supported_packet, PACKET_Z2 },
+  { "Z3", PACKET_SUPPORT_UNKNOWN, remote_supported_packet, PACKET_Z3 },
+  { "Z4", PACKET_SUPPORT_UNKNOWN, remote_supported_packet, PACKET_Z4 },
 };
 
 static char *remote_support_xml;
@@ -10843,6 +10851,13 @@ watchpoint_to_Z_packet (int type)
     }
 }
 
+bool
+remote_target::supports_z_point_type (int type)
+{
+  Z_packet_type packet = watchpoint_to_Z_packet (type);
+  return (m_features.packet_support (PACKET_Z0 + packet) != PACKET_DISABLE);
+}
+
 int
 remote_target::insert_watchpoint (CORE_ADDR addr, int len,
 				  enum target_hw_bp_type type, struct expression *cond)
diff --git a/gdbserver/mem-break.cc b/gdbserver/mem-break.cc
index c669842228d..20434f7c9b4 100644
--- a/gdbserver/mem-break.cc
+++ b/gdbserver/mem-break.cc
@@ -987,7 +987,7 @@ find_gdb_breakpoint (char z_type, CORE_ADDR addr, int kind)
   return NULL;
 }
 
-static int
+int
 z_type_supported (char z_type)
 {
   return (z_type >= '0' && z_type <= '4'
diff --git a/gdbserver/mem-break.h b/gdbserver/mem-break.h
index 9bf7aa84932..458ba0e6be2 100644
--- a/gdbserver/mem-break.h
+++ b/gdbserver/mem-break.h
@@ -276,4 +276,8 @@ int remove_memory_breakpoint (struct raw_breakpoint *bp);
 void clone_all_breakpoints (struct thread_info *child_thread,
 			    const struct thread_info *parent_thread);
 
+
+/* Returns TRUE if Z_TYPE is supported by the target.  */
+
+int z_type_supported (char z_type);
 #endif /* GDBSERVER_MEM_BREAK_H */
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 949849b63a2..e758cd0d509 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -2480,6 +2480,13 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
       if (target_supports_memory_tagging ())
 	strcat (own_buf, ";memory-tagging+");
 
+      /* Z points support.  */
+      strcat (own_buf, z_type_supported ('0') ? ";Z0+" : ";Z0-");
+      strcat (own_buf, z_type_supported ('1') ? ";Z1+" : ";Z1-");
+      strcat (own_buf, z_type_supported ('2') ? ";Z2+" : ";Z2-");
+      strcat (own_buf, z_type_supported ('3') ? ";Z3+" : ";Z3-");
+      strcat (own_buf, z_type_supported ('4') ? ";Z4+" : ";Z4-");
+
       /* Reinitialize components as needed for the new connection.  */
       hostio_handle_new_gdb_connection ();
       target_handle_new_gdb_connection ();
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 1/3] gdb, gdbserver, zpoint: report z_point support

[Guinevere Larsen]

On 28/03/2023 13:54, Mohamed Bouhaouel via Gdb-patches wrote:
> Report if the target supports Z-points when exchanging features
> between GDB and gdbserver.
>
> Signed-off-by: Mohamed Bouhaouel <mohamed.bouhaouel@intel.com>

Hi!

Thanks for working on this, and sorry for the long time to get a review.

The code changes look good, but the commit message could use some work. 
Calling them "Z-points" threw me for a loop that google couldn't help. 
Maybe rewording to something like:

Make GDB and gdbserver report which types of breakpoints and watchpoints 
are supported when exchanging features. This is done by replying with 
Z<type>+ (Supported) or Z<type>- (unsupported)

Also, I think this would warrant a NEWS entry and an update to the 
remote protocol documentation.

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

> ---
>   gdb/remote.c           | 15 +++++++++++++++
>   gdbserver/mem-break.cc |  2 +-
>   gdbserver/mem-break.h  |  4 ++++
>   gdbserver/server.cc    |  7 +++++++
>   4 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 526df313ea7..1139ce3ddf7 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -755,6 +755,9 @@ class remote_target : public process_stratum_target
>     int insert_watchpoint (CORE_ADDR, int, enum target_hw_bp_type,
>   			 struct expression *) override;
>   
> +  /* Returns TRUE if GDB Z breakpoint type is supported, FALSE otherwise.  */
> +  bool supports_z_point_type (int);
> +
>     int remove_watchpoint (CORE_ADDR, int, enum target_hw_bp_type,
>   			 struct expression *) override;
>   
> @@ -5604,6 +5607,11 @@ static const struct protocol_feature remote_protocol_features[] = {
>     { "no-resumed", PACKET_DISABLE, remote_supported_packet, PACKET_no_resumed },
>     { "memory-tagging", PACKET_DISABLE, remote_supported_packet,
>       PACKET_memory_tagging_feature },
> +  { "Z0", PACKET_SUPPORT_UNKNOWN, remote_supported_packet, PACKET_Z0 },
> +  { "Z1", PACKET_SUPPORT_UNKNOWN, remote_supported_packet, PACKET_Z1 },
> +  { "Z2", PACKET_SUPPORT_UNKNOWN, remote_supported_packet, PACKET_Z2 },
> +  { "Z3", PACKET_SUPPORT_UNKNOWN, remote_supported_packet, PACKET_Z3 },
> +  { "Z4", PACKET_SUPPORT_UNKNOWN, remote_supported_packet, PACKET_Z4 },
>   };
>   
>   static char *remote_support_xml;
> @@ -10843,6 +10851,13 @@ watchpoint_to_Z_packet (int type)
>       }
>   }
>   
> +bool
> +remote_target::supports_z_point_type (int type)
> +{
> +  Z_packet_type packet = watchpoint_to_Z_packet (type);
> +  return (m_features.packet_support (PACKET_Z0 + packet) != PACKET_DISABLE);
> +}
> +
>   int
>   remote_target::insert_watchpoint (CORE_ADDR addr, int len,
>   				  enum target_hw_bp_type type, struct expression *cond)
> diff --git a/gdbserver/mem-break.cc b/gdbserver/mem-break.cc
> index c669842228d..20434f7c9b4 100644
> --- a/gdbserver/mem-break.cc
> +++ b/gdbserver/mem-break.cc
> @@ -987,7 +987,7 @@ find_gdb_breakpoint (char z_type, CORE_ADDR addr, int kind)
>     return NULL;
>   }
>   
> -static int
> +int
>   z_type_supported (char z_type)
>   {
>     return (z_type >= '0' && z_type <= '4'
> diff --git a/gdbserver/mem-break.h b/gdbserver/mem-break.h
> index 9bf7aa84932..458ba0e6be2 100644
> --- a/gdbserver/mem-break.h
> +++ b/gdbserver/mem-break.h
> @@ -276,4 +276,8 @@ int remove_memory_breakpoint (struct raw_breakpoint *bp);
>   void clone_all_breakpoints (struct thread_info *child_thread,
>   			    const struct thread_info *parent_thread);
>   
> +
> +/* Returns TRUE if Z_TYPE is supported by the target.  */
> +
> +int z_type_supported (char z_type);
>   #endif /* GDBSERVER_MEM_BREAK_H */
> diff --git a/gdbserver/server.cc b/gdbserver/server.cc
> index 949849b63a2..e758cd0d509 100644
> --- a/gdbserver/server.cc
> +++ b/gdbserver/server.cc
> @@ -2480,6 +2480,13 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
>         if (target_supports_memory_tagging ())
>   	strcat (own_buf, ";memory-tagging+");
>   
> +      /* Z points support.  */
> +      strcat (own_buf, z_type_supported ('0') ? ";Z0+" : ";Z0-");
> +      strcat (own_buf, z_type_supported ('1') ? ";Z1+" : ";Z1-");
> +      strcat (own_buf, z_type_supported ('2') ? ";Z2+" : ";Z2-");
> +      strcat (own_buf, z_type_supported ('3') ? ";Z3+" : ";Z3-");
> +      strcat (own_buf, z_type_supported ('4') ? ";Z4+" : ";Z4-");
> +
>         /* Reinitialize components as needed for the new connection.  */
>         hostio_handle_new_gdb_connection ();
>         target_handle_new_gdb_connection ();

[PATCH 2/3] gdb, breakpoint: add a breakpoint type converter

[Mohamed Bouhaouel]

Add a new function 'bptype_to_target_hw_bp_type' to perform
the translation from 'bptype' to 'target_hw_bp_type'.

Signed-off-by: Mohamed Bouhaouel <mohamed.bouhaouel@intel.com>
---
 gdb/breakpoint.c | 20 ++++++++++++++++++++
 gdb/breakpoint.h |  3 +++
 2 files changed, 23 insertions(+)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 7228acfd8fe..e71cf7639d9 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -9581,6 +9581,26 @@ break_range_command (const char *arg, int from_tty)
   install_breakpoint (false, std::move (br), true);
 }
 
+/* See breakpoint.h.  */
+
+target_hw_bp_type
+bptype_to_target_hw_bp_type (bptype type)
+{
+  switch (type)
+    {
+    case bp_hardware_watchpoint:
+      return hw_write;
+    case bp_read_watchpoint:
+      return hw_read;
+    case bp_access_watchpoint:
+      return hw_access;
+    case bp_hardware_breakpoint:
+      return hw_execute;
+    default:
+      error (_ ("Bad breakpoint type: bptype %d."), type);
+    }
+}
+
 /*  Return non-zero if EXP is verified as constant.  Returned zero
     means EXP is variable.  Also the constant detection may fail for
     some constant expressions and in such case still falsely return
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 7c5cf3f2bef..12fa5c96e08 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1946,4 +1946,7 @@ extern void describe_other_breakpoints (struct gdbarch *,
 
 extern void enable_disable_bp_location (bp_location *loc, bool enable);
 
+/* Translate BPTYPE to TARGET_HW_BP_TYPE.  */
+
+extern target_hw_bp_type bptype_to_target_hw_bp_type (bptype type);
 #endif /* !defined (BREAKPOINT_H) */
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 3/3] gdb, zpoint: check for target hardware breakpoint support

[Mohamed Bouhaouel]

In 'can_use_hw_breakpoint', check if the target supports
hardware-assisted breakpoints.  This will prevent GDB
from trying to insert the hardware breakpoint in case
it is not supported.

Signed-off-by: Mohamed Bouhaouel <mohamed.bouhaouel@intel.com>
---
 gdb/remote.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 1139ce3ddf7..1b9883f263c 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -10833,7 +10833,7 @@ remote_target::remove_breakpoint (struct gdbarch *gdbarch,
 }
 
 static enum Z_packet_type
-watchpoint_to_Z_packet (int type)
+hw_bp_to_Z_packet (int type)
 {
   switch (type)
     {
@@ -10846,6 +10846,9 @@ watchpoint_to_Z_packet (int type)
     case hw_access:
       return Z_PACKET_ACCESS_WP;
       break;
+    case hw_execute:
+      return Z_PACKET_HARDWARE_BP;
+      break;
     default:
       internal_error (_("hw_bp_to_z: bad watchpoint type %d"), type);
     }
@@ -10854,7 +10857,7 @@ watchpoint_to_Z_packet (int type)
 bool
 remote_target::supports_z_point_type (int type)
 {
-  Z_packet_type packet = watchpoint_to_Z_packet (type);
+  Z_packet_type packet = hw_bp_to_Z_packet (type);
   return (m_features.packet_support (PACKET_Z0 + packet) != PACKET_DISABLE);
 }
 
@@ -10865,7 +10868,7 @@ remote_target::insert_watchpoint (CORE_ADDR addr, int len,
   struct remote_state *rs = get_remote_state ();
   char *endbuf = rs->buf.data () + get_remote_packet_size ();
   char *p;
-  enum Z_packet_type packet = watchpoint_to_Z_packet (type);
+  enum Z_packet_type packet = hw_bp_to_Z_packet (type);
 
   if (m_features.packet_support (PACKET_Z0 + packet) == PACKET_DISABLE)
     return 1;
@@ -10913,7 +10916,7 @@ remote_target::remove_watchpoint (CORE_ADDR addr, int len,
   struct remote_state *rs = get_remote_state ();
   char *endbuf = rs->buf.data () + get_remote_packet_size ();
   char *p;
-  enum Z_packet_type packet = watchpoint_to_Z_packet (type);
+  enum Z_packet_type packet = hw_bp_to_Z_packet (type);
 
   if (m_features.packet_support (PACKET_Z0 + packet) == PACKET_DISABLE)
     return -1;
@@ -10965,6 +10968,11 @@ remote_target::can_use_hw_breakpoint (enum bptype type, int cnt, int ot)
 {
   if (type == bp_hardware_breakpoint)
     {
+      /* Check if the target supports hardware-assisted breakpoints.
+	 Return 0 if not.  */
+      if (!supports_z_point_type (bptype_to_target_hw_bp_type (type)))
+	return 0;
+
       if (remote_hw_breakpoint_limit == 0)
 	return 0;
       else if (remote_hw_breakpoint_limit < 0)
@@ -10974,6 +10982,18 @@ remote_target::can_use_hw_breakpoint (enum bptype type, int cnt, int ot)
     }
   else
     {
+      /* Check if the target supports the hardware watchpoint type.
+	 Return 0 if not.  */
+      if (!supports_z_point_type (bptype_to_target_hw_bp_type (type)))
+	{
+	  /* If hw read watchpoints are not supported while hw access are,
+	     GDB will try to insert the watchpoint as hw access.  */
+	  bool access_support = supports_z_point_type (
+	    bptype_to_target_hw_bp_type (bp_access_watchpoint));
+	  if (!(type == bp_read_watchpoint && access_support))
+	    return 0;
+	}
+
       if (remote_hw_watchpoint_limit == 0)
 	return 0;
       else if (remote_hw_watchpoint_limit < 0)
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 0/3] Check for zpoint support when handling watchpoints

[Guinevere Larsen]

On 28/03/2023 13:54, Mohamed Bouhaouel via Gdb-patches wrote:
> Hi all,
>
> This set of patches improves the flow of inserting a watchpoint or a
> hardware breakpoint.  It consists of checking whether the target
> supports the z_point before doing the actual insertion.  Having such a
> flow would avoid unnecessary too many warning messages, not
> increment the breakpoint counter when not supported, and prevent
> commands from being aborted upon resuming.
>
> Below are the behaviors, with and without the patches, of trying to
> insert a hardware watchpoint when the target does not support that.
>
> * Original behavior
>
> (gdb) awatch dim0
> Hardware watchpoint 2: dim0
> Warning:
> Could not insert hardware watchpoint 2.
> Could not insert hardware breakpoints:
> You may have requested too many hardware breakpoints/watchpoint.
>
> (gdb) continue
> Hardware watchpoint 2: dim0
> Warning:
> Could not insert hardware watchpoint 2.
> Could not insert hardware breakpoints:
> You may have requested too many hardware breakpoints/watchpoint.
>
> Command aborted.
> (gdb)
>
> The warning messages keep popping up on each resume command (`continue`,
> `step`, ...) until the user manually deletes the watchpoint.
>
> * With patches applied
>
> (gdb) awatch dim0
> Target does not support this type of hardware watchpoint.
> (gdb)
Hi Mohamed!

Thanks for working on this, and sorry for the horrible delay in getting 
a review. I read through the series, and patches 2 and 3 look fine, you 
can add my review tag to them. My comments about patch 1 were sent in 
reply to that.

I hope some maintainer looks at this soon!
Reviewed-By: Guinevere Larsen <blarsen@redhat.com>

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

>
> Regards,
> Mohamed
>
> Mohamed Bouhaouel (3):
>    gdb, gdbserver, zpoint: report z_point support
>    gdb, breakpoint: add a breakpoint type converter
>    gdb, zpoint: check for target hardware breakpoint support
>
>   gdb/breakpoint.c       | 20 ++++++++++++++++++++
>   gdb/breakpoint.h       |  3 +++
>   gdb/remote.c           | 41 ++++++++++++++++++++++++++++++++++++++---
>   gdbserver/mem-break.cc |  2 +-
>   gdbserver/mem-break.h  |  4 ++++
>   gdbserver/server.cc    |  7 +++++++
>   6 files changed, 73 insertions(+), 4 deletions(-)
>