[PATCH 0/4] pr gdb/22882: fixing some thread related hangs

[PATCH 0/4] pr gdb/22882: fixing some thread related hangs

[Andrew Burgess]

This series sets out to address pr gdb/22882, GDB hangs when calling
inferior functions in different inferior threads when non-stop mode is
off, and scheduler-locking is on.

Patch #1 fixes some debug logging which I came across while trying to
understand this issue.

Patch #2 and #3 both fix pr gdb/22882.  Originally I'd planned to
present both patches and ask for guidance about which is the correct
approach.  However, having thought about it, I now think that both
patches are correct.  Hopefully the patch descriptions explain what
I'm trying to achieve in each.

Patch #4 is thread related, but not really related to the earlier
work.  It addresses an issue when an inferior call creates a new
thread.

---

Andrew Burgess (4):
  gdb: Fix an infrun debug log message
  gdb: Mark async event handler when event is already pending
  gdb: Run INF_EXEC_COMPLETE handler for additional cases
  gdb: After inferior call, finish all threads in process

 gdb/ChangeLog                                      | 23 ++++++
 gdb/infcall.c                                      |  2 +-
 gdb/infrun.c                                       | 24 +++---
 gdb/testsuite/ChangeLog                            | 12 +++
 gdb/testsuite/gdb.threads/infcall-create-thread.c  | 89 ++++++++++++++++++++++
 .../gdb.threads/infcall-create-thread.exp          | 71 +++++++++++++++++
 .../gdb.threads/multiple-successive-infcall.exp    | 15 +---
 7 files changed, 212 insertions(+), 24 deletions(-)
 create mode 100644 gdb/testsuite/gdb.threads/infcall-create-thread.c
 create mode 100644 gdb/testsuite/gdb.threads/infcall-create-thread.exp

-- 
2.14.3

[PATCH 2/4] gdb: Mark async event handler when event is already pending

[Andrew Burgess]

In PR22882 inferior functions are called on different threads while
scheduler-locking is turned on.  This results in a hang.  This was
discussed in this mailing list thread:

    https://sourceware.org/ml/gdb/2017-10/msg00032.html

The problem is that when the thread is set running in order to execute
the inferior call, a call to target_async is made.  If the target is
not already registered as 'target_async' then this will install the
async event handler, AND unconditionally mark the handler as having an
event pending.

However, if the target is already regitered as target_async then the
event handler is not installed (its already instealled) and the
handler is NOT marked as having an event pending.

If we try to set running a thread that already has a pending event,
then we do want to set target_async, however, there will not be an
external event incoming (the thread is already stopped) so we rely on
manually marking the event handler as having a pending event in order
to see the threads pending stop event.  This is fine, if, at the point
where we call target_async, the target is not already marked as async.
But, if it is, then the event handler will not be marked as ready, and
the threads pending stop event will never be processed.

A similar pattern of code can be seen in linux_nat_target::resume,
where, when a thread has a pending event, the call to target_async is
followed by a call to async_file_mark to ensure that the pending
thread event will be processed, even if target_async was already set.

gdb/ChangeLog:

	PR gdb/22882
	* infrun.c (resume_1): Add call to mark_async_event_handler.

gdb/testsuite/ChangeLog:

	* gdb.threads/multiple-successive-infcall.exp: Remove kfail case,
	rewrite test to describe action performed, rather than possible
	failure.
---
 gdb/ChangeLog                                             |  6 ++++++
 gdb/infrun.c                                              |  6 +++++-
 gdb/testsuite/ChangeLog                                   |  7 +++++++
 gdb/testsuite/gdb.threads/multiple-successive-infcall.exp | 15 ++-------------
 4 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index f86b2aa1c1a..1c4cd55e8f3 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2408,7 +2408,11 @@ resume_1 (enum gdb_signal sig)
       tp->suspend.stop_signal = GDB_SIGNAL_0;
 
       if (target_can_async_p ())
-	target_async (1);
+	{
+	  target_async (1);
+	  /* Tell the event loop we have an event to process. */
+	  mark_async_event_handler (infrun_async_inferior_event_token);
+	}
       return;
     }
 
diff --git a/gdb/testsuite/gdb.threads/multiple-successive-infcall.exp b/gdb/testsuite/gdb.threads/multiple-successive-infcall.exp
index a71f991d6f2..bb92c1d487d 100644
--- a/gdb/testsuite/gdb.threads/multiple-successive-infcall.exp
+++ b/gdb/testsuite/gdb.threads/multiple-successive-infcall.exp
@@ -56,17 +56,6 @@ gdb_test "show scheduler-locking" \
 
 foreach_with_prefix thread {5 4 3 2 1}  {
   gdb_test "thread ${thread}" "Switching to .*"
-  set command "call get_value()"
-  set hang_message "testing if ${command} hangs"
-  gdb_test_multiple "${command}" "${hang_message}" {
-    -re "= ${thread}\[\r\n]+${gdb_prompt} $" {
-      pass "${hang_message}"
-    }
-    timeout {
-      kfail "gdb/22882" "${hang_message}"
-      # Exit. The debugger has hung, so there is no point in wasting
-      # time timing out on further calls to get_value().
-      return 0
-    }
-  }
+  gdb_test "call get_value()" "= ${thread}" \
+      "call inferior function"
 }
-- 
2.14.3

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

[Andrew Burgess]

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

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"
+}
-- 
2.14.3

[PATCH 1/4] gdb: Fix an infrun debug log message

[Andrew Burgess]

Run the test gdb.threads/multiple-successive-infcall.exp by hand, if
you turn on 'debug infrun 1', you'll see that the debug line fixed in
this commit is printed and contains the wrong $pc value.  Fixed in
this commit.

gdb/ChangeLog:

	* infrun.c (do_target_wait): Change old version of $pc printed.
---
 gdb/ChangeLog | 4 ++++
 gdb/infrun.c  | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index df19478ef3c..f86b2aa1c1a 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3533,7 +3533,7 @@ do_target_wait (ptid_t ptid, struct target_waitstatus *status, int options)
 	    fprintf_unfiltered (gdb_stdlog,
 				"infrun: PC of %s changed.  was=%s, now=%s\n",
 				target_pid_to_str (tp->ptid),
-				paddress (gdbarch, tp->prev_pc),
+				paddress (gdbarch, tp->suspend.stop_pc),
 				paddress (gdbarch, pc));
 	  discard = 1;
 	}
-- 
2.14.3

[PATCH 3/4] gdb: Run INF_EXEC_COMPLETE handler for additional cases

[Andrew Burgess]

When making an inferior call, and non-stop mode is off, then, once the
inferior call is complete all threads will be stopped, and we should
run the INF_EXEC_COMPLETE handler.  This will result in a call to
'target_async(0)' to remove the event handlers for the target.

This was discussed by Yao Qi in this mailing list thread:

    https://sourceware.org/ml/gdb/2017-10/msg00032.html

Without this then the target event handlers are left in place even
when the target is stopped, which is different to what happens during
a standard stop proceedure (for example when one thread hits a
breakpoint).

gdb/ChangeLog:

	PR gdb/22882
	* infrun.c (fetch_inferior_event): If GDB is not proceeding then
	run INF_EXEC_COMPLETE handler, even when not calling normal_stop.
	Move should_notify_stop local into more inner scope.
---
 gdb/ChangeLog |  8 ++++++++
 gdb/infrun.c  | 16 ++++++++--------
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 1c4cd55e8f3..80721e822ab 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3932,7 +3932,6 @@ fetch_inferior_event (void *client_data)
       struct inferior *inf = find_inferior_ptid (ecs->ptid);
       int should_stop = 1;
       struct thread_info *thr = ecs->event_thread;
-      int should_notify_stop = 1;
 
       delete_just_stopped_threads_infrun_breakpoints ();
 
@@ -3950,6 +3949,9 @@ fetch_inferior_event (void *client_data)
 	}
       else
 	{
+	  int should_notify_stop = 1;
+	  int proceeded = 0;
+
 	  clean_up_just_stopped_threads_fsms (ecs);
 
 	  if (thr != NULL && thr->thread_fsm != NULL)
@@ -3960,17 +3962,15 @@ fetch_inferior_event (void *client_data)
 
 	  if (should_notify_stop)
 	    {
-	      int proceeded = 0;
-
 	      /* We may not find an inferior if this was a process exit.  */
 	      if (inf == NULL || inf->control.stop_soon == NO_STOP_QUIETLY)
 		proceeded = normal_stop ();
+	    }
 
-	      if (!proceeded)
-		{
-		  inferior_event_handler (INF_EXEC_COMPLETE, NULL);
-		  cmd_done = 1;
-		}
+	  if (!proceeded)
+	    {
+	      inferior_event_handler (INF_EXEC_COMPLETE, NULL);
+	      cmd_done = 1;
 	    }
 	}
     }
-- 
2.14.3

Re: [PATCH 1/4] gdb: Fix an infrun debug log message

[Pedro Alves]

On 05/24/2018 11:50 AM, Andrew Burgess wrote:
> Run the test gdb.threads/multiple-successive-infcall.exp by hand, if
> you turn on 'debug infrun 1', you'll see that the debug line fixed in
> this commit is printed and contains the wrong $pc value.  Fixed in
> this commit.
> 
> gdb/ChangeLog:
> 
> 	* infrun.c (do_target_wait): Change old version of $pc printed.

OK.

Thanks,
Pedro Alves

Re: [PATCH 2/4] gdb: Mark async event handler when event is already pending

[Pedro Alves]

On 05/24/2018 11:50 AM, Andrew Burgess wrote:

> However, if the target is already regitered as target_async then the
> event handler is not installed (its already instealled) and the
> handler is NOT marked as having an event pending.
> 

"regitered" -> "registered"

"instealled" -> "installed"

> gdb/ChangeLog:
> 
> 	PR gdb/22882
> 	* infrun.c (resume_1): Add call to mark_async_event_handler.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.threads/multiple-successive-infcall.exp: Remove kfail case,
> 	rewrite test to describe action performed, rather than possible
> 	failure.

OK.

Thanks,
Pedro Alves

Re: [PATCH 3/4] gdb: Run INF_EXEC_COMPLETE handler for additional cases

[Pedro Alves]

On 05/24/2018 11:50 AM, Andrew Burgess wrote:
> When making an inferior call, and non-stop mode is off, then, once the
> inferior call is complete all threads will be stopped, and we should
> run the INF_EXEC_COMPLETE handler.  This will result in a call to
> 'target_async(0)' to remove the event handlers for the target.
> 
> This was discussed by Yao Qi in this mailing list thread:
> 
>     https://sourceware.org/ml/gdb/2017-10/msg00032.html
> 
> Without this then the target event handlers are left in place even
> when the target is stopped, which is different to what happens during
> a standard stop proceedure (for example when one thread hits a
> breakpoint).
> 
> gdb/ChangeLog:
> 
> 	PR gdb/22882
> 	* infrun.c (fetch_inferior_event): If GDB is not proceeding then
> 	run INF_EXEC_COMPLETE handler, even when not calling normal_stop.
> 	Move should_notify_stop local into more inner scope.

OK.

Thanks,
Pedro Alves

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

[Pedro Alves]

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