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

[Jan Kratochvil <jan.kratochvil@redhat.com>]

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"