From: Andrew Burgess <aburgess@redhat.com>
To: Pedro Alves <pedro@palves.net>, gdb-patches@sourceware.org
Cc: Eli Zaretskii <eliz@gnu.org>
Subject: Re: [PATCH 23/31] Don't resume new threads if scheduler-locking is in effect
Date: Thu, 08 Jun 2023 19:24:49 +0100 [thread overview]
Message-ID: <874jnhzua6.fsf@redhat.com> (raw)
In-Reply-To: <20221212203101.1034916-24-pedro@palves.net>
Pedro Alves <pedro@palves.net> writes:
> If scheduler-locking is in effect, like e.g., with "set
I think 'like' in 'like e.g.' is redundant. But is there any other way
to enable scheduler-locking mode? So wouldn't i.e. be a better choice?
But really, this all LGTM.
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
Thanks,
Andrew
> 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>
> Change-Id: Ie12140138b37534b7fc1d904da34f0f174aa11ce
> ---
> gdb/NEWS | 7 ++
> gdb/doc/gdb.texinfo | 4 +-
> gdb/infrun.c | 41 +++++++++---
> .../gdb.threads/schedlock-new-thread.c | 54 +++++++++++++++
> .../gdb.threads/schedlock-new-thread.exp | 67 +++++++++++++++++++
> 5 files changed, 164 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 c4ccfcc9e32..0aa153b83da 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -15,6 +15,13 @@
> 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 5b566669975..5e75f32e0cd 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -7037,7 +7037,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 31321d758da..09391d85256 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;
> @@ -1891,7 +1893,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. */
> @@ -2464,9 +2472,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;
> @@ -2475,6 +2488,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
> @@ -6103,16 +6123,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..aba32f3c19d
> --- /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-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)
> +{
> +#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
> index 00000000000..0a22cd178f2
> --- /dev/null
> +++ b/gdb/testsuite/gdb.threads/schedlock-new-thread.exp
> @@ -0,0 +1,67 @@
> +# 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
> +
> +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}
> + }
> +}
> --
> 2.36.0
next prev parent reply other threads:[~2023-06-08 18:24 UTC|newest]
Thread overview: 100+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-12 20:30 [PATCH 00/31] Step over thread clone and thread exit Pedro Alves
2022-12-12 20:30 ` [PATCH 01/31] displaced step: pass down target_waitstatus instead of gdb_signal Pedro Alves
2023-02-03 10:44 ` Andrew Burgess
2023-03-10 17:15 ` Pedro Alves
2023-03-16 16:07 ` Andrew Burgess
2023-03-22 21:29 ` Andrew Burgess
2023-03-23 15:15 ` Pedro Alves
2023-03-27 12:40 ` Andrew Burgess
2023-03-27 16:21 ` Pedro Alves
2022-12-12 20:30 ` [PATCH 02/31] linux-nat: introduce pending_status_str Pedro Alves
2023-02-03 12:00 ` Andrew Burgess
2023-03-10 17:15 ` Pedro Alves
2023-03-16 16:19 ` Andrew Burgess
2023-03-27 18:05 ` Pedro Alves
2022-12-12 20:30 ` [PATCH 03/31] gdb/linux: Delete all other LWPs immediately on ptrace exec event Pedro Alves
2023-03-21 14:50 ` Andrew Burgess
2023-04-04 13:57 ` Pedro Alves
2023-04-14 19:29 ` Pedro Alves
2023-05-26 15:04 ` Andrew Burgess
2023-11-13 14:04 ` Pedro Alves
2023-05-26 14:45 ` Andrew Burgess
2022-12-12 20:30 ` [PATCH 04/31] Step over clone syscall w/ breakpoint, TARGET_WAITKIND_THREAD_CLONED Pedro Alves
2023-02-04 15:38 ` Andrew Burgess
2023-03-10 17:16 ` Pedro Alves
2023-03-21 16:06 ` Andrew Burgess
2023-11-13 14:05 ` Pedro Alves
2022-12-12 20:30 ` [PATCH 05/31] Support clone events in the remote protocol Pedro Alves
2023-03-22 15:46 ` Andrew Burgess
2023-11-13 14:05 ` Pedro Alves
2022-12-12 20:30 ` [PATCH 06/31] Avoid duplicate QThreadEvents packets Pedro Alves
2023-05-26 15:53 ` Andrew Burgess
2022-12-12 20:30 ` [PATCH 07/31] enum_flags to_string Pedro Alves
2023-01-30 20:07 ` Simon Marchi
2022-12-12 20:30 ` [PATCH 08/31] Thread options & clone events (core + remote) Pedro Alves
2023-01-31 12:25 ` Lancelot SIX
2023-03-10 19:16 ` Pedro Alves
2023-06-06 13:29 ` Andrew Burgess
2023-11-13 14:07 ` Pedro Alves
2022-12-12 20:30 ` [PATCH 09/31] Thread options & clone events (native Linux) Pedro Alves
2023-06-06 13:43 ` Andrew Burgess
2022-12-12 20:30 ` [PATCH 10/31] Thread options & clone events (Linux GDBserver) Pedro Alves
2023-06-06 14:12 ` Andrew Burgess
2023-11-13 14:07 ` Pedro Alves
2022-12-12 20:30 ` [PATCH 11/31] gdbserver: Hide and don't detach pending clone children Pedro Alves
2023-06-07 16:10 ` Andrew Burgess
2023-11-13 14:08 ` Pedro Alves
2022-12-12 20:30 ` [PATCH 12/31] Remove gdb/19675 kfails (displaced stepping + clone) Pedro Alves
2023-06-07 17:08 ` Andrew Burgess
2022-12-12 20:30 ` [PATCH 13/31] Add test for stepping over clone syscall Pedro Alves
2023-06-07 17:42 ` Andrew Burgess
2023-11-13 14:09 ` Pedro Alves
2022-12-12 20:30 ` [PATCH 14/31] all-stop/synchronous RSP support thread-exit events Pedro Alves
2023-06-07 17:52 ` Andrew Burgess
2023-11-13 14:11 ` Pedro Alves
2023-12-15 18:15 ` Pedro Alves
2022-12-12 20:30 ` [PATCH 15/31] gdbserver/linux-low.cc: Ignore event_ptid if TARGET_WAITKIND_IGNORE Pedro Alves
2022-12-12 20:30 ` [PATCH 16/31] Move deleting thread on TARGET_WAITKIND_THREAD_EXITED to core Pedro Alves
2023-06-08 12:27 ` Andrew Burgess
2022-12-12 20:30 ` [PATCH 17/31] Introduce GDB_THREAD_OPTION_EXIT thread option, fix step-over-thread-exit Pedro Alves
2023-06-08 13:17 ` Andrew Burgess
2022-12-12 20:30 ` [PATCH 18/31] Implement GDB_THREAD_OPTION_EXIT support for Linux GDBserver Pedro Alves
2023-06-08 14:14 ` Andrew Burgess
2022-12-12 20:30 ` [PATCH 19/31] Implement GDB_THREAD_OPTION_EXIT support for native Linux Pedro Alves
2023-06-08 14:17 ` Andrew Burgess
2022-12-12 20:30 ` [PATCH 20/31] gdb: clear step over information on thread exit (PR gdb/27338) Pedro Alves
2023-06-08 15:29 ` Andrew Burgess
2022-12-12 20:30 ` [PATCH 21/31] stop_all_threads: (re-)enable async before waiting for stops Pedro Alves
2023-06-08 15:49 ` Andrew Burgess
2023-11-13 14:12 ` Pedro Alves
2022-12-12 20:30 ` [PATCH 22/31] gdbserver: Queue no-resumed event after thread exit Pedro Alves
2023-06-08 18:16 ` Andrew Burgess
2023-11-13 14:12 ` Pedro Alves
2022-12-12 20:30 ` [PATCH 23/31] Don't resume new threads if scheduler-locking is in effect Pedro Alves
2023-06-08 18:24 ` Andrew Burgess [this message]
2023-11-13 14:12 ` Pedro Alves
2022-12-12 20:30 ` [PATCH 24/31] Report thread exit event for leader if reporting thread exit events Pedro Alves
2023-06-09 13:11 ` Andrew Burgess
2022-12-12 20:30 ` [PATCH 25/31] Ignore failure to read PC when resuming Pedro Alves
2023-06-10 10:33 ` Andrew Burgess
2023-11-13 14:13 ` Pedro Alves
2022-12-12 20:30 ` [PATCH 26/31] gdb/testsuite/lib/my-syscalls.S: Refactor new SYSCALL macro Pedro Alves
2023-06-10 10:33 ` Andrew Burgess
2022-12-12 20:30 ` [PATCH 27/31] Testcases for stepping over thread exit syscall (PR gdb/27338) Pedro Alves
2023-06-12 9:53 ` Andrew Burgess
2022-12-12 20:30 ` [PATCH 28/31] Document remote clone events, and QThreadOptions packet Pedro Alves
2023-06-05 15:53 ` Andrew Burgess
2023-11-13 14:13 ` Pedro Alves
2023-06-12 12:06 ` Andrew Burgess
2023-11-13 14:15 ` Pedro Alves
2022-12-12 20:30 ` [PATCH 29/31] inferior::clear_thread_list always silent Pedro Alves
2023-06-12 12:20 ` Andrew Burgess
2022-12-12 20:31 ` [PATCH 30/31] Centralize "[Thread ...exited]" notifications Pedro Alves
2023-02-04 16:05 ` Andrew Burgess
2023-03-10 17:21 ` Pedro Alves
2023-02-16 15:40 ` Andrew Burgess
2023-06-12 12:23 ` Andrew Burgess
2022-12-12 20:31 ` [PATCH 31/31] Cancel execution command on thread exit, when stepping, nexting, etc Pedro Alves
2023-06-12 13:12 ` Andrew Burgess
2023-01-24 19:47 ` [PATCH v3 00/31] Step over thread clone and thread exit Pedro Alves
2023-11-13 14:24 ` [PATCH " Pedro Alves
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=874jnhzua6.fsf@redhat.com \
--to=aburgess@redhat.com \
--cc=eliz@gnu.org \
--cc=gdb-patches@sourceware.org \
--cc=pedro@palves.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).