diff mbox series

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

Message ID 20231114133532.3877-4-mohamed.bouhaouel@intel.com
State New
Headers
Series Check for zpoint support when handling watchpoints |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed

Commit Message

Bouhaouel, Mohamed Nov. 14, 2023, 1:35 p.m. UTC 
  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.

Reviewed-By: Guinevere Larsen <blarsen@redhat.com>
---
 gdb/remote.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

Comments

Tom Tromey Nov. 14, 2023, 2:45 p.m. UTC | #1 
>>>>> Mohamed Bouhaouel <mohamed.bouhaouel@intel.com> writes:

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

Thanks.

One question I have is how this is tested.

Another oddity is that supposedly a remote can reply '' to a z packet,
meaning it isn't supported.  However, insert_hw_breakpoint (at least)
doesn't seem to record this response.

> +	  /* 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));

Formatting looks wrong here.

Tom
Bouhaouel, Mohamed Nov. 15, 2023, 6:18 p.m. UTC | #2 
Hi,

Thanks Tom for the feedback,

> One question I have is how this is tested.

We would need a remote target that does not support hardware
watchpoints.  But I am not sure how can we build such a test.

> Another oddity is that supposedly a remote can reply '' to a z packet,
> meaning it isn't supported.  However, insert_hw_breakpoint (at least)
> doesn't seem to record this response.

Could you please elaborate?  The remote can or can't replay to z packets?

> > +	  /* 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));
> 
> Formatting looks wrong here.

Hmm, how else can we format it? One suggestion is to use an additional variable
to store the return of " bptype_to_target_hw_bp_type (bp_access_watchpoint)".

Thanks,
Mohamed
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
Tom Tromey Nov. 17, 2023, 1:42 p.m. UTC | #3 
>>>>> Bouhaouel, Mohamed <mohamed.bouhaouel@intel.com> writes:

>> > +	  /* 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));
>> 
>> Formatting looks wrong here.

> Hmm, how else can we format it? One suggestion is to use an additional variable
> to store the return of " bptype_to_target_hw_bp_type (bp_access_watchpoint)".

Yeah, that would work.  A dangling open paren isn't really gdb style.

Another way would be

	  bool access_support
            = (supports_z_point_type
               (bptype_to_target_hw_bp_type (bp_access_watchpoint)));

kind of uglier than a new variable but it's done elsewhere.

Tom
diff mbox series

Patch

diff --git a/gdb/remote.c b/gdb/remote.c
index 7aa380a5989..10504696a57 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -10982,7 +10982,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)
     {
@@ -10995,6 +10995,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);
     }
@@ -11003,7 +11006,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);
 }
 
@@ -11014,7 +11017,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 ((to_underlying (PACKET_Z0)
 				  + to_underlying (packet))) == PACKET_DISABLE)
@@ -11064,7 +11067,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 ((to_underlying (PACKET_Z0)
 				  + to_underlying (packet))) == PACKET_DISABLE)
@@ -11118,6 +11121,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)
@@ -11127,6 +11135,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)