This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch 3/4] Fix hw watchpoints #2: reordered / simultaneously hit
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 (¤t_target, &addr);
+ return target_stopped_data_address (¤t_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 (¤t_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
+}