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

[Simon Marchi <simon dot marchi at polymtl dot ca>]

On 2018-04-09 12:12 PM, Richard Bunt wrote:
> 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

Thanks for the update.  I pushed the patch with a few formatting fixes,
here's the result.

Simon


>From d27d16bfdc3f275b379793214db9bd9467b6f0f8 Mon Sep 17 00:00:00 2001
From: Richard Bunt <richard.bunt@arm.com>
Date: Thu, 19 Apr 2018 23:02:35 -0400
Subject: [PATCH] Add test case for a known hang in infrun

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/testsuite/ChangeLog                       |   5 +
 .../gdb.threads/multiple-successive-infcall.c | 111 ++++++++++++++++++
 .../multiple-successive-infcall.exp           |  72 ++++++++++++
 3 files changed, 188 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/ChangeLog b/gdb/testsuite/ChangeLog
index 68ae3d7b5a26..d5e942904130 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2018-04-19  Richard Bunt  <richard.bunt@arm.com>
+
+	* gdb.threads/multiple-successive-infcall.c: New test.
+	* gdb.threads/multiple-successive-infcall.exp: New file.
+
 2018-04-17  Tom Tromey  <tom@tromey.com>

 	* gdb.rust/simple.rs (Union): New type.
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 000000000000..9bdb9a272778
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/multiple-successive-infcall.c
@@ -0,0 +1,111 @@
+/* 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.  */
+static pthread_barrier_t print_barrier;
+
+/* Define global thread identifiers.  */
+static pthread_t threads[THREADCOUNT];
+
+/* Hold values for each thread at the index supplied to the thread
+   on creation.  */
+static int thread_ids[THREADCOUNT];
+
+/* Find the value associated with the calling thread.  */
+static 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.  */
+static 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.  */
+static 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);
+}
+
+int
+main (void)
+{
+  int err = pthread_barrier_init (&print_barrier, NULL, THREADCOUNT);
+  if (err != 0)
+    {
+      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 != 0)
+	{
+	  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);
+
+  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 000000000000..a71f991d6f26
--- /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.17.0