[1/3] Implement "to_stopped_by_hw_breakpoint" for x86 debug registers.

Message ID 20180228014656.32372-2-jhb@FreeBSD.org
State New
Headers show
Series
  • Use signal information to determine SIGTRAP type on FreeBSD
Related show

Commit Message

John Baldwin Feb. 28, 2018, 1:46 a.m.
Report that a thread is stopped by a hardware breakpoint if a non-data
watchpoint is set in DR6.  This change should be a no-op since a target
still needs to implement the "to_supports_stopped_by_hw_breakpoint"
method before this function is used.

gdb/ChangeLog:

	* nat/x86-dregs.c (x86_dr_stopped_by_breakpoint): New function.
	* nat/x86-dregs.h (x86_dr_stopped_by_breakpoint): New prototype.
	* x86-nat.c (x86_stopped_by_hw_breakpoint): New function.
	(x86_use_watchpoints): Set "stopped_by_hw_breakpoint" target
	method.
---
 gdb/ChangeLog       |  8 ++++++++
 gdb/nat/x86-dregs.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 gdb/nat/x86-dregs.h |  4 ++++
 gdb/x86-nat.c       | 13 +++++++++++++
 4 files changed, 70 insertions(+)

-- 
2.15.1

Comments

Pedro Alves March 2, 2018, 8:19 p.m. | #1
Hi John,

This LGTM, with the nits below addressed.

On 02/28/2018 01:46 AM, John Baldwin wrote:
> This change should be a no-op since a target

> still needs to implement the "to_supports_stopped_by_hw_breakpoint"

> method before this function is used.


I think it'd be good to dd something like this as a comment somewhere.
Maybe next to where t->to_stopped_by_hw_breakpoint is set?

Because while looking at the patch, I didn't notice that comment in
the git log, and it took me a bit to realize that this does not affect
all x86 ports as is.

> +/* Return non-zero if the inferior has some breakpoint that triggered.

> +   Otherwise return zero.  */

> +

> +int

> +x86_dr_stopped_by_breakpoint (struct x86_debug_reg_state *state)

> +{


I was also slightly confused by this until I realized that you
meant _hardware_ breakpoint.  Can you rename this to
x86_dr_stopped_by_hw_breakpoint, and update the comment too?

Thanks,
Pedro Alves
John Baldwin March 4, 2018, 5:32 a.m. | #2
On 3/2/18 12:19 PM, Pedro Alves wrote:
> Hi John,

> 

> This LGTM, with the nits below addressed.

> 

> On 02/28/2018 01:46 AM, John Baldwin wrote:

>> This change should be a no-op since a target

>> still needs to implement the "to_supports_stopped_by_hw_breakpoint"

>> method before this function is used.

> 

> I think it'd be good to dd something like this as a comment somewhere.

> Maybe next to where t->to_stopped_by_hw_breakpoint is set?

> 

> Because while looking at the patch, I didn't notice that comment in

> the git log, and it took me a bit to realize that this does not affect

> all x86 ports as is.


Ok, will do.  The comment is also useful to help clarify the difference
with 'to_stopped_by_watchpoint' which doesn't have a matching
'to_supports_stopped_by_watchpoint'.  (I found this difference a bit
confusing myself.)

>> +/* Return non-zero if the inferior has some breakpoint that triggered.

>> +   Otherwise return zero.  */

>> +

>> +int

>> +x86_dr_stopped_by_breakpoint (struct x86_debug_reg_state *state)

>> +{

> 

> I was also slightly confused by this until I realized that you

> meant _hardware_ breakpoint.  Can you rename this to

> x86_dr_stopped_by_hw_breakpoint, and update the comment too?


Yes.  I had originally modeled this on the 'stopped_by_watchpoint' naming
and had just assumed 'hw_' since x86 debug registers aren't used for
software breakpoints, but I agree that the more explicit name is clearer.

-- 
John Baldwin

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 8ab6381add..0b4a308ef5 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@ 
+2018-02-28  John Baldwin  <jhb@FreeBSD.org>
+
+	* nat/x86-dregs.c (x86_dr_stopped_by_breakpoint): New function.
+	* nat/x86-dregs.h (x86_dr_stopped_by_breakpoint): New prototype.
+	* x86-nat.c (x86_stopped_by_hw_breakpoint): New function.
+	(x86_use_watchpoints): Set "stopped_by_hw_breakpoint" target
+	method.
+
 2018-02-28  John Baldwin  <jhb@FreeBSD.org>
 
 	* fbsd-nat.c (fbsd_resume): Use PT_SETSTEP for stepping and a
diff --git a/gdb/nat/x86-dregs.c b/gdb/nat/x86-dregs.c
index c816473628..514fde0253 100644
--- a/gdb/nat/x86-dregs.c
+++ b/gdb/nat/x86-dregs.c
@@ -649,3 +649,48 @@  x86_dr_stopped_by_watchpoint (struct x86_debug_reg_state *state)
   CORE_ADDR addr = 0;
   return x86_dr_stopped_data_address (state, &addr);
 }
+
+/* Return non-zero if the inferior has some breakpoint that triggered.
+   Otherwise return zero.  */
+
+int
+x86_dr_stopped_by_breakpoint (struct x86_debug_reg_state *state)
+{
+  CORE_ADDR addr = 0;
+  int i;
+  int rc = 0;
+  /* The current thread's DR_STATUS.  We always need to read this to
+     check whether some watchpoint caused the trap.  */
+  unsigned status;
+  /* We need DR_CONTROL as well, but only iff DR_STATUS indicates a
+     breakpoint trap.  Only fetch it when necessary, to avoid an
+     unnecessary extra syscall when no watchpoint triggered.  */
+  int control_p = 0;
+  unsigned control = 0;
+
+  /* As above, always read the current thread's debug registers rather
+     than trusting dr_mirror.  */
+  status = x86_dr_low_get_status ();
+
+  ALL_DEBUG_ADDRESS_REGISTERS (i)
+    {
+      if (!X86_DR_WATCH_HIT (status, i))
+	continue;
+
+      if (!control_p)
+	{
+	  control = x86_dr_low_get_control ();
+	  control_p = 1;
+	}
+
+      if (X86_DR_GET_RW_LEN (control, i) == 0)
+	{
+	  addr = x86_dr_low_get_addr (i);
+	  rc = 1;
+	  if (show_debug_regs)
+	    x86_show_dr (state, "watchpoint_hit", addr, -1, hw_execute);
+	}
+    }
+
+  return rc;
+}
diff --git a/gdb/nat/x86-dregs.h b/gdb/nat/x86-dregs.h
index dd6242eda9..84d710c34e 100644
--- a/gdb/nat/x86-dregs.h
+++ b/gdb/nat/x86-dregs.h
@@ -128,4 +128,8 @@  extern int x86_dr_stopped_data_address (struct x86_debug_reg_state *state,
    Otherwise return false.  */
 extern int x86_dr_stopped_by_watchpoint (struct x86_debug_reg_state *state);
 
+/* Return true if the inferior has some breakpoint that triggered.
+   Otherwise return false.  */
+extern int x86_dr_stopped_by_breakpoint (struct x86_debug_reg_state *state);
+
 #endif /* X86_DREGS_H */
diff --git a/gdb/x86-nat.c b/gdb/x86-nat.c
index b126c47c94..e32450afdf 100644
--- a/gdb/x86-nat.c
+++ b/gdb/x86-nat.c
@@ -260,6 +260,18 @@  x86_can_use_hw_breakpoint (struct target_ops *self,
   return 1;
 }
 
+/* Return non-zero if the inferior has some breakpoint that triggered.
+   Otherwise return zero.  */
+
+static int
+x86_stopped_by_hw_breakpoint (struct target_ops *ops)
+{
+  struct x86_debug_reg_state *state
+    = x86_debug_reg_state (ptid_get_pid (inferior_ptid));
+
+  return x86_dr_stopped_by_breakpoint (state);
+}
+
 static void
 add_show_debug_regs_command (void)
 {
@@ -293,6 +305,7 @@  x86_use_watchpoints (struct target_ops *t)
   t->to_region_ok_for_hw_watchpoint = x86_region_ok_for_watchpoint;
   t->to_stopped_by_watchpoint = x86_stopped_by_watchpoint;
   t->to_stopped_data_address = x86_stopped_data_address;
+  t->to_stopped_by_hw_breakpoint = x86_stopped_by_hw_breakpoint;
   t->to_insert_watchpoint = x86_insert_watchpoint;
   t->to_remove_watchpoint = x86_remove_watchpoint;
   t->to_insert_hw_breakpoint = x86_insert_hw_breakpoint;