From patchwork Tue Oct 14 21:37:27 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joel Brobecker X-Patchwork-Id: 3228 Received: (qmail 17176 invoked by alias); 14 Oct 2014 21:37: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 17163 invoked by uid 89); 14 Oct 2014 21:37:30 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL, BAYES_00 autolearn=ham version=3.3.2 X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Tue, 14 Oct 2014 21:37:29 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 90BA1116284; Tue, 14 Oct 2014 17:37:27 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id f3hBw5fZu3Hn; Tue, 14 Oct 2014 17:37:27 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 59E9F116277; Tue, 14 Oct 2014 17:37:27 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 8F0E940DC3; Tue, 14 Oct 2014 14:37:27 -0700 (PDT) Date: Tue, 14 Oct 2014 14:37:27 -0700 From: Joel Brobecker To: Pedro Alves Cc: gdb-patches@sourceware.org Subject: Re: RFC: gdbserver crash on win XP during watchpoint removal Message-ID: <20141014213727.GA25846@adacore.com> References: <20141014184647.GB8879@adacore.com> <543D782F.10506@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <543D782F.10506@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) > 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. 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! From 8f41c96eda3e46363d6e68e9fc0179e24d0c8922 Mon Sep 17 00:00:00 2001 From: Joel Brobecker 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