Re: [PATCH v2 21/29] Don't resume new threads if scheduler-locking is in effect

[Simon Marchi <simark@simark.ca>]

On 2022-07-13 18:24, Pedro Alves wrote:
> If scheduler-locking is in effect, like e.g., with "set
> scheduler-locking on", and you step over a function that spawns a new
> thread, the new thread is allowed to run free, at least until some
> event is hit, at which point, whether the new thread is re-resumed
> depends on a number of seemingly random factors.  E.g., if the target
> is all-stop, and the parent thread hits a breakpoint, and gdb decides
> the breakpoint isn't interesting to report to the user, then the
> parent thread is resumed, but the new thread is left stopped.
> 
> I think that letting the new threads run with scheduler-locking
> enabled is a defect.  This commit fixes that, making use of the new
> clone events on Linux, and of target_thread_events() on targets where
> new threads have no connection to the thread that spawned them.
> 
> Testcase and documentation changes included.
> 
> Change-Id: Ie12140138b37534b7fc1d904da34f0f174aa11ce
> ---
>  gdb/NEWS                                      |  7 +++
>  gdb/doc/gdb.texinfo                           |  4 +-
>  gdb/infrun.c                                  | 41 +++++++++---
>  .../gdb.threads/schedlock-new-thread.c        | 46 ++++++++++++++
>  .../gdb.threads/schedlock-new-thread.exp      | 63 +++++++++++++++++++
>  5 files changed, 152 insertions(+), 9 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.threads/schedlock-new-thread.c
>  create mode 100644 gdb/testsuite/gdb.threads/schedlock-new-thread.exp
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 742f4fe47fb..f0f6990dc59 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,13 @@
>  
>  *** Changes since GDB 12
>  
> +* Scheduler-locking and new threads
> +
> +  When scheduler-locking is in effect, only the current thread may run
> +  when the inferior is resumed.  However, previously, new threads
> +  created by the resumed thread would still be able to run free.  Now,
> +  they are held stopped.
> +
>  * "info breakpoints" now displays enabled breakpoint locations of
>    disabled breakpoints as in the "y-" state.  For example:
>  
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 9a8b010476a..a15ef0ed0db 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -6947,7 +6947,9 @@ the following:
>  There is no locking and any thread may run at any time.
>  
>  @item on
> -Only the current thread may run when the inferior is resumed.
> +Only the current thread may run when the inferior is resumed.  New
> +threads created by the resumed thread are held stopped at their entry
> +point, before they execute any instruction.
>  
>  @item step
>  Behaves like @code{on} when stepping, and @code{off} otherwise.
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 50270ec4759..6df70e374b4 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -103,6 +103,8 @@ static bool start_step_over (void);
>  
>  static bool step_over_info_valid_p (void);
>  
> +static bool schedlock_applies (struct thread_info *tp);
> +
>  /* Asynchronous signal handler registered as event loop source for
>     when we have pending events ready to be passed to the core.  */
>  static struct async_event_handler *infrun_async_inferior_event_token;
> @@ -1823,7 +1825,13 @@ static void
>  update_thread_events_after_step_over (thread_info *event_thread,
>  				      const target_waitstatus &event_status)
>  {
> -  if (target_supports_set_thread_options (0))
> +  if (schedlock_applies (event_thread))
> +    {
> +      /* If scheduler-locking applies, continue reporting
> +	 thread-created/thread-cloned events.  */
> +      return;
> +    }
> +  else if (target_supports_set_thread_options (0))
>      {
>        /* We can control per-thread options.  Disable events for the
>  	 event thread, unless the thread is gone.  */
> @@ -2395,9 +2403,14 @@ do_target_resume (ptid_t resume_ptid, bool step, enum gdb_signal sig)
>         to start stopped.  We need to release the displaced stepping
>         buffer if the stepped thread exits, so we also enable
>         thread-exit events.
> +
> +     - If scheduler-locking applies, threads that the current thread
> +       spawns should remain halted.  It's not strictly necessary to
> +       enable thread-exit events in this case, but it doesn't hurt.
>    */
>    if (step_over_info_valid_p ()
> -      || displaced_step_in_progress_thread (tp))
> +      || displaced_step_in_progress_thread (tp)
> +      || schedlock_applies (tp))
>      {
>        gdb_thread_options options = GDB_TO_CLONE | GDB_TO_EXIT;
>        if (target_supports_set_thread_options (options))
> @@ -2405,6 +2418,13 @@ do_target_resume (ptid_t resume_ptid, bool step, enum gdb_signal sig)
>        else
>  	target_thread_events (true);
>      }
> +  else
> +    {
> +      if (target_supports_set_thread_options (0))
> +	tp->set_thread_options (0);
> +      else if (!displaced_step_in_progress_any_thread ())
> +	target_thread_events (false);
> +    }
>  
>    /* If we're resuming more than one thread simultaneously, then any
>       thread other than the leader is being set to run free.  Clear any
> @@ -6007,16 +6027,21 @@ handle_inferior_event (struct execution_control_state *ecs)
>  	    parent->set_running (false);
>  
>  	  /* If resuming the child, mark it running.  */
> -	  if (ecs->ws.kind () == TARGET_WAITKIND_THREAD_CLONED
> -	      || (follow_child || (!detach_fork && (non_stop || sched_multi))))
> +	  if ((ecs->ws.kind () == TARGET_WAITKIND_THREAD_CLONED
> +	       && !schedlock_applies (ecs->event_thread))
> +	      || (ecs->ws.kind () != TARGET_WAITKIND_THREAD_CLONED
> +		  && (follow_child
> +		      || (!detach_fork && (non_stop || sched_multi)))))
>  	    child->set_running (true);
>  
>  	  /* In non-stop mode, also resume the other branch.  */
>  	  if ((ecs->ws.kind () == TARGET_WAITKIND_THREAD_CLONED
> -	       && target_is_non_stop_p ())
> -	      || (!detach_fork && (non_stop
> -				   || (sched_multi
> -				       && target_is_non_stop_p ()))))
> +	       && target_is_non_stop_p ()
> +	       && !schedlock_applies (ecs->event_thread))
> +	      || (ecs->ws.kind () != TARGET_WAITKIND_THREAD_CLONED
> +		  && (!detach_fork && (non_stop
> +				       || (sched_multi
> +					   && target_is_non_stop_p ())))))
>  	    {
>  	      if (follow_child)
>  		switch_to_thread (parent);
> diff --git a/gdb/testsuite/gdb.threads/schedlock-new-thread.c b/gdb/testsuite/gdb.threads/schedlock-new-thread.c
> new file mode 100644
> index 00000000000..67a2ef61a7b
> --- /dev/null
> +++ b/gdb/testsuite/gdb.threads/schedlock-new-thread.c
> @@ -0,0 +1,46 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2021-2022 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 <pthread.h>
> +#include <assert.h>
> +#include <unistd.h>
> +
> +static void *
> +thread_func (void *arg)
> +{
> +  while (1)
> +    sleep (1);
> +}
> +
> +int
> +main (void)
> +{
> +  pthread_t thread;
> +  int ret;
> +
> +  ret = pthread_create (&thread, NULL, thread_func, NULL); /* set break 1 here */
> +  assert (ret == 0);
> +
> +  /* When testing with schedlock enabled, the new thread won't run, so
> +     we can't join it, as that would hang forever.  Instead, sleep for
> +     a bit, enough that if the spawned thread is scheduled, it hits
> +     the thread_func breakpoint before the main thread reaches the
> +     "return 0" line below.  */
> +  sleep (3);
> +
> +  return 0; /* set break 2 here */
> +}
> diff --git a/gdb/testsuite/gdb.threads/schedlock-new-thread.exp b/gdb/testsuite/gdb.threads/schedlock-new-thread.exp
> new file mode 100644
> index 00000000000..8952cb7531c
> --- /dev/null
> +++ b/gdb/testsuite/gdb.threads/schedlock-new-thread.exp
> @@ -0,0 +1,63 @@
> +# Copyright 2021-2022 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/>.
> +
> +# Test continuing over a thread spawn with scheduler-locking on.
> +
> +standard_testfile .c
> +
> +set syscalls_src $srcdir/lib/my-syscalls.S

Is this needed?

> +
> +if { [build_executable "failed to prepare" $testfile \
> +	  [list $srcfile $syscalls_src] {debug pthreads}] == -1 } {
> +    return
> +}
> +
> +proc test {non-stop schedlock} {
> +    save_vars ::GDBFLAGS {
> +	append ::GDBFLAGS " -ex \"set non-stop ${non-stop}\""
> +	clean_restart $::binfile
> +    }
> +
> +    set linenum1 [gdb_get_line_number "set break 1 here"]
> +
> +    if { ![runto $::srcfile:$linenum1] } {
> +	return
> +    }
> +
> +    delete_breakpoints
> +
> +    set linenum2 [gdb_get_line_number "set break 2 here"]
> +    gdb_breakpoint $linenum2
> +
> +    gdb_breakpoint "thread_func"
> +
> +    gdb_test_no_output "set scheduler-locking $schedlock"
> +
> +    if {$schedlock} {
> +	gdb_test "continue" \
> +	    "return 0.*set break 2 here .*" \
> +	    "continue does not stop in new thread"
> +    } else {
> +	gdb_test "continue" \
> +	    "thread_func .*" \
> +	    "continue stops in new thread"
> +    }

In the non-schedlock case, if you imagine a scheduler that messes with
you on purpose, wouldn't it be possible for main to hit its breakpoint
before the thread hits thread_func?  If so, any idea how to change the
test to avoid that?  Maybe main can join the thread in that case,
instead of doing a sleep.  The thread would have to not be an infinite
loop, but I don't see why it needs to be.

Simon