Don't resume new threads if scheduler-locking is in effect

If scheduler-locking is in effect, 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.

Approved-By: Eli Zaretskii <eliz@gnu.org>
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
Change-Id: Ie12140138b37534b7fc1d904da34f0f174aa11ce

diff --git a/gdb/NEWS b/gdb/NEWS
index d85a13b64feabcce55403700399ec9c8db8f1a52..f2861b1ace184705a2a0fd50b58a91a994327f71 100644 (file)
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -392,6 +392,13 @@ show tui mouse-events
   from the current process state.  GDB will show this additional information
   automatically, or through one of the memory-tag subcommands.
 
+* 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 4cbaaa6804f9e427b9386b7a4f4a5a3c3e54d9b8..79b7431dd78f6acdc5794341a0de0d6b4dd39696 100644 (file)
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -7123,7 +7123,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 409c2249b13bdbf7eff1c1ae14a4f036d7130ae0..943ea88538cfc02e368cd2699d2c8a4310bfe3cc 100644 (file)
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -107,6 +107,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;
@@ -1961,7 +1963,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.  */
@@ -2535,9 +2543,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_THREAD_OPTION_CLONE | GDB_THREAD_OPTION_EXIT;
@@ -2546,6 +2559,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
@@ -6295,16 +6315,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 (file)
index 0000000..4fe7769
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/schedlock-new-thread.c
@@ -0,0 +1,54 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2021-2023 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)
+{
+#if !SCHEDLOCK
+  while (1)
+    sleep (1);
+#endif
+
+  return NULL;
+}
+
+int
+main (void)
+{
+  pthread_t thread;
+  int ret;
+
+  ret = pthread_create (&thread, NULL, thread_func, NULL); /* set break 1 here */
+  assert (ret == 0);
+
+#if SCHEDLOCK
+  /* 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);
+#else
+  pthread_join (thread, NULL);
+#endif
+
+  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 (file)
index 0000000..ecaeea5
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/schedlock-new-thread.exp
@@ -0,0 +1,67 @@
+# Copyright 2021-2023 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
+
+foreach_with_prefix schedlock {off on} {
+    set sl [expr $schedlock == "on" ? 1 : 0]
+    if { [build_executable "failed to prepare" $testfile-$sl \
+             $srcfile \
+             [list debug pthreads additional_flags=-DSCHEDLOCK=$sl]] \
+            == -1 } {
+       return
+    }
+}
+
+proc test {non-stop schedlock} {
+    save_vars ::GDBFLAGS {
+       append ::GDBFLAGS " -ex \"set non-stop ${non-stop}\""
+       set sl [expr $schedlock == "on" ? 1 : 0]
+       clean_restart $::binfile-$sl
+    }
+
+    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"
+    }
+}
+
+foreach_with_prefix non-stop {off on} {
+    foreach_with_prefix schedlock {off on} {
+       test ${non-stop} ${schedlock}
+    }
+}