From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id E7648383A37A for ; Thu, 21 Jul 2022 18:49:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E7648383A37A Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 9E6C11E21F; Thu, 21 Jul 2022 14:49:44 -0400 (EDT) Message-ID: Date: Thu, 21 Jul 2022 14:49:44 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH v2 21/29] Don't resume new threads if scheduler-locking is in effect Content-Language: en-US To: Pedro Alves , gdb-patches@sourceware.org References: <20220713222433.374898-1-pedro@palves.net> <20220713222433.374898-22-pedro@palves.net> From: Simon Marchi In-Reply-To: <20220713222433.374898-22-pedro@palves.net> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 21 Jul 2022 18:49:47 -0000 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 . */ > + > +#include > +#include > +#include > + > +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 . > + > +# 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