From a2abc7de6804e7e9882a86375767b24a6c215f28 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Wed, 15 Oct 2014 19:55:50 +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. Joel tested it on x86-windows using AdaCore's testsuite. gdb/gdbserver/ 2014-10-15 Pedro Alves PR server/17487 * 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) : 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/ChangeLog | 41 ++++++++++ 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 ++-- 5 files changed, 175 insertions(+), 101 deletions(-) diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog index ae028616f88..0940fa0358f 100644 --- a/gdb/gdbserver/ChangeLog +++ b/gdb/gdbserver/ChangeLog @@ -1,3 +1,44 @@ +2014-10-15 Pedro Alves + + PR server/17487 + * 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) + : 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. + 2014-10-08 Gary Benson * server.h: Do not include common-exceptions.h. diff --git a/gdb/gdbserver/win32-arm-low.c b/gdb/gdbserver/win32-arm-low.c index cf64514bf2b..f4667ff4859 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 b4b99e87e10..6d6b3104b85 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 ee99fe4126a..e714933eb01 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, ¤t_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, ¤t_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 4e84957b9f3..4acd81cf16d 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 changed 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. -- 2.30.2