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

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

[Jan Kratochvil]

Hi,

standalone without the patch 2/4 it errors on gdb_assert()
when using `set debug infrun 1' as in this testcase.

------------------------------------------------------------------------------

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-11-16  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-11-16  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/watchthreads-reorder.exp, gdb.base/watchthreads-reorder.c:
	New.

--- a/gdb/i386-nat.c
+++ b/gdb/i386-nat.c
@@ -585,7 +585,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);
 }
 
 /* Insert a hardware-assisted breakpoint at BP_TGT->placed_address.
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -2570,6 +2570,47 @@ maybe_clear_ignore_sigint (struct lwp_info *lp)
     }
 }
 
+/* Fetch the possible triggered data watchpoint info and store it to LP.
+   See the comment at update_watchpoint for the trigger lifecycle.  */
+
+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.
+
+   See the comment at update_watchpoint for the trigger lifecycle.  */
+
+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
@@ -2628,6 +2669,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);
@@ -3027,9 +3070,13 @@ linux_nat_filter_event (int lwpid, int status, int options)
 	return NULL;
     }
 
-  /* 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);
+    }
 
   /* Check if the thread has exited.  */
   if ((WIFEXITED (status) || WIFSIGNALED (status))
@@ -5368,6 +5415,8 @@ linux_nat_add_target (struct target_ops *t)
   t->to_pid_to_str = linux_nat_pid_to_str;
   t->to_has_thread_control = tc_schedlock;
   t->to_thread_address_space = linux_nat_thread_address_space;
+  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: ^[$B!F^[(BunusedX_rwatch^[$B!G^[(B 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,105 @@
+# 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"
+
+    # While the debug output itself is not checked in this testcase one bug was
+    # found in the DEBUG_INFRUN code path.
+    gdb_test "set debug infrun 1"
+
+    gdb_continue_to_breakpoint "break-at-exit" ".*break-at-exit.*"
+
+    set pf_prefix $prefix_test
+}

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

[Joel Brobecker]

Jan,

> 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.

Can you be more specific as to what happens in this case?

> gdb/
> 2009-11-16  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.

Well, I agree now that this stopped_by_watchpoint/stopped_data_address
can get you confused... It took me a LONG time to understand how the
patch is working...

Some comments below.

> gdb/testsuite/
> 2009-11-16  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
>         * gdb.base/watchthreads-reorder.exp, gdb.base/watchthreads-reorder.c:
>         New.

Pre-approved, with just a few minor comments.


> +  /* 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;

I think we should avoid bitfields unless we can show that they make
a difference in terms of memory usage. I don't think that this structure
is critical.  Do you think that a name like "watchpoint_hit" or
"watchpoint_hit_p" might be simpler?

    int watchpoint_hit;

for instance?

> +  CORE_ADDR watchpoint_hit;

I'm being a little bit on the picky side (again! Sorry...), but I think
it's better to use names that are consistent with the associated target
methods.  We may want to change these names later if we decide to go
ahead with your proposed interface cleanup (merging the two watchpoint
target routines into one), but for now, I would personally prefer:

     CORE_ADDR stopped_data_address;

> +  /* linux_nat_stopped_data_address is not even installed in this case.  */
> +  if (linux_ops->to_stopped_data_address == NULL)
> +    return;

I think the comment is a little obscure and that it should be placed
after the if. Or, if you want to place it before the if, I'd make the
commend conditional.

Suggestions:

  if (linux_ops->to_stopped_data_address == NULL)
    /* This platform does not seem to support watchpoints.  */
    return;

Or:

  /* Nothing to do if this platform does not seem to support watchpoints.  */
  if (linux_ops->to_stopped_data_address == NULL)
    return;

> +/* 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.
> +
> +   See the comment at update_watchpoint for the trigger lifecycle.  */

Part of this comment, I think, would be easier to understand if it was
placed in the description of save_sigtrap(). (the part that explains
that the data might change, etc, and thus you have to save it).
For this function, I'd just stick to a general description, explaining
that you base the results on the info cached in the lwp struct, and
refer them to save_sigtrap.

> +  /* Just prevent compiler `warning: ^[$B!F^[(BunusedX_rwatch^[$B!G^[(B defined but not used'.  */

I see some weird codes in that comment...

> +# two watchpoint being hit at the same time, without reordering them during the
         ^^^^^^^^^^ watchpoints

> +    if ![runto_main] {
> +	gdb_suppress_tests
> +    }

Please avoid the use of gdb_suppress_tests.  If you can't run to main,
then there is no point in continuing. Just abort via return.

> +    # 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.

"we are interested in not losing"

-- 
Joel

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

[Eli Zaretskii]

> Date: Mon, 16 Nov 2009 19:10:56 -0500
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> > +  unsigned watchpoint_hit_set : 1;
> 
> I think we should avoid bitfields unless we can show that they make
> a difference in terms of memory usage.

??? Why?  What's wrong with bitfields that we should avoid them?

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

[Joel Brobecker]

> ??? Why?  What's wrong with bitfields that we should avoid them?

It makes access to the field more difficult and require more instructions.

-- 
Joel

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

[Jan Kratochvil]

On Tue, 17 Nov 2009 15:11:39 +0100, Joel Brobecker wrote:
> > ??? Why?  What's wrong with bitfields that we should avoid them?
> 
> It makes access to the field more difficult and require more instructions.

While true...

 0000000000000000 <get>:
    0:  0f b6 47 04             movzbl 0x4(%rdi),%eax
+   4:  83 e0 01                and    $0x1,%eax
        c3                      retq   
 
 0000000000000010 <set>:
-  10:  c6 47 04 01             movb   $0x1,0x4(%rdi)
+  10:  80 4f 04 01             orb    $0x1,0x4(%rdi)

... one may miss turning it to a flag for sharing the byte for other flags in
future possibly creating the struct needlessly larger.

But OK.


Regards,
Jan

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

[Eli Zaretskii]

> Date: Tue, 17 Nov 2009 16:29:12 +0100
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, gdb-patches@sourceware.org
> 
>  0000000000000000 <get>:
>     0:  0f b6 47 04             movzbl 0x4(%rdi),%eax
> +   4:  83 e0 01                and    $0x1,%eax
>         c3                      retq   
>  
>  0000000000000010 <set>:
> -  10:  c6 47 04 01             movb   $0x1,0x4(%rdi)
> +  10:  80 4f 04 01             orb    $0x1,0x4(%rdi)

This is what I remembered, at least for x86.  A single additional
instruction can hardly cause any visible slowdown, at least not with
access patterns typical for such flags.

Any other reasons not to use bitfields?

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

[Eli Zaretskii]

> Date: Tue, 17 Nov 2009 09:11:39 -0500
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: jan.kratochvil@redhat.com, gdb-patches@sourceware.org
> 
> > ??? Why?  What's wrong with bitfields that we should avoid them?
> 
> It makes access to the field more difficult and require more instructions.

On what architectures will the performance hit be significant enough
to bother?

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

[Joel Brobecker]

> This is what I remembered, at least for x86.  A single additional
> instruction can hardly cause any visible slowdown, at least not with
> access patterns typical for such flags.
> 
> Any other reasons not to use bitfields?

No. I just think it is unnecessary, and I'd rather avoid them.  If you are
so strongly opinionated about this, or if others agree with you that it
is better, I really don't mind all that much.

-- 
Joel

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

[Eli Zaretskii]

> Date: Tue, 17 Nov 2009 14:41:10 -0500
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: Jan Kratochvil <jan.kratochvil@redhat.com>, gdb-patches@sourceware.org
> 
> > This is what I remembered, at least for x86.  A single additional
> > instruction can hardly cause any visible slowdown, at least not with
> > access patterns typical for such flags.
> > 
> > Any other reasons not to use bitfields?
> 
> No. I just think it is unnecessary, and I'd rather avoid them.  If you are
> so strongly opinionated about this, or if others agree with you that it
> is better, I really don't mind all that much.

I don't have strong opinions either way.  I was just surprised that
you seemed to have opinions strong enough to comment on the usage of
bitfields in Jan's code.

FWIW, I've seen lots of good code using bitfields.  Emacs comes to
mind.  I've also seen lots of code that didn't use them.  I don't
think we should care too much about this, except, as you point out,
when memory is or could be at a premium.

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

[Jan Kratochvil]

Hi Joel,

On Tue, 17 Nov 2009 01:10:56 +0100, Joel Brobecker wrote:
> > 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.
> 
> Can you be more specific as to what happens in this case?
[...]
> It took me a LONG time to understand how the patch is working...

OK, sorry you could ask for this more descriptive intro already before, I see
it could be more clear with it.


x86* CPU does not report directly the 'stopped_data_address' (=the data
address which was being watched and the watchpoint got triggered).  x86* CPU
reports only a trigger bit for each the data address watchpoint setup.

When the inferior stops and single thread caused a trap everything is OK as
that trigger bit is read, displayed to the user (possibly cleared if the patch
1/4 is already applied) etc.

If a trigger happens for two threads at once on a single stop GDB currently
presents only (all) stop events from a single thread.  The events which
happened in the other thread are presented only after `continue' - which in
real does not continue execution of the other thread.

Problem (a) - caused by patch 1/4 - if we correctly start to clear the
triggers (on removal of a watchpoint from inferior) the trigger will get lost
for the other thread before it gets continued + reported.  As the SIGTRAP is
still stored in LWP_INFO->STATUS GDB has no clue why that SIGTRAP happened:
	Program received signal SIGTRAP, Trace/breakpoint trap.
	126       rwatch_store = thread2_rwatch;
	-> FAIL: gdb.threads/watchthreads-reorder.exp: reorder{0,1}: continue b

Problem (b) - even in FSF GDB - it will not clear the trigger (intentionally
or unintentiaonally, who knows, workarounding that it would lose the trigger
otherwise) but it associates the triggered CPU watchpoint register slots with
their very current mapping.  But the CPU watchpoint registers contained
different addresses the time the trigger occured therefore GDB interprets the
trigger possibly wrong.
The testcase gdb.threads/watchthreads-reorder.exp will print:
	Hardware read watchpoint 6: unused2_rwatch
	-> FAIL: gdb.threads/watchthreads-reorder.exp: reorder1: continue b
Despite nobody (in this execution part) touches `unused2_rwatch'.
I had to update the testcase now to make it reproducible again as it got
accidentally unreproducible as now the watchpoints get inserted in their
address (and not watchpoint number) order due to:
	Re: [patch] Performance optimize large bp_location count
	http://sourceware.org/ml/gdb-patches/2009-10/msg00632.html


> I think we should avoid bitfields unless we can show that they make
> a difference in terms of memory usage.

OK, used:
	unsigned char stopped_data_address_p;


> "watchpoint_hit_p" might be simpler?

While I agree the "_p" suffix matches better the GDB coding style I am still
curious after the years - what does that "_p" mean?  Guessing "_present"?


> it's better to use names that are consistent with the associated target
> methods.  We may want to change these names later if we decide to go
> ahead with your proposed interface cleanup (merging the two watchpoint
> target routines into one), but for now, I would personally prefer:
> 
>      CORE_ADDR stopped_data_address;

OK, it was more a mistake from me, unsure why now.


>   if (linux_ops->to_stopped_data_address == NULL)
>     /* This platform does not seem to support watchpoints.  */
>     return;

FYI s390-nat.c supports hardware watchpoints (to_stopped_by_watchpoint) but it
has no to_stopped_data_address.

Anyway such comment would just duplicate the conditional and ... it is just
a comment, dropped.


Otherwise followed your comments, thanks for reading the code.


Thanks,
Jan


gdb/
2009-11-18  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 stopped_data_address_p and
	stopped_data_address.

gdb/testsuite/
2009-11-18  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/watchthreads-reorder.exp, gdb.base/watchthreads-reorder.c:
	New.

--- a/gdb/i386-nat.c
+++ b/gdb/i386-nat.c
@@ -585,7 +585,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);
 }
 
 /* Insert a hardware-assisted breakpoint at BP_TGT->placed_address.
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -2570,6 +2570,52 @@ maybe_clear_ignore_sigint (struct lwp_info *lp)
     }
 }
 
+/* Fetch the possible triggered data watchpoint info and store it to LP.
+
+   GNU/Linux native target may be directed by the watchpoint/debug register
+   number.  During inferior stop the assignment of watchpoint/debug registers
+   may change making the register number specific trigger info stale.
+
+   See the comment at update_watchpoint for the trigger lifecycle.  The stored
+   information gets used from linux_nat_stopped_data_address.  */
+
+static void
+save_sigtrap (struct lwp_info *lp)
+{
+  struct cleanup *old_chain;
+
+  if (linux_ops->to_stopped_data_address == NULL)
+    return;
+
+  old_chain = save_inferior_ptid ();
+  inferior_ptid = lp->ptid;
+
+  lp->stopped_data_address_p =
+    linux_ops->to_stopped_data_address (&current_target,
+					&lp->stopped_data_address);
+
+  do_cleanups (old_chain);
+}
+
+/* Base the reported value on the triggered data address as we could report
+   different address being stored now in the triggered debug register in our
+   GNU/Linux native target when we would call it this moment.
+
+   See the comment at update_watchpoint for the trigger lifecycle.  The fetched
+   information was stored by save_sigtrap.  */
+
+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->stopped_data_address;
+
+  return lp->stopped_data_address_p;
+}
+
 /* Wait until LP is stopped.  */
 
 static int
@@ -2628,6 +2674,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);
@@ -3027,9 +3075,13 @@ linux_nat_filter_event (int lwpid, int status, int options)
 	return NULL;
     }
 
-  /* 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);
+    }
 
   /* Check if the thread has exited.  */
   if ((WIFEXITED (status) || WIFSIGNALED (status))
@@ -5368,6 +5420,8 @@ linux_nat_add_target (struct target_ops *t)
   t->to_pid_to_str = linux_nat_pid_to_str;
   t->to_has_thread_control = tc_schedlock;
   t->to_thread_address_space = linux_nat_thread_address_space;
+  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,13 @@ struct lwp_info
      be the address of a hardware watchpoint.  */
   struct siginfo siginfo;
 
+  /* STOPPED_DATA_ADDRESS_P is non-zero if this LWP stopped with a trap and
+     a data watchpoint has been found as triggered.  In such case
+     STOPPED_DATA_ADDRESS contains data address of the triggered data
+     watchpoint.  */
+  unsigned char stopped_data_address_p;
+  CORE_ADDR stopped_data_address;
+
   /* Non-zero if we expect a duplicated SIGINT.  */
   int ignore_sigint;
 
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/watchthreads-reorder.c
@@ -0,0 +1,369 @@
+/* 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;
+
+/* These variables must have lower in-memory addresses than thread1_rwatch and
+   thread2_rwatch so that they take their watchpoint slots.  */
+
+static int unused1_rwatch;
+static int unused2_rwatch;
+
+static volatile int thread1_rwatch;
+static volatile int thread2_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,103 @@
+# 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 watchpoints 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 {(![istarget "i?86-*-*"] && ![istarget "x86_64-*-*"]
+     && ![istarget "ia64-*-*"] && ![istarget "s390*-*-*"])
+    || [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] {
+	return -1
+    }
+
+    # 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 in not
+    # losing the hardware watchpoint trigger.
+
+    gdb_test "rwatch thread1_rwatch" "Hardware read watchpoint \[0-9\]+: thread1_rwatch"
+    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_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 orders watchpoints by their addresses so inserting new variables
+	# with lower addresses will shift the former watchpoints to higher
+	# debug registers.
+
+	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 "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"
+
+    # While the debug output itself is not checked in this testcase one bug was
+    # found in the DEBUG_INFRUN code path.
+    gdb_test "set debug infrun 1"
+
+    gdb_continue_to_breakpoint "break-at-exit" ".*break-at-exit.*"
+
+    set pf_prefix $prefix_test
+}

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

[Pedro Alves]

On Wednesday 18 November 2009 10:08:10, Jan Kratochvil wrote:
> > "watchpoint_hit_p" might be simpler?
> 
> While I agree the "_p" suffix matches better the GDB coding style I am still
> curious after the years - what does that "_p" mean?  Guessing "_present"?

"predicate".








(((

FTR, remote.c names these variables as:

  int stopped_by_watchpoint_p;
  CORE_ADDR watch_data_address;

)))

-- 
Pedro Alves

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

[Pedro Alves]

On Wednesday 18 November 2009 10:08:10, Jan Kratochvil wrote:

> @@ -5368,6 +5420,8 @@ linux_nat_add_target (struct target_ops *t)
>    t->to_pid_to_str = linux_nat_pid_to_str;
>    t->to_has_thread_control = tc_schedlock;
>    t->to_thread_address_space = linux_nat_thread_address_space;
> +  if (linux_ops->to_stopped_data_address)
> +    t->to_stopped_data_address = linux_nat_stopped_data_address;

We were talking on IRC about this ..

> --- a/gdb/i386-nat.c
> +++ b/gdb/i386-nat.c
> @@ -585,7 +585,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);
>  }

.. and this changes, which didn't look that clean, because they mix
up the single-threaded layer linux_ops, the thread agnostic i386-nat.c
and the multi-lwp layer (linux-nat.c).

Here's the alternative I was talking about.  Have linux_ops worry
about extracting stopped_by_watchpoint/stopped_data_address
from the inferior, either by reading debug regs, or from the
siginfo, and have linux_nat.c always return what's cached in
the lwp structure.

I've also rewritten the comments around save_sigtrap.

WDYT?

-- 
Pedro Alves

gdb/
2009-11-20  Jan Kratochvil  <jan.kratochvil@redhat.com>
	    Pedro Alves  <pedro@codesourcery.com>

	Fix reordered watchpoints triggered in other threads during all-stop.

	* linux-nat.c (resume_callback, linux_nat_resume): Clear
	stopped_by_watchpoint_p.
	(save_sigtrap, linux_nat_stopped_by_watchpoint)
	(linux_nat_stopped_data_address): New.
	(stop_wait_callback, linux_nat_filter_event): Call save_sigtrap.
	(linux_nat_add_target): Install linux_nat_stopped_by_watchpoint
	and linux_nat_stopped_data_address.
	* linux-nat.h (struct lwp_info): New fields stopped_by_watchpoint,
	stopped_data_address_p and stopped_data_address.

gdb/testsuite/
2009-11-20  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/watchthreads-reorder.exp,
	gdb.base/watchthreads-reorder.c: New.

---
 gdb/linux-nat.c                                    |   83 ++++
 gdb/linux-nat.h                                    |   13 
 gdb/testsuite/gdb.threads/watchthreads-reorder.c   |  369 +++++++++++++++++++++
 gdb/testsuite/gdb.threads/watchthreads-reorder.exp |  103 +++++
 4 files changed, 566 insertions(+), 2 deletions(-)

Index: src/gdb/linux-nat.c
===================================================================
--- src.orig/gdb/linux-nat.c	2009-11-20 16:54:06.000000000 +0000
+++ src/gdb/linux-nat.c	2009-11-20 16:54:13.000000000 +0000
@@ -1882,6 +1882,7 @@ resume_callback (struct lwp_info *lp, vo
       lp->stopped = 0;
       lp->step = 0;
       memset (&lp->siginfo, 0, sizeof (lp->siginfo));
+      lp->stopped_by_watchpoint_p = 0;
     }
   else if (lp->stopped && debug_linux_nat)
     fprintf_unfiltered (gdb_stdlog, "RC: Not resuming sibling %s (has pending)\n",
@@ -2019,6 +2020,7 @@ linux_nat_resume (struct target_ops *ops
 
   linux_ops->to_resume (linux_ops, ptid, step, signo);
   memset (&lp->siginfo, 0, sizeof (lp->siginfo));
+  lp->stopped_by_watchpoint_p = 0;
 
   if (debug_linux_nat)
     fprintf_unfiltered (gdb_stdlog,
@@ -2570,6 +2572,75 @@ maybe_clear_ignore_sigint (struct lwp_in
     }
 }
 
+/* Fetch the possible triggered data watchpoint info and store it in
+   LP.
+
+   On some archs, like x86, that use debug registers to set
+   watchpoints, it's possible that the way to know which watched
+   address trapped, is to check the register that is used to select
+   which address to watch.  Problem is, between setting the watchpoint
+   and reading back which data address trapped, the user may change
+   the set of watchpoints, and, as a consequence, GDB changes the
+   debug registers in the inferior.  To avoid reading back a stale
+   stopped-data-address when that happens, we cache in LP the fact
+   that a watchpoint trapped, and the corresponding data address, as
+   soon as we see LP stop with a SIGTRAP.  If GDB changes the debug
+   registers meanwhile, we have the cached data we can rely on.  */
+
+static void
+save_sigtrap (struct lwp_info *lp)
+{
+  struct cleanup *old_chain;
+
+  if (linux_ops->to_stopped_by_watchpoint == NULL)
+    {
+      lp->stopped_by_watchpoint_p = 0;
+      return;
+    }
+
+  if (linux_ops->to_stopped_data_address == NULL)
+    {
+      lp->stopped_data_address_p = 0;
+      return;
+    }
+
+  old_chain = save_inferior_ptid ();
+  inferior_ptid = lp->ptid;
+
+  lp->stopped_by_watchpoint_p = linux_ops->to_stopped_by_watchpoint ();
+
+  if (lp->stopped_by_watchpoint_p)
+    lp->stopped_data_address_p =
+      linux_ops->to_stopped_data_address (&current_target,
+					  &lp->stopped_data_address);
+
+  do_cleanups (old_chain);
+}
+
+/* See save_sigtrap.  */
+
+static int
+linux_nat_stopped_by_watchpoint (void)
+{
+  struct lwp_info *lp = find_lwp_pid (inferior_ptid);
+
+  gdb_assert (lp != NULL);
+
+  return lp->stopped_by_watchpoint_p;
+}
+
+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->stopped_data_address;
+
+  return lp->stopped_data_address_p;
+}
+
 /* Wait until LP is stopped.  */
 
 static int
@@ -2628,6 +2699,8 @@ stop_wait_callback (struct lwp_info *lp,
 	      /* 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);
@@ -3027,9 +3100,13 @@ linux_nat_filter_event (int lwpid, int s
 	return NULL;
     }
 
-  /* 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);
+    }
 
   /* Check if the thread has exited.  */
   if ((WIFEXITED (status) || WIFSIGNALED (status))
@@ -5368,6 +5445,8 @@ linux_nat_add_target (struct target_ops 
   t->to_pid_to_str = linux_nat_pid_to_str;
   t->to_has_thread_control = tc_schedlock;
   t->to_thread_address_space = linux_nat_thread_address_space;
+  t->to_stopped_by_watchpoint = linux_nat_stopped_by_watchpoint;
+  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;
Index: src/gdb/linux-nat.h
===================================================================
--- src.orig/gdb/linux-nat.h	2009-11-20 16:54:06.000000000 +0000
+++ src/gdb/linux-nat.h	2009-11-20 16:54:40.000000000 +0000
@@ -62,6 +62,19 @@ struct lwp_info
      be the address of a hardware watchpoint.  */
   struct siginfo siginfo;
 
+  /* STOPPED_BY_WATCHPOINT is non-zero if this LWP stopped with a data
+     watchpoint trap.  In such case STOPPED_DATA_ADDRESS contains data
+     address of the triggered data watchpoint.  */
+  int stopped_by_watchpoint_p;
+
+  /* On architectures where it is possible to know the data address of
+     a triggered watchpoint, STOPPED_DATA_ADDRESS_P is non-zero, and
+     STOPPED_DATA_ADDRESS contains such the data address.  Otherwise,
+     STOPPED_DATA_ADDRESS_P is false, and STOPPED_DATA_ADDRESS is
+     undefined.  Only valid if STOPPED_BY_WATCHPOINT_P is true.  */
+  int stopped_data_address_p;
+  CORE_ADDR stopped_data_address;
+
   /* Non-zero if we expect a duplicated SIGINT.  */
   int ignore_sigint;
 
Index: src/gdb/testsuite/gdb.threads/watchthreads-reorder.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.threads/watchthreads-reorder.c	2009-11-20 16:54:13.000000000 +0000
@@ -0,0 +1,369 @@
+/* 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;
+
+/* These variables must have lower in-memory addresses than thread1_rwatch and
+   thread2_rwatch so that they take their watchpoint slots.  */
+
+static int unused1_rwatch;
+static int unused2_rwatch;
+
+static volatile int thread1_rwatch;
+static volatile int thread2_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;
+}
Index: src/gdb/testsuite/gdb.threads/watchthreads-reorder.exp
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.threads/watchthreads-reorder.exp	2009-11-20 16:54:13.000000000 +0000
@@ -0,0 +1,103 @@
+# 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 watchpoints 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 {(![istarget "i?86-*-*"] && ![istarget "x86_64-*-*"]
+     && ![istarget "ia64-*-*"] && ![istarget "s390*-*-*"])
+    || [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] {
+	return -1
+    }
+
+    # 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 in not
+    # losing the hardware watchpoint trigger.
+
+    gdb_test "rwatch thread1_rwatch" "Hardware read watchpoint \[0-9\]+: thread1_rwatch"
+    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_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 orders watchpoints by their addresses so inserting new variables
+	# with lower addresses will shift the former watchpoints to higher
+	# debug registers.
+
+	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 "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"
+
+    # While the debug output itself is not checked in this testcase one bug was
+    # found in the DEBUG_INFRUN code path.
+    gdb_test "set debug infrun 1"
+
+    gdb_continue_to_breakpoint "break-at-exit" ".*break-at-exit.*"
+
+    set pf_prefix $prefix_test
+}

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

[Pedro Alves]

> +static void
> +save_sigtrap (struct lwp_info *lp)
> +{
> +  struct cleanup *old_chain;
> +
> +  if (linux_ops->to_stopped_by_watchpoint == NULL)
> +    {
> +      lp->stopped_by_watchpoint_p = 0;
> +      return;
> +    }
> +
> +  if (linux_ops->to_stopped_data_address == NULL)
> +    {
> +      lp->stopped_data_address_p = 0;
> +      return;
> +    }
> +
> +  old_chain = save_inferior_ptid ();
> +  inferior_ptid = lp->ptid;
> +
> +  lp->stopped_by_watchpoint_p = linux_ops->to_stopped_by_watchpoint ();
> +
> +  if (lp->stopped_by_watchpoint_p)
> +    lp->stopped_data_address_p =
> +      linux_ops->to_stopped_data_address (&current_target,
> +                                         &lp->stopped_data_address);
> +
> +  do_cleanups (old_chain);
> +}
> +


Ooops, this bit broken if is the target doesn't support
to_stopped_data_address.  Here's what I meant.

-- 
Pedro Alves

gdb/
2009-11-20  Jan Kratochvil  <jan.kratochvil@redhat.com>
	    Pedro Alves  <pedro@codesourcery.com>

	Fix reordered watchpoints triggered in other threads during all-stop.

	* linux-nat.c (resume_callback, linux_nat_resume): Clear
	stopped_by_watchpoint_p.
	(save_sigtrap, linux_nat_stopped_by_watchpoint)
	(linux_nat_stopped_data_address): New.
	(stop_wait_callback, linux_nat_filter_event): Call save_sigtrap.
	(linux_nat_add_target): Install linux_nat_stopped_by_watchpoint
	and linux_nat_stopped_data_address.
	* linux-nat.h (struct lwp_info): New fields stopped_by_watchpoint,
	stopped_data_address_p and stopped_data_address.

gdb/testsuite/
2009-11-20  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/watchthreads-reorder.exp,
	gdb.base/watchthreads-reorder.c: New.

---
 gdb/linux-nat.c                                    |   82 ++++
 gdb/linux-nat.h                                    |   13 
 gdb/testsuite/gdb.threads/watchthreads-reorder.c   |  369 +++++++++++++++++++++
 gdb/testsuite/gdb.threads/watchthreads-reorder.exp |  103 +++++
 4 files changed, 565 insertions(+), 2 deletions(-)

Index: src/gdb/linux-nat.c
===================================================================
--- src.orig/gdb/linux-nat.c	2009-11-20 16:54:06.000000000 +0000
+++ src/gdb/linux-nat.c	2009-11-20 17:04:47.000000000 +0000
@@ -1882,6 +1882,7 @@ resume_callback (struct lwp_info *lp, vo
       lp->stopped = 0;
       lp->step = 0;
       memset (&lp->siginfo, 0, sizeof (lp->siginfo));
+      lp->stopped_by_watchpoint = 0;
     }
   else if (lp->stopped && debug_linux_nat)
     fprintf_unfiltered (gdb_stdlog, "RC: Not resuming sibling %s (has pending)\n",
@@ -2019,6 +2020,7 @@ linux_nat_resume (struct target_ops *ops
 
   linux_ops->to_resume (linux_ops, ptid, step, signo);
   memset (&lp->siginfo, 0, sizeof (lp->siginfo));
+  lp->stopped_by_watchpoint = 0;
 
   if (debug_linux_nat)
     fprintf_unfiltered (gdb_stdlog,
@@ -2570,6 +2572,74 @@ maybe_clear_ignore_sigint (struct lwp_in
     }
 }
 
+/* Fetch the possible triggered data watchpoint info and store it in
+   LP.
+
+   On some archs, like x86, that use debug registers to set
+   watchpoints, it's possible that the way to know which watched
+   address trapped, is to check the register that is used to select
+   which address to watch.  Problem is, between setting the watchpoint
+   and reading back which data address trapped, the user may change
+   the set of watchpoints, and, as a consequence, GDB changes the
+   debug registers in the inferior.  To avoid reading back a stale
+   stopped-data-address when that happens, we cache in LP the fact
+   that a watchpoint trapped, and the corresponding data address, as
+   soon as we see LP stop with a SIGTRAP.  If GDB changes the debug
+   registers meanwhile, we have the cached data we can rely on.  */
+
+static void
+save_sigtrap (struct lwp_info *lp)
+{
+  struct cleanup *old_chain;
+
+  if (linux_ops->to_stopped_by_watchpoint == NULL)
+    {
+      lp->stopped_by_watchpoint = 0;
+      return;
+    }
+
+  old_chain = save_inferior_ptid ();
+  inferior_ptid = lp->ptid;
+
+  lp->stopped_by_watchpoint = linux_ops->to_stopped_by_watchpoint ();
+
+  if (lp->stopped_by_watchpoint)
+    {
+      if (linux_ops->to_stopped_data_address != NULL)
+	lp->stopped_data_address_p =
+	  linux_ops->to_stopped_data_address (&current_target,
+					      &lp->stopped_data_address);
+      else
+	lp->stopped_data_address_p = 0;
+    }
+
+  do_cleanups (old_chain);
+}
+
+/* See save_sigtrap.  */
+
+static int
+linux_nat_stopped_by_watchpoint (void)
+{
+  struct lwp_info *lp = find_lwp_pid (inferior_ptid);
+
+  gdb_assert (lp != NULL);
+
+  return lp->stopped_by_watchpoint;
+}
+
+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->stopped_data_address;
+
+  return lp->stopped_data_address_p;
+}
+
 /* Wait until LP is stopped.  */
 
 static int
@@ -2628,6 +2698,8 @@ stop_wait_callback (struct lwp_info *lp,
 	      /* 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);
@@ -3027,9 +3099,13 @@ linux_nat_filter_event (int lwpid, int s
 	return NULL;
     }
 
-  /* 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);
+    }
 
   /* Check if the thread has exited.  */
   if ((WIFEXITED (status) || WIFSIGNALED (status))
@@ -5368,6 +5444,8 @@ linux_nat_add_target (struct target_ops 
   t->to_pid_to_str = linux_nat_pid_to_str;
   t->to_has_thread_control = tc_schedlock;
   t->to_thread_address_space = linux_nat_thread_address_space;
+  t->to_stopped_by_watchpoint = linux_nat_stopped_by_watchpoint;
+  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;
Index: src/gdb/linux-nat.h
===================================================================
--- src.orig/gdb/linux-nat.h	2009-11-20 16:54:06.000000000 +0000
+++ src/gdb/linux-nat.h	2009-11-20 17:03:59.000000000 +0000
@@ -62,6 +62,19 @@ struct lwp_info
      be the address of a hardware watchpoint.  */
   struct siginfo siginfo;
 
+  /* STOPPED_BY_WATCHPOINT is non-zero if this LWP stopped with a data
+     watchpoint trap.  In such case STOPPED_DATA_ADDRESS contains data
+     address of the triggered data watchpoint.  */
+  int stopped_by_watchpoint;
+
+  /* On architectures where it is possible to know the data address of
+     a triggered watchpoint, STOPPED_DATA_ADDRESS_P is non-zero, and
+     STOPPED_DATA_ADDRESS contains such the data address.  Otherwise,
+     STOPPED_DATA_ADDRESS_P is false, and STOPPED_DATA_ADDRESS is
+     undefined.  Only valid if STOPPED_BY_WATCHPOINT_P is true.  */
+  int stopped_data_address_p;
+  CORE_ADDR stopped_data_address;
+
   /* Non-zero if we expect a duplicated SIGINT.  */
   int ignore_sigint;
 
Index: src/gdb/testsuite/gdb.threads/watchthreads-reorder.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.threads/watchthreads-reorder.c	2009-11-20 16:54:13.000000000 +0000
@@ -0,0 +1,369 @@
+/* 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;
+
+/* These variables must have lower in-memory addresses than thread1_rwatch and
+   thread2_rwatch so that they take their watchpoint slots.  */
+
+static int unused1_rwatch;
+static int unused2_rwatch;
+
+static volatile int thread1_rwatch;
+static volatile int thread2_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;
+}
Index: src/gdb/testsuite/gdb.threads/watchthreads-reorder.exp
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.threads/watchthreads-reorder.exp	2009-11-20 16:54:13.000000000 +0000
@@ -0,0 +1,103 @@
+# 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 watchpoints 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 {(![istarget "i?86-*-*"] && ![istarget "x86_64-*-*"]
+     && ![istarget "ia64-*-*"] && ![istarget "s390*-*-*"])
+    || [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] {
+	return -1
+    }
+
+    # 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 in not
+    # losing the hardware watchpoint trigger.
+
+    gdb_test "rwatch thread1_rwatch" "Hardware read watchpoint \[0-9\]+: thread1_rwatch"
+    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_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 orders watchpoints by their addresses so inserting new variables
+	# with lower addresses will shift the former watchpoints to higher
+	# debug registers.
+
+	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 "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"
+
+    # While the debug output itself is not checked in this testcase one bug was
+    # found in the DEBUG_INFRUN code path.
+    gdb_test "set debug infrun 1"
+
+    gdb_continue_to_breakpoint "break-at-exit" ".*break-at-exit.*"
+
+    set pf_prefix $prefix_test
+}

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

[Daniel Jacobowitz]

On Fri, Nov 20, 2009 at 04:57:41PM +0000, Pedro Alves wrote:
> WDYT?

Sounds reasonable to me; I'm not entirely convinced this should be in
Linux-specific code, but there's precedent, and we can always move it
higher.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Jan Kratochvil]

On Fri, 20 Nov 2009 17:57:41 +0100, Pedro Alves wrote:
> .. and this changes, which didn't look that clean, because they mix
> up the single-threaded layer linux_ops, the thread agnostic i386-nat.c
> and the multi-lwp layer (linux-nat.c).

On Fri, 20 Nov 2009 18:05:39 +0100, Pedro Alves wrote:
> Here's what I meant.

Removing the i386-nat.c change is a proper cleanup of my patch, I agree.


Thanks,
Jan