[1/3] Implement "to_stopped_by_hw_breakpoint" for x86 debug registers.
Commit Message
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(+)
Comments
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
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.
@@ -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
@@ -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;
+}
@@ -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 */
@@ -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;