RFC: gdbserver crash on win XP during watchpoint removal

From: Pedro Alves <palves@redhat.com>

  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(-)

RFC: gdbserver crash on win XP during watchpoint removal

Commit Message

Comments

Patch