Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver))

[Pedro Alves <pedro@codesourcery.com>]

On Tuesday 31 May 2011 20:07:09, Pedro Alves wrote:
> First things first.  This assertion is actually bogus ( and I'm to blame
> for it :-) ).  Patch below.  We get here with regnum == 3, which is quite
> valid.  This means gdbserver is asserting whenever a watchpoint on DR3
> triggers.  Vis:
> 
> (gdb) watch s1
> (gdb) watch s2
> (gdb) watch s3
> 
> (gdb) b 24
> (gdb) c
> ...
> 
> on gdbserver side we have:
> 
> stopped_data_addr:
>         CONTROL (DR7): 51150155          STATUS (DR6): 00000000
>         DR0: addr=0x603768, ref.count=1  DR1: addr=0x60376a, ref.count=1
>         DR2: addr=0x609a08, ref.count=1  DR3: addr=0x60d8e8, ref.count=1
>                                                    ^^^^^^^^
> 
> so:
> 
> (gdb) p p = 0x60d8e8
> (gdb) c
> 
> ... puff!
> 
> ../../../src/gdb/gdbserver/linux-x86-low.c:511: A problem internal to GDBserver has been detected.
> Assertion `DR_FIRSTADDR <= regnum && regnum < DR_LASTADDR' failed.
> 
> Program exited with code 01.
> (gdb) 
> 

Now with new testcase.  Applied to both mainline and 7.3.

Thanks,
-- 
Pedro Alves

2011-05-31  Pedro Alves  <pedro@codesourcery.com>

	gdb/gdbserver/
	* linux-x86-low.c (i386_dr_low_get_addr): Fix off by one in
	assertion.
	* win32-i386-low.c (i386_dr_low_get_addr): Ditto.

	gdb/testsuite/
	* gdb.arch/i386-dr3-watch.c: New file.
	* gdb.arch/i386-dr3-watch.exp: New file.

---
 gdb/gdbserver/linux-x86-low.c             |    2 -
 gdb/gdbserver/win32-i386-low.c            |    2 -
 gdb/testsuite/gdb.arch/i386-dr3-watch.c   |   43 ++++++++++++++++++++++++++
 gdb/testsuite/gdb.arch/i386-dr3-watch.exp |   49 ++++++++++++++++++++++++++++++
 4 files changed, 94 insertions(+), 2 deletions(-)

Index: src/gdb/gdbserver/linux-x86-low.c
===================================================================
--- src.orig/gdb/gdbserver/linux-x86-low.c	2011-05-31 22:04:53.000000000 +0100
+++ src/gdb/gdbserver/linux-x86-low.c	2011-05-31 22:05:24.224956802 +0100
@@ -508,7 +508,7 @@ i386_dr_low_get_addr (int regnum)
   ptid_t ptid = ptid_of (lwp);
 
   /* DR6 and DR7 are retrieved with some other way.  */
-  gdb_assert (DR_FIRSTADDR <= regnum && regnum < DR_LASTADDR);
+  gdb_assert (DR_FIRSTADDR <= regnum && regnum <= DR_LASTADDR);
 
   return x86_linux_dr_get (ptid, regnum);
 }
Index: src/gdb/gdbserver/win32-i386-low.c
===================================================================
--- src.orig/gdb/gdbserver/win32-i386-low.c	2011-05-31 22:04:53.000000000 +0100
+++ src/gdb/gdbserver/win32-i386-low.c	2011-05-31 22:05:24.224956802 +0100
@@ -61,7 +61,7 @@ i386_dr_low_set_addr (const struct i386_
 CORE_ADDR
 i386_dr_low_get_addr (int regnum)
 {
-  gdb_assert (DR_FIRSTADDR <= regnum && regnum < DR_LASTADDR);
+  gdb_assert (DR_FIRSTADDR <= regnum && regnum <= DR_LASTADDR);
 
   return debug_reg_state.dr_mirror[regnum];
 }
Index: src/gdb/testsuite/gdb.arch/i386-dr3-watch.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.arch/i386-dr3-watch.c	2011-05-31 21:41:33.764957296 +0100
@@ -0,0 +1,43 @@
+/* Copyright 2011 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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 <http://www.gnu.org/licenses/>.  */
+
+int i1;
+char gap1[32];
+
+int i2;
+char gap2[32];
+
+int i3;
+char gap3[32];
+
+int i4;
+
+void
+trigger (void)
+{
+  i1 = 1;
+  i2 = 2;
+  i3 = 3;
+  i4 = 4;
+}
+
+int
+main ()
+{
+  trigger ();
+  return 0;
+}
Index: src/gdb/testsuite/gdb.arch/i386-dr3-watch.exp
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.arch/i386-dr3-watch.exp	2011-05-31 22:07:16.714956763 +0100
@@ -0,0 +1,49 @@
+# Copyright 2011 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 <http://www.gnu.org/licenses/>.
+
+# x86 and amd64 gdbserver had a bug where a watchpoint triggered by
+# the DR3 debug register would trip on a bogus assertion.
+
+# This test relies on being able to set 4 hardware watchpoints.  Since
+# that is not a valid assumption across most targets, and we're
+# testing a x86 specific bug, skip everywhere else.
+if { ![istarget "i?86-*-*"] && ![istarget "x86_64-*-*"] } then {
+    return 0
+}
+
+set testfile "i386-dr3-watch"
+set srcfile ${testfile}.c
+
+if [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} {debug $additional_flags}] {
+    untested "failed to compile ${testfile}"
+    return -1
+}
+
+if ![runto_main] then {
+    untested "could not run to main"
+    return -1
+}
+
+gdb_test_no_output "set breakpoint always-inserted on"
+
+gdb_test "watch i1" "Hardware watchpoint .*: i1"
+gdb_test "watch i2" "Hardware watchpoint .*: i2"
+gdb_test "watch i3" "Hardware watchpoint .*: i3"
+gdb_test "watch i4" "Hardware watchpoint .*: i4"
+
+gdb_test "c" "Hardware watchpoint.*: i1.*" "continue to i1 watchpoint"
+gdb_test "c" "Hardware watchpoint.*: i2.*" "continue to i2 watchpoint"
+gdb_test "c" "Hardware watchpoint.*: i3.*" "continue to i3 watchpoint"
+gdb_test "c" "Hardware watchpoint.*: i4.*" "continue to i4 watchpoint"