From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3393 invoked by alias); 13 Dec 2011 16:12:54 -0000 Received: (qmail 3370 invoked by uid 22791); 13 Dec 2011 16:12:51 -0000 X-SWARE-Spam-Status: No, hits=-1.6 required=5.0 tests=AWL,BAYES_00,FROM_12LTRDOM X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 13 Dec 2011 16:12:36 +0000 Received: from nat-ies.mentorg.com ([192.94.31.2] helo=EU1-MAIL.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1RaUy3-00059j-L1 from pedro_alves@mentor.com ; Tue, 13 Dec 2011 08:12:35 -0800 Received: from scottsdale.localnet ([172.16.63.104]) by EU1-MAIL.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.1830); Tue, 13 Dec 2011 16:12:33 +0000 From: Pedro Alves To: Jan Kratochvil Subject: Fix PR remote/13492 (Re: [PATCH] PR threads/10729: x86 hw watchpoints and non-stop mode) Date: Tue, 13 Dec 2011 16:21:00 -0000 User-Agent: KMail/1.13.6 (Linux/2.6.38-13-generic; KDE/4.7.2; x86_64; ; ) Cc: gdb-patches@sourceware.org References: <201112051601.59664.pedro@codesourcery.com> <201112122030.25365.pedro@codesourcery.com> <20111212203409.GA28419@host2.jankratochvil.net> In-Reply-To: <20111212203409.GA28419@host2.jankratochvil.net> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201112131612.30315.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-12/txt/msg00408.txt.bz2 On Monday 12 December 2011 20:34:09, Jan Kratochvil wrote: > On Mon, 12 Dec 2011 21:30:25 +0100, Pedro Alves wrote: > > so it seems there's no point in trying to retain all the other > > bits of DR_STATUS. > > I agree and I agree with this patch part. Thanks. I've applied the patch below to gdbserver, which adds these bits we've discussed, and fixes PR13492. > (I do not agree for example with the lwp->stopped_by_watchpoint part as > multiple watchpoints may get hit on one stop which isn't handled well but that > is out of the scope of this discussion.) I'm not 100% certain that is possible, but in any case, I think that even if we handled that, we wouldn't ever get as far as prepare_to_resume before handling all the multiple watchpoints. 2011-12-13 Pedro Alves PR remote/13492 * i386-low.c (i386_low_stopped_data_address): Avoid fetching DR_CONTROL unless necessary. Extend comments. * linux-x86-low.c (x86_linux_prepare_to_resume): Don't write to DR0-3 if not used. If any watchpoint was set, clear DR_STATUS. --- gdb/gdbserver/i386-low.c | 58 +++++++++++++++++++++++++++++++++-------- gdb/gdbserver/linux-x86-low.c | 14 ++++++++-- 2 files changed, 59 insertions(+), 13 deletions(-) Index: src/gdb/gdbserver/i386-low.c =================================================================== --- src.orig/gdb/gdbserver/i386-low.c 2011-07-25 19:59:47.693736296 +0100 +++ src/gdb/gdbserver/i386-low.c 2011-12-13 16:06:49.422184996 +0000 @@ -559,24 +559,60 @@ i386_low_stopped_data_address (struct i3 CORE_ADDR addr = 0; int i; int rc = 0; + /* The current thread's DR_STATUS. We always need to read this to + check whether some watchpoint caused the trap. */ unsigned status; + /* We need DR_CONTROL as well, but only iff DR_STATUS indicates a + data breakpoint trap. Only fetch it when necessary, to avoid an + unnecessary extra syscall when no watchpoint triggered. */ + int control_p = 0; unsigned control; - /* Get the current values the inferior has. If the thread was - running when we last changed watchpoints, the mirror no longer - represents what was set in this LWP's debug registers. */ + /* In non-stop/async, threads can be running while we change the + global dr_mirror (and friends). Say, we set a watchpoint, and + let threads resume. Now, say you delete the watchpoint, or + add/remove watchpoints such that dr_mirror changes while threads + are running. On targets that support non-stop, + inserting/deleting watchpoints updates the global dr_mirror only. + It does not update the real thread's debug registers; that's only + done prior to resume. Instead, if threads are running when the + mirror changes, a temporary and transparent stop on all threads + is forced so they can get their copy of the debug registers + updated on re-resume. Now, say, a thread hit a watchpoint before + having been updated with the new dr_mirror contents, and we + haven't yet handled the corresponding SIGTRAP. If we trusted + dr_mirror below, we'd mistake the real trapped address (from the + last time we had updated debug registers in the thread) with + whatever was currently in dr_mirror. So to fix this, dr_mirror + always represents intention, what we _want_ threads to have in + debug registers. To get at the address and cause of the trap, we + need to read the state the thread still has in its debug + registers. + + In sum, always get the current debug register values the current + thread has, instead of trusting the global mirror. If the thread + was running when we last changed watchpoints, the mirror no + longer represents what was set in this thread's debug + registers. */ status = i386_dr_low_get_status (); - control = i386_dr_low_get_control (); ALL_DEBUG_REGISTERS (i) { - if (I386_DR_WATCH_HIT (status, i) - /* This second condition makes sure DRi is set up for a data - watchpoint, not a hardware breakpoint. The reason is - that GDB doesn't call the target_stopped_data_address - method except for data watchpoints. In other words, I'm - being paranoiac. */ - && I386_DR_GET_RW_LEN (control, i) != 0) + if (!I386_DR_WATCH_HIT (status, i)) + continue; + + if (!control_p) + { + control = i386_dr_low_get_control (); + control_p = 1; + } + + /* This second condition makes sure DRi is set up for a data + watchpoint, not a hardware breakpoint. The reason is that + GDB doesn't call the target_stopped_data_address method + except for data watchpoints. In other words, I'm being + paranoiac. */ + if (I386_DR_GET_RW_LEN (control, i) != 0) { addr = i386_dr_low_get_addr (i); rc = 1; Index: src/gdb/gdbserver/linux-x86-low.c =================================================================== --- src.orig/gdb/gdbserver/linux-x86-low.c 2011-11-22 13:39:14.172391873 +0000 +++ src/gdb/gdbserver/linux-x86-low.c 2011-12-13 16:08:11.862184967 +0000 @@ -655,6 +655,7 @@ static void x86_linux_prepare_to_resume (struct lwp_info *lwp) { ptid_t ptid = ptid_of (lwp); + int clear_status = 0; if (lwp->arch_private->debug_registers_changed) { @@ -665,14 +666,23 @@ x86_linux_prepare_to_resume (struct lwp_ = &proc->private->arch_private->debug_reg_state; for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++) - x86_linux_dr_set (ptid, i, state->dr_mirror[i]); + if (state->dr_ref_count[i] > 0) + { + x86_linux_dr_set (ptid, i, state->dr_mirror[i]); + + /* If we're setting a watchpoint, any change the inferior + had done itself to the debug registers needs to be + discarded, otherwise, i386_low_stopped_data_address can + get confused. */ + clear_status = 1; + } x86_linux_dr_set (ptid, DR_CONTROL, state->dr_control_mirror); lwp->arch_private->debug_registers_changed = 0; } - if (lwp->stopped_by_watchpoint) + if (clear_status || lwp->stopped_by_watchpoint) x86_linux_dr_set (ptid, DR_STATUS, 0); }