This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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



On 04/20/2018 04:06 AM, Simon Marchi wrote:

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


Thanks again for your time and review comments.

Rich



 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
+    }
+  }
+}
Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]