[PATCH v2] Add test case for a known hang in infrun

[Richard Bunt <richard dot bunt at arm dot com>]

Hi Simon,

Many thanks for taking the time to review this patch. All the comments
have been addressed in the following prose and patch.

> Thanks for taking the time to write and submit the test.  We don't allow
> adding new tests that FAIL in the testsuite, but I guess it's fine to
> add one that KFAILs. The day someone fixes that bug (perhaps by
> indirectly/by chance), it will start KPASSing (at which point we can
> remove the KFAIL), and we'll already have a test for that. So I think
> it's a valuable contribution.

I hope to, in time, fix the issue invoked by this test case; this is
just a precursor to this effort.

> In the test, get_value is always called with index == 0, so I'm not sure
> what the thread_ids array is for.  I was under the impression that it
> was going to be used to confirm that the infcall was made using the
> correct thread (which would be a good thing to test for). Is this what
> you intended? I guess this could be achieved by using thread local
> storage in which the thread id is stored. If the wrong thread is used
> to make the infcall, the wrong value will be returned.

The original requirement for this test was to return some value, it did not
matter what as the bug is with the call rather than the value returned.
However, I agree that checking the thread the call was made on is
valuable, as this test might start passing erroneously if some other change
in behaviour causes the call to be made on the same thread every time.

> Test messages throughout the fil should start with a lower case and not
> have a period at the end:
>
> https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Follow_the_test_name_convention

Thanks for the link to this page; I have adjusted all the messages to follow
this convention.

> This will test with the main thread (thread 1), and the first 3 of the 4
> spawned threads.  Is this what you want?  We might as well test with the
> main thread and all 4 spawned threads.

This was an error on my part and I have fixed this in the latest patch
revision to test the call from the main and all 4 spawned threads.

> Also, each test message should be unique. Executing the loop body
> multiple times will give multiples tests with the same message. To make
> each loop iteration unique, you can use foreach_with_prefix:
>
>    foreach_with_prefix thread {4 3 2 1}  {
>             ...
>    }
>
> which will prefix each message with "thread=$thread: ".  See:

I have adjusted the test case based on this and confirmed the uniqueness
of messages using uniq.

> If the return value is predictable, it would be better to match it, at
> least the " = 0" part.

The call to get_value will now return the thread identifier which was
passed in at thread creation. Additionally, GDB has been used to ensure
that the thread creation (by the test case) and detection (by GDB) align.

> When using gdb_test_multiple, all test outcomes should ideally use the
> same message.  So whatever you pass as the second argument to
> gdb_test_multiple (which may be the same as the command, if it's clear
> enough) you would also usually pass to "pass", "fail" or "kfail" inside
> the body.  Also, make sure the message follows the convention linked
> above.

I have homogenized the messages in this block.

Many thanks,

Rich

The hang occurs when GDB tries to call inferior functions on two
different threads with scheduler-locking turned on. The first call
works fine, with the call to infrun_async(1) causing the
signal_handler to be marked and the event to be handled, but then
the event loop resets the "ready" member to zero, while leaving
infrun_is_async set to 1. As a result, GDB hangs if the user switches
to another thread and calls a second function because calling
infrun_async(1) a second time has no effect, meaning the inferior
call events are never handled.

The added test case provokes the above issue.

gdb/testsuite/ChangeLog:

	* gdb.threads/multiple-successive-infcall.c: New test.
	* gdb.threads/multiple-successive-infcall.exp: New file.
---
 .../gdb.threads/multiple-successive-infcall.c      | 119 +++++++++++++++++++++
 .../gdb.threads/multiple-successive-infcall.exp    |  72 +++++++++++++
 2 files changed, 191 insertions(+)
 create mode 100644 gdb/testsuite/gdb.threads/multiple-successive-infcall.c
 create mode 100644 gdb/testsuite/gdb.threads/multiple-successive-infcall.exp

diff --git a/gdb/testsuite/gdb.threads/multiple-successive-infcall.c b/gdb/testsuite/gdb.threads/multiple-successive-infcall.c
new file mode 100644
index 0000000..761f16e
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/multiple-successive-infcall.c
@@ -0,0 +1,119 @@
+/* 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>
+
+/* This defines the number of threads to spawn.  */
+#define THREADCOUNT 4
+
+/* Global barrier type to control synchronization between threads.  */
+pthread_barrier_t print_barrier;
+
+/* Define global thread identifiers.  */
+pthread_t threads[THREADCOUNT];
+
+/* Hold values for each thread at the index supplied to the thread
+   on creation.  */
+int thread_ids[THREADCOUNT];
+
+/* Find the value associated with the calling thread.  */
+int
+get_value ()
+{
+  for (int tid = 0; tid < THREADCOUNT; ++tid)
+    {
+      if (pthread_equal (threads[tid], pthread_self ()))
+	{
+	  return thread_ids[tid];
+	}
+    }
+  // Value for the main thread.
+  return 1;
+}
+
+/* Return the nth Fibonacci number.  */
+unsigned long
+fast_fib (unsigned int n)
+{
+  int a = 0;
+  int b = 1;
+  int t;
+  for (unsigned int i = 0; i < n; ++i)
+    {
+      t = b;
+      b = a + b;
+      a = t;
+    }
+  return a;
+}
+
+/* Encapsulate the synchronization of the threads. Perform a barrier before
+   and after the computation.  */
+void *
+thread_function (void *args)
+{
+  int tid = *((int *) args);
+  int status = pthread_barrier_wait (&print_barrier);
+  if (status == PTHREAD_BARRIER_SERIAL_THREAD)
+    {
+      printf ("All threads entering compute region\n");
+    }
+  unsigned long result = fast_fib (100);	// testmarker01
+  status = pthread_barrier_wait (&print_barrier);
+  if (status == PTHREAD_BARRIER_SERIAL_THREAD)
+    {
+      printf ("All threads outputting results\n");
+    }
+  pthread_barrier_wait (&print_barrier);
+  printf ("Thread %d Result: %lu\n", tid, result);
+  pthread_exit (NULL);
+}
+
+int
+main (void)
+{
+  int err = pthread_barrier_init (&print_barrier, NULL, THREADCOUNT);
+  if (err)
+    {
+      fprintf (stderr, "Barrier creation failed\n");
+      return EXIT_FAILURE;
+    }
+  // Create the worker threads (main).
+  printf ("Spawning worker threads\n");
+  for (int tid = 0; tid < THREADCOUNT; ++tid)
+    {
+      // Add 2 so the value maps to the debugger's thread identifiers.
+      thread_ids[tid] = tid + 2;	//prethreadcreationmarker
+      err =
+	pthread_create (&threads[tid], NULL, thread_function,
+			(void *) &thread_ids[tid]);
+      if (err)
+	{
+	  fprintf (stderr, "Thread creation failed\n");
+	  return EXIT_FAILURE;
+	}
+    }
+  // Wait for the threads to complete then exit.
+  for (int tid = 0; tid < THREADCOUNT; ++tid)
+    {
+      pthread_join (threads[tid], NULL);
+    }
+  pthread_exit (NULL);
+  return EXIT_SUCCESS;
+}
diff --git a/gdb/testsuite/gdb.threads/multiple-successive-infcall.exp b/gdb/testsuite/gdb.threads/multiple-successive-infcall.exp
new file mode 100644
index 0000000..a71f991
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/multiple-successive-infcall.exp
@@ -0,0 +1,72 @@
+# 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/>.  */
+
+# multiple-successive-infcall.exp -- Test if GDB can invoke functions on
+# multiple inferiors, one after the other.
+
+standard_testfile
+
+if [get_compiler_info] {
+  return -1
+}
+
+if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
+  executable {debug}] != "" } {
+  return -1
+}
+
+clean_restart "${binfile}"
+
+if ![runto_main] then {
+   fail "can't run to main"
+   return 0
+}
+
+# Ensure that each new thread is detected by GDB in the order that the
+# test case creates them, so the thread identifiers match between
+# test and test case.
+gdb_breakpoint [gdb_get_line_number "prethreadcreationmarker"]
+gdb_continue_to_breakpoint "prethreadcreationmarker"
+set after_new_thread_message "created new thread"
+foreach_with_prefix thread {5 4 3}  {
+  gdb_test_multiple "continue" "${after_new_thread_message}" {
+    -re "\\\[New Thread ${hex} \\\(LWP \[0-9\]+\\\)\\\].*${gdb_prompt}" {
+      pass "${after_new_thread_message}"
+    }
+  }
+}
+
+gdb_breakpoint [gdb_get_line_number "testmarker01"]
+gdb_continue_to_breakpoint "testmarker01"
+gdb_test_no_output "set scheduler-locking on"
+gdb_test "show scheduler-locking" \
+  "Mode for locking scheduler during execution is \"on\"."
+
+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
+    }
+  }
+}
-- 
2.7.4