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

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

[Jan Kratochvil]

Hi,

this is a new post of:
	[patch 4/4] hw watchpoints made multi-inferior
	http://sourceware.org/ml/gdb-patches/2010-12/msg00045.html

Currently watchpoints somehow apply to all the inferiors but they do not work
that way anyway.  I believe watchpoints should be specific for each inferior.

The testcase has some problems with gdbserver, it has been skipped now.

No regressions on {x86_64,x86_64-m32,i686}-fedorarawhide-linux-gnu.

For ppc* the testcase will be difficult, to test the issue properly one needs
`awatch' (that is `watch' can hide a problem) but PPC does not feature
`awatch'.


Thanks,
Jan


gdb/
2012-01-02  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/testsuite/
2012-01-02  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
@@ -1225,9 +1225,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/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
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1288,7 +1288,11 @@ linux_nat_iterate_watchpoint_lwps
 
   if (inf->pid == inferior_pid)
     {
-      iterate_over_lwps (minus_one_ptid, callback, callback_data);
+      /* Iterate all the threads of the current inferior.  Without specifying
+	 INFERIOR_PID it would iterate all threads of all inferiors, which is
+	 inappropriate for watchpoints.  */
+
+      iterate_over_lwps (pid_to_ptid (inferior_pid), callback, callback_data);
     }
   else
     {
--- /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,92 @@
+# 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/>.
+
+if [is_remote target] {
+    # It is KFAIL.
+    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"
+
+gdb_breakpoint "marker_exit"
+
+gdb_test "inferior 1" "witching to inferior 1 .*"
+
+set have_awatch_b 0
+set test "awatch b"
+gdb_test_multiple $test $test {
+    -re "Hardware access \\(read/write\\) watchpoint \[0-9\]+: b\r\n$gdb_prompt $" {
+	pass $test
+	set have_awatch_b 1
+    }
+    -re "There are not enough available hardware resources for this watchpoint\\.\r\n$gdb_prompt $" {
+	untested $test
+	return
+    }
+}
+
+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"

Re: [patch 2/2] Fix watchpoints for multi-inferior

[Pedro Alves]

On 01/02/2012 04:46 PM, Jan Kratochvil wrote:
> Hi,
>
> this is a new post of:
> 	[patch 4/4] hw watchpoints made multi-inferior
> 	http://sourceware.org/ml/gdb-patches/2010-12/msg00045.html
>
> Currently watchpoints somehow apply to all the inferiors but they do not work
> that way anyway.  I believe watchpoints should be specific for each inferior.

Yeah.

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

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


> --- /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,92 @@
> +# 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/>.
> +
> +if [is_remote target] {
> +    # It is KFAIL.
> +    continue

Did you mean to turn this into a real kfail?  What are the
gdbserver problems, btw?

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

Otherwise looks okay.

-- 
Pedro Alves

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

[Jan Kratochvil]

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"

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

[Pedro Alves]

On 01/20/2012 09:31 PM, Jan Kratochvil wrote:
> 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.

Good, thanks.

> Plus I think this issue is unrelated to this multi-inferiorization patch.

It would be if the multi-inferiorization would make debug registers stale,
hence my question.  Please try to keep an open spirit.

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

Ah.  Could you please split the gdbserver bits into a separate patch?
I'd like to take a good look at them, but if the watchpoint
bits proper are already in, it'd be easier.  The non-gdbserver bits
look okay to me.

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

Just above we see:

      thread = (struct thread_info *) find_inferior_id (&all_threads,
							cont_thread);

      /* No stepping, no signal - unless one is pending already, of course.  */
      if (thread == NULL)

So, cont_thread does not exist, which was the whole point of reaching
here.  Therefore there's no use trying to resuming it (at first sight).

BTW, I have just recently stumbled on this:

 http://sourceware.org/ml/gdb-patches/2012-01/msg00502.html

But as said, I'll need to take a better look at the gdbserver bits.

> +	  else
> +	    resume_info.thread = minus_one_ptid;
> +
>  	  resume_info.kind = resume_continue;
>  	  resume_info.sig = 0;
>  	  linux_resume (&resume_info, 1);

-- 
Pedro Alves

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

[Jan Kratochvil]

On Tue, 24 Jan 2012 14:19:34 +0100, Pedro Alves wrote:
> Please try to keep an open spirit.

This patch series took 4+ years (Oct 2007) with 5 repost series total, that is
a long time.


> Ah.  Could you please split the gdbserver bits into a separate patch?

Yes.  To be reposted.


> The non-gdbserver bits look okay to me.

Checked in.


Thanks,
Jan


http://sourceware.org/ml/gdb-cvs/2012-01/msg00197.html

--- src/gdb/ChangeLog	2012/01/24 13:46:50	1.13767
+++ src/gdb/ChangeLog	2012/01/24 13:49:53	1.13768
@@ -1,5 +1,17 @@
 2012-01-24  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.
+
+2012-01-24  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
 	Fix watchpoints across inferior fork.
 	* amd64-linux-nat.c (update_debug_registers_callback): Update the
 	comment for linux_nat_iterate_watchpoint_lwps.
--- src/gdb/testsuite/ChangeLog	2012/01/24 13:46:54	1.3033
+++ src/gdb/testsuite/ChangeLog	2012/01/24 13:49:58	1.3034
@@ -1,5 +1,11 @@
 2012-01-24  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.
+
+2012-01-24  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
 	Fix watchpoints across inferior fork.
 	* gdb.threads/watchpoint-fork-child.c: New file.
 	* gdb.threads/watchpoint-fork-mt.c: New file.
--- src/gdb/breakpoint.c	2012/01/16 20:40:50	1.642
+++ src/gdb/breakpoint.c	2012/01/24 13:49:55	1.643
@@ -1239,9 +1239,10 @@
 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
--- src/gdb/i386-nat.c	2012/01/24 13:46:54	1.39
+++ src/gdb/i386-nat.c	2012/01/24 13:49:56	1.40
@@ -181,16 +181,31 @@
   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))
     {
@@ -855,6 +870,10 @@
   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
--- src/gdb/testsuite/gdb.multi/watchpoint-multi.c
+++ src/gdb/testsuite/gdb.multi/watchpoint-multi.c	2012-01-24 13:52:56.759120000 +0000
@@ -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;
+}
--- src/gdb/testsuite/gdb.multi/watchpoint-multi.exp
+++ src/gdb/testsuite/gdb.multi/watchpoint-multi.exp	2012-01-24 13:52:57.147228000 +0000
@@ -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"

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

[Jan Kratochvil]

On Tue, 24 Jan 2012 14:19:34 +0100, Pedro Alves wrote:
> On 01/20/2012 09:31 PM, Jan Kratochvil wrote:
> > @@ -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;
> 
> Just above we see:
> 
>       thread = (struct thread_info *) find_inferior_id (&all_threads,
> 							cont_thread);
> 
>       /* No stepping, no signal - unless one is pending already, of course.  */
>       if (thread == NULL)
> 
> So, cont_thread does not exist, which was the whole point of reaching
> here.  Therefore there's no use trying to resuming it (at first sight).
> 
> BTW, I have just recently stumbled on this:
> 
>  http://sourceware.org/ml/gdb-patches/2012-01/msg00502.html
> 
> But as said, I'll need to take a better look at the gdbserver bits.

FYI I did not repost this patch part as it needs to be implemented by some
larger code rewrite IMO now, anyway this patch chunk is not good according to
your review.


Thanks,
Jan

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

[Pedro Alves]

On 01/25/2012 03:22 PM, Jan Kratochvil wrote:
> On Tue, 24 Jan 2012 14:19:34 +0100, Pedro Alves wrote:
>> On 01/20/2012 09:31 PM, Jan Kratochvil wrote:
>>> @@ -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;
>>
>> Just above we see:
>>
>>       thread = (struct thread_info *) find_inferior_id (&all_threads,
>> 							cont_thread);
>>
>>       /* No stepping, no signal - unless one is pending already, of course.  */
>>       if (thread == NULL)
>>
>> So, cont_thread does not exist, which was the whole point of reaching
>> here.  Therefore there's no use trying to resuming it (at first sight).
>>
>> BTW, I have just recently stumbled on this:
>>
>>  http://sourceware.org/ml/gdb-patches/2012-01/msg00502.html
>>
>> But as said, I'll need to take a better look at the gdbserver bits.
> 
> FYI I did not repost this patch part as it needs to be implemented by some
> larger code rewrite IMO now, anyway this patch chunk is not good according to
> your review.

Yeah.  That cont_thread bit should really go away.

I've been looking at your other patch (the linux_wait_for_event_1 one), and
seeing:

(gdb) run
Starting program: /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.multi/watchpoint-multi
handling possible serial event
getpkt ("QDisableRandomization:1");  [no ack sent]
[address space randomization disabled]
putpkt ("$OK#9a"); [noack mode]
handling possible serial event
getpkt ("vRun;2f686f6d652f706564726f2f6764622f6d796769742f6275696c642f6764622f7465737473756974652f6764622e6d756c74692f7761746368706f696e742d6d756c7469");  [no ack sent]
new_argv[0] = "/home/pedro/gdb/mygit/build/gdb/testsuite/gdb.multi/watchpoint-multi"
Process /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.multi/watchpoint-multi created; pid = 21270
linux_wait: [Process 21270]
pc is 0x40060f
Need step over [LWP 21269]? yes, but found GDB breakpoint at 0x40060f; skipping step over
Need step over [LWP 21270]? Ignoring, not stopped
Resuming, no pending status or step over needed
resuming LWP 21269
pc is 0x40060f
Resuming lwp 21269 (continue, signal 0, stop not expected)
  resuming from pc 0x40060f
resuming LWP 21270
linux_wait_for_lwp: <all threads>

And I had a wth moment -- Why are we resuming 21269 at all, since
we just spawned 21270.  I then realized that it is resumed exactly
that by broken cont_thread code in linux_wait_1...

I really would like to get back to getting rid of those cont_thread
bits, but, this patch, very similar to the one linked above (which fixed
it for vAttach), completely fixes this testcase as well.  The issue is
that cont_thread is also stale from the previous run, when we
start a new vRun.  So I think the patch below is correct, and should
be applied.

You patch may _also_ be correct, and necessary for other cases.
I'll go back to staring at it.

-- 
Pedro Alves

2012-01-25  Pedro Alves  <palves@redhat.com>

	* server.c (start_inferior): Clear `cont_thread'.

---

 0 files changed, 0 insertions(+), 0 deletions(-)

diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index c1788a9..9c50ecc 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -259,6 +259,10 @@ start_inferior (char **argv)
   signal (SIGTTIN, SIG_DFL);
 #endif

+  /* Clear this so the backend doesn't get confused, thinking
+     CONT_THREAD died, and it needs to resume all threads.  */
+  cont_thread = null_ptid;
+
   signal_pid = create_inferior (new_argv[0], new_argv);

   /* FIXME: we don't actually know at this point that the create

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

[Pedro Alves]

On 01/25/2012 05:27 PM, Pedro Alves wrote:
> On 01/25/2012 03:22 PM, Jan Kratochvil wrote:
>> On Tue, 24 Jan 2012 14:19:34 +0100, Pedro Alves wrote:
>>> On 01/20/2012 09:31 PM, Jan Kratochvil wrote:
>>>> @@ -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;
>>>
>>> Just above we see:
>>>
>>>       thread = (struct thread_info *) find_inferior_id (&all_threads,
>>> 							cont_thread);
>>>
>>>       /* No stepping, no signal - unless one is pending already, of course.  */
>>>       if (thread == NULL)
>>>
>>> So, cont_thread does not exist, which was the whole point of reaching
>>> here.  Therefore there's no use trying to resuming it (at first sight).
>>>
>>> BTW, I have just recently stumbled on this:
>>>
>>>  http://sourceware.org/ml/gdb-patches/2012-01/msg00502.html
>>>
>>> But as said, I'll need to take a better look at the gdbserver bits.
>>
>> FYI I did not repost this patch part as it needs to be implemented by some
>> larger code rewrite IMO now, anyway this patch chunk is not good according to
>> your review.
> 
> Yeah.  That cont_thread bit should really go away.
> 
> I've been looking at your other patch (the linux_wait_for_event_1 one), and
> seeing:
> 
> (gdb) run
> Starting program: /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.multi/watchpoint-multi
> handling possible serial event
> getpkt ("QDisableRandomization:1");  [no ack sent]
> [address space randomization disabled]
> putpkt ("$OK#9a"); [noack mode]
> handling possible serial event
> getpkt ("vRun;2f686f6d652f706564726f2f6764622f6d796769742f6275696c642f6764622f7465737473756974652f6764622e6d756c74692f7761746368706f696e742d6d756c7469");  [no ack sent]
> new_argv[0] = "/home/pedro/gdb/mygit/build/gdb/testsuite/gdb.multi/watchpoint-multi"
> Process /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.multi/watchpoint-multi created; pid = 21270
> linux_wait: [Process 21270]
> pc is 0x40060f
> Need step over [LWP 21269]? yes, but found GDB breakpoint at 0x40060f; skipping step over
> Need step over [LWP 21270]? Ignoring, not stopped
> Resuming, no pending status or step over needed
> resuming LWP 21269
> pc is 0x40060f
> Resuming lwp 21269 (continue, signal 0, stop not expected)
>   resuming from pc 0x40060f
> resuming LWP 21270
> linux_wait_for_lwp: <all threads>
> 
> And I had a wth moment -- Why are we resuming 21269 at all, since
> we just spawned 21270.  I then realized that it is resumed exactly
> that by broken cont_thread code in linux_wait_1...
> 
> I really would like to get back to getting rid of those cont_thread
> bits, but, this patch, very similar to the one linked above (which fixed
> it for vAttach), completely fixes this testcase as well.

Bah, no, it is still not sufficient.  Don't know why it passed for me
before.  Looking again...

> The issue is that cont_thread is also stale from the previous run, when we
> start a new vRun.  So I think the patch below is correct, and should
> be applied.

-- 
Pedro Alves

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

[Pedro Alves]

On 01/25/2012 06:03 PM, Pedro Alves wrote:
> On 01/25/2012 05:27 PM, Pedro Alves wrote:
>> On 01/25/2012 03:22 PM, Jan Kratochvil wrote:

>> I really would like to get back to getting rid of those cont_thread
>> bits, but, this patch, very similar to the one linked above (which fixed
>> it for vAttach), completely fixes this testcase as well.
> 
> Bah, no, it is still not sufficient.  Don't know why it passed for me
> before.  Looking again...
> 
>> The issue is that cont_thread is also stale from the previous run, when we
>> start a new vRun.  So I think the patch below is correct, and should
>> be applied.

This one is sufficient.  The hints were all in your patch already, but
I guess I'm slow today.

Makes no sense setting the cont_thread to a process wildcard, there's no
such thing in the protocol.  So this brings in a variant of your patch,
that fixes it early on, instead of late in the target, along with clearing
cont_thread on vRun.

gdb/gdbserver/
2012-01-25  Pedro Alves  <palves@redhat.com>
	    Jan Kratochvil  <jan.kratochvil@redhat.com>

	* server.c (start_inferior): Clear `cont_thread'.
	(handle_v_cont): Don't set `cont_thread' if resuming all threads
	of a process.
---

 gdb/gdbserver/server.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index c1788a9..aedf6f5 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -259,6 +259,10 @@ start_inferior (char **argv)
   signal (SIGTTIN, SIG_DFL);
 #endif

+  /* Clear this so the backend doesn't get confused, thinking
+     CONT_THREAD died, and it needs to resume all threads.  */
+  cont_thread = null_ptid;
+
   signal_pid = create_inferior (new_argv[0], new_argv);

   /* FIXME: we don't actually know at this point that the create
@@ -1902,7 +1906,8 @@ handle_v_cont (char *own_buf)

   /* Still used in occasional places in the backend.  */
   if (n == 1
-      && !ptid_equal (resume_info[0].thread, minus_one_ptid)
+      && !(ptid_equal (resume_info[0].thread, minus_one_ptid)
+	   || ptid_get_lwp (resume_info[0].thread) == -1)
       && resume_info[0].kind != resume_stop)
     cont_thread = resume_info[0].thread;
   else

[patch] protocol doc vs. gdbserver on H and pPID.-1 etc. [Re: [patch 2/2] Fix watchpoints for multi-inferior #2]

[Jan Kratochvil]

On Wed, 25 Jan 2012 20:59:56 +0100, Pedro Alves wrote:
> Makes no sense setting the cont_thread to a process wildcard, there's no
> such thing in the protocol.  So this brings in a variant of your patch,
> that fixes it early on, instead of late in the target, along with clearing
> cont_thread on vRun.

This behavior matches what what does gdbserver for the `H' packet - it also
does not recognize pPID.-1.

But gdb.texinfo implicitly says pPID.-1 should be recognized as pPID but that
does not correspond to what gdbserver does.

So I made somehow doc <-> gdbserver in sync and made it all more clear.
Do you agree with it this way?


(I do not think that patch of yours / idea of it comes from me, feel free to
even remove my credit but I am also fine with the credit kept there.)


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


Thanks,
Jan


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

	* gdb.texinfo (Packets): Document values for the `H' packet.
	Deprecate `Hc'.

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

	* server.c (cont_thread): New comment for it.
	(process_serial_event): Handle pPID.-1 values for 'H'.

--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -33798,7 +33798,13 @@ it should be @samp{c} for step and continue operations (note that this
 is deprecated, supporting the @samp{vCont} command is a better
 option), @samp{g} for other operations.  The thread designator
 @var{thread-id} has the format and interpretation described in
-@ref{thread-id syntax}.
+@ref{thread-id syntax} - value @samp{0} will act like @samp{-1} - to
+choose all threads of all inferiors.  @samp{pPID} (or equivalent
+@samp{pPID.-1}) means to choose any single thread of that @samp{PID}.
+It does not mean all threads of that @samp{PID} process - use
+@samp{vCont} packet instead in such case.
+
+Use of @samp{Hc} is deprecated in favor of @xref{vCont packet}.
 
 Reply:
 @table @samp
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -29,7 +29,15 @@
 #include <sys/wait.h>
 #endif
 
+/* Deprecated.  See gdb.texinfo description of the `Hc' packet - use
+   'vCont' packet instead.  It can be null_ptid for no inferior,
+   minus_one_ptid for resuming all inferior or a specific thread ptid_t.
+   Particularly process wildcard (pid > 0 && lwp == -1) for resuming
+   whole one inferior is not supported.  This variable is provided only
+   for backward compatibility with both clients and backend, backend
+   'resume' method supports process wildcards instead.  */
 ptid_t cont_thread;
+
 ptid_t general_thread;
 
 int server_waiting;
@@ -2980,8 +2988,8 @@ process_serial_event (void)
 	      || ptid_equal (gdb_id, minus_one_ptid))
 	    thread_id = null_ptid;
 	  else if (pid != 0
-		   && ptid_equal (pid_to_ptid (pid),
-				  gdb_id))
+		   && (ptid_equal (pid_to_ptid (pid), gdb_id)
+		       || ptid_equal (ptid_build (pid, -1, 0), gdb_id)))
 	    {
 	      struct thread_info *thread =
 		(struct thread_info *) find_inferior (&all_threads,

Re: [patch] protocol doc vs. gdbserver on H and pPID.-1 etc. [Re: [patch 2/2] Fix watchpoints for multi-inferior #2]

[Pedro Alves]

Hi Jan,

On 01/26/2012 09:50 PM, Jan Kratochvil wrote:
> On Wed, 25 Jan 2012 20:59:56 +0100, Pedro Alves wrote:
>> Makes no sense setting the cont_thread to a process wildcard, there's no
>> such thing in the protocol.  So this brings in a variant of your patch,
>> that fixes it early on, instead of late in the target, along with clearing
>> cont_thread on vRun.
> 
> This behavior matches what what does gdbserver for the `H' packet - it also
> does not recognize pPID.-1.
> 
> But gdb.texinfo implicitly says pPID.-1 should be recognized as pPID but that
> does not correspond to what gdbserver does.

A stub that supports multi-process is required to support vCont -- vCont
is not optional in that case.  When you use vCont, Hc is not used.  They're mutually
exclusive.  This what I was thinking about when I said Hc pPID.-1 does not
really make sense in the protocol.  It could have some meaning, but we chose not to
support it, since s/c/Hc is broken anyway for multi-threaded, and vCont fixes
the breakage.  Since multiprocess extensions were new, we just made vCont
required.  As such, whatever gdbserver may be doing with Hc pPID.-1 doesn't
really matter much as it's undefined territory -- GDB will never send it.

  The stub must support @samp{vCont} if it reports support for
  multiprocess extensions (@pxref{multiprocess extensions}).  Note that in
  this case @samp{vCont} actions can be specified to apply to all threads
  in a process by using the @samp{p@var{pid}.-1} form of the
  @var{thread-id}.

Unfortunately, the text has been misplaced until very recently:

 http://sourceware.org/ml/gdb-patches/2012-01/msg00908.html

> 
> So I made somehow doc <-> gdbserver in sync and made it all more clear.
> Do you agree with it this way?

It is more important what does GDB do and the docs say than what
gdbserver does, especially with regards to Hc, since gdbserver hasn't
been relying on it for a long while, and the multi-process changes may
have changed the legacy Hc support by mistake (I added support for
starting gdbserver with --disable-packet=vCont so we can occasionally test
it in legacy modes easily, though).

> gdb/doc/
> 2012-01-26  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* gdb.texinfo (Packets): Document values for the `H' packet.
> 	Deprecate `Hc'.
> 
> gdb/gdbserver/
> 2012-01-26  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* server.c (cont_thread): New comment for it.
> 	(process_serial_event): Handle pPID.-1 values for 'H'.
> 
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -33798,7 +33798,13 @@ it should be @samp{c} for step and continue operations (note that this
>  is deprecated, supporting the @samp{vCont} command is a better
>  option), @samp{g} for other operations.  The thread designator
>  @var{thread-id} has the format and interpretation described in
> -@ref{thread-id syntax}.
> +@ref{thread-id syntax} - value @samp{0} will act like @samp{-1} - to
> +choose all threads of all inferiors.  

This isn't right, and it isn't what gdbserver is doing.

	  if (ptid_equal (gdb_id, null_ptid)
	      || ptid_equal (gdb_id, minus_one_ptid))
	    thread_id = null_ptid;
          ...

	  if (own_buf[1] == 'g')
	    {
	      if (ptid_equal (thread_id, null_ptid))
		{
		  /* GDB is telling us to choose any thread.  Check if
		     the currently selected thread is still valid. If
		     it is not, select the first available.  */
		  struct thread_info *thread =
		    (struct thread_info *) find_inferior_id (&all_threads,
							     general_thread);
		  if (thread == NULL)
		    thread_id = all_threads.head->id;
		}

	      general_thread = thread_id;
	      set_desired_inferior (1);


Hg0 means pick any thread, not all threads.  But that is documented in the first
paragraph of the @ref{thread-id syntax}.  I don't think it's necessary or good
to re-state it here.

> @samp{pPID} (or equivalent
> +@samp{pPID.-1}) means to choose any single thread of that @samp{PID}.
> +It does not mean all threads of that @samp{PID} process - use
> +@samp{vCont} packet instead in such case.

You're only talking about Hc then, right?  It isn't clear
due to the way the text flows.  It looked as though the text
was talking about all kinds of H packets right until the vCont
suggestion in the end.

> +
> +Use of @samp{Hc} is deprecated in favor of @xref{vCont packet}.

This is a good idea.

>  
>  Reply:
>  @table @samp
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -29,7 +29,15 @@
>  #include <sys/wait.h>
>  #endif
>  
> +/* Deprecated.  See gdb.texinfo description of the `Hc' packet - use
> +   'vCont' packet instead.  It can be null_ptid for no inferior,
> +   minus_one_ptid for resuming all inferior or a specific thread ptid_t.
> +   Particularly process wildcard (pid > 0 && lwp == -1) for resuming
> +   whole one inferior is not supported.  This variable is provided only
> +   for backward compatibility with both clients and backend, backend
> +   'resume' method supports process wildcards instead.  */
>  ptid_t cont_thread;
> +
>  ptid_t general_thread;
>  
>  int server_waiting;
> @@ -2980,8 +2988,8 @@ process_serial_event (void)
>  	      || ptid_equal (gdb_id, minus_one_ptid))
>  	    thread_id = null_ptid;
>  	  else if (pid != 0
> -		   && ptid_equal (pid_to_ptid (pid),
> -				  gdb_id))
> +		   && (ptid_equal (pid_to_ptid (pid), gdb_id)
> +		       || ptid_equal (ptid_build (pid, -1, 0), gdb_id)))

The only H packet that makes sense in multiprocess presently is Hg.
Which means select thread foo as general thread.  But `Hg pPID.-1'
would mean all threads of PID, which looks to be a nonsense packet
for GDB to send.

If this is for `Hc pPID.-1', as said above, that packet doesn't
really "exist".  So although we could find some sense to that
packet, this really trades one undefined behavior for another.

So I don't mind if you want to put it in, but IMO, if it doesn't
serve a useful purpose (which I may well be missing), best leave
things as they are.

>  	    {
>  	      struct thread_info *thread =
>  		(struct thread_info *) find_inferior (&all_threads,


-- 
Pedro Alves

Re: [patch] protocol doc vs. gdbserver on H and pPID.-1 etc. [Re: [patch 2/2] Fix watchpoints for multi-inferior #2]

[Pedro Alves]

On 01/26/2012 09:50 PM, Jan Kratochvil wrote:
> +/* Deprecated.  See gdb.texinfo description of the `Hc' packet - use
> +   'vCont' packet instead.  It can be null_ptid for no inferior,
> +   minus_one_ptid for resuming all inferior or a specific thread ptid_t.
> +   Particularly process wildcard (pid > 0 && lwp == -1) for resuming
> +   whole one inferior is not supported.  This variable is provided only
> +   for backward compatibility with both clients and backend, backend
> +   'resume' method supports process wildcards instead.  */
>  ptid_t cont_thread;
> +

I forgot to say in the previous reply, but FAOD, this one is a good idea too.

-- 
Pedro Alves

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

[Pedro Alves]

Here's what I'm applying.  I've added comments inspired on your follow-up patch.

This makes watchpoint-multi.exp pass cleanly with the extended-remote
board, on x86_64 Fedora 16.

gdb/gdbserver/
2012-03-16  Pedro Alves  <palves@redhat.com>
	    Jan Kratochvil  <jan.kratochvil@redhat.com>

	* server.c (cont_thread, general_thread): Add describing comments.
	(start_inferior): Clear `cont_thread'.
	(handle_v_cont): Don't set `cont_thread' if resuming all threads
	of a process.
---

 gdb/gdbserver/server.c |   24 ++++++++++++++++++++++--
 1 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 3c97dbd..a4e9e57 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -30,7 +30,19 @@
 #include <sys/wait.h>
 #endif

+/* The thread set with an `Hc' packet.  `Hc' is deprecated in favor of
+   `vCont'.  Note the multi-process extensions made `vCont' a
+   requirement, so `Hc pPID.TID' is pretty much undefined.  So
+   CONT_THREAD can be null_ptid for no `Hc' thread, minus_one_ptid for
+   resuming all threads of the process (again, `Hc' isn't used for
+   multi-process), or a specific thread ptid_t.
+
+   We also set this when handling a single-thread `vCont' resume, as
+   some places in the backends check it to know when (and for which
+   thread) single-thread scheduler-locking is in effect.  */
 ptid_t cont_thread;
+
+/* The thread set with an `Hg' packet.  */
 ptid_t general_thread;

 int server_waiting;
@@ -262,6 +274,10 @@ start_inferior (char **argv)
   signal (SIGTTIN, SIG_DFL);
 #endif

+  /* Clear this so the backend doesn't get confused, thinking
+     CONT_THREAD died, and it needs to resume all threads.  */
+  cont_thread = null_ptid;
+
   signal_pid = create_inferior (new_argv[0], new_argv);

   /* FIXME: we don't actually know at this point that the create
@@ -1962,9 +1978,13 @@ handle_v_cont (char *own_buf)
   if (i < n)
     resume_info[i] = default_action;

-  /* Still used in occasional places in the backend.  */
+  /* `cont_thread' is still used in occasional places in the backend,
+     to implement single-thread scheduler-locking.  Doesn't make sense
+     to set it if we see a stop request, or any form of wildcard
+     vCont.  */
   if (n == 1
-      && !ptid_equal (resume_info[0].thread, minus_one_ptid)
+      && !(ptid_equal (resume_info[0].thread, minus_one_ptid)
+	   || ptid_get_lwp (resume_info[0].thread) == -1)
       && resume_info[0].kind != resume_stop)
     cont_thread = resume_info[0].thread;
   else

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

[Jan Kratochvil]

On Fri, 16 Mar 2012 21:10:55 +0100, Pedro Alves wrote:
> Here's what I'm applying.  I've added comments inspired on your follow-up patch.

OK, thanks.  I have not yet replied to that mail but I do not understand the
gdbserver/ code well enough to add anything anyway.


Thanks,
Jan