This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[gdbserver/i386] watchpoints when threads are running


GDBserver doesn't actually error out like native gdb
does in PR10729, exactly because this had already been considered
when gdbserve i386 watchpoints support went in --- we only update the
inferior's debug registers when we go and resume the lwp, just like
non-debug registers.  The symptom with gdbserver is that threads that
are running when we set the watchpoint don't trigger watchpoint hits,
until they stop and resume for some reason.  So, to fix this, if a thread is
running when we change watchpoint setting, we just need to force it
to momentarily pause, and things will magically work out themselves.

One thing that also needs changing is that when determining if an
lwp just stopped for a watchpoint, and which data address triggered it,
we should not trust the debug register mirrors, because there's a window
where we change the mirrors, and an lwp that was running with some
watchpoint previously set, that might have not been updated with the new
debug register contents yet could trigger a watchpoint (from the old
settings).  If we trusted the debug registers mirrors, we might not be
able to recognize that stop as being caused by a watchpoint, and
report a spurious SIGTRAP to the user.

I'm putting this under PR10729, because I think the native gdb code
should be adjusted to do the same (set a "debug registers changed"
flag, and update debug registers on resume), instead of trying to poke
the debug registers immediately when the master debug register mirrors
change.

Tested on x86_64-unknown-linux.  I've also confirmed a mingw32 gdbserver
still builds.

-- 
Pedro Alves

2010-08-25  Pedro Alves  <pedro@codesourcery.com>

	PR threads/10729

	* linux-x86-low.c (update_debug_registers_callback): New.
	(i386_dr_low_set_addr): Use it.
	(i386_dr_low_get_addr): New.
	(i386_dr_low_set_control): Use update_debug_registers_callback.
	(i386_dr_low_get_control): New.
	(i386_dr_low_get_status): Adjust.
	* linux-low.c (linux_stop_lwp): New.
	* linux-low.h (linux_stop_lwp): Declare.

	* i386-low.c (I386_DR_GET_RW_LEN): Take the dr7 contents as
	argument instead of a i386_debug_reg_state.
	(I386_DR_WATCH_HIT): Take the dr6 contents as argument instead of
	a i386_debug_reg_state.
	(i386_insert_aligned_watchpoint): Adjust.
	(i386_remove_aligned_watchpoint): Adjust.
	(i386_low_stopped_data_address): Read the debug registers from the
	inferior instead of from the mirrors.
	* i386-low.h (struct i386_debug_reg_state): Extend comment.
	(i386_dr_low_get_addr): Declare.
	(i386_dr_low_get_control): Declare.
	(i386_dr_low_get_status): Change prototype.

	* win32-i386-low.c (dr_status_mirror, dr_control_mirror): New globals.
	(i386_dr_low_get_addr): New.
	(i386_dr_low_get_control): New.
	(i386_dr_low_get_status): Adjust prototype.  Return
	dr_status_mirror.
	(i386_initial_stuff): Clear dr_status_mirror and
	dr_control_mirror.
	(i386_get_thread_context): Adjust.
	(i386_set_thread_context): Adjust.
	(i386_thread_added): Adjust.

---
 gdb/gdbserver/i386-low.c       |   26 ++++++++-----
 gdb/gdbserver/i386-low.h       |   16 ++++++--
 gdb/gdbserver/linux-low.c      |    6 +++
 gdb/gdbserver/linux-low.h      |    1 
 gdb/gdbserver/linux-x86-low.c  |   77 +++++++++++++++++++++++++++--------------
 gdb/gdbserver/win32-i386-low.c |   35 ++++++++++++++----
 6 files changed, 114 insertions(+), 47 deletions(-)

Index: src/gdb/gdbserver/linux-x86-low.c
===================================================================
--- src.orig/gdb/gdbserver/linux-x86-low.c	2010-08-24 20:49:45.000000000 +0100
+++ src/gdb/gdbserver/linux-x86-low.c	2010-08-24 21:28:43.000000000 +0100
@@ -461,30 +461,55 @@ x86_linux_dr_set (ptid_t ptid, int regnu
     error ("Couldn't write debug register");
 }
 
+static int
+update_debug_registers_callback (struct inferior_list_entry *entry,
+				 void *pid_p)
+{
+  struct lwp_info *lwp = (struct lwp_info *) entry;
+  int pid = *(int *) pid_p;
+
+  /* Only update the threads of this process.  */
+  if (pid_of (lwp) == pid)
+    {
+      /* The actual update is done later just before resuming the lwp,
+	 we just mark that the registers need updating.  */
+      lwp->arch_private->debug_registers_changed = 1;
+
+      /* If the lwp isn't stopped, force it to momentarily pause, so
+	 we can update its debug registers.  */
+      if (!lwp->stopped)
+	linux_stop_lwp (lwp);
+    }
+
+  return 0;
+}
+
 /* Update the inferior's debug register REGNUM from STATE.  */
 
 void
 i386_dr_low_set_addr (const struct i386_debug_reg_state *state, int regnum)
 {
-  struct inferior_list_entry *lp;
-  CORE_ADDR addr;
-  /* Only need to update the threads of this process.  */
+  /* Only update the threads of this process.  */
   int pid = pid_of (get_thread_lwp (current_inferior));
 
   if (! (regnum >= 0 && regnum <= DR_LASTADDR - DR_FIRSTADDR))
     fatal ("Invalid debug register %d", regnum);
 
-  addr = state->dr_mirror[regnum];
+  find_inferior (&all_lwps, update_debug_registers_callback, &pid);
+}
 
-  for (lp = all_lwps.head; lp; lp = lp->next)
-    {
-      struct lwp_info *lwp = (struct lwp_info *) lp;
+/* Return the inferior's debug register REGNUM.  */
 
-      /* The actual update is done later, we just mark that the register
-	 needs updating.  */
-      if (pid_of (lwp) == pid)
-	lwp->arch_private->debug_registers_changed = 1;
-    }
+CORE_ADDR
+i386_dr_low_get_addr (int regnum)
+{
+  struct lwp_info *lwp = get_thread_lwp (current_inferior);
+  ptid_t ptid = ptid_of (lwp);
+
+  /* DR6 and DR7 are retrieved with some other way.  */
+  gdb_assert (DR_FIRSTADDR <= regnum && regnum < DR_LASTADDR);
+
+  return x86_linux_dr_get (ptid, regnum);
 }
 
 /* Update the inferior's DR7 debug control register from STATE.  */
@@ -492,31 +517,33 @@ i386_dr_low_set_addr (const struct i386_
 void
 i386_dr_low_set_control (const struct i386_debug_reg_state *state)
 {
-  struct inferior_list_entry *lp;
-  /* Only need to update the threads of this process.  */
+  /* Only update the threads of this process.  */
   int pid = pid_of (get_thread_lwp (current_inferior));
 
-  for (lp = all_lwps.head; lp; lp = lp->next)
-    {
-      struct lwp_info *lwp = (struct lwp_info *) lp;
+  find_inferior (&all_lwps, update_debug_registers_callback, &pid);
+}
 
-      /* The actual update is done later, we just mark that the register
-	 needs updating.  */
-      if (pid_of (lwp) == pid)
-	lwp->arch_private->debug_registers_changed = 1;
-    }
+/* Return the inferior's DR7 debug control register.  */
+
+unsigned
+i386_dr_low_get_control (void)
+{
+  struct lwp_info *lwp = get_thread_lwp (current_inferior);
+  ptid_t ptid = ptid_of (lwp);
+
+  return x86_linux_dr_get (ptid, DR_CONTROL);
 }
 
 /* Get the value of the DR6 debug status register from the inferior
    and record it in STATE.  */
 
-void
-i386_dr_low_get_status (struct i386_debug_reg_state *state)
+unsigned
+i386_dr_low_get_status (void)
 {
   struct lwp_info *lwp = get_thread_lwp (current_inferior);
   ptid_t ptid = ptid_of (lwp);
 
-  state->dr_status_mirror = x86_linux_dr_get (ptid, DR_STATUS);
+  return x86_linux_dr_get (ptid, DR_STATUS);
 }
 
 /* Watchpoint support.  */
Index: src/gdb/gdbserver/linux-low.c
===================================================================
--- src.orig/gdb/gdbserver/linux-low.c	2010-08-24 20:51:14.000000000 +0100
+++ src/gdb/gdbserver/linux-low.c	2010-08-24 21:28:43.000000000 +0100
@@ -2478,6 +2478,12 @@ kill_lwp (unsigned long lwpid, int signo
   return kill (lwpid, signo);
 }
 
+void
+linux_stop_lwp (struct lwp_info *lwp)
+{
+  send_sigstop (lwp);
+}
+
 static void
 send_sigstop (struct lwp_info *lwp)
 {
Index: src/gdb/gdbserver/linux-low.h
===================================================================
--- src.orig/gdb/gdbserver/linux-low.h	2010-08-24 20:55:14.000000000 +0100
+++ src/gdb/gdbserver/linux-low.h	2010-08-24 21:28:43.000000000 +0100
@@ -259,6 +259,7 @@ int elf_64_file_p (const char *file);
 
 void linux_attach_lwp (unsigned long pid);
 struct lwp_info *find_lwp_pid (ptid_t ptid);
+void linux_stop_lwp (struct lwp_info *lwp);
 
 /* From thread-db.c  */
 int thread_db_init (int use_events);
Index: src/gdb/gdbserver/i386-low.c
===================================================================
--- src.orig/gdb/gdbserver/i386-low.c	2010-08-24 20:49:45.000000000 +0100
+++ src/gdb/gdbserver/i386-low.c	2010-08-24 21:28:43.000000000 +0100
@@ -132,12 +132,12 @@ enum target_hw_bp_type
   } while (0)
 
 /* Get from DR7 the RW and LEN fields for the I'th debug register.  */
-#define I386_DR_GET_RW_LEN(state, i) \
-  (((state)->dr_control_mirror \
+#define I386_DR_GET_RW_LEN(dr7, i) \
+  (((dr7) \
     >> (DR_CONTROL_SHIFT + DR_CONTROL_SIZE * (i))) & 0x0f)
 
 /* Did the watchpoint whose address is in the I'th register break?  */
-#define I386_DR_WATCH_HIT(state,i) ((state)->dr_status_mirror & (1 << (i)))
+#define I386_DR_WATCH_HIT(dr6, i) ((dr6) & (1 << (i)))
 
 /* A macro to loop over all debug registers.  */
 #define ALL_DEBUG_REGISTERS(i)	for (i = 0; i < DR_NADDR; i++)
@@ -271,7 +271,7 @@ i386_insert_aligned_watchpoint (struct i
     {
       if (!I386_DR_VACANT (state, i)
 	  && state->dr_mirror[i] == addr
-	  && I386_DR_GET_RW_LEN (state, i) == len_rw_bits)
+	  && I386_DR_GET_RW_LEN (state->dr_control_mirror, i) == len_rw_bits)
 	{
 	  state->dr_ref_count[i]++;
 	  return 0;
@@ -329,7 +329,7 @@ i386_remove_aligned_watchpoint (struct i
     {
       if (!I386_DR_VACANT (state, i)
 	  && state->dr_mirror[i] == addr
-	  && I386_DR_GET_RW_LEN (state, i) == len_rw_bits)
+	  && I386_DR_GET_RW_LEN (state->dr_control_mirror, i) == len_rw_bits)
 	{
 	  if (--state->dr_ref_count[i] == 0) /* No longer in use?  */
 	    {
@@ -539,21 +539,27 @@ i386_low_stopped_data_address (struct i3
   CORE_ADDR addr = 0;
   int i;
   int rc = 0;
+  unsigned status;
+  unsigned control;
 
-  /* Get dr_status_mirror for use by I386_DR_WATCH_HIT.  */
-  i386_dr_low_get_status (state);
+  /* 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.  */
+  status = i386_dr_low_get_status ();
+  control = i386_dr_low_get_control ();
 
   ALL_DEBUG_REGISTERS (i)
     {
-      if (I386_DR_WATCH_HIT (state, 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 (state, i) != 0)
+	  && I386_DR_GET_RW_LEN (control, i) != 0)
 	{
-	  addr = state->dr_mirror[i];
+	  addr = i386_dr_low_get_addr (i);
+
 	  rc = 1;
 	  if (debug_hw_points)
 	    i386_show_dr (state, "watchpoint_hit", addr, -1, hw_write);
Index: src/gdb/gdbserver/i386-low.h
===================================================================
--- src.orig/gdb/gdbserver/i386-low.h	2010-08-24 20:49:45.000000000 +0100
+++ src/gdb/gdbserver/i386-low.h	2010-08-24 21:28:43.000000000 +0100
@@ -42,7 +42,10 @@
 struct i386_debug_reg_state
 {
   /* Mirror the inferior's DRi registers.  We keep the status and
-     control registers separated because they don't hold addresses.  */
+     control registers separated because they don't hold addresses.
+     Note that since we can change these mirrors while threads are
+     running, we never trust them to explain a cause of a SIGTRAP.
+     For that, we need to peek directly in the inferior registers.  */
   CORE_ADDR dr_mirror[DR_NADDR];
   unsigned dr_status_mirror, dr_control_mirror;
 
@@ -100,9 +103,14 @@ extern int i386_low_stopped_by_watchpoin
 extern void i386_dr_low_set_addr (const struct i386_debug_reg_state *state,
 				  int regnum);
 
+/* Return the inferior's debug register REGNUM.  */
+extern CORE_ADDR i386_dr_low_get_addr (int regnum);
+
 /* Update the inferior's DR7 debug control register from STATE.  */
 extern void i386_dr_low_set_control (const struct i386_debug_reg_state *state);
 
-/* Get the value of the inferior's DR6 debug status register
-   and record it in STATE.  */
-extern void i386_dr_low_get_status (struct i386_debug_reg_state *state);
+/* Return the value of the inferior's DR7 debug control register.  */
+extern unsigned i386_dr_low_get_control (void);
+
+/* Return the value of the inferior's DR6 debug status register.  */
+extern unsigned i386_dr_low_get_status (void);
Index: src/gdb/gdbserver/win32-i386-low.c
===================================================================
--- src.orig/gdb/gdbserver/win32-i386-low.c	2010-08-24 20:49:45.000000000 +0100
+++ src/gdb/gdbserver/win32-i386-low.c	2010-08-24 21:28:43.000000000 +0100
@@ -37,6 +37,8 @@ void init_registers_i386 (void);
 #endif
 
 static struct i386_debug_reg_state debug_reg_state;
+static unsigned dr_status_mirror;
+static unsigned dr_control_mirror;
 
 static int debug_registers_changed = 0;
 static int debug_registers_used = 0;
@@ -56,6 +58,14 @@ i386_dr_low_set_addr (const struct i386_
   debug_registers_used = 1;
 }
 
+CORE_ADDR
+i386_dr_low_get_addr (int regnum)
+{
+  gdb_assert (DR_FIRSTADDR <= regnum && regnum < DR_LASTADDR);
+
+  return debug_reg_state.dr_mirror[regnum];
+}
+
 /* Update the inferior's DR7 debug control register from STATE.  */
 
 void
@@ -68,14 +78,21 @@ i386_dr_low_set_control (const struct i3
   debug_registers_used = 1;
 }
 
+unsigned
+i386_dr_low_get_control (void)
+{
+  return dr_control_mirror;
+}
+
 /* Get the value of the DR6 debug status register from the inferior
    and record it in STATE.  */
 
-void
-i386_dr_low_get_status (struct i386_debug_reg_state *state)
+unsigned
+i386_dr_low_get_status (void)
 {
   /* We don't need to do anything here, the last call to thread_rec for
      current_event.dwThreadId id has already set it.  */
+  return dr_status_mirror;
 }
 
 /* Watchpoint support.  */
@@ -133,6 +150,8 @@ i386_initial_stuff (void)
   i386_low_init_dregs (&debug_reg_state);
   debug_registers_changed = 0;
   debug_registers_used = 0;
+  dr_status_mirror = 0;
+  dr_control_mirror = 0;
 }
 
 static void
@@ -171,8 +190,8 @@ i386_get_thread_context (win32_thread_in
       dr->dr_mirror[1] = th->context.Dr1;
       dr->dr_mirror[2] = th->context.Dr2;
       dr->dr_mirror[3] = th->context.Dr3;
-      dr->dr_status_mirror = th->context.Dr6;
-      dr->dr_control_mirror = th->context.Dr7;
+      dr_status_mirror = th->context.Dr6;
+      dr_control_mirror = th->context.Dr7;
     }
 }
 
@@ -186,9 +205,9 @@ i386_set_thread_context (win32_thread_in
       th->context.Dr1 = dr->dr_mirror[1];
       th->context.Dr2 = dr->dr_mirror[2];
       th->context.Dr3 = dr->dr_mirror[3];
-      /* th->context.Dr6 = dr->dr_status_mirror;
+      /* th->context.Dr6 = dr_status_mirror;
 	 FIXME: should we set dr6 also ?? */
-      th->context.Dr7 = dr->dr_control_mirror;
+      th->context.Dr7 = dr_control_mirror;
     }
 
   SetThreadContext (th->h, &th->context);
@@ -208,9 +227,9 @@ i386_thread_added (win32_thread_info *th
       th->context.Dr1 = dr->dr_mirror[1];
       th->context.Dr2 = dr->dr_mirror[2];
       th->context.Dr3 = dr->dr_mirror[3];
-      /* th->context.Dr6 = dr->dr_status_mirror;
+      /* th->context.Dr6 = dr_status_mirror;
 	 FIXME: should we set dr6 also ?? */
-      th->context.Dr7 = dr->dr_control_mirror;
+      th->context.Dr7 = dr_control_mirror;
 
       SetThreadContext (th->h, &th->context);
       th->context.ContextFlags = 0;


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]