This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Don't delete local watchpoints just because a different thread stopped.
- From: Pedro Alves <pedro at codesourcery dot com>
- To: gdb-patches at sourceware dot org
- Date: Thu, 19 Nov 2009 02:07:01 +0000
- Subject: Don't delete local watchpoints just because a different thread stopped.
(I've actually written this with non-stop in mind, but it applies
equally to all-stop.)
Currently, this:
"Hardware watchpoint foo deleted because the program has left the block"
" in which its expression is valid".
triggers when a thread other than the thread the local watchpoint
had been set through, stops. IMO, that's a bug. The thread the
local watchpoint was set through could still be running in a
block where the watchpoint expression is valid.
When the local watchpoint is set, GDB stores the
selected frame id in the watchpoint (breakpoint) structure, but
the frame id alone is insuficient. When a different thread
stops, GDB considers the program has left the watchpoint block,
because it can't find the watchpoint's frame id in this other
thread's frame chain ...
One could even come up with a test case where the current
frame id on this other thread does happen to be in the
frame chain. Shouldn't be hard to trigger if the threads
are running the same function --- GDB can then mistakenly
try to extract a new current watchpoint value on the wrong
thread context.
Here's a patch to address this, and a new testcase.
2009-11-19 Pedro Alves <pedro@codesourcery.com>
gdb/
* breakpoint.h (struct breakpoint) <watchpoint_thread>: New field.
* breakpoint.c (watchpoint_thread_match): New.
(update_watchpoint): Skip if the local watchpoint's thread doesn't
match the current thread.
(watchpoint_check): Ditto.
(watch_command_1): Set the watchpoint's watchpoint_thread field.
2009-11-19 Pedro Alves <pedro@codesourcery.com>
gdb/testsuite/
* local-watch-wrong-thread.c, local-watch-wrong-thread.exp: New
files.
WDYT?
Tested on x86_64-linux, native and gdbserver.
--
Pedro Alves
2009-11-19 Pedro Alves <pedro@codesourcery.com>
gdb/
* breakpoint.h (struct breakpoint) <watchpoint_thread>: New field.
* breakpoint.c (watchpoint_thread_match): New.
(update_watchpoint): Skip if the local watchpoint's thread doesn't
match the current thread.
(watchpoint_check): Ditto.
(watch_command_1): Set the watchpoint's watchpoint_thread field.
---
gdb/breakpoint.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
gdb/breakpoint.h | 4 ++++
2 files changed, 48 insertions(+), 2 deletions(-)
Index: src/gdb/breakpoint.h
===================================================================
--- src.orig/gdb/breakpoint.h 2009-11-19 01:10:05.000000000 +0000
+++ src/gdb/breakpoint.h 2009-11-19 01:10:16.000000000 +0000
@@ -457,6 +457,10 @@ struct breakpoint
should be evaluated on the outermost frame. */
struct frame_id watchpoint_frame;
+ /* Holds the thread which identifies the frame this watchpoint
+ should be considered in scope for, or -1 if don't care. */
+ int watchpoint_thread;
+
/* For hardware watchpoints, the triggered status according to the
hardware. */
enum watchpoint_triggered watchpoint_triggered;
Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c 2009-11-19 01:10:05.000000000 +0000
+++ src/gdb/breakpoint.c 2009-11-19 01:11:21.000000000 +0000
@@ -985,6 +985,30 @@ fetch_watchpoint_value (struct expressio
}
}
+/* Assuming that B is a watchpoint: if B is a watchpoint on a local
+ expression, returns true if the current thread is the same as the
+ watchpoint's thread. Always returns true for non-local
+ watchpoints. */
+
+static int
+watchpoint_thread_match (struct breakpoint *b)
+{
+ if (b->watchpoint_thread != -1)
+ {
+ int thread;
+
+ if (!ptid_equal (inferior_ptid, null_ptid))
+ thread = inferior_thread ()->num;
+ else
+ thread = -1;
+
+ if (thread != b->watchpoint_thread)
+ return 0;
+ }
+
+ return 1;
+}
+
/* Assuming that B is a watchpoint:
- Reparse watchpoint expression, if REPARSE is non-zero
- Evaluate expression and store the result in B->val
@@ -1004,6 +1028,12 @@ update_watchpoint (struct breakpoint *b,
bpstat bs;
struct program_space *frame_pspace;
+ /* If this is a local watchpoint, we only want to check if the
+ watchpoint frame is in scope if the current thread is the thread
+ that was used to create the watchpoint. */
+ if (!watchpoint_thread_match (b))
+ return;
+
/* We don't free locations. They are stored in bp_location array and
update_global_locations will eventually delete them and remove
breakpoints if needed. */
@@ -3085,6 +3115,12 @@ watchpoint_check (void *p)
b = bs->breakpoint_at->owner;
+ /* If this is a local watchpoint, we only want to check if the
+ watchpoint frame is in scope if the current thread is the thread
+ that was used to create the watchpoint. */
+ if (!watchpoint_thread_match (b))
+ return WP_VALUE_NOT_CHANGED;
+
if (b->exp_valid_block == NULL)
within_current_scope = 1;
else
@@ -7151,9 +7187,15 @@ watch_command_1 (char *arg, int accessfl
b->cond_string = 0;
if (frame)
- b->watchpoint_frame = get_frame_id (frame);
+ {
+ b->watchpoint_frame = get_frame_id (frame);
+ b->watchpoint_thread = inferior_thread ()->num;
+ }
else
- b->watchpoint_frame = null_frame_id;
+ {
+ b->watchpoint_frame = null_frame_id;
+ b->watchpoint_thread = -1;
+ }
if (scope_breakpoint != NULL)
{
===================================================================
2009-11-19 Pedro Alves <pedro@codesourcery.com>
gdb/testsuite/
* local-watch-wrong-thread.c, local-watch-wrong-thread.exp: New
files.
---
gdb/testsuite/gdb.threads/local-watch-wrong-thread.c | 80 +++++++++++
gdb/testsuite/gdb.threads/local-watch-wrong-thread.exp | 118 +++++++++++++++++
2 files changed, 198 insertions(+)
Index: src/gdb/testsuite/gdb.threads/local-watch-wrong-thread.c
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.threads/local-watch-wrong-thread.c 2009-11-19 01:10:15.000000000 +0000
@@ -0,0 +1,80 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2002, 2003, 2004, 2007, 2008, 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/>.
+
+*/
+
+#include <unistd.h>
+#include <stdlib.h>
+#include <pthread.h>
+
+unsigned int args[2];
+int trigger = 0;
+
+void *
+thread_function0 (void *arg)
+{
+ int my_number = (long) arg;
+ volatile int *myp = (volatile int *) &args[my_number];
+
+ while (*myp > 0)
+ {
+ (*myp) ++;
+ usleep (1); /* Loop increment 1. */
+ }
+
+ return NULL;
+}
+
+void *
+thread_function1 (void *arg)
+{
+ int my_number = (long) arg;
+
+ volatile int *myp = (volatile int *) &args[my_number];
+
+ while (*myp > 0)
+ {
+ (*myp) ++;
+ usleep (1); /* Loop increment 2. */
+ }
+
+ return NULL;
+}
+
+int
+main ()
+{
+ int res;
+ pthread_t threads[2];
+ void *thread_result;
+ long i = 0;
+
+ args[i] = 1; /* Init value. */
+ res = pthread_create (&threads[i], NULL,
+ thread_function0,
+ (void *) i);
+
+ i++;
+ args[i] = 1; /* Init value. */
+ res = pthread_create(&threads[i], NULL,
+ thread_function1,
+ (void *) i);
+
+ pthread_join (threads[0], &thread_result);
+ pthread_join (threads[1], &thread_result);
+ exit(EXIT_SUCCESS);
+}
Index: src/gdb/testsuite/gdb.threads/local-watch-wrong-thread.exp
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.threads/local-watch-wrong-thread.exp 2009-11-19 02:02:12.000000000 +0000
@@ -0,0 +1,118 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2004, 2007, 2008, 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/>.
+
+# Check that GDB can support multiple watchpoints across threads.
+
+# This test verifies that a local watchpoint isn't deleted when a
+# thread other than the thread the local watchpoint was set in stops
+# for a breakpoint.
+if [target_info exists gdb,no_hardware_watchpoints] {
+ return 0;
+}
+
+set testfile "local-watch-wrong-thread"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+if {[gdb_compile_pthreads \
+ "${srcdir}/${subdir}/${srcfile}" \
+ "${binfile}" executable {debug} ] != "" } {
+ return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+gdb_test "set can-use-hw-watchpoints 1" "" ""
+
+if ![runto_main] then {
+ fail "Can't run to main"
+ return
+}
+
+set inc_line_1 [gdb_get_line_number "Loop increment 1"]
+set inc_line_2 [gdb_get_line_number "Loop increment 2"]
+
+# Run to the loop within thread_function0, so we can set our local
+# watchpoint.
+gdb_test "break ${srcfile}:${inc_line_1}" \
+ "Breakpoint 2 at .*: file .*${srcfile}, line .*" \
+ "breakpoint on thread_function0"
+
+gdb_test "continue" \
+ ".*Breakpoint 2.*thread_function0.*" \
+ "continue to thread_function0"
+
+delete_breakpoints
+
+# Set the local watchpoint, and confirm that it traps as expected.
+gdb_test "watch *myp" \
+ "Hardware watchpoint 3\: \\*myp.*" \
+ "set local watchpoint on *myp"
+
+gdb_test "continue" \
+ "Hardware watchpoint.*Old value.*New value.*thread_function0.*" \
+ "local watchpoint triggers"
+
+delete_breakpoints
+
+# Recreate the watchpoint, but this time with a condition we know
+# won't trigger. This is so the watchpoint is inserted, and the
+# target reports events, but GDB doesn't stop for them. We want to
+# hit the breakpoints on the other thread, and make sure this
+# watchpoint isn't deleted then.
+gdb_test "watch *myp if trigger != 0" \
+ "Hardware watchpoint 4\: \\*myp.*" \
+ "set local watchpoint on *myp, with false conditional"
+
+# Run to a breakpoint on a different thread. The previous local
+# watchpoint should not be deleted with the standard 'the program has
+# left the block in which its expression is valid', because the
+# thread_function0 thread should still be running in the loop.
+gdb_test "break ${srcfile}:${inc_line_2}" \
+ "Breakpoint 5 at .*: file .*${srcfile}, line .*" \
+ "breakpoint on the other thread"
+
+gdb_test "continue" \
+ "Breakpoint 5, thread_function1.*" \
+ "the other thread stopped on breakpoint"
+
+# Delete the new breakpoint, we don't need it anymore.
+gdb_test "delete 5" "" ""
+
+# Check if the local watchpoint hasn't been deleted (is still listed).
+# This is simpler to check than expecting 'the program has left ...',
+# and, is immune to string changes in that warning.
+gdb_test "info breakpoints" \
+ ".*4.*hw watchpoint.*keep.*y.*\\*myp.*" \
+ "local watchpoint is still in breakpoint list"
+
+# Make the watchpoint condition eval true.
+gdb_test "set trigger=1" "" "let local watchpoint trigger"
+
+gdb_test "continue" \
+ "Hardware watchpoint.*Old value.*New value.*thread_function0.*" \
+ "local watchpoint still triggers"
+
+# Confirm that the local watchpoint is indeed deleted if
+# thread_function0 returns.
+gdb_test "set *myp=0" "" "let thread_function0 return"
+
+gdb_test "continue" \
+ ".*Watchpoint.*deleted.*" \
+ "local watchpoint automatically deleted"