RFC: gdbserver crash on win XP during watchpoint removal

RFC: gdbserver crash on win XP during watchpoint removal

[Joel Brobecker]

Hello,

I started investigating an issue that seems to only show up on Windows
XP, and only when debugging via GDBserver. I used the following code,
which matches the code in testsuite/gdb.ada/int_deref:

    with Pck;

    procedure Foo is
    begin
       Pck.Watch := Pck.Watch + 1;
    end Foo;

The test just breaks on line 5, just before the increment, inserts
a watchhpoint on it, and then continues:

    (gdb) b foo.adb:5
    Breakpoint 1 at 0x4017c2: file foo.adb, line 5.
    (gdb) cont
    Continuing.

    Breakpoint 1, foo () at foo.adb:5
    5          Pck.Watch := Pck.Watch + 1;
    (gdb) watch watch
    Hardware watchpoint 2: watch
    (gdb) c
    Continuing.
    Remote communication error.  Target disconnected.: Invalid argument.

The immediate cause for the communication error is easily explained,
gdbserver crashes due to a failed assertion:

    x86_remove_aligned_watchpoint: Assertion `state->dr_control_mirror == 0' failed.

I think the assertion might be invalid, in this case. Turning debug
register traces on, I see:

insert_watchpoint (addr=000000000041c010, len=4, type=data-write):
        CONTROL (DR7): 00000000000d0101          STATUS (DR6): 0000000000000000
        DR0: addr=0x0041c010, ref.count=1  DR1: addr=0x00000000, ref.count=0
        DR2: addr=0x00000000, ref.count=0  DR3: addr=0x00000000, ref.count=0
stopped_data_addr:
        CONTROL (DR7): 00000000000d0501          STATUS (DR6): 00000000ffff4ff0
        DR0: addr=0x0041c010, ref.count=1  DR1: addr=0x00000000, ref.count=0
        DR2: addr=0x00000000, ref.count=0  DR3: addr=0x00000000, ref.count=0
watchpoint_hit (addr=000000000041c010, len=-1, type=data-write):
        CONTROL (DR7): 00000000000d0501          STATUS (DR6): 00000000ffff4ff1
        DR0: addr=0x0041c010, ref.count=1  DR1: addr=0x00000000, ref.count=0
        DR2: addr=0x00000000, ref.count=0  DR3: addr=0x00000000, ref.count=0
watchpoint_hit (addr=000000000041c010, len=-1, type=data-write):
        CONTROL (DR7): 00000000000d0501          STATUS (DR6): 00000000ffff4ff1
        DR0: addr=0x0041c010, ref.count=1  DR1: addr=0x00000000, ref.count=0
        DR2: addr=0x00000000, ref.count=0  DR3: addr=0x00000000, ref.count=0

What's interesting is the value of DR7, which suddenly gets an extra
0x400. So when we unset the bits we set before, we still that that
extra bit on the 10th bit.

But looking at Intel documentation I have (IA-32 Intel Architecture
Software Developer's Manual - Volume 3), it says that bits 12-11-10
are 001.

I'm tempted to either clear those bits when reading the register value,
or else to mask those bits in the assertion. I feel that masking
those bits would be cleaner, since we'd carry a register value which
better matches reality. But the fact that this is working elsewhere
makes me wonder if we might actually be relying elsewhere on those bits
being zero, or maybe at least assuming that they are.

Any thoughts before I send a patch?

Thanks!
-- 
Joel

Re: RFC: gdbserver crash on win XP during watchpoint removal

[Pedro Alves]

On 10/14/2014 07:46 PM, Joel Brobecker wrote:
> Hello,
> 
> I started investigating an issue that seems to only show up on Windows
> XP, and only when debugging via GDBserver. I used the following code,
> which matches the code in testsuite/gdb.ada/int_deref:
> 
>     with Pck;
> 
>     procedure Foo is
>     begin
>        Pck.Watch := Pck.Watch + 1;
>     end Foo;
> 
> The test just breaks on line 5, just before the increment, inserts
> a watchhpoint on it, and then continues:
> 
>     (gdb) b foo.adb:5
>     Breakpoint 1 at 0x4017c2: file foo.adb, line 5.
>     (gdb) cont
>     Continuing.
> 
>     Breakpoint 1, foo () at foo.adb:5
>     5          Pck.Watch := Pck.Watch + 1;
>     (gdb) watch watch
>     Hardware watchpoint 2: watch
>     (gdb) c
>     Continuing.
>     Remote communication error.  Target disconnected.: Invalid argument.
> 
> The immediate cause for the communication error is easily explained,
> gdbserver crashes due to a failed assertion:
> 
>     x86_remove_aligned_watchpoint: Assertion `state->dr_control_mirror == 0' failed.
> 
> I think the assertion might be invalid, in this case. Turning debug
> register traces on, I see:

dr_control_mirror is never supposed to be updated from the target.  It's a
one way street - the state we _want_ dr7 to have on next resume.  Where is this
0x400 coming from then?

Also, what's different in native GDB?  This debug registers code is
shared between GDB and GDBserver now.

Thanks,
Pedro Alves

> 
> insert_watchpoint (addr=000000000041c010, len=4, type=data-write):
>         CONTROL (DR7): 00000000000d0101          STATUS (DR6): 0000000000000000
>         DR0: addr=0x0041c010, ref.count=1  DR1: addr=0x00000000, ref.count=0
>         DR2: addr=0x00000000, ref.count=0  DR3: addr=0x00000000, ref.count=0
> stopped_data_addr:
>         CONTROL (DR7): 00000000000d0501          STATUS (DR6): 00000000ffff4ff0
>         DR0: addr=0x0041c010, ref.count=1  DR1: addr=0x00000000, ref.count=0
>         DR2: addr=0x00000000, ref.count=0  DR3: addr=0x00000000, ref.count=0
> watchpoint_hit (addr=000000000041c010, len=-1, type=data-write):
>         CONTROL (DR7): 00000000000d0501          STATUS (DR6): 00000000ffff4ff1
>         DR0: addr=0x0041c010, ref.count=1  DR1: addr=0x00000000, ref.count=0
>         DR2: addr=0x00000000, ref.count=0  DR3: addr=0x00000000, ref.count=0
> watchpoint_hit (addr=000000000041c010, len=-1, type=data-write):
>         CONTROL (DR7): 00000000000d0501          STATUS (DR6): 00000000ffff4ff1
>         DR0: addr=0x0041c010, ref.count=1  DR1: addr=0x00000000, ref.count=0
>         DR2: addr=0x00000000, ref.count=0  DR3: addr=0x00000000, ref.count=0
> 
> What's interesting is the value of DR7, which suddenly gets an extra
> 0x400. So when we unset the bits we set before, we still that that
> extra bit on the 10th bit.
> 
> But looking at Intel documentation I have (IA-32 Intel Architecture
> Software Developer's Manual - Volume 3), it says that bits 12-11-10
> are 001.
> 
> I'm tempted to either clear those bits when reading the register value,
> or else to mask those bits in the assertion. I feel that masking
> those bits would be cleaner, since we'd carry a register value which
> better matches reality. But the fact that this is working elsewhere
> makes me wonder if we might actually be relying elsewhere on those bits
> being zero, or maybe at least assuming that they are.
> 
> Any thoughts before I send a patch?

Re: RFC: gdbserver crash on win XP during watchpoint removal

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 2263 bytes --]

> dr_control_mirror is never supposed to be updated from the target.  It's a
> one way street - the state we _want_ dr7 to have on next resume.  Where is this
> 0x400 coming from then?
> 
> Also, what's different in native GDB?  This debug registers code is
> shared between GDB and GDBserver now.

It is read back in win32-i386-low.c::i386_get_thread_context:

  if (th->tid == current_event->dwThreadId)
    {
      /* Copy dr values from the current thread.  */
      struct x86_debug_reg_state *dr = &debug_reg_state;
      dr->dr_mirror[0] = th->context.Dr0;
      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;
    }

debug_reg_state is a global, which is passed to x86_dr_insert_watchpoint
in win32-i386-low.c::i386_insert_point:

        return x86_dr_insert_watchpoint (&debug_reg_state,
                                         hw_type, addr, size);

i386_get_thread_context gets called after we receive the watchpoint
signal (TARGET_WAITKIND_STOPPED) in win32_wait:

        case TARGET_WAITKIND_STOPPED:
        case TARGET_WAITKIND_LOADED:
          OUTMSG2 (("Child Stopped with signal = %d \n",
                    ourstatus->value.sig));

          regcache = get_thread_regcache (current_thread, 1);
          child_fetch_inferior_registers (regcache, -1);
          return debug_event_ptid (&current_event);

It looks like just removing the statement that sets dr_control_mirror
from the thread's DR7 is sufficient to allow hardware watchpoints
to work again.

gdb/gdbserver/ChangeLog:

        * win32-i386-low.c (i386_get_thread_context): Do not set
        dr->dr_control_mirror.

Tested on x86-windows using AdaCore's testsuite. No regression while
all watchpoint-related issues get resolved.

I admit I could have spent a little more time trying to understand
the bigger picture, but I'm a bit under time pressure, so I'm hoping
this is enough. Otherwise, I'll look at a better fix.

Once we have a fix, I think we'll want it for GDB 7.8.1. I haven't
completely checked eitherr, but I think it's there as well. I'll create
a PR as soon as I have a minute.

Thanks!
-- 
Joel

[-- Attachment #2: 0001-state-dr_control_mirror-0-failed-assertion-in-gdbser.patch --]
[-- Type: text/x-diff, Size: 2928 bytes --]

From 8f41c96eda3e46363d6e68e9fc0179e24d0c8922 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Tue, 14 Oct 2014 23:18:35 +0200
Subject: [PATCH] state->dr_control_mirror == 0 failed assertion in gdbserver
 on Windows XP

When using GDBserver on Windows XP, GDBserver reports and assertion
failure after hitting a hardware watchpoint. The problem was reproduced
using the sources from gdb.ada/int_deref, but should probably reproduce
with any scenario involving hardware watchpoints.

In our scenario, we break on line 5, just before the increment, insert
a watchhpoint on it, and then continue:

    (gdb) b foo.adb:5
    Breakpoint 1 at 0x4017c2: file foo.adb, line 5.
    (gdb) cont
    Continuing.

    Breakpoint 1, foo () at foo.adb:5
    5          Pck.Watch := Pck.Watch + 1;
    (gdb) watch watch
    Hardware watchpoint 2: watch
    (gdb) c
    Continuing.
    Remote communication error.  Target disconnected.: Invalid argument.

The immediate cause for the communication error is easily explained,
gdbserver crashes due to a failed assertion:

    x86_remove_aligned_watchpoint: Assertion `state->dr_control_mirror == 0' failed.

The assertion occurs because debug_reg_state.dr_control_mirror gets
overwritten by the value read from the inferior, when processing
the watchpoint event in win32_wait: win32_wait finds that we stopped,
calls get_thread_regcache which causes i386_get_thread_context to
get called, which then...

  if (th->tid == current_event->dwThreadId)
    {
      /* Copy dr values from the current thread.  */
      struct x86_debug_reg_state *dr = &debug_reg_state;
      [...]
      dr->dr_control_mirror = th->context.Dr7;
    }

Both should be identical, normally making this a no-op, but
it turns out that bits 12-11-10 are documented as being fixed
and equal to 001. Our handling of dr_control_mirror does not
manage those bits, and leaves them as zeros instead. So, when
we overwrite the value from the thread's DR7 register, we
accidentally set bit 10, causing state->dr_control_mirror
to be 0x400 after we've cleared everything internally.

This patch fixes the issue by removing the statement setting
state->dr_control_mirror to the thread's DR7 register value.

gdb/gdbserver/ChangeLog:

        * win32-i386-low.c (i386_get_thread_context): Do not set
        dr->dr_control_mirror.
---
 gdb/gdbserver/win32-i386-low.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/gdb/gdbserver/win32-i386-low.c b/gdb/gdbserver/win32-i386-low.c
index b4b99e8..3cd4b97 100644
--- a/gdb/gdbserver/win32-i386-low.c
+++ b/gdb/gdbserver/win32-i386-low.c
@@ -222,7 +222,6 @@ i386_get_thread_context (win32_thread_info *th, DEBUG_EVENT* current_event)
       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;
     }
 }
 
-- 
1.7.9

Re: RFC: gdbserver crash on win XP during watchpoint removal

[Pedro Alves]

On 10/14/2014 10:37 PM, Joel Brobecker wrote:
>> dr_control_mirror is never supposed to be updated from the target.  It's a
>> one way street - the state we _want_ dr7 to have on next resume.  Where is this
>> 0x400 coming from then?
>>
>> Also, what's different in native GDB?  This debug registers code is
>> shared between GDB and GDBserver now.
> 
> It is read back in win32-i386-low.c::i386_get_thread_context:
> 
>   if (th->tid == current_event->dwThreadId)
>     {
>       /* Copy dr values from the current thread.  */
>       struct x86_debug_reg_state *dr = &debug_reg_state;
>       dr->dr_mirror[0] = th->context.Dr0;
>       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;
>     }
> 
> debug_reg_state is a global, which is passed to x86_dr_insert_watchpoint
> in win32-i386-low.c::i386_insert_point:
> 
>         return x86_dr_insert_watchpoint (&debug_reg_state,
>                                          hw_type, addr, size);
> 
> i386_get_thread_context gets called after we receive the watchpoint
> signal (TARGET_WAITKIND_STOPPED) in win32_wait:
> 
>         case TARGET_WAITKIND_STOPPED:
>         case TARGET_WAITKIND_LOADED:
>           OUTMSG2 (("Child Stopped with signal = %d \n",
>                     ourstatus->value.sig));
> 
>           regcache = get_thread_regcache (current_thread, 1);
>           child_fetch_inferior_registers (regcache, -1);
>           return debug_event_ptid (&current_event);
> 
> It looks like just removing the statement that sets dr_control_mirror
> from the thread's DR7 is sufficient to allow hardware watchpoints
> to work again.

Yes, that sounds good enough for 7.8.1.  This is OK.

> I admit I could have spent a little more time trying to understand
> the bigger picture, but I'm a bit under time pressure, so I'm hoping
> this is enough. Otherwise, I'll look at a better fix.

Hmm, I've wanted to do this for a while.  So...  Below's is what I
think is the better fix.

------ 8< ---------
From 64451d0e90e363c86539236aa3b1761ca2eff0a7 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 14 Oct 2014 22:52:41 +0100
Subject: [PATCH] gdbserver/win32: Rewrite debug registers handling

Don't use debug_reg_state for both:

 * "intent" - what we want the debug registers to look like

 * "reality" - what/which were the contents of the DR registers when
   the event triggered

Reserve it for the former only, like in the GNU/Linux port.

Otherwise the core x86 debug registers code can get confused if the
inferior itself changes the debug registers since GDB last set them.

This is also a requirement for being able to set watchpoints while the
target is running, if/when we get to it on Windows.  See the big
comment in x86_dr_stopped_data_address.

Seems to me this may also fixes propagating watchpoints to all threads
-- continue_one_thread only calls win32_set_thread_context (what
copies the DR registers to the thread), if something already fetched
the thread's context before.  Something else may be masking this
issue, I haven't checked.

Smoke tested by running gdbserver under Wine, connecting to it from
GNU/Linux, and checking that I could trigger a watchpoint as expected.

gdb/
2014-10-14  Pedro Alves  <palves@redhat.com>

	* win32-arm-low.c (arm_set_thread_context): Remove current_event
	parameter.
	(arm_set_thread_context): Delete.
	(the_low_target): Adjust.
	* win32-i386-low.c (debug_registers_changed)
	(debug_registers_used): Delete.
	(update_debug_registers_callback): New function.
	(x86_dr_low_set_addr, x86_dr_low_set_control): Mark all threads as
	needing to update their debug registers.
	(win32_get_current_dr): New function.
	(x86_dr_low_get_addr, x86_dr_low_get_control)
	(x86_dr_low_get_status): Fetch the debug register from the thread
	record's context.
	(i386_initial_stuff): Adjust.
	(i386_get_thread_context): Remove current_event parameter.  Don't
	clear debug_registers_changed nor copy DR values to
	debug_reg_state.
	(i386_set_thread_context): Delete.
	(i386_prepare_to_resume): New function.
	(i386_thread_added): Mark the thread as needing to update irs
	debug registers.
	(the_low_target): Remove i386_set_thread_context and install
	i386_prepare_to_resume.
	* win32-low.c (win32_get_thread_context): Adjust.
	(win32_set_thread_context): Use SetThreadContext
	directly.
	(win32_prepare_to_resume): New function.
	(win32_require_context): New function, factored out from ...
	(thread_rec): ... this.
	(continue_one_thread): Call win32_prepare_to_resume on each thread
	we're about to continue.
	(win32_resume): Call win32_prepare_to_resume on the event thread.
	* win32-low.h (struct win32_thread_info)
	<debug_registers_changed>: New field.
	(struct win32_target_ops): Change prototype of set_thread_context,
	delete set_thread_context and add prepare_to_resume.
	(win32_require_context): New declaration.
---
 gdb/gdbserver/win32-arm-low.c  |  10 +--
 gdb/gdbserver/win32-i386-low.c | 137 ++++++++++++++++++++++-------------------
 gdb/gdbserver/win32-low.c      |  73 ++++++++++++++--------
 gdb/gdbserver/win32-low.h      |  15 +++--
 4 files changed, 134 insertions(+), 101 deletions(-)

diff --git a/gdb/gdbserver/win32-arm-low.c b/gdb/gdbserver/win32-arm-low.c
index cf64514..f4667ff 100644
--- a/gdb/gdbserver/win32-arm-low.c
+++ b/gdb/gdbserver/win32-arm-low.c
@@ -27,7 +27,7 @@ void init_registers_arm (void);
 extern const struct target_desc *tdesc_arm;
 
 static void
-arm_get_thread_context (win32_thread_info *th, DEBUG_EVENT* current_event)
+arm_get_thread_context (win32_thread_info *th)
 {
   th->context.ContextFlags = \
     CONTEXT_FULL | \
@@ -36,12 +36,6 @@ arm_get_thread_context (win32_thread_info *th, DEBUG_EVENT* current_event)
   GetThreadContext (th->h, &th->context);
 }
 
-static void
-arm_set_thread_context (win32_thread_info *th, DEBUG_EVENT* current_event)
-{
-  SetThreadContext (th->h, &th->context);
-}
-
 #define context_offset(x) ((int)&(((CONTEXT *)NULL)->x))
 static const int mappings[] = {
   context_offset (R0),
@@ -124,7 +118,7 @@ struct win32_target_ops the_low_target = {
   sizeof (mappings) / sizeof (mappings[0]),
   NULL, /* initial_stuff */
   arm_get_thread_context,
-  arm_set_thread_context,
+  NULL, /* prepare_to_resume */
   NULL, /* thread_added */
   arm_fetch_inferior_register,
   arm_store_inferior_register,
diff --git a/gdb/gdbserver/win32-i386-low.c b/gdb/gdbserver/win32-i386-low.c
index b4b99e8..6d6b310 100644
--- a/gdb/gdbserver/win32-i386-low.c
+++ b/gdb/gdbserver/win32-i386-low.c
@@ -40,29 +40,36 @@ extern const struct target_desc *tdesc_i386;
 
 static struct x86_debug_reg_state debug_reg_state;
 
-static int debug_registers_changed = 0;
-static int debug_registers_used = 0;
+static int
+update_debug_registers_callback (struct inferior_list_entry *entry,
+				 void *pid_p)
+{
+  struct thread_info *thr = (struct thread_info *) entry;
+  win32_thread_info *th = inferior_target_data (thr);
+  int pid = *(int *) pid_p;
+
+  /* Only update the threads of this process.  */
+  if (pid_of (thr) == pid)
+    {
+      /* The actual update is done later just before resuming the lwp,
+	 we just mark that the registers need updating.  */
+      th->debug_registers_changed = 1;
+    }
+
+  return 0;
+}
 
 /* Update the inferior's debug register REGNUM from STATE.  */
 
 static void
 x86_dr_low_set_addr (int regnum, CORE_ADDR addr)
 {
-  gdb_assert (DR_FIRSTADDR <= regnum && regnum <= DR_LASTADDR);
-
-  /* debug_reg_state.dr_mirror is already set.
-     Just notify i386_set_thread_context, i386_thread_added
-     that the registers need to be updated.  */
-  debug_registers_changed = 1;
-  debug_registers_used = 1;
-}
+  /* Only update the threads of this process.  */
+  int pid = pid_of (current_thread);
 
-static CORE_ADDR
-x86_dr_low_get_addr (int regnum)
-{
   gdb_assert (DR_FIRSTADDR <= regnum && regnum <= DR_LASTADDR);
 
-  return debug_reg_state.dr_mirror[regnum];
+  find_inferior (&all_threads, update_debug_registers_callback, &pid);
 }
 
 /* Update the inferior's DR7 debug control register from STATE.  */
@@ -70,17 +77,53 @@ x86_dr_low_get_addr (int regnum)
 static void
 x86_dr_low_set_control (unsigned long control)
 {
-  /* debug_reg_state.dr_control_mirror is already set.
-     Just notify i386_set_thread_context, i386_thread_added
-     that the registers need to be updated.  */
-  debug_registers_changed = 1;
-  debug_registers_used = 1;
+  /* Only update the threads of this process.  */
+  int pid = pid_of (current_thread);
+
+  find_inferior (&all_threads, update_debug_registers_callback, &pid);
+}
+
+/* Return the current value of a DR register of the current thread's
+   context.  */
+
+static DWORD64
+win32_get_current_dr (int dr)
+{
+  win32_thread_info *th = inferior_target_data (current_thread);
+
+  win32_require_context (th);
+
+#define RET_DR(DR)				\
+  case DR:					\
+    return th->context.Dr ## DR
+
+  switch (dr)
+    {
+      RET_DR (0);
+      RET_DR (1);
+      RET_DR (2);
+      RET_DR (3);
+      RET_DR (6);
+      RET_DR (7);
+    }
+
+#undef RET_DR
+
+  gdb_assert_not_reached ("unhandled dr");
+}
+
+static CORE_ADDR
+x86_dr_low_get_addr (int regnum)
+{
+  gdb_assert (DR_FIRSTADDR <= regnum && regnum <= DR_LASTADDR);
+
+  return win32_get_current_dr (regnum - DR_FIRSTADDR);
 }
 
 static unsigned long
 x86_dr_low_get_control (void)
 {
-  return debug_reg_state.dr_control_mirror;
+  return win32_get_current_dr (7);
 }
 
 /* Get the value of the DR6 debug status register from the inferior
@@ -89,9 +132,7 @@ x86_dr_low_get_control (void)
 static unsigned long
 x86_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 debug_reg_state.dr_status_mirror;
+  return win32_get_current_dr (6);
 }
 
 /* Low-level function vector.  */
@@ -181,12 +222,10 @@ static void
 i386_initial_stuff (void)
 {
   x86_low_init_dregs (&debug_reg_state);
-  debug_registers_changed = 0;
-  debug_registers_used = 0;
 }
 
 static void
-i386_get_thread_context (win32_thread_info *th, DEBUG_EVENT* current_event)
+i386_get_thread_context (win32_thread_info *th)
 {
   /* Requesting the CONTEXT_EXTENDED_REGISTERS register set fails if
      the system doesn't support extended registers.  */
@@ -210,28 +249,17 @@ i386_get_thread_context (win32_thread_info *th, DEBUG_EVENT* current_event)
 
       error ("GetThreadContext failure %ld\n", (long) e);
     }
-
-  debug_registers_changed = 0;
-
-  if (th->tid == current_event->dwThreadId)
-    {
-      /* Copy dr values from the current thread.  */
-      struct x86_debug_reg_state *dr = &debug_reg_state;
-      dr->dr_mirror[0] = th->context.Dr0;
-      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;
-    }
 }
 
 static void
-i386_set_thread_context (win32_thread_info *th, DEBUG_EVENT* current_event)
+i386_prepare_to_resume (win32_thread_info *th)
 {
-  if (debug_registers_changed)
+  if (th->debug_registers_changed)
     {
       struct x86_debug_reg_state *dr = &debug_reg_state;
+
+      win32_require_context (th);
+
       th->context.Dr0 = dr->dr_mirror[0];
       th->context.Dr1 = dr->dr_mirror[1];
       th->context.Dr2 = dr->dr_mirror[2];
@@ -239,32 +267,15 @@ i386_set_thread_context (win32_thread_info *th, DEBUG_EVENT* current_event)
       /* th->context.Dr6 = dr->dr_status_mirror;
 	 FIXME: should we set dr6 also ?? */
       th->context.Dr7 = dr->dr_control_mirror;
-    }
 
-  SetThreadContext (th->h, &th->context);
+      th->debug_registers_changed = 0;
+    }
 }
 
 static void
 i386_thread_added (win32_thread_info *th)
 {
-  /* Set the debug registers for the new thread if they are used.  */
-  if (debug_registers_used)
-    {
-      struct x86_debug_reg_state *dr = &debug_reg_state;
-      th->context.ContextFlags = CONTEXT_DEBUG_REGISTERS;
-      GetThreadContext (th->h, &th->context);
-
-      th->context.Dr0 = dr->dr_mirror[0];
-      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;
-	 FIXME: should we set dr6 also ?? */
-      th->context.Dr7 = dr->dr_control_mirror;
-
-      SetThreadContext (th->h, &th->context);
-      th->context.ContextFlags = 0;
-    }
+  th->debug_registers_changed = 1;
 }
 
 static void
@@ -450,7 +461,7 @@ struct win32_target_ops the_low_target = {
   sizeof (mappings) / sizeof (mappings[0]),
   i386_initial_stuff,
   i386_get_thread_context,
-  i386_set_thread_context,
+  i386_prepare_to_resume,
   i386_thread_added,
   i386_fetch_inferior_register,
   i386_store_inferior_register,
diff --git a/gdb/gdbserver/win32-low.c b/gdb/gdbserver/win32-low.c
index ee99fe4..e714933 100644
--- a/gdb/gdbserver/win32-low.c
+++ b/gdb/gdbserver/win32-low.c
@@ -129,7 +129,7 @@ static void
 win32_get_thread_context (win32_thread_info *th)
 {
   memset (&th->context, 0, sizeof (CONTEXT));
-  (*the_low_target.get_thread_context) (th, &current_event);
+  (*the_low_target.get_thread_context) (th);
 #ifdef _WIN32_WCE
   memcpy (&th->base_context, &th->context, sizeof (CONTEXT));
 #endif
@@ -153,23 +153,24 @@ win32_set_thread_context (win32_thread_info *th)
      it between stopping and resuming.  */
   if (memcmp (&th->context, &th->base_context, sizeof (CONTEXT)) != 0)
 #endif
-    (*the_low_target.set_thread_context) (th, &current_event);
+    SetThreadContext (th->h, &th->context);
 }
 
-/* Find a thread record given a thread id.  If GET_CONTEXT is set then
-   also retrieve the context for this thread.  */
-static win32_thread_info *
-thread_rec (ptid_t ptid, int get_context)
+/* Set the thread context of the thread associated with TH.  */
+
+static void
+win32_prepare_to_resume (win32_thread_info *th)
 {
-  struct thread_info *thread;
-  win32_thread_info *th;
+  if (the_low_target.prepare_to_resume != NULL)
+    (*the_low_target.prepare_to_resume) (th);
+}
 
-  thread = (struct thread_info *) find_inferior_id (&all_threads, ptid);
-  if (thread == NULL)
-    return NULL;
+/* See win32-low.h.  */
 
-  th = inferior_target_data (thread);
-  if (get_context && th->context.ContextFlags == 0)
+void
+win32_require_context (win32_thread_info *th)
+{
+  if (th->context.ContextFlags == 0)
     {
       if (!th->suspended)
 	{
@@ -185,7 +186,23 @@ thread_rec (ptid_t ptid, int get_context)
 
       win32_get_thread_context (th);
     }
+}
 
+/* Find a thread record given a thread id.  If GET_CONTEXT is set then
+   also retrieve the context for this thread.  */
+static win32_thread_info *
+thread_rec (ptid_t ptid, int get_context)
+{
+  struct thread_info *thread;
+  win32_thread_info *th;
+
+  thread = (struct thread_info *) find_inferior_id (&all_threads, ptid);
+  if (thread == NULL)
+    return NULL;
+
+  th = inferior_target_data (thread);
+  if (get_context)
+    win32_require_context (th);
   return th;
 }
 
@@ -419,22 +436,26 @@ continue_one_thread (struct inferior_list_entry *this_thread, void *id_ptr)
   int thread_id = * (int *) id_ptr;
   win32_thread_info *th = inferior_target_data (thread);
 
-  if ((thread_id == -1 || thread_id == th->tid)
-      && th->suspended)
+  if (thread_id == -1 || thread_id == th->tid)
     {
-      if (th->context.ContextFlags)
-	{
-	  win32_set_thread_context (th);
-	  th->context.ContextFlags = 0;
-	}
+      win32_prepare_to_resume (th);
 
-      if (ResumeThread (th->h) == (DWORD) -1)
+      if (th->suspended)
 	{
-	  DWORD err = GetLastError ();
-	  OUTMSG (("warning: ResumeThread failed in continue_one_thread, "
-		   "(error %d): %s\n", (int) err, strwinerror (err)));
+	  if (th->context.ContextFlags)
+	    {
+	      win32_set_thread_context (th);
+	      th->context.ContextFlags = 0;
+	    }
+
+	  if (ResumeThread (th->h) == (DWORD) -1)
+	    {
+	      DWORD err = GetLastError ();
+	      OUTMSG (("warning: ResumeThread failed in continue_one_thread, "
+		       "(error %d): %s\n", (int) err, strwinerror (err)));
+	    }
+	  th->suspended = 0;
 	}
-      th->suspended = 0;
     }
 
   return 0;
@@ -937,6 +958,8 @@ win32_resume (struct thread_resume *resume_info, size_t n)
   th = thread_rec (ptid, FALSE);
   if (th)
     {
+      win32_prepare_to_resume (th);
+
       if (th->context.ContextFlags)
 	{
 	  /* Move register values from the inferior into the thread
diff --git a/gdb/gdbserver/win32-low.h b/gdb/gdbserver/win32-low.h
index 4e84957..f4422ed 100644
--- a/gdb/gdbserver/win32-low.h
+++ b/gdb/gdbserver/win32-low.h
@@ -47,6 +47,10 @@ typedef struct win32_thread_info
 
   /* The context of the thread, including any manipulations.  */
   CONTEXT context;
+
+  /* Whether debug registers change since we last set CONTEXT back to
+     the thread.  */
+  int debug_registers_changed;
 } win32_thread_info;
 
 struct win32_target_ops
@@ -61,12 +65,10 @@ struct win32_target_ops
   void (*initial_stuff) (void);
 
   /* Fetch the context from the inferior.  */
-  void (*get_thread_context) (win32_thread_info *th,
-			      DEBUG_EVENT *current_event);
+  void (*get_thread_context) (win32_thread_info *th);
 
-  /* Flush the context back to the inferior.  */
-  void (*set_thread_context) (win32_thread_info *th,
-			      DEBUG_EVENT *current_event);
+  /* Called just before resuming the thread.  */
+  void (*prepare_to_resume) (win32_thread_info *th);
 
   /* Called when a thread was added.  */
   void (*thread_added) (win32_thread_info *th);
@@ -96,6 +98,9 @@ struct win32_target_ops
 
 extern struct win32_target_ops the_low_target;
 
+/* Retrieve the context for this thread, if not already retrieved.  */
+extern void win32_require_context (win32_thread_info *th);
+
 /* Map the Windows error number in ERROR to a locale-dependent error
    message string and return a pointer to it.  Typically, the values
    for ERROR come from GetLastError.
-- 
1.9.3

Re: RFC: gdbserver crash on win XP during watchpoint removal

[Joel Brobecker]

> > It looks like just removing the statement that sets dr_control_mirror
> > from the thread's DR7 is sufficient to allow hardware watchpoints
> > to work again.
> 
> Yes, that sounds good enough for 7.8.1.  This is OK.

Thanks, Pedro. I created PR server/17487 for this, and pushed
the patch on gdb-7.8-branch using that PR.

> > I admit I could have spent a little more time trying to understand
> > the bigger picture, but I'm a bit under time pressure, so I'm hoping
> > this is enough. Otherwise, I'll look at a better fix.
> 
> Hmm, I've wanted to do this for a while.  So...  Below's is what I
> think is the better fix.

Thanks! I tested your patch on x86-windows using AdaCore's testsuite,
and it also fixes the problem, with no regression. Do you want to push
it? I left the PR open pending your patch going in...

-- 
Joel

Re: RFC: gdbserver crash on win XP during watchpoint removal

[Pedro Alves]

On 10/15/2014 03:35 PM, Joel Brobecker wrote:
>>> It looks like just removing the statement that sets dr_control_mirror
>>> from the thread's DR7 is sufficient to allow hardware watchpoints
>>> to work again.
>>
>> Yes, that sounds good enough for 7.8.1.  This is OK.
> 
> Thanks, Pedro. I created PR server/17487 for this, and pushed
> the patch on gdb-7.8-branch using that PR.
> 
>>> I admit I could have spent a little more time trying to understand
>>> the bigger picture, but I'm a bit under time pressure, so I'm hoping
>>> this is enough. Otherwise, I'll look at a better fix.
>>
>> Hmm, I've wanted to do this for a while.  So...  Below's is what I
>> think is the better fix.
> 
> Thanks! I tested your patch on x86-windows using AdaCore's testsuite,
> and it also fixes the problem, with no regression. Do you want to push
> it? 

Awesome, I pushed it in then.

> I left the PR open pending your patch going in...

Thanks,
Pedro Alves