[patch 2/4] Fix hw watchpoints: reordered / simultaneously hit

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

Hi,

if you hit two watchpoints at the same time in the default all-stop mode GDB
does not cope well with the pending watchpoint hit in non-current LWP.
Moreover when you change the watchpoints setup during such stop.

The is a bit problem the dependency on debug register number is specific to
x86* (on other arches hardware may directly report the address of triggered
watchpoint).  But it is also dependent on Linux as it depends on LWPs (so
i386-nat.c is too general).  And it would need to be duplicate in both
{i386,amd64}-linux-nat.c.   Therefore the code is put in linux-nat.c where it
does not hurt non-x86* arches (it may not even get installed on those).

The testcase reproducer is difficult to make reliable (as is now): GDB must
find two threads both with a watchpoint hit at the single stop.  The used
SIGSTOP of GDB is a bit fragile as the inferior being traced must not trigger
a ptrace event otherwise it deadlocks with its own debugger SIGSTOPped.


Thanks,
Jan


gdb/
2009-08-17  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix reordered watchpoints triggered in other threads during all-stop.
	* i386-nat.c (i386_stopped_by_watchpoint): Call
	i386_stopped_data_address through the target vector.
	* linux-nat.c (save_sigtrap, linux_nat_stopped_data_address): New.
	(stop_wait_callback, linux_nat_filter_event): Call save_sigtrap.
	(linux_nat_add_target): Install linux_nat_stopped_data_address.
	* linux-nat.h (struct lwp_info): New fields watchpoint_hit_set and
	watchpoint_hit.

gdb/testsuite/
2009-08-17  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* watchthreads-reorder.exp, watchthreads-reorder.c: New.

--- a/gdb/i386-nat.c
+++ b/gdb/i386-nat.c
@@ -587,7 +587,7 @@ static int
 i386_stopped_by_watchpoint (void)
 {
   CORE_ADDR addr = 0;
-  return i386_stopped_data_address (&current_target, &addr);
+  return target_stopped_data_address (&current_target, &addr);
 }
 
 /* Return non-zero if the inferior has some break/watchpoint that
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -2164,6 +2164,45 @@ maybe_clear_ignore_sigint (struct lwp_info *lp)
     }
 }
 
+/* Fetch the possible triggered data watchpoint info and store it to LP.
+   The hardware data watchpoint trigger gets cleared during this fetch.  */
+
+static void
+save_sigtrap (struct lwp_info *lp)
+{
+  struct cleanup *old_chain;
+
+  /* linux_nat_stopped_data_address is not even installed in this case.  */
+  if (linux_ops->to_stopped_data_address == NULL)
+    return;
+
+  old_chain = save_inferior_ptid ();
+  inferior_ptid = lp->ptid;
+
+  lp->watchpoint_hit_set =
+    linux_ops->to_stopped_data_address (&current_target, &lp->watchpoint_hit);
+
+  do_cleanups (old_chain);
+}
+
+/* Wrap target_stopped_data_address where the GNU/Linux native target may be
+   directed by the watchpoint/debug register number.  Base the reported value
+   on the triggered data address instead.  During inferior stop the assignment
+   of watchpoint/debug registers may change making the register number specific
+   trigger info stale.  */
+
+static int
+linux_nat_stopped_data_address (struct target_ops *ops, CORE_ADDR *addr_p)
+{
+  struct lwp_info *lp = find_lwp_pid (inferior_ptid);
+
+  gdb_assert (lp != NULL);
+
+  *addr_p = lp->watchpoint_hit;
+
+  return lp->watchpoint_hit_set;
+}
+
 /* Wait until LP is stopped.  */
 
 static int
@@ -2215,6 +2254,8 @@ stop_wait_callback (struct lwp_info *lp, void *data)
 	      /* Save the trap's siginfo in case we need it later.  */
 	      save_siginfo (lp);
 
+	      save_sigtrap (lp);
+
 	      /* Now resume this LWP and get the SIGSTOP event. */
 	      errno = 0;
 	      ptrace (PTRACE_CONT, GET_LWP (lp->ptid), 0, 0);
@@ -2579,9 +2620,13 @@ linux_nat_filter_event (int lwpid, int status, int options)
       add_thread (lp->ptid);
     }
 
-  /* Save the trap's siginfo in case we need it later.  */
   if (WIFSTOPPED (status) && WSTOPSIG (status) == SIGTRAP)
-    save_siginfo (lp);
+    {
+      /* Save the trap's siginfo in case we need it later.  */
+      save_siginfo (lp);
+
+      save_sigtrap (lp);
+    }
 
   /* Handle GNU/Linux's extended waitstatus for trace events.  */
   if (WIFSTOPPED (status) && WSTOPSIG (status) == SIGTRAP && status >> 16 != 0)
@@ -4871,6 +4916,8 @@ linux_nat_add_target (struct target_ops *t)
   t->to_thread_alive = linux_nat_thread_alive;
   t->to_pid_to_str = linux_nat_pid_to_str;
   t->to_has_thread_control = tc_schedlock;
+  if (linux_ops->to_stopped_data_address)
+    t->to_stopped_data_address = linux_nat_stopped_data_address;
 
   t->to_can_async_p = linux_nat_can_async_p;
   t->to_is_async_p = linux_nat_is_async_p;
--- a/gdb/linux-nat.h
+++ b/gdb/linux-nat.h
@@ -62,6 +62,12 @@ struct lwp_info
      be the address of a hardware watchpoint.  */
   struct siginfo siginfo;
 
+  /* WATCHPOINT_HIT_SET is non-zero if this LWP stopped with a trap and a data
+     watchpoint has been found as triggered.  In such case WATCHPOINT_HIT
+     contains data address of the triggered data watchpoint.  */
+  unsigned watchpoint_hit_set : 1;
+  CORE_ADDR watchpoint_hit;
+
   /* Non-zero if we expect a duplicated SIGINT.  */
   int ignore_sigint;
 
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/watchthreads-reorder.c
@@ -0,0 +1,366 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   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 <http://www.gnu.org/licenses/>.  */
+
+#define _GNU_SOURCE
+#include <pthread.h>
+#include <stdio.h>
+#include <limits.h>
+#include <errno.h>
+#include <stdlib.h>
+#include <string.h>
+#include <assert.h>
+#include <sys/types.h>
+#include <signal.h>
+#include <unistd.h>
+#include <asm/unistd.h>
+
+#define gettid() syscall (__NR_gettid)
+
+/* Terminate always in the main task, it can lock up with SIGSTOPped GDB
+   otherwise.  */
+#define TIMEOUT (gettid () == getpid() ? 10 : 15)
+
+static pthread_mutex_t gdbstop_mutex = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP;
+
+static pid_t thread1_tid;
+static pthread_cond_t thread1_tid_cond = PTHREAD_COND_INITIALIZER;
+static pthread_mutex_t thread1_tid_mutex = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP;
+
+static pid_t thread2_tid;
+static pthread_cond_t thread2_tid_cond = PTHREAD_COND_INITIALIZER;
+static pthread_mutex_t thread2_tid_mutex = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP;
+
+static pthread_mutex_t terminate_mutex = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP;
+
+static volatile int thread1_rwatch;
+static volatile int thread2_rwatch;
+
+static int unused1_rwatch;
+static int unused2_rwatch;
+
+/* Do not use alarm as it would create a ptrace event which would hang up us if
+ * we are being traced by GDB which we stopped ourselves.  */
+
+static void timed_mutex_lock (pthread_mutex_t *mutex)
+{
+  int i;
+  struct timespec start, now;
+
+  i = clock_gettime (CLOCK_MONOTONIC, &start);
+  assert (i == 0);
+
+  do
+    {
+      i = pthread_mutex_trylock (mutex);
+      if (i == 0)
+	return;
+      assert (i == EBUSY);
+
+      i = clock_gettime (CLOCK_MONOTONIC, &now);
+      assert (i == 0);
+      assert (now.tv_sec >= start.tv_sec);
+    }
+  while (now.tv_sec - start.tv_sec < TIMEOUT);
+
+  fprintf (stderr, "Timed out waiting for internal lock!\n");
+  exit (EXIT_FAILURE);
+}
+
+static void *
+thread1_func (void *unused)
+{
+  int i;
+  volatile int rwatch_store;
+
+  thread1_tid = gettid ();
+  i = pthread_cond_signal (&thread1_tid_cond);
+  assert (i == 0);
+
+  /* Be sure GDB is already stopped before continuing.  */
+  timed_mutex_lock (&gdbstop_mutex);
+  i = pthread_mutex_unlock (&gdbstop_mutex);
+  assert (i == 0);
+
+  rwatch_store = thread1_rwatch;
+
+  /* Be sure the "T (tracing stop)" test can proceed for both threads.  */
+  timed_mutex_lock (&terminate_mutex);
+  i = pthread_mutex_unlock (&terminate_mutex);
+  assert (i == 0);
+
+  return NULL;
+}
+
+static void *
+thread2_func (void *unused)
+{
+  int i;
+  volatile int rwatch_store;
+
+  thread2_tid = gettid ();
+  i = pthread_cond_signal (&thread2_tid_cond);
+  assert (i == 0);
+
+  /* Be sure GDB is already stopped before continuing.  */
+  timed_mutex_lock (&gdbstop_mutex);
+  i = pthread_mutex_unlock (&gdbstop_mutex);
+  assert (i == 0);
+
+  rwatch_store = thread2_rwatch;
+
+  /* Be sure the "T (tracing stop)" test can proceed for both threads.  */
+  timed_mutex_lock (&terminate_mutex);
+  i = pthread_mutex_unlock (&terminate_mutex);
+  assert (i == 0);
+
+  return NULL;
+}
+
+static const char *
+proc_string (const char *filename, const char *line)
+{
+  FILE *f;
+  static char buf[LINE_MAX];
+  size_t line_len = strlen (line);
+
+  f = fopen (filename, "r");
+  if (f == NULL)
+    {
+      fprintf (stderr, "fopen (\"%s\") for \"%s\": %s\n", filename, line,
+	       strerror (errno));
+      exit (EXIT_FAILURE);
+    }
+  while (errno = 0, fgets (buf, sizeof (buf), f))
+    {
+      char *s;
+
+      s = strchr (buf, '\n');
+      assert (s != NULL);
+      *s = 0;
+
+      if (strncmp (buf, line, line_len) != 0)
+	continue;
+
+      if (fclose (f))
+	{
+	  fprintf (stderr, "fclose (\"%s\") for \"%s\": %s\n", filename, line,
+		   strerror (errno));
+	  exit (EXIT_FAILURE);
+	}
+
+      return &buf[line_len];
+    }
+  if (errno != 0)
+    {
+      fprintf (stderr, "fgets (\"%s\": %s\n", filename, strerror (errno));
+      exit (EXIT_FAILURE);
+    }
+  fprintf (stderr, "\"%s\": No line \"%s\" found.\n", filename, line);
+  exit (EXIT_FAILURE);
+}
+
+static unsigned long
+proc_ulong (const char *filename, const char *line)
+{
+  const char *s = proc_string (filename, line);
+  long retval;
+  char *end;
+
+  errno = 0;
+  retval = strtol (s, &end, 10);
+  if (retval < 0 || retval >= LONG_MAX || (end && *end))
+    {
+      fprintf (stderr, "\"%s\":\"%s\": %ld, %s\n", filename, line, retval,
+	       strerror (errno));
+      exit (EXIT_FAILURE);
+    }
+  return retval;
+}
+
+static void
+state_wait (pid_t process, const char *wanted)
+{
+  char *filename;
+  int i;
+  struct timespec start, now;
+  const char *state;
+
+  i = asprintf (&filename, "/proc/%lu/status", (unsigned long) process);
+  assert (i > 0);
+
+  i = clock_gettime (CLOCK_MONOTONIC, &start);
+  assert (i == 0);
+
+  do
+    {
+      state = proc_string (filename, "State:\t");
+      if (strcmp (state, wanted) == 0)
+	{
+	  free (filename);
+	  return;
+	}
+
+      if (sched_yield ())
+	{
+	  perror ("sched_yield()");
+	  exit (EXIT_FAILURE);
+	}
+
+      i = clock_gettime (CLOCK_MONOTONIC, &now);
+      assert (i == 0);
+      assert (now.tv_sec >= start.tv_sec);
+    }
+  while (now.tv_sec - start.tv_sec < TIMEOUT);
+
+  fprintf (stderr, "Timed out waiting for PID %lu \"%s\" (now it is \"%s\")!\n",
+	   (unsigned long) process, wanted, state);
+  exit (EXIT_FAILURE);
+}
+
+static volatile pid_t tracer = 0;
+static pthread_t thread1, thread2;
+
+static void
+cleanup (void)
+{
+  printf ("Resuming GDB PID %lu.\n", (unsigned long) tracer);
+
+  if (tracer)
+    {
+      int i;
+      int tracer_save = tracer;
+
+      tracer = 0;
+
+      i = kill (tracer_save, SIGCONT);
+      assert (i == 0);
+    }
+}
+
+int
+main (int argc, char **argv)
+{
+  int i;
+  int standalone = 0;
+
+  if (argc == 2 && strcmp (argv[1], "-s") == 0)
+    standalone = 1;
+  else
+    assert (argc == 1);
+
+  setbuf (stdout, NULL);
+
+  timed_mutex_lock (&gdbstop_mutex);
+
+  timed_mutex_lock (&terminate_mutex);
+
+  i = pthread_create (&thread1, NULL, thread1_func, NULL);
+  assert (i == 0);
+
+  i = pthread_create (&thread2, NULL, thread2_func, NULL);
+  assert (i == 0);
+
+  if (!standalone)
+    {
+      tracer = proc_ulong ("/proc/self/status", "TracerPid:\t");
+      if (tracer == 0)
+	{
+	  fprintf (stderr, "The testcase must be run by GDB!\n");
+	  exit (EXIT_FAILURE);
+	}
+      if (tracer != getppid ())
+	{
+	  fprintf (stderr, "The testcase parent must be our GDB tracer!\n");
+	  exit (EXIT_FAILURE);
+	}
+    }
+
+  /* SIGCONT our debugger in the case of our crash as we would deadlock
+     otherwise.  */
+
+  atexit (cleanup);
+
+  printf ("Stopping GDB PID %lu.\n", (unsigned long) tracer);
+
+  if (tracer)
+    {
+      i = kill (tracer, SIGSTOP);
+      assert (i == 0);
+      state_wait (tracer, "T (stopped)");
+    }
+
+  timed_mutex_lock (&thread1_tid_mutex);
+  timed_mutex_lock (&thread2_tid_mutex);
+
+  /* Let the threads start.  */
+  i = pthread_mutex_unlock (&gdbstop_mutex);
+  assert (i == 0);
+
+  printf ("Waiting till the threads initialize their TIDs.\n");
+
+  if (thread1_tid == 0)
+    {
+      i = pthread_cond_wait (&thread1_tid_cond, &thread1_tid_mutex);
+      assert (i == 0);
+
+      assert (thread1_tid > 0);
+    }
+
+  if (thread2_tid == 0)
+    {
+      i = pthread_cond_wait (&thread2_tid_cond, &thread2_tid_mutex);
+      assert (i == 0);
+
+      assert (thread2_tid > 0);
+    }
+
+  printf ("Thread 1 TID = %lu, thread 2 TID = %lu, PID = %lu.\n",
+	  (unsigned long) thread1_tid, (unsigned long) thread2_tid,
+	  (unsigned long) getpid ());
+
+  printf ("Waiting till the threads get trapped by the watchpoints.\n");
+
+  if (tracer)
+    {
+      /* s390x-unknown-linux-gnu will fail with "R (running)".  */
+
+      state_wait (thread1_tid, "T (tracing stop)");
+
+      state_wait (thread2_tid, "T (tracing stop)");
+    }
+
+  cleanup ();
+
+  printf ("Joining the threads.\n");
+
+  i = pthread_mutex_unlock (&terminate_mutex);
+  assert (i == 0);
+
+  i = pthread_join (thread1, NULL);
+  assert (i == 0);
+
+  i = pthread_join (thread2, NULL);
+  assert (i == 0);
+
+  printf ("Exiting.\n");	/* break-at-exit */
+
+  /* Just prevent compiler `warning: ‘unusedX_rwatch’ defined but not used'.  */
+  unused1_rwatch = 1;
+  unused2_rwatch = 2;
+
+  return EXIT_SUCCESS;
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/watchthreads-reorder.exp
@@ -0,0 +1,101 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# 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 <http://www.gnu.org/licenses/>.
+
+# Test GDB can cope with two watchpoints being hit by different threads at the
+# same time, GDB reports one of them and after "continue" to report the other
+# one GDB should not be confused by differently set watchpoints that time.
+# This is the goal of "reorder1".  "reorder0" tests the basic functionality of
+# two watchpoint being hit at the same time, without reordering them during the
+# stop.  The formerly broken functionality is due to the all-stop mode default
+# "show breakpoint always-inserted" being "off".  Formerly the remembered hit
+# could be assigned during continuation of a thread with pending SIGTRAP to the
+# different/new watchpoint, just based on the watchpoint/debug register number.
+
+if {[target_info exists gdb,no_hardware_watchpoints]
+    || ![istarget *-*-linux*]} {
+    return 0;
+}
+
+set testfile "watchthreads-reorder"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" ${binfile} executable [list debug additional_flags=-lrt]] != "" } {
+    return -1
+}
+
+foreach reorder {0 1} {
+
+    global pf_prefix
+    set prefix_test $pf_prefix
+    lappend pf_prefix "reorder$reorder:"
+
+    clean_restart $testfile
+
+    gdb_test "set can-use-hw-watchpoints 1"
+
+    if ![runto_main] {
+	gdb_suppress_tests
+    }
+
+    # Use "rwatch" as "watch" would report the watchpoint changed just based on its
+    # read memory value during a stop by unrelated event.  We are interested to not
+    # to lose the hardware watchpoint trigger.
+
+    gdb_test "rwatch thread1_rwatch" "Hardware read watchpoint \[0-9\]+: thread1_rwatch"
+    gdb_test {set $rwatch1=$bpnum}
+    set test "rwatch thread2_rwatch"
+    gdb_test_multiple $test $test {
+	-re "Target does not support this type of hardware watchpoint\\.\r\n$gdb_prompt $" {
+	    # ppc64 supports at most 1 hw watchpoints.
+	    unsupported $test
+	    return
+	}
+	-re "Hardware read watchpoint \[0-9\]+: thread2_rwatch\r\n$gdb_prompt $" {
+	    pass $test
+	}
+    }
+    gdb_test {set $rwatch2=$bpnum}
+    gdb_breakpoint [gdb_get_line_number "break-at-exit"]
+
+    # The watchpoints can happen in arbitrary order depending on random:
+    # SEL: Found 2 SIGTRAP events, selecting #[01]
+    # As GDB contains no srand() on the specific host/OS it will behave always the
+    # same.  Such order cannot be guaranteed for GDB in general.
+
+    gdb_test "continue" \
+	     "Hardware read watchpoint \[0-9\]+: thread\[12\]_rwatch\r\n\r\nValue = 0\r\n0x\[0-9a-f\]+ in thread\[12\]_func .*" \
+	     "continue a"
+
+    if $reorder {
+	gdb_test {delete $rwatch1}
+	gdb_test {delete $rwatch2}
+
+	gdb_test "rwatch unused1_rwatch" "Hardware read watchpoint \[0-9\]+: unused1_rwatch"
+	gdb_test "rwatch unused2_rwatch" "Hardware read watchpoint \[0-9\]+: unused2_rwatch"
+
+	gdb_test "rwatch thread1_rwatch" "Hardware read watchpoint \[0-9\]+: thread1_rwatch"
+	gdb_test "rwatch thread2_rwatch" "Hardware read watchpoint \[0-9\]+: thread2_rwatch"
+    }
+
+    gdb_test "continue" \
+	     "Hardware read watchpoint \[0-9\]+: thread\[12\]_rwatch\r\n\r\nValue = 0\r\n0x\[0-9a-f\]+ in thread\[12\]_func .*" \
+	     "continue b"
+
+    gdb_continue_to_breakpoint "break-at-exit" ".*break-at-exit.*"
+
+    set pf_prefix $prefix_test
+}