[patch 2/2] Fix watchpoints for multi-inferior #2

Jan Kratochvil
2012-01-20T21:31:38Z

On Mon, 02 Jan 2012 20:14:23 +0100, Pedro Alves wrote:
> Do we clear inf_data or inf_data's contents anywhere on inferior
> exit or startup, so to not leave debug registers stale across runs?
> (The cleanup only runs when the inferior is deleted.)

Yes, it is already cleared in FSF GDB. Plus I think this issue is unrelated
to this multi-inferiorization patch.

#0 i386_init_dregs (state=0x24ad330) at i386-nat.c:165
#1 in i386_cleanup_dregs () at i386-nat.c:311
#2 in amd64_linux_child_post_startup_inferior (ptid=...) at amd64-linux-nat.c:502
#3 in inf_ptrace_create_inferior (ops=0x1f4d100, exec_file=0x20d4390 "/home/jkratoch/redhat/archer-jankratochvil-watchpoint3/gdb/gdb", allargs=0x24ad3d0 "", env=0x20b84a0, from_tty=1) at inf-ptrace.c:148

> >--- /dev/null
> >+++ b/gdb/testsuite/gdb.multi/watchpoint-multi.exp
[...]
> >+if [is_remote target] {
> >+ # It is KFAIL.
> >+ continue
>
> Did you mean to turn this into a real kfail? What are the
> gdbserver problems, btw?

It is no longer KFAIL, included gdbserver fixes.

The first one is for dead-loop of:
Got an event from pending child 10373 (057f)
Got a pending child 10373
Got an event from pending child 10373 (057f)
Got a pending child 10373
because linux_wait_for_event creates creates status_pending_p and then asks
linux_wait_for_event_1 for the next event which apparently returns the newly
created status_pending_p so linux_wait_for_event stores it back and so on.

The second fix is that despite default `set schedule-multiple off' gdbserver
sometimes resumed all the inferiors on GDB "continue".

Both cases are visible with the testcase (the first one in ~50% of runs).

> >+# 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"
>
> 'if ![support_displaced_stepping] bail' missing, as well as the usual
> hw watchpoints support checks.

Fixed.

The testcase runs in extended-remote mode only with Pedro's new board I have
not seen but it follows the advice:
Re: testsuite: native/non-extended/extended modes
http://sourceware.org/ml/gdb-patches/2012-01/msg00740.html

No regressions on {x86_64,x86_64-m32,i686}-fedorarawhide-linux-gnu and in
gdbserver mode and on ppc64-rhel62-linux-gnu.

Thanks,
Jan

gdb/
2012-01-20 Jan Kratochvil <jan.kratochvil@redhat.com>

Fix watchpoints to be specific for each inferior.
* breakpoint.c (watchpoint_in_thread_scope): Verify also
current_program_space.
* i386-nat.c (i386_inferior_data_cleanup): New.
(i386_inferior_data_get): Replace variable inf_data_local by an
inferior_data call.
(i386_use_watchpoints): Initialize i386_inferior_data.
* linux-nat.c (linux_nat_iterate_watchpoint_lwps): Use INFERIOR_PTID
specific iterate_over_lwps.

gdb/gdbserver/
2012-01-20 Jan Kratochvil <jan.kratochvil@redhat.com>

* linux-low.c (linux_wait_for_event_1): Rename to ...
(linux_wait_for_event): ... here and merge it with former
linux_wait_for_event - new variable wait_ptid, use it.
(linux_wait_for_event): Remove - merge it to linux_wait_for_event_1.
(linux_wait_1): Resume only single inferior if CONT_THREAD is set so.

Gdb/testsuite/
2012-01-20 Jan Kratochvil <jan.kratochvil@redhat.com>

Fix watchpoints to be specific for each inferior.
* gdb.multi/watchpoint-multi.c: New file.
* gdb.multi/watchpoint-multi.exp: New file.

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1239,9 +1239,10 @@ is_watchpoint (const struct breakpoint *bpt)
static int
watchpoint_in_thread_scope (struct watchpoint *b)
{
- return (ptid_equal (b->watchpoint_thread, null_ptid)
- || (ptid_equal (inferior_ptid, b->watchpoint_thread)
- && !is_executing (inferior_ptid)));
+ return (b->base.pspace == current_program_space
+ && (ptid_equal (b->watchpoint_thread, null_ptid)
+ || (ptid_equal (inferior_ptid, b->watchpoint_thread)
+ && !is_executing (inferior_ptid))));
}

/* Set watchpoint B to disp_del_at_next_stop, even including its possible
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -1569,9 +1569,10 @@ ptid_t step_over_bkpt;
the stopped child otherwise. */

static int
-linux_wait_for_event_1 (ptid_t ptid, int *wstat, int options)
+linux_wait_for_event (ptid_t ptid, int *wstat, int options)
{
struct lwp_info *event_child, *requested_child;
+ ptid_t wait_ptid;

event_child = NULL;
requested_child = NULL;
@@ -1621,13 +1622,24 @@ linux_wait_for_event_1 (ptid_t ptid, int *wstat, int options)
return lwpid_of (event_child);
}

+ if (ptid_is_pid (ptid))
+ {
+ /* A request to wait for a specific tgid. This is not possible
+ with waitpid, so instead, we wait for any child, and leave
+ children we're not interested in right now with a pending
+ status to report later. */
+ wait_ptid = minus_one_ptid;
+ }
+ else
+ wait_ptid = ptid;
+
/* We only enter this loop if no process has a pending wait status. Thus
any action taken in response to a wait status inside this loop is
responding as soon as we detect the status, not after any pending
events. */
while (1)
{
- event_child = linux_wait_for_lwp (ptid, wstat, options);
+ event_child = linux_wait_for_lwp (wait_ptid, wstat, options);

if ((options & WNOHANG) && event_child == NULL)
{
@@ -1639,6 +1651,19 @@ linux_wait_for_event_1 (ptid_t ptid, int *wstat, int options)
if (event_child == NULL)
error ("event from unknown child");

+ if (ptid_is_pid (ptid)
+ && ptid_get_pid (ptid) != ptid_get_pid (ptid_of (event_child)))
+ {
+ if (! WIFSTOPPED (*wstat))
+ mark_lwp_dead (event_child, *wstat);
+ else
+ {
+ event_child->status_pending_p = 1;
+ event_child->status_pending = *wstat;
+ }
+ continue;
+ }
+
current_inferior = get_lwp_thread (event_child);

/* Check for thread exit. */
@@ -1731,48 +1756,6 @@ linux_wait_for_event_1 (ptid_t ptid, int *wstat, int options)
return 0;
}

-static int
-linux_wait_for_event (ptid_t ptid, int *wstat, int options)
-{
- ptid_t wait_ptid;
-
- if (ptid_is_pid (ptid))
- {
- /* A request to wait for a specific tgid. This is not possible
- with waitpid, so instead, we wait for any child, and leave
- children we're not interested in right now with a pending
- status to report later. */
- wait_ptid = minus_one_ptid;
- }
- else
- wait_ptid = ptid;
-
- while (1)
- {
- int event_pid;
-
- event_pid = linux_wait_for_event_1 (wait_ptid, wstat, options);
-
- if (event_pid > 0
- && ptid_is_pid (ptid) && ptid_get_pid (ptid) != event_pid)
- {
- struct lwp_info *event_child
- = find_lwp_pid (pid_to_ptid (event_pid));
-
- if (! WIFSTOPPED (*wstat))
- mark_lwp_dead (event_child, *wstat);
- else
- {
- event_child->status_pending_p = 1;
- event_child->status_pending = *wstat;
- }
- }
- else
- return event_pid;
- }
-}
-
-
/* Count the LWP's that have had events. */

static int
@@ -2107,7 +2090,14 @@ retry:
if (thread == NULL)
{
struct thread_resume resume_info;
- resume_info.thread = minus_one_ptid;
+
+ /* Resume only a single process if requested so. */
+ if (!ptid_equal (cont_thread, minus_one_ptid)
+ && ptid_get_lwp (cont_thread) == -1)
+ resume_info.thread = cont_thread;
+ else
+ resume_info.thread = minus_one_ptid;
+
resume_info.kind = resume_continue;
resume_info.sig = 0;
linux_resume (&resume_info, 1);
--- a/gdb/i386-nat.c
+++ b/gdb/i386-nat.c
@@ -181,16 +181,31 @@ struct i386_inferior_data
struct i386_debug_reg_state state;
};

+/* Per-inferior hook for register_inferior_data_with_cleanup. */
+
+static void
+i386_inferior_data_cleanup (struct inferior *inf, void *arg)
+{
+ struct i386_inferior_data *inf_data = arg;
+
+ xfree (inf_data);
+}
+
/* Get data specific for INFERIOR_PTID LWP. Return special data area
for processes being detached. */

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))
{
@@ -856,6 +871,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
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/watchpoint-multi.c
@@ -0,0 +1,51 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2012 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_exit (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_exit ();
+ return 0;
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/watchpoint-multi.exp
@@ -0,0 +1,91 @@
+# Copyright 2012 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/>.
+
+set testfile "watchpoint-multi"
+
+# Multiple inferiors are needed, therefore both native and extended gdbserver
+# modes are supported. Only non-extended gdbserver is not supported.
+if [target_info exists use_gdb_stub] {
+ untested ${testfile}.exp
+ return
+}
+
+# Do not use simple hardware watchpoints ("watch") as its false hit may be
+# unnoticed by GDB if it reads it still has the same value.
+if [skip_hw_watchpoint_access_tests] {
+ untested "${testfile} (no hardware access watchpoints)"
+ return
+}
+
+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
+
+# 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.
+# Without the support this test just is not as thorough as it could be.
+if [support_displaced_stepping] {
+ 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"
+
+gdb_breakpoint main {temporary}
+gdb_test "run" "Temporary breakpoint.* main .*" "start to main inferior 1"
+
+gdb_test "add-inferior" "Added inferior 2"
+gdb_test "inferior 2" "witching to inferior 2 .*"
+gdb_load $binfile
+
+gdb_breakpoint main {temporary}
+gdb_test "run" "Temporary breakpoint.* main .*" "start to main inferior 2"
+
+gdb_test "awatch c" "Hardware access \\(read/write\\) watchpoint \[0-9\]+: c"
+
+gdb_breakpoint "marker_exit"
+
+gdb_test "inferior 1" "witching to inferior 1 .*"
+
+if [skip_hw_watchpoint_multi_tests] {
+ # On single hardware watchpoint at least test the watchpoint in inferior
+ # 2 is not hit.
+} else {
+ gdb_test "awatch b" "Hardware access \\(read/write\\) watchpoint \[0-9\]+: b"
+
+ 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"
+
+ gdb_test "continue" "Breakpoint \[0-9\]+, marker_exit .*" "catch marker_exit in inferior 2"
+
+ 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"
+}
+
+gdb_test "continue" "Breakpoint \[0-9\]+, marker_exit .*" "catch marker_exit in inferior 1"