This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch 3/4] hw watchpoints made multi-inferior
Hello,
an update as the second post included the fix of amd64_linux_dr
+ i386_linux_dr removal which needs to be done also for non-x86* arches for
multi-inferior but so far has not been done. Therefore there IMO really
should be a testcase for it as this kind of bugs is never easy to find.
This new testcase really FAILs with the first post (this month) of this patch.
Had to file + KFAIL breakpoints/12312
repeated watchpoints hits with breakpoint always-inserted
but this is IMO an unrelated fix, it just affects this testcase. I did not
want to merge a 3rd unrelated fix to this patchset.
Thanks,
Jan
gdb/
2010-12-12 Jan Kratochvil <jan.kratochvil@redhat.com>
Fix watchpoints for multi-inferior.
* amd64-linux-nat.c (amd64_linux_dr): Remove.
(amd64_linux_dr_set_control, amd64_linux_dr_set_addr): Remove setting
AMD64_LINUX_DR.
(amd64_linux_new_thread): New variable dr_mirror, use it instead of
amd64_linux_dr. New assert.
* breakpoint.c (update_watchpoint): Return directly if breakpoint's
pspace is not current_program_space.
(insert_breakpoint_locations): New variable
saved_current_program_space. Exclude breakpoints not matching it. Do
not report false errors on such breakpoints not matching it.
(watch_command_1): Initialize b->PSPACE.
(update_global_location_list): Exclude non-matching old_loc->PSPACE.
* i386-linux-nat.c (i386_linux_dr): Remove.
(i386_linux_dr_set_control, i386_linux_dr_set_addr): Remove setting
I386_LINUX_DR.
(i386_linux_new_thread): New variable dr_mirror, use it instead of
i386_linux_dr. New assert.
* i386-nat.c (DR_NADDR, struct i386_dr_mirror): Move to i386-nat.h.
(i386_inferior_data_cleanup): New.
(i386_inferior_data_get): Remove variable inf_data_local. Initialize
inf_data from an inferior_data call.
(i386_dr_mirror_get): Make it global.
(i386_use_watchpoints): Initialize I386_INFERIOR_DATA.
* i386-nat.h (DR_NADDR, struct i386_dr_mirror): Moved here from
i386-nat.c.
(i386_dr_mirror_get): New declaration.
* linux-nat.c (linux_nat_iterate_watchpoint_lwps): Fix
iterate_over_lwps FILTER for multi-inferior. Extend the comment.
gdb/testsuite/
2010-12-06 Jan Kratochvil <jan.kratochvil@redhat.com>
Fix watchpoints for multi-inferior.
* gdb.multi/watchpoint-multi.c: New file.
* gdb.multi/watchpoint-multi.exp: New file.
--- a/gdb/amd64-linux-nat.c
+++ b/gdb/amd64-linux-nat.c
@@ -265,8 +265,6 @@ amd64_linux_store_inferior_registers (struct target_ops *ops,
/* Support for debug registers. */
-static unsigned long amd64_linux_dr[DR_CONTROL + 1];
-
static unsigned long
amd64_linux_dr_get (int tid, int regnum)
{
@@ -317,8 +315,6 @@ amd64_linux_dr_set_control_callback (int tid, void *control_voidp)
static void
amd64_linux_dr_set_control (unsigned long control)
{
- amd64_linux_dr[DR_CONTROL] = control;
-
linux_nat_iterate_watchpoint_lwps (amd64_linux_dr_set_control_callback,
&control);
}
@@ -349,8 +345,6 @@ amd64_linux_dr_set_addr (int regnum, CORE_ADDR addr)
gdb_assert (regnum >= 0 && regnum <= DR_LASTADDR - DR_FIRSTADDR);
- amd64_linux_dr[DR_FIRSTADDR + regnum] = addr;
-
data.regnum = regnum;
data.addr = addr;
linux_nat_iterate_watchpoint_lwps (amd64_linux_dr_set_addr_callback, &data);
@@ -404,16 +398,20 @@ amd64_linux_dr_unset_status (unsigned long mask)
static void
amd64_linux_new_thread (ptid_t ptid)
{
+ struct i386_dr_mirror *dr_mirror = i386_dr_mirror_get ();
int i, tid;
+ /* Verify DR_MIRROR is valid. */
+ gdb_assert (PIDGET (ptid) == PIDGET (inferior_ptid));
+
tid = TIDGET (ptid);
if (tid == 0)
tid = PIDGET (ptid);
- for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++)
- amd64_linux_dr_set (tid, i, amd64_linux_dr[i]);
+ for (i = 0; i < DR_LASTADDR - DR_FIRSTADDR; i++)
+ amd64_linux_dr_set (tid, DR_FIRSTADDR + i, dr_mirror->addr[i]);
- amd64_linux_dr_set (tid, DR_CONTROL, amd64_linux_dr[DR_CONTROL]);
+ amd64_linux_dr_set (tid, DR_CONTROL, dr_mirror->control);
}
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1302,6 +1302,9 @@ update_watchpoint (struct breakpoint *b, int reparse)
if (!watchpoint_in_thread_scope (b))
return;
+ if (b->pspace != current_program_space)
+ 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. */
@@ -1879,6 +1882,7 @@ insert_breakpoint_locations (void)
int val = 0;
int disabled_breaks = 0;
int hw_breakpoint_error = 0;
+ struct program_space *saved_current_program_space = current_program_space;
struct ui_file *tmp_error_stream = mem_fileopen ();
struct cleanup *cleanups = make_cleanup_ui_file_delete (tmp_error_stream);
@@ -1906,9 +1910,13 @@ insert_breakpoint_locations (void)
/* For targets that support global breakpoints, there's no need
to select an inferior to insert breakpoint to. In fact, even
if we aren't attached to any process yet, we should still
- insert breakpoints. */
+ insert breakpoints.
+
+ Also inserting breakpoints into inappropriate inferior must be
+ prevented. */
if (!gdbarch_has_global_breakpoints (target_gdbarch)
- && ptid_equal (inferior_ptid, null_ptid))
+ && (ptid_equal (inferior_ptid, null_ptid)
+ || b->pspace != saved_current_program_space))
continue;
val = insert_bp_location (b, tmp_error_stream,
@@ -1933,13 +1941,19 @@ insert_breakpoint_locations (void)
if (bpt->disposition == disp_del_at_next_stop)
continue;
-
+
for (loc = bpt->loc; loc; loc = loc->next)
- if (!loc->inserted && should_be_inserted (loc))
- {
- some_failed = 1;
- break;
- }
+ {
+ /* Verify the first loop above really tried to insert this LOC. */
+ if (!loc->inserted && should_be_inserted (loc)
+ && (gdbarch_has_global_breakpoints (target_gdbarch)
+ || (!ptid_equal (inferior_ptid, null_ptid)
+ && loc->pspace == saved_current_program_space)))
+ {
+ some_failed = 1;
+ break;
+ }
+ }
if (some_failed)
{
for (loc = bpt->loc; loc; loc = loc->next)
@@ -8328,6 +8342,7 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
b = set_raw_breakpoint_without_location (NULL, bp_type);
set_breakpoint_number (internal, b);
b->thread = thread;
+ b->pspace = current_program_space;
b->disposition = disp_donttouch;
b->exp = exp;
b->exp_valid_block = exp_valid_block;
@@ -9478,6 +9493,9 @@ update_global_location_list (int should_insert)
int keep_in_target = 0;
int removed = 0;
+ if (old_loc->pspace != current_program_space)
+ continue;
+
/* Skip LOCP entries which will definitely never be needed. Stop either
at or being the one matching OLD_LOC. */
while (locp < bp_location + bp_location_count
--- a/gdb/i386-linux-nat.c
+++ b/gdb/i386-linux-nat.c
@@ -633,10 +633,6 @@ i386_linux_store_inferior_registers (struct target_ops *ops,
}
-/* Support for debug registers. */
-
-static unsigned long i386_linux_dr[DR_CONTROL + 1];
-
/* Get debug register REGNUM value from only the one LWP of PTID. */
static unsigned long
@@ -689,8 +685,6 @@ i386_linux_dr_set_control_callback (int tid, void *control_voidp)
static void
i386_linux_dr_set_control (unsigned long control)
{
- i386_linux_dr[DR_CONTROL] = control;
-
linux_nat_iterate_watchpoint_lwps (i386_linux_dr_set_control_callback,
&control);
}
@@ -721,8 +715,6 @@ i386_linux_dr_set_addr (int regnum, CORE_ADDR addr)
gdb_assert (regnum >= 0 && regnum <= DR_LASTADDR - DR_FIRSTADDR);
- i386_linux_dr[DR_FIRSTADDR + regnum] = addr;
-
data.regnum = regnum;
data.addr = addr;
linux_nat_iterate_watchpoint_lwps (i386_linux_dr_set_addr_callback, &data);
@@ -776,16 +768,20 @@ i386_linux_dr_unset_status (unsigned long mask)
static void
i386_linux_new_thread (ptid_t ptid)
{
+ struct i386_dr_mirror *dr_mirror = i386_dr_mirror_get ();
int i, tid;
+ /* Verify DR_MIRROR is valid. */
+ gdb_assert (PIDGET (ptid) == PIDGET (inferior_ptid));
+
tid = TIDGET (ptid);
if (tid == 0)
tid = PIDGET (ptid);
- for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++)
- i386_linux_dr_set (tid, i, i386_linux_dr[i]);
+ for (i = 0; i < DR_LASTADDR - DR_FIRSTADDR; i++)
+ i386_linux_dr_set (tid, DR_FIRSTADDR + i, dr_mirror->addr[i]);
- i386_linux_dr_set (tid, DR_CONTROL, i386_linux_dr[DR_CONTROL]);
+ i386_linux_dr_set (tid, DR_CONTROL, dr_mirror->control);
}
--- a/gdb/i386-nat.c
+++ b/gdb/i386-nat.c
@@ -45,7 +45,6 @@ struct i386_dr_low_type i386_dr_low;
#define TARGET_HAS_DR_LEN_8 (i386_dr_low.debug_register_length == 8)
/* Debug registers' indices. */
-#define DR_NADDR 4 /* The number of debug address registers. */
#define DR_STATUS 6 /* Index of debug status register (DR6). */
#define DR_CONTROL 7 /* Index of debug control register (DR7). */
@@ -105,19 +104,6 @@ struct i386_dr_low_type i386_dr_low;
FIXME: My Intel manual says we should use 0xF800, not 0xFC00. */
#define DR_CONTROL_RESERVED (0xFC00)
-/* Copy of hardware debug registers for performance reasons. */
-
-struct i386_dr_mirror
- {
- /* Mirror the inferior's DRi registers. We keep the status and
- control registers separated because they don't hold addresses. */
- CORE_ADDR addr[DR_NADDR];
- unsigned long status, control;
-
- /* Reference counts for each debug register. */
- int ref_count[DR_NADDR];
- };
-
/* Auxiliary helper macros. */
/* A value that masks all fields in DR7 that are reserved by Intel. */
@@ -229,13 +215,26 @@ struct i386_inferior_data
struct i386_dr_mirror dr_mirror;
};
+static void
+i386_inferior_data_cleanup (struct inferior *inf, void *arg)
+{
+ struct i386_inferior_data *inf_data = arg;
+
+ xfree (inf_data);
+}
+
static struct i386_inferior_data *
i386_inferior_data_get (void)
{
- /* Intermediate patch stub. */
- static struct i386_inferior_data inf_data_local;
struct inferior *inf = current_inferior ();
- struct i386_inferior_data *inf_data = &inf_data_local;
+ struct i386_inferior_data *inf_data;
+
+ inf_data = inferior_data (inf, i386_inferior_data);
+ if (inf_data == NULL)
+ {
+ inf_data = xzalloc (sizeof (*inf_data));
+ set_inferior_data (current_inferior (), i386_inferior_data, inf_data);
+ }
if (inf->pid != ptid_get_pid (inferior_ptid))
{
@@ -260,7 +259,7 @@ i386_inferior_data_get (void)
/* Clear the reference counts and forget everything we knew about the
debug registers. */
-static struct i386_dr_mirror *
+struct i386_dr_mirror *
i386_dr_mirror_get (void)
{
return &i386_inferior_data_get ()->dr_mirror;
@@ -761,6 +760,10 @@ i386_use_watchpoints (struct target_ops *t)
t->to_remove_watchpoint = i386_remove_watchpoint;
t->to_insert_hw_breakpoint = i386_insert_hw_breakpoint;
t->to_remove_hw_breakpoint = i386_remove_hw_breakpoint;
+
+ if (i386_inferior_data == NULL)
+ i386_inferior_data
+ = register_inferior_data_with_cleanup (i386_inferior_data_cleanup);
}
void
--- a/gdb/i386-nat.h
+++ b/gdb/i386-nat.h
@@ -78,6 +78,25 @@ struct i386_dr_low_type
extern struct i386_dr_low_type i386_dr_low;
+/* The number of debug address registers. */
+#define DR_NADDR 4
+
+/* Copy of hardware debug registers for performance reasons. */
+
+struct i386_dr_mirror
+ {
+ /* Mirror the inferior's DRi registers. We keep the status and
+ control registers separated because they don't hold addresses. */
+ CORE_ADDR addr[DR_NADDR];
+
+ /* Reference counts for each debug register. */
+ int ref_count[DR_NADDR];
+
+ unsigned long status, control;
+ };
+
+extern struct i386_dr_mirror *i386_dr_mirror_get (void);
+
/* Use this function to set i386_dr_low debug_register_length field
rather than setting it directly to check that the length is only
set once. It also enables the 'maint set/show show-debug-regs'
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1286,13 +1286,17 @@ linux_nat_iterate_watchpoint_lwps
if (inf->pid == inferior_pid)
{
- /* Standard mode. */
- iterate_over_lwps (minus_one_ptid,
+ /* Standard mode. Iterate all the threads of the current inferior.
+ Without specifying INFERIOR_PID it would iterate all the threads of
+ all the inferiors, which is inappropriate for watchpoints. */
+
+ iterate_over_lwps (pid_to_ptid (inferior_pid),
iterate_watchpoint_lwps_callback, &data);
}
else
{
/* Detaching a new child PID temporarily present in INFERIOR_PID. */
+
callback (inferior_pid, callback_data);
}
}
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/watchpoint-multi.c
@@ -0,0 +1,59 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2010 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 <pthread.h>
+#include <assert.h>
+
+static volatile int a, b, c;
+
+static void
+marker_exit1 (void)
+{
+ a = 1;
+}
+
+/* Workaround PR breakpoints/12272 by two different breakpoint locations. */
+static void
+marker_exit2 (void)
+{
+ a = 1;
+}
+
+static void *
+start (void *arg)
+{
+ b = 2;
+ c = 3;
+
+ return NULL;
+}
+
+int
+main (void)
+{
+ pthread_t thread;
+ int i;
+
+ i = pthread_create (&thread, NULL, start, NULL);
+ assert (i == 0);
+ i = pthread_join (thread, NULL);
+ assert (i == 0);
+
+ marker_exit1 ();
+ marker_exit2 ();
+ return 0;
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/watchpoint-multi.exp
@@ -0,0 +1,113 @@
+# Copyright 2010 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/>.
+
+if { [is_remote target] || ![isnative] } then {
+ continue
+}
+
+set testfile "watchpoint-multi"
+
+set executable ${testfile}
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${executable}
+
+if { [gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+ untested ${testfile}.exp
+ return -1
+}
+
+clean_restart $executable
+
+if ![runto_main] {
+ return
+}
+# Never keep/use any non-hw breakpoints to workaround a multi-inferior bug.
+delete_breakpoints
+
+gdb_test "add-inferior" "Added inferior 2"
+gdb_test "inferior 2" "witching to inferior 2 .*"
+gdb_load $binfile
+
+if ![runto_main] {
+ return
+}
+delete_breakpoints
+
+# Simulate non-stop+target-async which also uses breakpoint always-inserted.
+gdb_test_no_output "set breakpoint always-inserted on"
+# displaced-stepping is also needed as other GDB sometimes still removes the
+# breakpoints, even with always-inserted on.
+gdb_test_no_output "set displaced-stepping on"
+
+# Debugging of this testcase:
+#gdb_test_no_output "maintenance set show-debug-regs on"
+#gdb_test_no_output "set debug infrun 1"
+
+# Do not use simple hardware watchpoint ("watch") as its false hit may be
+# unnoticed by GDB if it reads it still has the same value.
+gdb_test "awatch c" "Hardware access \\(read/write\\) watchpoint \[0-9\]+: c"
+# Never keep/use any non-hw breakpoints to workaround a multi-inferior bug.
+# Use `*' to workaround a multi-inferior bug.
+set test "hbreak *marker_exit2"
+gdb_test_multiple $test $test {
+ -re "Hardware assisted breakpoint \[0-9\]+ at .*\r\n$gdb_prompt $" {
+ pass $test
+ }
+ -re "(No hardware breakpoint support in the target\\.|Hardware breakpoints used exceeds limit\\.)\r\n$gdb_prompt $" {
+ pass $test
+ untested ${testfile}.exp
+ return
+ }
+}
+
+gdb_test "inferior 1" "witching to inferior 1 .*"
+
+gdb_test "awatch b" "Hardware access \\(read/write\\) watchpoint \[0-9\]+: b"
+gdb_test "hbreak *marker_exit1" {Hardware assisted breakpoint [0-9]+ at .*}
+
+gdb_test "inferior 2" "witching to inferior 2 .*"
+
+# FAIL would be a hit on watchpoint for `b' - that one is for the other
+# inferior.
+gdb_test "continue" "Hardware access \\(read/write\\) watchpoint \[0-9\]+: c\r\n\r\nOld value = 0\r\nNew value = 3\r\n.*" "catch c"
+
+set test "catch marker_exit2"
+gdb_test_multiple "continue" $test {
+ -re "Breakpoint \[0-9\]+, marker_exit2 .*\r\n$gdb_prompt $" {
+ setup_kfail breakpoints/12312 *-*-*
+ pass $test
+ }
+ -re "Hardware access \\(read/write\\) watchpoint \[0-9\]+: c\r\n\r\nValue = 3\r\n.* in __nptl_death_event .*\r\n$gdb_prompt $" {
+ setup_kfail breakpoints/12312 *-*-*
+ fail $test
+ }
+}
+
+gdb_test "inferior 1" "witching to inferior 1 .*"
+
+gdb_test "continue" "Hardware access \\(read/write\\) watchpoint \[0-9\]+: b\r\n\r\nOld value = 0\r\nNew value = 2\r\n.*" "catch b"
+
+set test "catch marker_exit1"
+gdb_test_multiple "continue" $test {
+ -re "Breakpoint \[0-9\]+, marker_exit1 .*\r\n$gdb_prompt $" {
+ setup_kfail breakpoints/12312 *-*-*
+ pass $test
+ }
+ -re "Hardware access \\(read/write\\) watchpoint \[0-9\]+: b\r\n\r\nValue = 2\r\n.* in __nptl_death_event .*\r\n$gdb_prompt $" {
+ setup_kfail breakpoints/12312 *-*-*
+ fail $test
+ }
+}
+