PR threads/18127 - threads spawned by infcall end up stuck in "running" state

Refs:
 https://sourceware.org/ml/gdb/2015-03/msg00024.html
 https://sourceware.org/ml/gdb/2015-06/msg00005.html

On GNU/Linux, if an infcall spawns a thread, that thread ends up with
stuck running state.  This happens because:

 - when linux-nat.c detects a new thread, it marks them as running,
   and does not report anything to the core.

 - we skip finish_thread_state when the thread that is running the
   infcall stops.

As result, that new thread ends up with stuck "running" state, even
though it really is stopped.

On Windows, _all_ threads end up stuck in running state, not just the
one that was spawned.  That happens because when a new thread is
detected, unlike linux-nat.c, windows-nat.c reports
TARGET_WAITKIND_SPURIOUS to infrun.  It's the fact that that event
does not cause a user-visible stop that triggers the problem.  When
the target is re-resumed, we call set_running with a wildcard ptid,
which marks all thread as running.  That set_running is not suppressed
because the (leader) thread being resumed does not have in_infcall
set.  Later, when the infcall finally finishes successfully, nothing
marks all threads back to stopped.

We can trigger the same problem on all targets by having a thread
other than the one that is running the infcall report a breakpoint hit
to infrun, and then have that breakpoint not cause a stop.  That's
what the included test does.

The fix is to stop GDB from suppressing the set_running calls while
doing an infcall, and then set the threads back to stopped when the
call finishes, iff they were originally stopped before the infcall
started.  (Note the MI *running/*stopped event suppression isn't
affected.)

Tested on x86_64 GNU/Linux.

gdb/ChangeLog:
2015-06-29  Pedro Alves  <palves@redhat.com>

	PR threads/18127
	* infcall.c (run_inferior_call): On infcall success, if the thread
	was marked stopped before, reset it back to stopped.
	* infrun.c (resume): Don't suppress the set_running calls when
	doing an infcall.
	(normal_stop): Only discard the finish_thread_state cleanup if the
	infcall succeeded.

gdb/testsuite/ChangeLog:
2015-06-29  Pedro Alves  <palves@redhat.com>

	PR threads/18127
	* gdb.threads/hand-call-new-thread.c: New file.
	* gdb.threads/hand-call-new-thread.c: New file.

6 files changed, 148 insertions, 18 deletions

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index b25dff2..a8aa0ba 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,13 @@
+2015-06-29  Pedro Alves  <palves@redhat.com>
+
+	PR threads/18127
+	* infcall.c (run_inferior_call): On infcall success, if the thread
+	was marked stopped before, reset it back to stopped.
+	* infrun.c (resume): Don't suppress the set_running calls when
+	doing an infcall.
+	(normal_stop): Only discard the finish_thread_state cleanup if the
+	infcall succeeded.
+
 2015-06-29  Pierre Langlois  <pierre.langlois@arm.com>
 
 	* MAINTAINERS (Write After Approval): Update my email address.
diff --git a/gdb/infcall.c b/gdb/infcall.c
index f79afea..e3bd72a 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -387,6 +387,7 @@ run_inferior_call (struct thread_info *call_thread, CORE_ADDR real_pc)
   int saved_in_infcall = call_thread->control.in_infcall;
   ptid_t call_thread_ptid = call_thread->ptid;
   int saved_sync_execution = sync_execution;
+  int was_running = call_thread->state == THREAD_RUNNING;
 
   /* Infcalls run synchronously, in the foreground.  */
   if (target_can_async_p ())
@@ -433,6 +434,26 @@ run_inferior_call (struct thread_info *call_thread, CORE_ADDR real_pc)
      CALL_THREAD as it could be invalid if its thread has exited.  */
   call_thread = find_thread_ptid (call_thread_ptid);
 
+  /* If the infcall does NOT succeed, normal_stop will have already
+     finished the thread states.  However, on success, normal_stop
+     defers here, so that we can set back the thread states to what
+     they were before the call.  Note that we must also finish the
+     state of new threads that might have spawned while the call was
+     running.  The main cases to handle are:
+
+     - "(gdb) print foo ()", or any other command that evaluates an
+     expression at the prompt.  (The thread was marked stopped before.)
+
+     - "(gdb) break foo if return_false()" or similar cases where we
+     do an infcall while handling an event (while the thread is still
+     marked running).  In this example, whether the condition
+     evaluates true and thus we'll present a user-visible stop is
+     decided elsewhere.  */
+  if (!was_running
+      && ptid_equal (call_thread_ptid, inferior_ptid)
+      && stop_stack_dummy == STOP_STACK_DUMMY)
+    finish_thread_state (user_visible_resume_ptid (0));
+
   enable_watchpoints_after_interactive_call_stop ();
 
   /* Call breakpoint_auto_delete on the current contents of the bpstat
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 792f847..445a612 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2264,11 +2264,8 @@ resume (enum gdb_signal sig)
 	     requests finish.  The thread is not executing at this
 	     point, and the call to set_executing will be made later.
 	     But we need to call set_running here, since from the
-	     user/frontend's point of view, threads were set running.
-	     Unless we're calling an inferior function, as in that
-	     case we pretend the inferior doesn't run at all.  */
-	  if (!tp->control.in_infcall)
-	    set_running (user_visible_resume_ptid (user_step), 1);
+	     user/frontend's point of view, threads were set running.  */
+	  set_running (user_visible_resume_ptid (user_step), 1);
 	  discard_cleanups (old_cleanups);
 	  return;
 	}
@@ -2346,10 +2343,8 @@ resume (enum gdb_signal sig)
   /* Even if RESUME_PTID is a wildcard, and we end up resuming less
      (e.g., we might need to step over a breakpoint), from the
      user/frontend's point of view, all threads in RESUME_PTID are now
-     running.  Unless we're calling an inferior function, as in that
-     case pretend we inferior doesn't run at all.  */
-  if (!tp->control.in_infcall)
-    set_running (resume_ptid, 1);
+     running.  */
+  set_running (resume_ptid, 1);
 
   /* Maybe resume a single thread after all.  */
   if ((step || thread_has_single_step_breakpoints_set (tp))
@@ -6664,15 +6659,15 @@ normal_stop (void)
   if (has_stack_frames () && !stop_stack_dummy)
     set_current_sal_from_frame (get_current_frame ());
 
-  /* Let the user/frontend see the threads as stopped, but do nothing
-     if the thread was running an infcall.  We may be e.g., evaluating
-     a breakpoint condition.  In that case, the thread had state
-     THREAD_RUNNING before the infcall, and shall remain set to
-     running, all without informing the user/frontend about state
-     transition changes.  If this is actually a call command, then the
-     thread was originally already stopped, so there's no state to
-     finish either.  */
-  if (target_has_execution && inferior_thread ()->control.in_infcall)
+  /* Let the user/frontend see the threads as stopped, but defer to
+     call_function_by_hand if the thread finished an infcall
+     successfully.  We may be e.g., evaluating a breakpoint condition.
+     In that case, the thread had state THREAD_RUNNING before the
+     infcall, and shall remain marked running, all without informing
+     the user/frontend about state transition changes.  */
+  if (target_has_execution
+      && inferior_thread ()->control.in_infcall
+      && stop_stack_dummy == STOP_STACK_DUMMY)
     discard_cleanups (old_chain);
   else
     do_cleanups (old_chain);
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 89d8e32..d9ec546 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2015-06-29  Pedro Alves  <palves@redhat.com>
+
+	PR threads/18127
+	* gdb.threads/hand-call-new-thread.c: New file.
+	* gdb.threads/hand-call-new-thread.c: New file.
+
 2015-06-26  Keith Seitz  <keiths@redhat.com>
 	    Doug Evans  <dje@google.com>
 
diff --git a/gdb/testsuite/gdb.threads/hand-call-new-thread.c b/gdb/testsuite/gdb.threads/hand-call-new-thread.c
new file mode 100644
index 0000000..042be9b
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/hand-call-new-thread.c
@@ -0,0 +1,50 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2015 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 <unistd.h>
+#include <stdlib.h>
+#include <pthread.h>
+#include <assert.h>
+
+static int
+foo (void)
+{
+  usleep (1);
+}
+
+static void *
+thread_function (void *arg)
+{
+  while (1)
+    foo ();
+}
+
+void
+new_thread (void)
+{
+  pthread_t thread;
+  int res;
+
+  res = pthread_create (&thread, NULL, thread_function, NULL);
+  assert (res == 0);
+}
+
+int
+main (int argc, char **argv)
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.threads/hand-call-new-thread.exp b/gdb/testsuite/gdb.threads/hand-call-new-thread.exp
new file mode 100644
index 0000000..a02d8ba
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/hand-call-new-thread.exp
@@ -0,0 +1,48 @@
+# Copyright (C) 2015 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/>.
+
+# Ensure that new threads created while an infcall is ongoing are set
+# to stopped state once the call finishes.
+
+standard_testfile
+
+if [prepare_for_testing "failed to prepare" $testfile $srcfile {debug pthreads}] {
+    return -1
+}
+
+if ![runto_main] {
+    continue
+}
+
+# Set a thread-specific breakpoint that the wrong thread trips on
+# while running the infcall.  Check that no thread ends up in stale
+# "running" state once the call finishes.
+gdb_test "b foo thread 1" "Breakpoint .*$srcfile.*"
+
+for {set i 0} {$i < 3} {incr i} {
+    with_test_prefix "iter $i" {
+	gdb_test "p new_thread ()" " = void"
+
+	set message "no thread marked running"
+	gdb_test_multiple "info threads" $message {
+	    -re "\\\(running\\\).*$gdb_prompt $" {
+		fail $message
+	    }
+	    -re "$gdb_prompt $" {
+		pass $message
+	    }
+	}
+    }
+}