From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13710 invoked by alias); 3 Jun 2009 22:58:22 -0000 Received: (qmail 13701 invoked by uid 22791); 3 Jun 2009 22:58:20 -0000 X-SWARE-Spam-Status: No, hits=-1.2 required=5.0 tests=AWL,BAYES_05,J_CHICKENPOX_12 X-Spam-Check-By: sourceware.org Received: from mailhost.u-strasbg.fr (HELO mailhost.u-strasbg.fr) (130.79.200.152) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 03 Jun 2009 22:58:13 +0000 Received: from baal.u-strasbg.fr (baal.u-strasbg.fr [IPv6:2001:660:2402::41]) by mailhost.u-strasbg.fr (8.14.2/jtpda-5.5pre1) with ESMTP id n53Mw9Pr072670 for ; Thu, 4 Jun 2009 00:58:09 +0200 (CEST) Received: from mailserver.u-strasbg.fr (ms1.u-strasbg.fr [IPv6:2001:660:2402:d::10]) by baal.u-strasbg.fr (8.14.0/jtpda-5.5pre1) with ESMTP id n53Mw9lN094022 for ; Thu, 4 Jun 2009 00:58:09 +0200 (CEST) (envelope-from muller@ics.u-strasbg.fr) Received: from d620muller (lec67-4-82-230-53-140.fbx.proxad.net [82.230.53.140]) (user=mullerp mech=LOGIN) by mailserver.u-strasbg.fr (8.14.3/jtpda-5.5pre1) with ESMTP id n53Mw8b6010112 (version=TLSv1/SSLv3 cipher=RC4-MD5 bits=128 verify=NO) for ; Thu, 4 Jun 2009 00:58:09 +0200 (CEST) (envelope-from muller@ics.u-strasbg.fr) From: "Pierre Muller" To: Subject: [RFA] Fix a windows bug if two watchpoints are used Date: Wed, 03 Jun 2009 22:58:00 -0000 Message-ID: <000301c9e49e$c479eba0$4d6dc2e0$@u-strasbg.fr> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit 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: 2009-06/txt/msg00054.txt.bz2 When using watchpoints with cygwin GDB, I sometimes ended up getting a SIGTRAP at the exact location of the watch. I finally figured out a reproducible way of generation this bug. If I set two hardware watchpoints on two integers ival1 and ival3, dr[0] gets set to the address of ival1 and dr[1] to the address of ival3. If I disable the first watchpoint, but leave the second active, dr[0] is changed to &ival3, but dr[1] is not reset to zero, as i386_low.reset_addr function is not set in windows-nat.c. The strange thing now is that on the next hit of &ival3, dr[6] get the value 0xffff0ff3 as if both dr[0] and dr[1] have been hit... This despite the fact that dr[7] correctly says that only the first (i.e. dr[0] debug register is active). Now comes the tricky part, why does that generate a SIGTRAP? In i386_stopped_data_address, the bits of dr_status_mirror (copied from current value of dr[6]) are checked from 0 to 3, but if hit is found for I, the address value is taken from dr_mirror array. Thus, according to dr[6], both debug registers 0 and 1 have been hit. at i=0, the correct address of ival3 is found in dr_mirror[0], but after for i=1, the value of dr_mirror[1] is used, but this value is 0, as only one watch is active. I don't really understand the reason why dr[6] gets the second bit set, and to complicate things even more, this seems to be processor or OS version dependent! The bug shows up on a Dell Latitude D620 with Intel Core Duo T7200, but not on a HP Pavilion with AMD Athlon ... both running Windows XP (Dell Prof and HP Home, which might also make a difference). I propose below a patch that adds a testcase with the two watches, plus a fix for windows-nat.c (implementing reset_addr) plus an additional change to i386-nat.c to further avoid similar problems on other native targets, by resetting also the RW_LEN bits of the control register when a given debug register is disabled. Is this patch OK? Pierre ChangeLog entry: 2009-06-04 Pierre Muller * i386-nat.c (I386_DR_DISABLE): Also reset the RW_LEN part of control register. * windows-nat.c (cygwin_reset_dr): New forward declaration. (windows_continue): Force debug registers overwrite if debug_register_changed is set. (windows_resume): Likewise. (init_windows_ops): Register cygwin_reset_dr. (cygwin_reset_dr): New function. testsuite/ChangeLog entry: 2009-06-04 Pierre Muller * gdb.base/watchpoints.c: New file. * gdb.base/watchpoints.exp: New test. Index: i386-nat.c =================================================================== RCS file: /cvs/src/src/gdb/i386-nat.c,v retrieving revision 1.22 diff -u -p -r1.22 i386-nat.c --- i386-nat.c 14 May 2009 09:37:00 -0000 1.22 +++ i386-nat.c 3 Jun 2009 21:48:19 -0000 @@ -124,8 +124,9 @@ struct i386_dr_low_type i386_dr_low; /* Disable the break/watchpoint in the I'th debug register. */ #define I386_DR_DISABLE(i) \ - dr_control_mirror &= ~(3 << (DR_ENABLE_SIZE * (i))) - + dr_control_mirror &= ~(3 << (DR_ENABLE_SIZE * (i)) | \ + 0xf << (DR_CONTROL_SHIFT+DR_CONTROL_SIZE*(i))) + /* Set in DR7 the RW and LEN fields for the I'th debug register. */ #define I386_DR_SET_RW_LEN(i,rwlen) \ do { \ Index: windows-nat.c =================================================================== RCS file: /cvs/src/src/gdb/windows-nat.c,v retrieving revision 1.192 diff -u -p -r1.192 windows-nat.c --- windows-nat.c 24 May 2009 12:27:35 -0000 1.192 +++ windows-nat.c 3 Jun 2009 21:48:19 -0000 @@ -142,6 +142,7 @@ static int windows_thread_alive (struct static void windows_kill_inferior (struct target_ops *); static void cygwin_set_dr (int i, CORE_ADDR addr); +static void cygwin_reset_dr (int i); static void cygwin_set_dr7 (unsigned long val); static unsigned long cygwin_get_dr6 (void); @@ -1133,7 +1134,7 @@ windows_continue (DWORD continue_status, for (th = &thread_head; (th = th->next) != NULL;) if ((id == -1 || id == (int) th->id) - && th->suspended) + && (th->suspended || debug_registers_changed)) { if (debug_registers_changed) { @@ -1252,7 +1253,7 @@ windows_resume (struct target_ops *ops, th->context.EFlags |= FLAG_TRACE_BIT; } - if (th->context.ContextFlags) + if (th->context.ContextFlags || debug_registers_changed) { if (debug_registers_changed) { @@ -1262,6 +1263,7 @@ windows_resume (struct target_ops *ops, th->context.Dr3 = dr[3]; th->context.Dr6 = DR6_CLEAR_VALUE; th->context.Dr7 = dr[7]; + th->context.ContextFlags |= CONTEXT_DEBUG_REGISTERS; } CHECK (SetThreadContext (th->h, &th->context)); th->context.ContextFlags = 0; @@ -2176,7 +2178,7 @@ init_windows_ops (void) i386_dr_low.set_control = cygwin_set_dr7; i386_dr_low.set_addr = cygwin_set_dr; - i386_dr_low.reset_addr = NULL; + i386_dr_low.reset_addr = cygwin_reset_dr; i386_dr_low.get_status = cygwin_get_dr6; /* i386_dr_low.debug_register_length field is set by @@ -2295,6 +2297,13 @@ cygwin_set_dr (int i, CORE_ADDR addr) debug_registers_used = 1; } +/* Reset dr[i], to stay in sync with i386-nat dr_mirror. */ +static void +cygwin_reset_dr (int i) +{ + cygwin_set_dr (i, 0); +} + /* Pass the value VAL to the inferior in the DR7 debug control register. Here we just store the address in D_REGS, the watchpoint will be actually set up in windows_wait. */ Index: testsuite/gdb.base/watchpoints.c =================================================================== RCS file: testsuite/gdb.base/watchpoints.c diff -N testsuite/gdb.base/watchpoints.c --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ testsuite/gdb.base/watchpoints.c 3 Jun 2009 21:48:19 -0000 @@ -0,0 +1,33 @@ +#include +#include +/* + * This source is mainly to test + * what happens when a watchpoint is removed + * what another watchpoint, inserted later is + * left active. + */ + +int count = -1; +int ival1 = -1; +int ival2 = -1; +int ival3 = -1; +int ival4 = -1; + +int +main () +{ +#ifdef usestubs + set_debug_traps(); + breakpoint(); +#endif + + for (count = 0; count < 4; count++) { + ival1 = count; ival2 = count; + ival3 = count; ival4 = count; + } + + ival1 = count; ival2 = count; /* Outside loop */ + ival3 = count; ival4 = count; + + return 0; +} Index: testsuite/gdb.base/watchpoints.exp =================================================================== RCS file: testsuite/gdb.base/watchpoints.exp diff -N testsuite/gdb.base/watchpoints.exp --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ testsuite/gdb.base/watchpoints.exp 3 Jun 2009 21:48:19 -0000 @@ -0,0 +1,101 @@ +# Copyright 2009 +# Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . + +# This file was written by Pierre Muller. (muller@ics.u-strasbg.fr) + +if $tracelevel then { + strace $tracelevel +} + +set prms_id 0 +set bug_id 0 + +set testfile "watchpoints" +set srcfile ${testfile}.c +set binfile ${objdir}/${subdir}/${testfile} + +set wp_set 1 + +if [get_compiler_info ${binfile}] { + return -1 +} + +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } { + untested watchpoint.exp + return -1 +} + +# Start with a fresh gdb. + +gdb_exit +gdb_start +gdb_reinitialize_dir $srcdir/$subdir +gdb_load $binfile + + runto_main + gdb_test "watch ival1" "" "" + gdb_test "watch ival3" "" "" + + set timeout 600 + + gdb_test "cont" "Continuing.*\[Ww\]atchpoint.*ival1.*Old value = -1.*New value = 0.*ival1 = count; ival2 = count;.*" "watchpoint hit, first time" + + gdb_test "cont" "Continuing.*\[Ww\]atchpoint.*ival3.*Old value = -1.*New value = 0.*ival3 = count; ival4 = count;.*" "watchpoint hit, first time" + + # Check that the ival3 hit count is reported correctly + gdb_test "info break" ".*watchpoint\[ \t\]+keep\[ \t\]+y\[ \t\]+ival3\r\n\[ \t]+breakpoint already hit 1 time.*" "Watchpoint hit count is 1" + + # Continue until the next change for ival1, from 0 to 1. + gdb_test "cont" "Continuing.*\[Ww\]atchpoint.*ival1.*Old value = 0.*New value = 1.*ival1 = count; ival2 = count;.*" "watchpoint ival1 hit, second time" + + # Check that the hit count for ival1 is reported correctly + gdb_test "info break" ".*watchpoint\[ \t\]+keep\[ \t\]+y\[ \t\]+ival1\r\n\[ \t]+breakpoint already hit 2 times.*" "Watchpoint ival1 hit count is 2" + + # Continue until the next change for ival3, from 0 to 1. + gdb_test "cont" "Continuing.*\[Ww\]atchpoint.*ival3.*Old value = 0.*New value = 1.*ival3 = count; ival4 = count;.*" "watchpoint hit, second time" + + # Check that the hit count is reported correctly + gdb_test "info break" ".*watchpoint\[ \t\]+keep\[ \t\]+y\[ \t\]+ival3\r\n\[ \t]+breakpoint already hit 2 times.*" "Watchpoint hit count is 2" + + # Continue until the next change, from 1 to 2. + gdb_test "cont" "Continuing.*\[Ww\]atchpoint.*ival1.*Old value = 1.*New value = 2.*ival1 = count; ival2 = count;.*" "watchpoint ival1 hit, third time" + + # Check that the hit count is reported correctly + gdb_test "info break" ".*watchpoint\[ \t\]+keep\[ \t\]+y\[ \t\]+ival1\r\n\[ \t]+breakpoint already hit 3 times.*" "Watchpoint ival1 hit count is 3" + # Disable ival1 watchpoint + gdb_test "disable 2" "" "" + + # Continue until the next change, from 1 to 2. + gdb_test "cont" "Continuing.*\[Ww\]atchpoint.*ival3.*Old value = 1.*New value = 2.*ival3 = count; ival4 = count;.*" "watchpoint hit, third time" + + # Check that the hit count is reported correctly + gdb_test "info break" ".*watchpoint\[ \t\]+keep\[ \t\]+y\[ \t\]+ival3\r\n\[ \t]+breakpoint already hit 3 times.*" "Watchpoint hit count is 3" + + # Continue until the next change, from 2 to 3. + gdb_test "cont" "Continuing.*\[Ww\]atchpoint.*ival3.*Old value = 2.*New value = 3.*ival3 = count; ival4 = count;.*" "watchpoint hit, fourth time" + + # Check that the hit count is reported correctly + gdb_test "info break" ".*watchpoint\[ \t\]+keep\[ \t\]+y\[ \t\]+ival3\r\n\[ \t]+breakpoint already hit 4 times.*" "Watchpoint hit count is 4" + + # Continue until the next change, from 3 to 4. + # Note that this one is outside the loop. + + gdb_test "cont" "Continuing.*\[Ww\]atchpoint.*ival3.*Old value = 3.*New value = 4.*ival3 = count; ival4 = count;.*" "watchpoint hit, fifth time" + + # Check that the hit count is reported correctly + gdb_test "info break" ".*watchpoint\[ \t\]+keep\[ \t\]+y\[ \t\]+ival3\r\n\[ \t]+breakpoint already hit 5 times.*" "Watchpoint hit count is 5" + +