[PATCH v2] Add test case for a known hang in infrun
Richard Bunt
richard.bunt@arm.com
Fri Apr 20 15:42:00 GMT 2018
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
> + }
> + }
> +}
More information about the Gdb-patches
mailing list