Fix PR remote/13492 (Re: [PATCH] PR threads/10729: x86 hw watchpoints and non-stop mode)

[Pedro Alves <pedro@codesourcery.com>]

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);
 }
 \f