Re: [PATCH] PR threads/10729: x86 hw watchpoints and non-stop mode

Jan Kratochvil
2011-12-11T23:39:16Z

Hi Pedro,

on simple non-hit (inferior does not touch "j" at all) watchpoint case:
strace -o 2 -q ./gdb -nx ./36 -ex start -ex 'watch j' -ex stepi -ex 'set confirm no' -ex q
grep 'PTRACE_....USER' 1 >1b; grep 'PTRACE_....USER' 2 >2b

It has performance regression of 15 ptrace syscalls -> 27 ptrace syscalls.

On Fri, 09 Dec 2011 17:30:20 +0100, Pedro Alves wrote:
> @@ -513,22 +499,7 @@ i386_update_inferior_debug_regs (struct i386_debug_reg_state *new_state)
> ALL_DEBUG_REGISTERS (i)
> {
> if (I386_DR_VACANT (new_state, i) != I386_DR_VACANT (&dr_mirror, i))
> - {
> - if (!I386_DR_VACANT (new_state, i))
> - {
> - i386_dr_low.set_addr (i, new_state->dr_mirror[i]);
> -

> - /* Only a sanity check for leftover bits (set possibly only
> - by inferior). */
> - if (i386_dr_low.unset_status)
> - i386_dr_low.unset_status (I386_DR_WATCH_MASK (i));

Deleting this part is a regression. Testcase for that part is attached.

> - }
> - else
> - {
> - if (i386_dr_low.reset_addr)
> - i386_dr_low.reset_addr (i);
> - }
> - }
> + i386_dr_low.set_addr (i, new_state->dr_mirror[i]);
> else
> gdb_assert (new_state->dr_mirror[i] == dr_mirror.dr_mirror[i]);
> }

> @@ -636,11 +607,12 @@ i386_stopped_data_address (struct target_ops *ops, CORE_ADDR *addr_p)
> int rc = 0;
> unsigned status;
> unsigned control;
> - struct i386_debug_reg_state *state = &dr_mirror;
>
> - dr_mirror.dr_status_mirror = i386_dr_low.get_status ();
> - status = dr_mirror.dr_status_mirror;
> - control = dr_mirror.dr_control_mirror;
> + /* 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 thread's debug registers. */
> + status = i386_dr_low.get_status ();
> + control = i386_dr_low.get_control ();
>
> ALL_DEBUG_REGISTERS(i)
> {
> @@ -650,12 +622,9 @@ i386_stopped_data_address (struct target_ops *ops, CORE_ADDR *addr_p)
> 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

> - /* This third condition makes sure DRi is not vacant, this
> - avoids false positives in windows-nat.c. */
> - && !I386_DR_VACANT (state, i))

This removal is probably safe as everything gets pre-cleared but after you fix
the performance regressions I am not sure if it should not be kept there.

> + && I386_DR_GET_RW_LEN (control, i) != 0)
> {
> - addr = state->dr_mirror[i];
> + addr = i386_dr_low.get_addr (i);

Why to do this change? Why we can no longer trust DR_MIRROR? This is
a performance regression.

> rc = 1;
> if (maint_show_dr)
> i386_show_dr (&dr_mirror, "watchpoint_hit", addr, -1, hw_write);

gdb/testsuite/
2011-12-11 Jan Kratochvil <jan.kratochvil@redhat.com>

* gdb.base/watchpoint-hw-pre-set.c: New file.
* gdb.base/watchpoint-hw-pre-set.exp: New file.

--- /dev/null
+++ b/gdb/testsuite/gdb.base/watchpoint-hw-pre-set.c
@@ -0,0 +1,155 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ 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/>. */
+
+#define _GNU_SOURCE 1
+#include <sys/ptrace.h>
+#include <linux/ptrace.h>
+#include <sys/types.h>
+#include <sys/user.h>
+#include <sys/debugreg.h>
+
+#include <assert.h>
+#include <unistd.h>
+#include <sys/wait.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <stddef.h>
+#include <errno.h>
+
+#define SET_WATCHPOINT set_watchpoint
+
+static void
+set_watchpoint (pid_t pid, volatile void *addr)
+{
+ unsigned long dr7;
+ long l;
+
+ errno = 0;
+ l = ptrace (PTRACE_POKEUSER, pid,
+ offsetof (struct user, u_debugreg[0]), (unsigned long) addr);
+ assert_perror (errno);
+ assert (l == 0);
+
+ dr7 = (DR_RW_WRITE << DR_CONTROL_SHIFT);
+ dr7 |= (DR_LEN_4 << DR_CONTROL_SHIFT);
+ dr7 |= (1UL << DR_LOCAL_ENABLE_SHIFT);
+ dr7 |= (1UL << DR_GLOBAL_ENABLE_SHIFT);
+
+ l = ptrace (PTRACE_POKEUSER, pid, offsetof (struct user, u_debugreg[7]), dr7);
+ assert_perror (errno);
+ assert (l == 0);
+}
+
+static pid_t child;
+
+static void
+cleanup (void)
+{
+ if (child > 0)
+ kill (child, SIGKILL);
+ child = 0;
+}
+
+static void
+handler_fail (int signo)
+{
+ cleanup ();
+ signal (signo, SIG_DFL);
+ raise (signo);
+}
+
+static volatile long long check, dummy;
+
+static void
+marker (void)
+{
+ dummy++;
+}
+
+static int resume;
+
+int
+main (void)
+{
+ pid_t got_pid;
+ int i, status, cycles = 0;
+ long l;
+
+ atexit (cleanup);
+ signal (SIGABRT, handler_fail);
+ signal (SIGINT, handler_fail);
+
+ child = fork ();
+ switch (child)
+ {
+ case -1:
+ assert (0);
+ case 0:
+ l = ptrace (PTRACE_TRACEME, 0, NULL, NULL);
+ assert (l == 0);
+
+ i = raise (SIGUSR1);
+ assert (i == 0);
+
+ check = 1;
+
+ while (!resume && cycles++ < 600 * 10)
+ usleep (1000000 / 10);
+
+ marker ();
+
+ i = raise (SIGUSR2);
+ assert (i == 0);
+ /* NOTREACHED */
+ assert (0);
+ default:
+ break;
+ }
+
+ got_pid = waitpid (child, &status, 0);
+ assert (got_pid == child);
+ assert (WIFSTOPPED (status));
+ assert (WSTOPSIG (status) == SIGUSR1);
+
+ SET_WATCHPOINT (child, &check);
+
+ errno = 0;
+ l = ptrace (PTRACE_CONT, child, NULL, NULL);
+ assert_perror (errno);
+ assert (l == 0);
+
+ got_pid = waitpid (child, &status, 0);
+ assert (got_pid == child);
+ assert (WIFSTOPPED (status));
+ if (WSTOPSIG (status) == SIGUSR2)
+ {
+ /* We missed the watchpoint - unsupported by hardware? Found on:
+ + qemu-system-x86_64 0.9.1-6.fc9.x86_64
+ + qemu-kvm kvm-65-7.fc9.x86_64 + kernel-2.6.25.9-76.fc9.x86_64. */
+ return 2;
+ }
+ assert (WSTOPSIG (status) == SIGTRAP);
+
+ errno = 0;
+ l = ptrace (PTRACE_DETACH, child, NULL, NULL);
+ assert_perror (errno);
+ assert (l == 0);
+
+ marker ();
+
+ return 0;
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watchpoint-hw-pre-set.exp
@@ -0,0 +1,62 @@
+# 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/>.
+
+# Test a newly created hardware watchpoint gets cleared its possible pre-set
+# hit in the status register. Otherwise a false hit may occur.
+
+if {[skip_hw_watchpoint_access_tests]
+ || (![istarget "i?86-*-linux*"] && ![istarget "x86_64-*-linux*"])
+ || [is_remote target]} {
+ return 0
+}
+
+set test watchpoint-hw-pre-set
+set srcfile ${test}.c
+if { [prepare_for_testing ${test}.exp ${test} ${srcfile}] } {
+ return -1
+}
+
+if ![runto "marker"] {
+ return -1
+}
+
+set test "print child"
+gdb_test_multiple $test $test {
+ -re " = (\[0-9\]+)\r\n$gdb_prompt $" {
+ pass $test
+ set child $expect_out(1,string)
+ }
+}
+
+gdb_test "attach $child" "Attaching to program: .*, process $child\r\n.*" \
+ "attach" \
+ {A program is being debugged already\. Kill it\? \(y or n\) } "y"
+
+gdb_test_no_output "set variable resume=1"
+#gdb_test "maintenance set show-debug-regs on"
+gdb_test "awatch check" {Hardware access \(read/write\) watchpoint [0-9]+: check}
+
+set test "stepi"
+gdb_test_multiple $test $test {
+ -re "\r\nHardware access \\(read/write\\) watchpoint \[0-9\]+: check\r\n.*\r\n$gdb_prompt $" {
+ fail $test
+ }
+ -re "\r\n$gdb_prompt $" {
+ pass $test
+ }
+}
+
+gdb_test "kill" "" "kill" \
+ {Kill the program being debugged\? \(y or n\) } "y"