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 E39EB3858418 for ; Mon, 12 Jun 2023 13:12:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org E39EB3858418 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=1686575527; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=9Ao4uQDiQNbQxMtbAzFSSr+l+oZ+GYMIC0+PHhsuSpY=; b=S0VW8IYZZgZqiIcmbYoJYBYlDfnCpG01tU7NAlbKGG0AOEher5RW5lHCF1ovgnzgRCmT+8 9XseXUtNVAMGDXzeWQzFn+O81R54clQrjbh1kUFrRCUa9am6vIo3dsBmxquU3co6oeKo5r ZQNAob7zdhmP8Fc9q00IUJiwL8QEx00= Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-413-nYPDmnuGMEyYSxjxeMSrmg-1; Mon, 12 Jun 2023 09:12:06 -0400 X-MC-Unique: nYPDmnuGMEyYSxjxeMSrmg-1 Received: by mail-wr1-f69.google.com with SMTP id ffacd0b85a97d-30fa3ea38bcso991831f8f.1 for ; Mon, 12 Jun 2023 06:12:05 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686575524; x=1689167524; h=mime-version:message-id:date:references:in-reply-to:subject:to:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=9Ao4uQDiQNbQxMtbAzFSSr+l+oZ+GYMIC0+PHhsuSpY=; b=PC/zIRHvnH9FsVA6Apt41l/ns8JF0cTwVgGIIc+IVwzGSzJ9+vjbEh/397gQKzXInY WW0RS6a3j+QiQ2+8EKZaNBTw6QQKpHqIO10WvXEtLtjK35oby0KlJVf8R00jXg9Waq+j de8tzoBxn4NFOWujCMBT2OIM/npo9hFbV5WqBHejzt1Xng5jRBHERix6h9FBjA6Xw839 HUMJKGSTwZKrEvGmJo6S0cL/UMM8SuIo8gOu5RZ1ICGzWeTLKrWoYz0mFCO+UxVO0MQq ou/HKLAT6ws9gYxMyhqbpMzh/tzuK38buQxv8QsIn2Gwv22wwgsT8i+0Pf+shQm1oRdX qhAA== X-Gm-Message-State: AC+VfDy5qOCAzeHH6VkiSxqQjkgUl0i/pl3eQEMaGikwJXEbvHCNWJa9 ejJZiRDQ3LFpBQwoCyT/zuencSWp2VGve9/s3+rJRoplvxIsKpviLsQMxCm2ar7nI4USfJip26w sji4iPtjV9fhA5UEJBRZZZJXEJWhvqQ== X-Received: by 2002:a5d:590d:0:b0:30a:c341:920a with SMTP id v13-20020a5d590d000000b0030ac341920amr3814024wrd.28.1686575524640; Mon, 12 Jun 2023 06:12:04 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ4ZANV+QxKz0DjVKB6taIFYi72KQTmsTeRyZncJc+XqJf2vRY4vph8heQJynFx1ly3y60764Q== X-Received: by 2002:a5d:590d:0:b0:30a:c341:920a with SMTP id v13-20020a5d590d000000b0030ac341920amr3814002wrd.28.1686575524191; Mon, 12 Jun 2023 06:12:04 -0700 (PDT) Received: from localhost (2.72.115.87.dyn.plus.net. [87.115.72.2]) by smtp.gmail.com with ESMTPSA id v18-20020adfebd2000000b0030789698eebsm12456776wrn.89.2023.06.12.06.12.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 12 Jun 2023 06:12:03 -0700 (PDT) From: Andrew Burgess To: Pedro Alves , gdb-patches@sourceware.org Subject: Re: [PATCH 31/31] Cancel execution command on thread exit, when stepping, nexting, etc. In-Reply-To: <20221212203101.1034916-32-pedro@palves.net> References: <20221212203101.1034916-1-pedro@palves.net> <20221212203101.1034916-32-pedro@palves.net> Date: Mon, 12 Jun 2023 14:12:02 +0100 Message-ID: <871qigygd9.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=-11.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,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 your target has no support for TARGET_WAITKIND_NO_RESUMED events > (and no way to support them, such as the yet-unsubmitted AMDGPU > target), and you step over thread exit with scheduler-locking on, this > is what you get: > > (gdb) n > [Thread ... exited] > *hang* > > Getting back the prompt by typing Ctrl-C may not even work, since no > inferior thread is running to receive the SIGINT. Even if it works, > it seems unnecessarily harsh. If you started an execution command for > which there's a clear thread of interest (step, next, until, etc.), > and that thread disappears, then I think it's more user friendly if > GDB just detects the situation and aborts the command, giving back the > prompt. > > That is what this commit implements. It does this by explicitly > requesting the target to report thread exit events whenever the main > resumed thread has a thread_fsm. Note that unlike stepping over a > breakpoint, we don't need to enable clone events in this case. > > With this patch, we get: > > (gdb) n > [Thread 0x7ffff7d89700 (LWP 3961883) exited] > Command aborted, thread exited. > (gdb) LGTM. Reviewed-By: Andrew Burgess Thanks, Andrew > > Change-Id: I901ab64c91d10830590b2dac217b5264635a2b95 > --- > gdb/infrun.c | 73 ++++++++++++++--- > .../gdb.threads/step-over-thread-exit.exp | 79 ++++++++++++------- > 2 files changed, 115 insertions(+), 37 deletions(-) > > diff --git a/gdb/infrun.c b/gdb/infrun.c > index 21e5aa0f50e..61d2a14c646 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -1886,6 +1886,22 @@ displaced_step_prepare (thread_info *thread) > return status; > } > > +/* True if any thread of TARGET that matches RESUME_PTID requires > + target_thread_events enabled. This assumes TARGET does not support > + target thread options. */ > + > +static bool > +any_thread_needs_target_thread_events (process_stratum_target *target, > + ptid_t resume_ptid) > +{ > + for (thread_info *tp : all_non_exited_threads (target, resume_ptid)) > + if (displaced_step_in_progress_thread (tp) > + || schedlock_applies (tp) > + || tp->thread_fsm () != nullptr) > + return true; > + return false; > +} > + > /* Maybe disable thread-{cloned,created,exited} event reporting after > a step-over (either in-line or displaced) finishes. */ > > @@ -1909,9 +1925,10 @@ update_thread_events_after_step_over (thread_info *event_thread, > else > { > /* We can only control the target-wide target_thread_events > - setting. Disable it, but only if other threads don't need it > - enabled. */ > - if (!displaced_step_in_progress_any_thread ()) > + setting. Disable it, but only if other threads in the target > + don't need it enabled. */ > + process_stratum_target *target = event_thread->inf->process_target (); > + if (!any_thread_needs_target_thread_events (target, minus_one_ptid)) > target_thread_events (false); > } > } > @@ -2488,12 +2505,25 @@ do_target_resume (ptid_t resume_ptid, bool step, enum gdb_signal sig) > else > target_thread_events (true); > } > + else if (tp->thread_fsm () != nullptr) > + { > + gdb_thread_options options = GDB_THREAD_OPTION_EXIT; > + if (target_supports_set_thread_options (options)) > + tp->set_thread_options (options); > + 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); > + else > + { > + process_stratum_target *resume_target = tp->inf->process_target (); > + if (!any_thread_needs_target_thread_events (resume_target, > + resume_ptid)) > + target_thread_events (false); > + } > } > > /* If we're resuming more than one thread simultaneously, then any > @@ -5671,6 +5701,13 @@ handle_thread_exited (execution_control_state *ecs) > ecs->event_thread->stepping_over_breakpoint = 0; > ecs->event_thread->stepping_over_watchpoint = 0; > > + /* If the thread had an FSM, then abort the command. But only after > + finishing the step over, as in non-stop mode, aborting this > + thread's command should not interfere with other threads. We > + must check this before finish_step over, however, which may > + update the thread list and delete the event thread. */ > + bool abort_cmd = (ecs->event_thread->thread_fsm () != nullptr); > + > /* Maybe the thread was doing a step-over, if so release > resources and start any further pending step-overs. > > @@ -5684,6 +5721,13 @@ handle_thread_exited (execution_control_state *ecs) > the event thread has exited. */ > gdb_assert (ret == 0); > > + if (abort_cmd) > + { > + delete_thread (ecs->event_thread); > + ecs->event_thread = nullptr; > + return false; > + } > + > /* If finish_step_over started a new in-line step-over, don't > try to restart anything else. */ > if (step_over_info_valid_p ()) > @@ -9062,7 +9106,8 @@ normal_stop (void) > if (inferior_ptid != null_ptid) > finish_ptid = ptid_t (inferior_ptid.pid ()); > } > - else if (last.kind () != TARGET_WAITKIND_NO_RESUMED) > + else if (last.kind () != TARGET_WAITKIND_NO_RESUMED > + && last.kind () != TARGET_WAITKIND_THREAD_EXITED) > finish_ptid = inferior_ptid; > > gdb::optional maybe_finish_thread_state; > @@ -9105,7 +9150,8 @@ normal_stop (void) > { > if ((last.kind () != TARGET_WAITKIND_SIGNALLED > && last.kind () != TARGET_WAITKIND_EXITED > - && last.kind () != TARGET_WAITKIND_NO_RESUMED) > + && last.kind () != TARGET_WAITKIND_NO_RESUMED > + && last.kind () != TARGET_WAITKIND_THREAD_EXITED) > && target_has_execution () > && previous_thread != inferior_thread ()) > { > @@ -9121,7 +9167,8 @@ normal_stop (void) > update_previous_thread (); > } > > - if (last.kind () == TARGET_WAITKIND_NO_RESUMED) > + if (last.kind () == TARGET_WAITKIND_NO_RESUMED > + || last.kind () == TARGET_WAITKIND_THREAD_EXITED) > { > stop_print_frame = false; > > @@ -9129,7 +9176,12 @@ normal_stop (void) > if (current_ui->prompt_state == PROMPT_BLOCKED) > { > target_terminal::ours_for_output (); > - gdb_printf (_("No unwaited-for children left.\n")); > + if (last.kind () == TARGET_WAITKIND_NO_RESUMED) > + gdb_printf (_("No unwaited-for children left.\n")); > + else if (last.kind () == TARGET_WAITKIND_THREAD_EXITED) > + gdb_printf (_("Command aborted, thread exited.\n")); > + else > + gdb_assert_not_reached ("unhandled"); > } > } > > @@ -9214,7 +9266,8 @@ normal_stop (void) > { > if (last.kind () != TARGET_WAITKIND_SIGNALLED > && last.kind () != TARGET_WAITKIND_EXITED > - && last.kind () != TARGET_WAITKIND_NO_RESUMED) > + && last.kind () != TARGET_WAITKIND_NO_RESUMED > + && last.kind () != TARGET_WAITKIND_THREAD_EXITED) > /* Delete the breakpoint we stopped at, if it wants to be deleted. > Delete any breakpoint that is to be deleted at the next stop. */ > breakpoint_auto_delete (inferior_thread ()->control.stop_bpstat); > diff --git a/gdb/testsuite/gdb.threads/step-over-thread-exit.exp b/gdb/testsuite/gdb.threads/step-over-thread-exit.exp > index ed8534cf518..a0056740478 100644 > --- a/gdb/testsuite/gdb.threads/step-over-thread-exit.exp > +++ b/gdb/testsuite/gdb.threads/step-over-thread-exit.exp > @@ -29,7 +29,7 @@ if { [build_executable "failed to prepare" $testfile \ > # NS_STOP_ALL is only used if testing "set non-stop on", and indicates > # whether to have GDB explicitly stop all threads before continuing to > # thread exit. > -proc test {displaced-stepping non-stop target-non-stop schedlock ns_stop_all} { > +proc test {displaced-stepping non-stop target-non-stop schedlock cmd ns_stop_all} { > if {${non-stop} == "off" && $ns_stop_all} { > error "invalid arguments" > } > @@ -72,31 +72,54 @@ proc test {displaced-stepping non-stop target-non-stop schedlock ns_stop_all} { > > gdb_test_no_output "set scheduler-locking ${schedlock}" > > - gdb_test "continue" \ > - "No unwaited-for children left." \ > - "continue stops when thread exits" > + if {$cmd == "continue"} { > + gdb_test "continue" \ > + "No unwaited-for children left." \ > + "continue stops when thread exits" > + } else { > + gdb_test $cmd \ > + "Command aborted, thread exited\\." \ > + "command aborts when thread exits" > + } > } else { > gdb_test_no_output "set scheduler-locking ${schedlock}" > > - for { set i 0 } { $i < 100 } { incr i } { > - with_test_prefix "iter $i" { > - set ok 0 > - set thread "" > - gdb_test_multiple "continue" "" { > - -re -wrap "Thread ($::decimal) .*hit Breakpoint $::decimal.* my_exit_syscall .*" { > - set thread $expect_out(1,string) > - set ok 1 > - } > - } > - if {!${ok}} { > - # Exit if there's a failure to avoid lengthy > - # timeouts. > - break > + if {$cmd != "continue"} { > + set thread "" > + gdb_test_multiple "continue" "" { > + -re -wrap "Thread ($::decimal) .*hit Breakpoint $::decimal.* my_exit_syscall .*" { > + set thread $expect_out(1,string) > } > + } > + if {${non-stop}} { > + gdb_test -nopass "thread $thread" "Switching to thread .*" \ > + "switch to event thread" > + } > > - if {${non-stop}} { > - gdb_test "thread $thread" "Switching to thread .*" \ > - "switch to event thread" > + gdb_test $cmd \ > + "Command aborted, thread exited\\." \ > + "command aborts when thread exits" > + } else { > + for { set i 0 } { $i < 100 } { incr i } { > + with_test_prefix "iter $i" { > + set ok 0 > + set thread "" > + gdb_test_multiple "continue" "" { > + -re -wrap "Thread ($::decimal) .*hit Breakpoint $::decimal.* my_exit_syscall .*" { > + set thread $expect_out(1,string) > + set ok 1 > + } > + } > + if {!${ok}} { > + # Exit if there's a failure to avoid lengthy > + # timeouts. > + break > + } > + > + if {${non-stop}} { > + gdb_test -nopass "thread $thread" "Switching to thread .*" \ > + "switch to event thread" > + } > } > } > } > @@ -112,13 +135,15 @@ foreach_with_prefix displaced-stepping {off auto} { > } > > foreach_with_prefix schedlock {off on} { > - if {${non-stop} == "on"} { > - foreach_with_prefix ns_stop_all {0 1} { > - test ${displaced-stepping} ${non-stop} ${target-non-stop} \ > - ${schedlock} ${ns_stop_all} > + foreach_with_prefix cmd {"next" "continue"} { > + if {${non-stop} == "on"} { > + foreach_with_prefix ns_stop_all {0 1} { > + test ${displaced-stepping} ${non-stop} ${target-non-stop} \ > + ${schedlock} ${cmd} ${ns_stop_all} > + } > + } else { > + test ${displaced-stepping} ${non-stop} ${target-non-stop} ${schedlock} ${cmd} 0 > } > - } else { > - test ${displaced-stepping} ${non-stop} ${target-non-stop} ${schedlock} 0 > } > } > } > -- > 2.36.0