From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 68D413858C20 for ; Thu, 8 Jun 2023 18:24:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 68D413858C20 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1686248694; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=MdbWDFM/FwO1PUPd9ehMbcsXsvVmd2W6ShySpbaMaxA=; b=gYrp9AHJ3z1BcjuoIUJLkZXrlOEpK9z9/dFm0gN7aRetstryjBDlfya1FiOtd8iI74enar L1pwLnA6N8RpsrECIlI+mcuP+KRImXElDLSoeob9dbGO8nWqoKPP4jrw99DN+OHNoOmrFG GlMStQTrlZPPG9SXluSDhz6LOwCiR1A= Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-385-dBPv2PO4NYKCuGF-gMu0OQ-1; Thu, 08 Jun 2023 14:24:53 -0400 X-MC-Unique: dBPv2PO4NYKCuGF-gMu0OQ-1 Received: by mail-wm1-f70.google.com with SMTP id 5b1f17b1804b1-3f7eb414fcbso4491605e9.2 for ; Thu, 08 Jun 2023 11:24:53 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686248692; x=1688840692; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=MdbWDFM/FwO1PUPd9ehMbcsXsvVmd2W6ShySpbaMaxA=; b=PR91SXMv1RRWNf29eGcepPLHL7uyrC0nqV8ImQGj4BXT3wBeUEvdBQuEZKGVaXdm9x 0vTG2xAYnAqtzYSYgmkq/kBUjSzXpdsc6q4qZhmo9l0kSbYDBjTqsdczkubg+rd/2qj/ fIF6crFKNAYSSV6+KvEu3K74TFmPtarup1vnsnm5RkeWUH6IWZtlxee37JJeQ0bCoah8 DpPtWoj42VFG/pTG/ofiZUq3fDRMv7UwRzjTP25wDWUJgXnyMntqghhSy2d0CTChSDFW 7osE1HXq/I66W2JzCgZPWFvioS0s+NQaeqel3QyB2J6FEcaigSsfKr2tE9jFU2ARSVfG c9iA== X-Gm-Message-State: AC+VfDx7N7dyYOMVEOe2vntR6jEsHrSeembJ6EBluQDdsS9r3n7UXdFQ TOvtxJ9a+d9fm+3EqK4QirOmE34+U8uQmkCwrr1+awIpzlKHa+N+drEkcj5bi9S7K/SWAhvPzmh IEBsCWM2izy/ZV5qdSeyjNA== X-Received: by 2002:a05:600c:3786:b0:3f7:e7a3:4a1c with SMTP id o6-20020a05600c378600b003f7e7a34a1cmr1858298wmr.31.1686248692161; Thu, 08 Jun 2023 11:24:52 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ5mmx8TqvK8alhjBzxsk6NBAA5OpLjoWklGZ2mleZ2jZE6/w0HMabyE1sXPq+KjLqaVhkIe4Q== X-Received: by 2002:a05:600c:3786:b0:3f7:e7a3:4a1c with SMTP id o6-20020a05600c378600b003f7e7a34a1cmr1858281wmr.31.1686248691696; Thu, 08 Jun 2023 11:24:51 -0700 (PDT) Received: from localhost (92.40.218.122.threembb.co.uk. [92.40.218.122]) by smtp.gmail.com with ESMTPSA id q25-20020a7bce99000000b003f17848673fsm349660wmj.27.2023.06.08.11.24.51 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 08 Jun 2023 11:24:51 -0700 (PDT) From: Andrew Burgess To: Pedro Alves , gdb-patches@sourceware.org Cc: Eli Zaretskii Subject: Re: [PATCH 23/31] Don't resume new threads if scheduler-locking is in effect In-Reply-To: <20221212203101.1034916-24-pedro@palves.net> References: <20221212203101.1034916-1-pedro@palves.net> <20221212203101.1034916-24-pedro@palves.net> Date: Thu, 08 Jun 2023 19:24:49 +0100 Message-ID: <874jnhzua6.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-10.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,RCVD_IN_BARRACUDACENTRAL,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Pedro Alves 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 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 > 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 . */ > + > +#include > +#include > +#include > + > +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 . > + > +# 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