[PATCH] infrun: don't resume the inferior when we notice a new thread (new_thread_event).

[Pedro Alves <palves at redhat dot com>]

I've pointed fingers at this weird code before, but never got to do
anything about it.  Time to correct it.

When handle_inferior_event notices a new thread (the new_thread_event
bits), it resumes the inferior without handling the event further, but
I never understood _why_ that is so.  Why not just record the new
thread, and carry on handling the event as usual?  This can't be for
the remote target, as that target always adds new threads to the list
before presenting the event to the core.  This is really actively
harmful, as it can make GDB lose events.  We usually don't notice it
with regular breakpoints, as an immediate resume makes the target trip
on the same breakpoint again immediately, but with other random
signals, or continuable watchpoints, for example, resuming the
inferior like that, means we lose the just reported event.  For an
example, see the new test added by this patch; without the GDB fix, we
see:

 ...
 infrun: resume (step=0, signal=0), trap_expected=0, current thread [process 14162] at 0x3791803a58
 infrun: prepare_to_wait
 [LWP 14162 exited]
 infrun: target_wait (-1, status) =
 infrun:   14162 [LWP 14165],
 infrun:   status->kind = stopped, signal = SIGUSR1
 [New LWP 14165]
 infrun: infwait_normal_state
 infrun: TARGET_WAITKIND_STOPPED
 infrun: Switching context from process 14162 to LWP 14165
 infrun: prepare_to_wait
 infrun: target_wait (-1, status) =
 infrun:   14162 [LWP 14165],
 infrun:   status->kind = exited, status = 0
 infrun: infwait_normal_state
 infrun: TARGET_WAITKIND_EXITED
 [Inferior 1 (process 14162) exited normally]
 infrun: stop_stepping
 (gdb)

IOW, we don't report the SIGUSR1 to the user (Program stopped with
signal SIGUSR1).  With the fix we see the expected:

 [New LWP 17300]
 Program received signal SIGUSR1, User defined signal 1.
 [Switching to LWP 17300]
 0x0000003791c36567 in kill () at ../sysdeps/unix/syscall-template.S:82
 82      T_PSEUDO (SYSCALL_SYMBOL, SYSCALL_NAME, SYSCALL_NARGS)
 (gdb)

With GNU/Linux, the new_thread_event path is reacheable when debugging
a program that uses raw clone instead a pthreads.

The first appearance of new_thread_event in the sources I see is
around GDB 5.0, where we see:

    /* If it's a new process, add it to the thread database */

    ecs->new_thread_event = ((ecs->pid != inferior_pid) && !in_thread_list (ecs->pid));

    if (ecs->ws.kind != TARGET_WAITKIND_EXITED
        && ecs->ws.kind != TARGET_WAITKIND_SIGNALLED
        && ecs->new_thread_event)
      {
        add_thread (ecs->pid);

#ifdef UI_OUT
        ui_out_text (uiout, "[New ");
        ui_out_text (uiout, target_pid_or_tid_to_str (ecs->pid));
        ui_out_text (uiout, "]\n");
#else
        printf_filtered ("[New %s]\n", target_pid_or_tid_to_str (ecs->pid));
#endif

#if 0
        /* NOTE: This block is ONLY meant to be invoked in case of a
           "thread creation event"!  If it is invoked for any other
           sort of event (such as a new thread landing on a breakpoint),
           the event will be discarded, which is almost certainly
           a bad thing!

           To avoid this, the low-level module (eg. target_wait)
           should call in_thread_list and add_thread, so that the
           new thread is known by the time we get here.  */

        /* We may want to consider not doing a resume here in order
           to give the user a chance to play with the new thread.
           It might be good to make that a user-settable option.  */

        /* At this point, all threads are stopped (happens
           automatically in either the OS or the native code).
           Therefore we need to continue all threads in order to
           make progress.  */

        target_resume (-1, 0, TARGET_SIGNAL_0);
        prepare_to_wait (ecs);
        return;
#endif

...

and a bit further below:

    /* We may want to consider not doing a resume here in order to give
       the user a chance to play with the new thread.  It might be good
       to make that a user-settable option.  */

    /* At this point, all threads are stopped (happens automatically in
       either the OS or the native code).  Therefore we need to continue
       all threads in order to make progress.  */
    if (ecs->new_thread_event)
      {
	target_resume (-1, 0, TARGET_SIGNAL_0);
	prepare_to_wait (ecs);
	return;
      }



But note, the first new_thread_event block has the target_resume #if
0'd out (and has been removed since), and we see more comments
pointing out that this should only be used for thread creation events.
Both blocks still exist today.  It looks to me that at the time, there
was some plan to support _real_ thread events (like
TARGET_WAITKIND_NEW_THREAD or some such), where such resume would then
make sense, but somehow we ended up with what we have, and then much
later the #if 0'd comment was wiped away as simply being long unused
dead code
(<http://sourceware.org/ml/gdb-patches/2004-08/msg00689.html>).

Here's a patch.

If anyone as any idea why things are done in the current way, please
speak up.

gdb/
2012-05-28  Pedro Alves  <palves@redhat.com>

	* infrun.c (struct execution_control_state): Remove
	`new_thread_event' field.
	(handle_inferior_event): Simplify new threads handling; don't
	resume the inferior if we find a new thread.

gdb/testsuite/
2012-05-28  Pedro Alves  <palves@redhat.com>

	* gdb.threads/clone-new-thread-event.c: New file.
	* gdb.threads/clone-new-thread-event.exp: New file.
---
 gdb/infrun.c                                       |   43 ++-------
 gdb/testsuite/gdb.threads/clone-new-thread-event.c |   97 ++++++++++++++++++++
 .../gdb.threads/clone-new-thread-event.exp         |   32 +++++++
 3 files changed, 137 insertions(+), 35 deletions(-)
 create mode 100644 gdb/testsuite/gdb.threads/clone-new-thread-event.c
 create mode 100644 gdb/testsuite/gdb.threads/clone-new-thread-event.exp

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 45b1fe7..f36d532 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2393,7 +2393,6 @@ struct execution_control_state
   CORE_ADDR stop_func_start;
   CORE_ADDR stop_func_end;
   const char *stop_func_name;
-  int new_thread_event;
   int wait_some_more;
 };
 
@@ -3229,17 +3228,15 @@ handle_inferior_event (struct execution_control_state *ecs)
       return;
     }
 
-  /* If it's a new process, add it to the thread database.  */
-
-  ecs->new_thread_event = (!ptid_equal (ecs->ptid, inferior_ptid)
-			   && !ptid_equal (ecs->ptid, minus_one_ptid)
-			   && !in_thread_list (ecs->ptid));
-
+  /* If it's a new thread, add it to the thread database.  */
   if (ecs->ws.kind != TARGET_WAITKIND_EXITED
-      && ecs->ws.kind != TARGET_WAITKIND_SIGNALLED && ecs->new_thread_event)
-    add_thread (ecs->ptid);
-
-  ecs->event_thread = find_thread_ptid (ecs->ptid);
+      && ecs->ws.kind != TARGET_WAITKIND_SIGNALLED
+      && !ptid_equal (ecs->ptid, minus_one_ptid))
+    {
+      ecs->event_thread = find_thread_ptid (ecs->ptid);
+      if (ecs->event_thread == NULL)
+	ecs->event_thread = add_thread (ecs->ptid);
+    }
 
   /* Dependent on valid ECS->EVENT_THREAD.  */
   adjust_pc_after_break (ecs);
@@ -3713,30 +3710,6 @@ handle_inferior_event (struct execution_control_state *ecs)
       return;
     }
 
-  if (ecs->new_thread_event)
-    {
-      if (non_stop)
-	/* Non-stop assumes that the target handles adding new threads
-	   to the thread list.  */
-	internal_error (__FILE__, __LINE__,
-			"targets should add new threads to the thread "
-			"list themselves in non-stop mode.");
-
-      /* We may want to consider not doing a resume here in order to
-	 give the user a chance to play with the new thread.  It might
-	 be good to make that a user-settable option.  */
-
-      /* At this point, all threads are stopped (happens automatically
-	 in either the OS or the native code).  Therefore we need to
-	 continue all threads in order to make progress.  */
-
-      if (!ptid_equal (ecs->ptid, inferior_ptid))
-	context_switch (ecs->ptid);
-      target_resume (RESUME_ALL, 0, GDB_SIGNAL_0);
-      prepare_to_wait (ecs);
-      return;
-    }
-
   if (ecs->ws.kind == TARGET_WAITKIND_STOPPED)
     {
       /* Do we need to clean up the state of a thread that has
diff --git a/gdb/testsuite/gdb.threads/clone-new-thread-event.c b/gdb/testsuite/gdb.threads/clone-new-thread-event.c
new file mode 100644
index 0000000..dee63ad
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/clone-new-thread-event.c
@@ -0,0 +1,97 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2009-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/>.
+
+   Test that GDB doesn't lose an event for a thread it didn't know
+   about, until an event is reported for it.  */
+
+#define _GNU_SOURCE
+#include <sched.h>
+#include <assert.h>
+#include <stdlib.h>
+#include <signal.h>
+#include <sys/types.h>
+#include <unistd.h>
+#include <sys/syscall.h>
+
+#include <features.h>
+#ifdef __UCLIBC__
+#if !(defined(__UCLIBC_HAS_MMU__) || defined(__ARCH_HAS_MMU__))
+#define HAS_NOMMU
+#endif
+#endif
+
+#define STACK_SIZE 0x1000
+
+/* Send a signal to an LWP.  */
+
+static int
+kill_lwp (int lwpid, int signo)
+{
+  /* Use tkill, if possible, in case we are using nptl threads.  If tkill
+     fails, then we are not using nptl threads and we should be using kill.  */
+
+#ifdef HAVE_TKILL_SYSCALL
+  {
+    static int tkill_failed;
+
+    if (!tkill_failed)
+      {
+	int ret;
+
+	errno = 0;
+	ret = syscall (__NR_tkill, lwpid, signo);
+	if (errno != ENOSYS)
+	  return ret;
+	tkill_failed = 1;
+      }
+  }
+#endif
+
+  return kill (lwpid, signo);
+}
+
+static pid_t
+gettid (void)
+{
+  return syscall (__NR_gettid);
+}
+
+static int
+fn (void *unused)
+{
+  kill_lwp (gettid (), SIGUSR1);
+  return 0;
+}
+
+int
+main (int argc, char **argv)
+{
+  unsigned char *stack;
+  int new_pid;
+
+  stack = malloc (STACK_SIZE);
+  assert (stack != NULL);
+
+  new_pid = clone (fn, stack + STACK_SIZE, CLONE_FILES
+#if defined(__UCLIBC__) && defined(HAS_NOMMU)
+		   | CLONE_VM
+#endif /* defined(__UCLIBC__) && defined(HAS_NOMMU) */
+		   , NULL, NULL, NULL, NULL);
+  assert (new_pid > 0);
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.threads/clone-new-thread-event.exp b/gdb/testsuite/gdb.threads/clone-new-thread-event.exp
new file mode 100644
index 0000000..2867726
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/clone-new-thread-event.exp
@@ -0,0 +1,32 @@
+# 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/>.
+
+# This only works on targets with the Linux kernel.
+if ![istarget *-*-linux*] then {
+    return
+}
+
+if { [prepare_for_testing clone-new-thread-event.exp clone-new-thread-event] } {
+    return -1
+}
+
+if { ![runto_main] } {
+    untested "could not run to main"
+    return -1
+}
+
+gdb_test "continue" "Program received signal SIGUSR1, User defined signal 1.*"