From patchwork Wed Oct 15 00:38:23 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 3232 Received: (qmail 20325 invoked by alias); 15 Oct 2014 00:38:31 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 20315 invoked by uid 89); 15 Oct 2014 00:38:31 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL, BAYES_00, SPF_HELO_PASS, SPF_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 15 Oct 2014 00:38:29 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s9F0cQk7003132 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Tue, 14 Oct 2014 20:38:26 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s9F0cN4L016838; Tue, 14 Oct 2014 20:38:24 -0400 Message-ID: <543DC1FF.8030109@redhat.com> Date: Wed, 15 Oct 2014 01:38:23 +0100 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.1 MIME-Version: 1.0 To: Joel Brobecker CC: gdb-patches@sourceware.org Subject: Re: RFC: gdbserver crash on win XP during watchpoint removal References: <20141014184647.GB8879@adacore.com> <543D782F.10506@redhat.com> <20141014213727.GA25846@adacore.com> In-Reply-To: <20141014213727.GA25846@adacore.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 (¤t_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 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 * 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/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, ¤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 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.