[4/4] gdb: After inferior call, finish all threads in process
Commit Message
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
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
@@ -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 ();
new file mode 100644
@@ -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;
+}
new file mode 100644
@@ -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"
+}