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

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

[Pedro Alves]

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.*"

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

[Jan Kratochvil]

On Mon, 28 May 2012 22:46:23 +0200, Pedro Alves wrote:
> If anyone as any idea why things are done in the current way, please
> speak up.

Not much, just that linux-nat.c linux_handle_extended_wait has this code by
you:
          if (non_stop)
            {
              /* Add the new thread to GDB's lists as soon as possible
                 so that:
              if (!thread_db_attach_lwp (new_lp->ptid))
                {
                  /* We're not using thread_db.  Add it to GDB's
                     list.  */
                  target_post_attach (GET_LWP (new_lp->ptid));
                  add_thread (new_lp->ptid);
                }

And for !non_stop mode add_thread gets delayed till handle_inferior_event;
which should not hurt.  If this add would be done unconditionally in
linux-nat.c then this handle_inferior_event would never be reached.
But I do not see why to make that change, handle_inferior_event works.


Thanks,
Jan

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

[Pedro Alves]

On 05/28/2012 10:19 PM, Jan Kratochvil wrote:

> On Mon, 28 May 2012 22:46:23 +0200, Pedro Alves wrote:
>> If anyone as any idea why things are done in the current way, please
>> speak up.
> 
> Not much, just that linux-nat.c linux_handle_extended_wait has this code by
> you:
>           if (non_stop)
>             {
>               /* Add the new thread to GDB's lists as soon as possible
>                  so that:
>               if (!thread_db_attach_lwp (new_lp->ptid))
>                 {
>                   /* We're not using thread_db.  Add it to GDB's
>                      list.  */
>                   target_post_attach (GET_LWP (new_lp->ptid));
>                   add_thread (new_lp->ptid);
>                 }
> 


Yeah, I've mentioned before that I have second thoughts on that bit.
If the inferior spawns short-lived threads rapidly, doing this for all threads,
as opposed to refreshing the list on demand or when presenting a stop to user,
for example, slows down the debug, and generates a bunch of frontend refreshing
that is likely useless.

> And for !non_stop mode add_thread gets delayed till handle_inferior_event;
> which should not hurt.  If this add would be done unconditionally in
> linux-nat.c then this handle_inferior_event would never be reached.


Yeah, on GNU/Linux.  But it could in principle still be reached on
other targets.  I haven't audited all.

> But I do not see why to make that change, handle_inferior_event works.

-- 
Pedro Alves

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

[Pedro Alves]

On 05/28/2012 09:46 PM, Pedro Alves wrote:

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


I've applied this now.

-- 
Pedro Alves