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.

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 4798660b184a702b0142e8ff12e7295c60bfbe3d..c1142d302b301495b5af80afad8c00f0bbb9e726 100644 (file)
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,12 @@
+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.
+
 2011-12-13  Yao Qi  <yao@codesourcery.com>
 
        * tracepoint.c (trace_buffer_alloc): Replace magic numbers with

diff --git a/gdb/gdbserver/i386-low.c b/gdb/gdbserver/i386-low.c
index a9591794680d63bda2c426f1d31d2769fdfa0190..0ac37c85664311db94db20d07021e84244afadef 100644 (file)
--- a/gdb/gdbserver/i386-low.c
+++ b/gdb/gdbserver/i386-low.c
@@ -559,24 +559,60 @@ i386_low_stopped_data_address (struct i386_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
+     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;

diff --git a/gdb/gdbserver/linux-x86-low.c b/gdb/gdbserver/linux-x86-low.c
index d1c760e9e6cef1a9e8f48088b7040eaa35bf9b1a..db87ceec5bdd0e0c4bf8dd907a434a08734ef389 100644 (file)
--- a/gdb/gdbserver/linux-x86-low.c
+++ b/gdb/gdbserver/linux-x86-low.c
@@ -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_info *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);
 }
 \f