This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Fix PR remote/13492 (Re: [PATCH] PR threads/10729: x86 hw watchpoints and non-stop mode)
On Monday 12 December 2011 20:34:09, Jan Kratochvil wrote:
> On Mon, 12 Dec 2011 21:30:25 +0100, Pedro Alves wrote:
> > so it seems there's no point in trying to retain all the other
> > bits of DR_STATUS.
>
> I agree and I agree with this patch part.
Thanks. I've applied the patch below to gdbserver, which adds these
bits we've discussed, and fixes PR13492.
> (I do not agree for example with the lwp->stopped_by_watchpoint part as
> multiple watchpoints may get hit on one stop which isn't handled well but that
> is out of the scope of this discussion.)
I'm not 100% certain that is possible, but in any case, I think that
even if we handled that, we wouldn't ever get as far as
prepare_to_resume before handling all the multiple watchpoints.
2011-12-13 Pedro Alves <pedro@codesourcery.com>
PR remote/13492
* i386-low.c (i386_low_stopped_data_address): Avoid fetching
DR_CONTROL unless necessary. Extend comments.
* linux-x86-low.c (x86_linux_prepare_to_resume): Don't write to
DR0-3 if not used. If any watchpoint was set, clear DR_STATUS.
---
gdb/gdbserver/i386-low.c | 58 +++++++++++++++++++++++++++++++++--------
gdb/gdbserver/linux-x86-low.c | 14 ++++++++--
2 files changed, 59 insertions(+), 13 deletions(-)
Index: src/gdb/gdbserver/i386-low.c
===================================================================
--- src.orig/gdb/gdbserver/i386-low.c 2011-07-25 19:59:47.693736296 +0100
+++ src/gdb/gdbserver/i386-low.c 2011-12-13 16:06:49.422184996 +0000
@@ -559,24 +559,60 @@ i386_low_stopped_data_address (struct i3
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
+ data breakpoint trap. Only fetch it when necessary, to avoid an
+ unnecessary extra syscall when no watchpoint triggered. */
+ int control_p = 0;
unsigned control;
- /* Get the current values the inferior has. If the thread was
- running when we last changed watchpoints, the mirror no longer
- represents what was set in this LWP's debug registers. */
+ /* In non-stop/async, threads can be running while we change the
+ global dr_mirror (and friends). Say, we set a watchpoint, and
+ let threads resume. Now, say you delete the watchpoint, or
+ add/remove watchpoints such that dr_mirror changes while threads
+ are running. On targets that support non-stop,
+ inserting/deleting watchpoints updates the global dr_mirror only.
+ It does not update the real thread's debug registers; that's only
+ done prior to resume. Instead, if threads are running when the
+ mirror changes, a temporary and transparent stop on all threads
+ is forced so they can get their copy of the debug registers
+ updated on re-resume. Now, say, a thread hit a watchpoint before
+ having been updated with the new dr_mirror contents, and we
+ haven't yet handled the corresponding SIGTRAP. If we trusted
+ dr_mirror below, we'd mistake the real trapped address (from the
+ last time we had updated debug registers in the thread) with
+ whatever was currently in dr_mirror. So to fix this, dr_mirror
+ always represents intention, what we _want_ threads to have in
+ debug registers. To get at the address and cause of the trap, we
+ need to read the state the thread still has in its debug
+ registers.
+
+ In sum, always get the current debug register values the current
+ thread has, instead of trusting the global mirror. If the thread
+ was running when we last changed watchpoints, the mirror no
+ longer represents what was set in this thread's debug
+ registers. */
status = i386_dr_low_get_status ();
- control = i386_dr_low_get_control ();
ALL_DEBUG_REGISTERS (i)
{
- if (I386_DR_WATCH_HIT (status, i)
- /* This second condition makes sure DRi is set up for a data
- watchpoint, not a hardware breakpoint. The reason is
- that GDB doesn't call the target_stopped_data_address
- method except for data watchpoints. In other words, I'm
- being paranoiac. */
- && I386_DR_GET_RW_LEN (control, i) != 0)
+ if (!I386_DR_WATCH_HIT (status, i))
+ continue;
+
+ if (!control_p)
+ {
+ control = i386_dr_low_get_control ();
+ control_p = 1;
+ }
+
+ /* This second condition makes sure DRi is set up for a data
+ watchpoint, not a hardware breakpoint. The reason is that
+ GDB doesn't call the target_stopped_data_address method
+ except for data watchpoints. In other words, I'm being
+ paranoiac. */
+ if (I386_DR_GET_RW_LEN (control, i) != 0)
{
addr = i386_dr_low_get_addr (i);
rc = 1;
Index: src/gdb/gdbserver/linux-x86-low.c
===================================================================
--- src.orig/gdb/gdbserver/linux-x86-low.c 2011-11-22 13:39:14.172391873 +0000
+++ src/gdb/gdbserver/linux-x86-low.c 2011-12-13 16:08:11.862184967 +0000
@@ -655,6 +655,7 @@ static void
x86_linux_prepare_to_resume (struct lwp_info *lwp)
{
ptid_t ptid = ptid_of (lwp);
+ int clear_status = 0;
if (lwp->arch_private->debug_registers_changed)
{
@@ -665,14 +666,23 @@ x86_linux_prepare_to_resume (struct lwp_
= &proc->private->arch_private->debug_reg_state;
for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++)
- x86_linux_dr_set (ptid, i, state->dr_mirror[i]);
+ if (state->dr_ref_count[i] > 0)
+ {
+ x86_linux_dr_set (ptid, i, state->dr_mirror[i]);
+
+ /* If we're setting a watchpoint, any change the inferior
+ had done itself to the debug registers needs to be
+ discarded, otherwise, i386_low_stopped_data_address can
+ get confused. */
+ clear_status = 1;
+ }
x86_linux_dr_set (ptid, DR_CONTROL, state->dr_control_mirror);
lwp->arch_private->debug_registers_changed = 0;
}
- if (lwp->stopped_by_watchpoint)
+ if (clear_status || lwp->stopped_by_watchpoint)
x86_linux_dr_set (ptid, DR_STATUS, 0);
}