This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] x86 Linux watchpoints: Couldn't write debug register: Invalid, argument.


On 06/23/2014 06:41 PM, Pedro Alves wrote:
> On 06/22/2014 07:31 PM, Jan Kratochvil wrote:
> 
>> This is redundant, I386_DR_DISABLE already does both GLOBAL_DISABLE-like and
>> LOCAL_DISABLE-like.
> 
> Indeed!  I've removed that, and pushed the patch, as below.  The 7.8 branch
> doesn't have the shared nat/i386-dregs.c file, so it needed a tweaked version...

... here's what I pushed to the 7.8 branch.

------------
>From 02350eddb14a719e4c53a1fc93e3f5fd1e762504 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 23 Jun 2014 16:57:25 +0100
Subject: [PATCH] x86 Linux watchpoints: Couldn't write debug register: Invalid
 argument.

This patch fixes this on x86 Linux:

 (gdb) watch *buf@2
 Hardware watchpoint 8: *buf@2
 (gdb) si
 0x00000000004005a7      34        for (i = 0; i < 100000; i++); /* stepi line */
 (gdb) del
 Delete all breakpoints? (y or n) y
 (gdb) watch *(buf+1)@1
 Hardware watchpoint 9: *(buf+1)@1
 (gdb) si
 0x00000000004005a7 in main () at ../../../src/gdb/testsuite/gdb.base/watchpoint-reuse-slot.c:34
 34        for (i = 0; i < 100000; i++); /* stepi line */
 Couldn't write debug register: Invalid argument.
 (gdb)

In the example above the debug registers are being switched from this
state:

        CONTROL (DR7): 0000000000050101          STATUS (DR6): 0000000000000000
        DR0: addr=0x0000000000601040, ref.count=1  DR1: addr=0x0000000000000000, ref.count=0
        DR2: addr=0x0000000000000000, ref.count=0  DR3: addr=0x0000000000000000, ref.count=0

to this:

        CONTROL (DR7): 0000000000010101          STATUS (DR6): 0000000000000000
        DR0: addr=0x0000000000601041, ref.count=1  DR1: addr=0x0000000000000000, ref.count=0
        DR2: addr=0x0000000000000000, ref.count=0  DR3: addr=0x0000000000000000, ref.count=0

That is, before, DR7 was setup for watching a 2 byte region starting
at what's in DR0 (0x601040).

And after, DR7 is setup for watching a 1 byte region starting at
what's in DR0 (0x601041).

We always write DR0..DR3 before DR7, because if we enable a slot's
bits in DR7, you need to have already written the corresponding
DR0..DR3 registers -- the kernel rejects the DR7 write with EINVAL
otherwise.

The error shown above is the opposite scenario.  When we try to write
0x601041 to DR0, DR7's bits still indicate intent of watching a 2-byte
region.  That DR0/DR7 combination is invalid, because 0x601041 is
unaligned.  To watch two bytes, we'd have to use two slots.  So the
kernel errors out with EINVAL.

Fix this by always first clearing DR7, then writing DR0..DR3, and then
setting DR7's bits.

A little optimization -- if we're disabling the last watchpoint, then
we can clear DR7 just once.  The changes to nat/i386-dregs.c make that
easier to detect, and as bonus, they make it a little easier to make
sense of DR7 in the debug logs, as we no longer need to remember we're
seeing stale bits.

Tested on x86_64 Fedora 20, native and GDBserver.

This adds an exhaustive test that switches between many different
combinations of watchpoint types and addresses and widths.

gdb/
2014-06-23  Pedro Alves  <palves@redhat.com>

	* amd64-linux-nat.c (amd64_linux_prepare_to_resume): Clear
	DR_CONTROL before setting DR0..DR3.
	* i386-linux-nat.c (i386_linux_prepare_to_resume): Likewise.
	* i386-nat.c (i386_remove_aligned_watchpoint): Clear all bits of
	DR_CONTROL related to the debug register slot being disabled.  If
	all slots are vacant, clear local slowdown as well, and assert
	DR_CONTROL is 0.

gdb/gdbserver/
2014-06-23  Pedro Alves  <palves@redhat.com>

	* linux-x86-low.c (x86_linux_prepare_to_resume): Clear DR_CONTROL
	before setting DR0..DR3.
	* i386-low.c (i386_remove_aligned_watchpoint): Clear all bits of
	DR_CONTROL related to the debug register slot being disabled.  If
	all slots are vacant, clear local slowdown as well, and assert
	DR_CONTROL is 0.

gdb/testsuite/
2014-06-23  Pedro Alves  <palves@redhat.com>

	* gdb.base/watchpoint-reuse-slot.c: New file.
	* gdb.base/watchpoint-reuse-slot.exp: New file.
---
 gdb/ChangeLog                                    |  10 ++
 gdb/gdbserver/ChangeLog                          |   9 ++
 gdb/testsuite/ChangeLog                          |   5 +
 gdb/amd64-linux-nat.c                            |  10 +-
 gdb/gdbserver/i386-low.c                         |  20 +++
 gdb/gdbserver/linux-x86-low.c                    |   5 +-
 gdb/i386-linux-nat.c                             |   5 +-
 gdb/i386-nat.c                                   |  20 +++
 gdb/testsuite/gdb.base/watchpoint-reuse-slot.c   |  37 +++++
 gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp | 187 +++++++++++++++++++++++
 10 files changed, 305 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/watchpoint-reuse-slot.c
 create mode 100644 gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 5eebf91..b988630 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,13 @@
+2014-06-23  Pedro Alves  <palves@redhat.com>
+
+	* amd64-linux-nat.c (amd64_linux_prepare_to_resume): Clear
+	DR_CONTROL before setting DR0..DR3.
+	* i386-linux-nat.c (i386_linux_prepare_to_resume): Likewise.
+	* i386-nat.c (i386_remove_aligned_watchpoint): Clear all bits of
+	DR_CONTROL related to the debug register slot being disabled.  If
+	all slots are vacant, clear local slowdown as well, and assert
+	DR_CONTROL is 0.
+
 2014-06-19  Pedro Alves  <palves@redhat.com>
 
 	* gdbthread.h (ALL_THREADS): Delete.
diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 71c7bcb..8da676a 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,12 @@
+2014-06-23  Pedro Alves  <palves@redhat.com>
+
+	* linux-x86-low.c (x86_linux_prepare_to_resume): Clear DR_CONTROL
+	before setting DR0..DR3.
+	* i386-low.c (i386_remove_aligned_watchpoint): Clear all bits of
+	DR_CONTROL related to the debug register slot being disabled.  If
+	all slots are vacant, clear local slowdown as well, and assert
+	DR_CONTROL is 0.
+
 2014-06-05  Joel Brobecker  <brobecker@adacore.com>
 
 	* development.sh: Delete.
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index b4c4ff8..439e650 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2014-06-23  Pedro Alves  <palves@redhat.com>
+
+	* gdb.base/watchpoint-reuse-slot.c: New file.
+	* gdb.base/watchpoint-reuse-slot.exp: New file.
+
 2014-06-23  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
 	* gdb.arch/amd64-stap-special-operands.exp: Use is_lp64_target.
diff --git a/gdb/amd64-linux-nat.c b/gdb/amd64-linux-nat.c
index 06199af..1bf1b56 100644
--- a/gdb/amd64-linux-nat.c
+++ b/gdb/amd64-linux-nat.c
@@ -415,6 +415,11 @@ amd64_linux_prepare_to_resume (struct lwp_info *lwp)
 
 	 Ensure DR_CONTROL gets written as the very last register here.  */
 
+      /* Clear DR_CONTROL first.  In some cases, setting DR0-3 to a
+	 value that doesn't match what is enabled in DR_CONTROL
+	 results in EINVAL.  */
+      amd64_linux_dr_set (lwp->ptid, DR_CONTROL, 0);
+
       for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++)
 	if (state->dr_ref_count[i] > 0)
 	  {
@@ -427,7 +432,10 @@ amd64_linux_prepare_to_resume (struct lwp_info *lwp)
 	    clear_status = 1;
 	  }
 
-      amd64_linux_dr_set (lwp->ptid, DR_CONTROL, state->dr_control_mirror);
+      /* If DR_CONTROL is supposed to be zero, we've already set it
+	 above.  */
+      if (state->dr_control_mirror != 0)
+	amd64_linux_dr_set (lwp->ptid, DR_CONTROL, state->dr_control_mirror);
 
       lwp->arch_private->debug_registers_changed = 0;
     }
diff --git a/gdb/gdbserver/i386-low.c b/gdb/gdbserver/i386-low.c
index de2d11a..83ab718 100644
--- a/gdb/gdbserver/i386-low.c
+++ b/gdb/gdbserver/i386-low.c
@@ -313,6 +313,7 @@ i386_remove_aligned_watchpoint (struct i386_debug_reg_state *state,
 				CORE_ADDR addr, unsigned len_rw_bits)
 {
   int i, retval = -1;
+  int all_vacant = 1;
 
   ALL_DEBUG_REGISTERS (i)
     {
@@ -325,11 +326,30 @@ i386_remove_aligned_watchpoint (struct i386_debug_reg_state *state,
 	      /* Reset our mirror.  */
 	      state->dr_mirror[i] = 0;
 	      I386_DR_DISABLE (state, i);
+	      /* Even though not strictly necessary, clear out all
+		 bits in DR_CONTROL related to this debug register.
+		 Debug output is clearer when we don't have stale bits
+		 in place.  This also allows the assertion below.  */
+	      I386_DR_SET_RW_LEN (state, i, 0);
 	    }
 	  retval = 0;
 	}
+
+      if (!I386_DR_VACANT (state, i))
+	all_vacant = 0;
     }
 
+  if (all_vacant)
+    {
+      /* Even though not strictly necessary, clear out all of
+	 DR_CONTROL, so that when we have no debug registers in use,
+	 we end up with DR_CONTROL == 0.  The Linux support relies on
+	 this for an optimization.  Plus, it makes for clearer debug
+	 output.  */
+      state->dr_control_mirror &= ~DR_LOCAL_SLOWDOWN;
+
+      gdb_assert (state->dr_control_mirror == 0);
+    }
   return retval;
 }
 
diff --git a/gdb/gdbserver/linux-x86-low.c b/gdb/gdbserver/linux-x86-low.c
index e478394..066eafe 100644
--- a/gdb/gdbserver/linux-x86-low.c
+++ b/gdb/gdbserver/linux-x86-low.c
@@ -777,6 +777,8 @@ x86_linux_prepare_to_resume (struct lwp_info *lwp)
       struct i386_debug_reg_state *state
 	= &proc->private->arch_private->debug_reg_state;
 
+      x86_linux_dr_set (ptid, DR_CONTROL, 0);
+
       for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++)
 	if (state->dr_ref_count[i] > 0)
 	  {
@@ -789,7 +791,8 @@ x86_linux_prepare_to_resume (struct lwp_info *lwp)
 	    clear_status = 1;
 	  }
 
-      x86_linux_dr_set (ptid, DR_CONTROL, state->dr_control_mirror);
+      if (state->dr_control_mirror != 0)
+	x86_linux_dr_set (ptid, DR_CONTROL, state->dr_control_mirror);
 
       lwp->arch_private->debug_registers_changed = 0;
     }
diff --git a/gdb/i386-linux-nat.c b/gdb/i386-linux-nat.c
index bb2b911..a8d81fb 100644
--- a/gdb/i386-linux-nat.c
+++ b/gdb/i386-linux-nat.c
@@ -778,6 +778,8 @@ i386_linux_prepare_to_resume (struct lwp_info *lwp)
       /* See amd64_linux_prepare_to_resume for Linux kernel note on
 	 i386_linux_dr_set calls ordering.  */
 
+      i386_linux_dr_set (lwp->ptid, DR_CONTROL, 0);
+
       for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++)
 	if (state->dr_ref_count[i] > 0)
 	  {
@@ -790,7 +792,8 @@ i386_linux_prepare_to_resume (struct lwp_info *lwp)
 	    clear_status = 1;
 	  }
 
-      i386_linux_dr_set (lwp->ptid, DR_CONTROL, state->dr_control_mirror);
+      if (state->dr_control_mirror != 0)
+	i386_linux_dr_set (lwp->ptid, DR_CONTROL, state->dr_control_mirror);
 
       lwp->arch_private->debug_registers_changed = 0;
     }
diff --git a/gdb/i386-nat.c b/gdb/i386-nat.c
index 2f80a6e..d2a038f 100644
--- a/gdb/i386-nat.c
+++ b/gdb/i386-nat.c
@@ -472,6 +472,7 @@ i386_remove_aligned_watchpoint (struct i386_debug_reg_state *state,
 				CORE_ADDR addr, unsigned len_rw_bits)
 {
   int i, retval = -1;
+  int all_vacant = 1;
 
   ALL_DEBUG_REGISTERS(i)
     {
@@ -484,11 +485,30 @@ i386_remove_aligned_watchpoint (struct i386_debug_reg_state *state,
 	      /* Reset our mirror.  */
 	      state->dr_mirror[i] = 0;
 	      I386_DR_DISABLE (state, i);
+	      /* Even though not strictly necessary, clear out all
+		 bits in DR_CONTROL related to this debug register.
+		 Debug output is clearer when we don't have stale bits
+		 in place.  This also allows the assertion below.  */
+	      I386_DR_SET_RW_LEN (state, i, 0);
 	    }
 	  retval = 0;
 	}
+
+      if (!I386_DR_VACANT (state, i))
+	all_vacant = 0;
     }
 
+  if (all_vacant)
+    {
+      /* Even though not strictly necessary, clear out all of
+	 DR_CONTROL, so that when we have no debug registers in use,
+	 we end up with DR_CONTROL == 0.  The Linux support relies on
+	 this for an optimization.  Plus, it makes for clearer debug
+	 output.  */
+      state->dr_control_mirror &= ~DR_LOCAL_SLOWDOWN;
+
+      gdb_assert (state->dr_control_mirror == 0);
+    }
   return retval;
 }
 
diff --git a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.c b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.c
new file mode 100644
index 0000000..a5a8f5c
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.c
@@ -0,0 +1,37 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2014 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/>.  */
+
+union aligned_buf
+{
+  char byte[12];
+
+  /* So that testing consistently starts on an aligned address.  */
+  unsigned long long force_align;
+};
+
+union aligned_buf buf;
+
+int
+main (void)
+{
+  volatile int i = 0;
+
+  /* Must be a single line.  */
+  for (i = 0; i < 100000; i++); /* stepi line */
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
new file mode 100644
index 0000000..aa30398
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
@@ -0,0 +1,187 @@
+# Copyright 2014 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/>.
+
+# Test alternating between watchpoint types, watching a sliding window
+# of addresses (thus alternating between aligned and unaligned
+# addresses).  Only a single watchpoint exists at any given time.  On
+# targets that only update the debug registers on resume, this
+# stresses the debug register setup code, both in GDB and in the
+# target/kernel as one watchpoint replaces the other in a single
+# operation.  (Note that we don't have any of these watchpoints
+# trigger.)
+
+if [target_info exists gdb,no_hardware_watchpoints] {
+    unsupported "no target support"
+    return
+}
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
+    return -1
+}
+
+if ![runto_main] then {
+    fail "Can't run to main"
+    return 0
+}
+
+# The line we'll be stepping.
+set srcline [gdb_get_line_number "stepi line"]
+
+# The address the program is stopped at currently.
+set cur_addr ""
+
+# Get the current PC.
+
+proc get_pc {} {
+    global hex gdb_prompt
+
+    set addr ""
+    set test "get PC"
+    gdb_test_multiple "p /x \$pc" "$test" {
+	-re " = ($hex).*$gdb_prompt $" {
+	    set addr $expect_out(1,string)
+	    pass "$test"
+	}
+    }
+
+    return $addr
+}
+
+
+# Issue a stepi, and make sure the program advanced past the current
+# instruction (stored in the CUR_ADDR global).
+
+proc stepi {} {
+    global hex gdb_prompt cur_addr
+
+    set srcline "  for (i = 0; i < 100000; i++); /* stepi line */"
+    set test "stepi advanced"
+    gdb_test_multiple "stepi" $test {
+	-re "($hex).*[string_to_regexp $srcline]\r\n$gdb_prompt $" {
+	    set addr $expect_out(1,string)
+	    if {$addr != $cur_addr} {
+		pass $test
+	    } else {
+		fail $test
+	    }
+	    set cur_addr addr
+	}
+    }
+}
+
+gdb_breakpoint $srcline
+gdb_continue_to_breakpoint "stepi line"
+
+# The test tries various sequences of different types of watchpoints.
+# Probe for support first.
+
+# So we get an immediate warning/error if the target doesn't support a
+# given watchpoint type.
+gdb_test_no_output "set breakpoint always-inserted on"
+
+# The list of supported commands.  Below we'll probe for support and
+# add elements to this list.
+set cmds {}
+
+foreach cmd {"watch" "awatch" "rwatch"} {
+    set test $cmd
+    gdb_test_multiple "$cmd buf.byte\[0\]" $test {
+	-re "You may have requested too many.*$gdb_prompt $" {
+	    unsupported $test
+	}
+	-re "$gdb_prompt $" {
+	    pass $test
+	    lappend cmds $cmd
+	}
+    }
+
+   delete_breakpoints
+}
+
+set test "hbreak"
+gdb_test_multiple "hbreak main" $test {
+    -re "You may have requested too many.*$gdb_prompt $" {
+	pass $test
+    }
+    -re "$gdb_prompt $" {
+	pass $test
+	lappend cmds "hbreak"
+    }
+}
+
+delete_breakpoints
+
+set cur_addr [get_pc]
+
+# Watch WIDTH bytes at BASE + OFFSET.  CMD specifices the specific
+# type of watchpoint to use.  If CMD is "hbreak", WIDTH is ignored.
+
+proc watch_command {cmd base offset width} {
+    global srcfile srcline hex
+
+    if {$cmd == "hbreak"} {
+	set expr "*(buf.byte + $base + $offset)"
+	gdb_test "hbreak $expr" "Hardware assisted breakpoint \[0-9\]+ at $hex"
+    } elseif {$cmd == "watch"} {
+	set expr "*(buf.byte + $base + $offset)@$width"
+	gdb_test "$cmd $expr" \
+	    "Hardware watchpoint \[0-9\]+: [string_to_regexp $expr]"
+    } elseif {$cmd == "awatch"} {
+	set expr "*(buf.byte + $base + $offset)@$width"
+	gdb_test "$cmd $expr" \
+	    "Hardware access \\(read/write\\) watchpoint \[0-9\]+: [string_to_regexp $expr]"
+    } elseif {$cmd == "rwatch"} {
+	set expr "*(buf.byte + $base + $offset)@$width"
+	gdb_test "$cmd $expr" \
+	    "Hardware read watchpoint \[0-9\]+: [string_to_regexp $expr]"
+    }
+}
+
+# Run test proper.  See intro for description.
+
+foreach always_inserted {"off" "on" } {
+    gdb_test_no_output "set breakpoint always-inserted $always_inserted"
+    foreach cmd1 $cmds {
+	foreach cmd2 $cmds {
+	    for {set width 1} {$width < 4} {incr width} {
+
+		if {$cmd1 == "hbreak" && $cmd2 == "hbreak" && $width > 1} {
+		    # hbreak ignores WIDTH, no use testing more than
+		    # once.
+		    continue
+		}
+
+		for {set x 0} {$x < 4} {incr x} {
+		    set prefix "always-inserted $always_inserted: "
+		    append prefix "$cmd1 x $cmd2: "
+		    with_test_prefix "$prefix: width $width, iter $x" {
+			with_test_prefix "base + 0" {
+			    watch_command $cmd1 $x 0 $width
+			    stepi
+			    gdb_test_no_output "delete \$bpnum"
+			}
+			with_test_prefix "base + 1" {
+			    watch_command $cmd2 $x 1 $width
+			    stepi
+			    gdb_test_no_output "delete \$bpnum"
+			}
+		    }
+		}
+	    }
+	}
+    }
+}
-- 
1.9.3



-- 
Pedro Alves


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]