[4/4] gdb: After inferior call, finish all threads in process

Message ID 652d28f3e550ca9080e9d4dc487bd8fe2c8a6478.1527157948.git.andrew.burgess@embecosm.com
State New, archived
Headers

Commit Message

Andrew Burgess May 24, 2018, 10:50 a.m. UTC
  Currently, if non-stop mode is off and scheduler-locking is on, then
when we perform the inferior call we only call finish_thread_state on
the thread in which the call was made.

The problem with this is if the inferior call caused a new thread to
be created, the thread _will_ have been stopped when the inferior call
completed, but it's frontend state will be left as running (as
finish_thread_state was never called for it).

This change causes us to call finish_thread_state for all threads in
the process, which is what happens when scheduler-locking is off.
After this commit new threads, created in the inferior call, are
correctly shown as stopped once the inferior call has completed.

I did spend some time considering whether, when scheduler-locking is
on, whether it was even correct for the new thread to be allowed to
run at all (after it is created in the inferior call).  In the end I
decided that it probably was, if we imagine calling an inferior
function that creates a new thread, does some work, and then joins the
new thread again, we'd probably expect that to work, even with
scheduler-locking on.  As a result I've only conserned myself with the
state of new threads after the inferior call finishes.

gdb/ChangeLog:

	* infrun.c (run_inferior_call): Finish all threads in inferior
	process in order to catch possible new threads.

gdb/testsuite/ChangeLog:

	* gdb.threads/infcall-create-thread.c: New file.
	* gdb.threads/infcall-create-thread.exp: New file.
---
 gdb/ChangeLog                                      |  5 ++
 gdb/infcall.c                                      |  2 +-
 gdb/testsuite/ChangeLog                            |  5 ++
 gdb/testsuite/gdb.threads/infcall-create-thread.c  | 89 ++++++++++++++++++++++
 .../gdb.threads/infcall-create-thread.exp          | 71 +++++++++++++++++
 5 files changed, 171 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.threads/infcall-create-thread.c
 create mode 100644 gdb/testsuite/gdb.threads/infcall-create-thread.exp
  

Comments

Pedro Alves June 11, 2018, 6:13 p.m. UTC | #1
On 05/24/2018 11:50 AM, Andrew Burgess wrote:

> diff --git a/gdb/infcall.c b/gdb/infcall.c
> index b13f5b61d96..b33123d6c75 100644
> --- a/gdb/infcall.c
> +++ b/gdb/infcall.c
> @@ -658,7 +658,7 @@ run_inferior_call (struct call_thread_fsm *sm,
>    if (!was_running
>        && ptid_equal (call_thread_ptid, inferior_ptid)
>        && stop_stack_dummy == STOP_STACK_DUMMY)
> -    finish_thread_state (user_visible_resume_ptid (0));
> +    finish_thread_state (pid_to_ptid (ptid_get_pid (inferior_ptid)));

I have to leave for now, so I have to admit to not having thought
through this one fully, but off hand this doesn't look right, so
I thought I'd send this quick comment anyway.

E.g., if in non-stop mode, that "finishes" all threads of the process, while
some thread may be user-visibly running but internally suspended waiting
for its turn in the displaced stepping queue, for example?

And what if "set schedule-multiple on" is in effect?  Then more than
one process will have been resumed, but this only finishes the one
process?

I'll have to think about the schedlock on + new threads thing too.
I've thought about that multiple times, but I never recall what
my previous conclusion was.  :-P

Thanks,
Pedro Alves
  

Patch

diff --git a/gdb/infcall.c b/gdb/infcall.c
index b13f5b61d96..b33123d6c75 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -658,7 +658,7 @@  run_inferior_call (struct call_thread_fsm *sm,
   if (!was_running
       && ptid_equal (call_thread_ptid, inferior_ptid)
       && stop_stack_dummy == STOP_STACK_DUMMY)
-    finish_thread_state (user_visible_resume_ptid (0));
+    finish_thread_state (pid_to_ptid (ptid_get_pid (inferior_ptid)));
 
   enable_watchpoints_after_interactive_call_stop ();
 
diff --git a/gdb/testsuite/gdb.threads/infcall-create-thread.c b/gdb/testsuite/gdb.threads/infcall-create-thread.c
new file mode 100644
index 00000000000..165487d5981
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/infcall-create-thread.c
@@ -0,0 +1,89 @@ 
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2018 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 <stdio.h>
+#include <stdlib.h>
+#include <pthread.h>
+#include <unistd.h>
+
+/* This defines the maximum number of threads to spawn.  */
+#define THREADCOUNT 4
+
+/* Define global thread identifiers.  */
+static pthread_t threads[THREADCOUNT];
+
+/* Next thread ID to fill in.  */
+static int next_thread = 0;
+
+/* Encapsulate the synchronization of the threads. Perform a barrier before
+   and after the computation.  */
+static void *
+thread_function (void *args)
+{
+   while (1)
+    sleep (5);
+}
+
+/* Create a new thread.  */
+static void
+spawn_thread ()
+{
+  int err;
+
+  if (next_thread == THREADCOUNT)
+    {
+      fprintf (stderr, "Attempt to create too many threads.\n");
+      exit (EXIT_FAILURE);
+    }
+
+  err = pthread_create (&threads[next_thread], NULL, thread_function, NULL);
+  if (err != 0)
+    {
+      fprintf (stderr, "Thread creation failed\n");
+      exit (EXIT_FAILURE);
+    }
+
+  ++next_thread;
+}
+
+/* Place a breakpoint on this function.  */
+void
+breakpt ()
+{
+  asm ("" ::: "memory");
+}
+
+int
+main ()
+{
+  /* Create the worker threads.  */
+  breakpt ();
+  printf ("Spawning worker threads\n");
+  for (int tid = 0; tid < 2 /*THREADCOUNT*/; ++tid)
+    {
+      spawn_thread ();
+      breakpt ();
+    }
+
+  /* Wait for the threads to complete then exit.  Currently threads block
+     for ever and this will never complete, but it serves to block the
+     main thread.  */
+  for (int tid = 0; tid < THREADCOUNT; ++tid)
+    pthread_join (threads[tid], NULL);
+
+  return EXIT_SUCCESS;
+}
diff --git a/gdb/testsuite/gdb.threads/infcall-create-thread.exp b/gdb/testsuite/gdb.threads/infcall-create-thread.exp
new file mode 100644
index 00000000000..634d5f44b1e
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/infcall-create-thread.exp
@@ -0,0 +1,71 @@ 
+# Copyright (C) 2018 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/>.  */
+
+# Check the state of gdb after an inferior call creates a new thread.
+
+standard_testfile
+
+if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
+  executable {debug}] != "" } {
+  return -1
+}
+
+foreach_with_prefix use_schedlock { 0 1 } {
+
+    clean_restart "${binfile}"
+
+    if ![runto_main] then {
+	fail "can't run to main"
+	return 0
+    }
+
+    gdb_breakpoint "breakpt"
+    gdb_continue_to_breakpoint "breakpt"
+
+    if { $use_schedlock } {
+	gdb_test_no_output "set scheduler-locking on"
+	gdb_test "show scheduler-locking" \
+	    "Mode for locking scheduler during execution is \"on\"."
+    }
+
+    gdb_test "call spawn_thread ()" "\[New Thread $hex \\(LWP $decimal\\)\]" \
+	"spawn a new thread in an inferior call"
+
+    set seen_thr_1 0
+    set seen_thr_2 0
+    set test_name "check new thread is not running"
+    gdb_test_multiple "info threads" $test_name {
+	-re "\[^\r\n\]+Id\[^\r\n\]+Target Id\[^\r\n\]+Frame\[^\r\n\]+\[\r\n\]+" {
+	    exp_continue
+	}
+	-re "^\\*\[^\r\n0-9\]+1 \[^\r\n\]+breakpt \\(\\) at \[^\r\n\]+\[\r\n\]+" {
+	    # Thread 1 is active thread, and is NOT running.
+	    set seen_thr_1 1
+	    exp_continue
+	}
+	-re "^\[^*\r\n0-9\]+2 \[^\r\n\]+ (in|at) \[^\r\n\]+\[\r\n\]+" {
+	    set seen_thr_2 1
+	    exp_continue
+	}
+	-re "^\[^\r\n0-9\]+(1|2) \[^\r\n\]+\\(running\\)\[\r\n\]+" {
+	    fail "$test_name"
+	    exp_continue
+	}
+	-re "$gdb_prompt $" {
+	}
+    }
+
+    gdb_assert { $seen_thr_1 && $seen_thr_2 } "seen thread 1 and 2"
+}