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

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

On 2018-03-27 12:23, Richard Bunt wrote:
> 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.

Hi Richard,

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.

The test looks good in general, I just have have a few comments:

> 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      | 104 
> +++++++++++++++++++++
>  .../gdb.threads/multiple-successive-infcall.exp    |  55 +++++++++++
>  2 files changed, 159 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..3010c18
> --- /dev/null
> +++ b/gdb/testsuite/gdb.threads/multiple-successive-infcall.c
> @@ -0,0 +1,104 @@
> +/* 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];
> +/* Memory for threads to read from when get_value is called by the
> +   debugger.  */
> +int thread_ids[THREADCOUNT];
> +
> +/* Return the argument back, within range [0..THREADCOUNT).
> +   This is the function called on the inferiors from the debugger.  */
> +int
> +get_value (int index)
> +{
> +  return thread_ids[index];
> +}

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.

> +/* 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 = get_value (*((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);
> +  // Create the worker threads (main).
> +  printf ("Spawning worker threads\n");
> +  for (int tid = 0; tid < THREADCOUNT; ++tid)
> +    {
> +      thread_ids[tid] = tid;
> +      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..363f8e0
> --- /dev/null
> +++ b/gdb/testsuite/gdb.threads/multiple-successive-infcall.exp
> @@ -0,0 +1,55 @@
> +# 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"

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


> +   return 0
> +}
> +
> +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 thread {4 3 2 1}  {

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.

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:

https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique

> +  gdb_test "thread ${thread}" "Switching to .*"
> +  set command "call get_value(0)"
> +  gdb_test_multiple "${command}" "${command}" {
> +    -re ".*\[\r\n]+${gdb_prompt} $" {

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

> +      pass "Call on inferior returned successfully."

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.

> +    }
> +    timeout {
> +      kfail "gdb/22882" "Call on inferior hung."
> +      return 0
> +    }
> +  }

Thanks,

Simon