From patchwork Mon Nov 13 15:04:27 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 79767 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id F06E6388C579 for ; Mon, 13 Nov 2023 15:08:45 +0000 (GMT) X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from mail-lj1-f182.google.com (mail-lj1-f182.google.com [209.85.208.182]) by sourceware.org (Postfix) with ESMTPS id 40D493857B93 for ; Mon, 13 Nov 2023 15:06:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 40D493857B93 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 40D493857B93 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=209.85.208.182 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699887976; cv=none; b=HOn27gXjKMJlaIaJhPnCSKEo9ZzV1lZfbD/D2tgZocNIjVDB4izPgo8eKFCRVwLgU0MpUm5SMDY8N41SkMm240xGHVLIg/ILxNWoyOKsOnhoaCiK0G9AmJEf7FnfHgFNxsEZ1XmSr2fEhRpwdJ4FGSsCmnKC4hypi2rRhtUJHKE= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699887976; c=relaxed/simple; bh=BbLPxOBuI/U6OOOv1C/Yx2QgZ8SH/+ObnjGkkt2tGfQ=; h=From:To:Subject:Date:Message-Id:MIME-Version; b=Q+rwFkphyriSu0uZGIH98Ax2GysP2UvEpsjxe3g41SFxgoVUds7pN341PNpQBW3godSWoZ5wMDkGUS9o4DnBqzWXukIFU2SRDp3bNrFbRMSJwoWZ5XUydgAT1AvJ6J8/PXcP169ctomI8kxEbIYy5R5tANKWBHVQw5w9LaZU1jw= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-lj1-f182.google.com with SMTP id 38308e7fff4ca-2c509f2c46cso63912931fa.1 for ; Mon, 13 Nov 2023 07:06:14 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1699887973; x=1700492773; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=XX7c5sYzwvUwVRc78r3YlBm/YmFV47/cajlYfjJumv4=; b=ExxWh21EtYQZLtCqORaeSDx+rJx1kZIltcZLu8SYGapG324rD3GZOxuPqptR+7NB5j q6bCzCSie9DM/ccG3KEGrbY/4COUoHL7yiEzEPDM9NkpMXrf+fr72vIRKWx26Fioc2zv vIaDRcivbtY4QAPurGv/4dQhEhf8D4EFyyj4yGmiI7gIMYiTNtrIKI9zlIiqomHfHAf9 avWKDMPg6Oo2BVJScRlibFrx9fu4XDO0FRDwzBhQ60LkKpVdGdpMdn7I8PVbYURStN+T 00KfDR8MQ6jsZb37IGTTyipgg5kMXiA2zgAHCKj6IFMxVD2jKM6Wt58Tth3y/iHtiPzA id+g== X-Gm-Message-State: AOJu0YyHZtSB/JnsaKWodkfLVhj7yHgncJarBdtLAvw1iFg15l6yBwA6 P53konTR4geY+ghPysOM1lrxCOgvhew= X-Google-Smtp-Source: AGHT+IHN1ui+zQ7xHjJGqUHYnr/SFFukw13Q+wauKb2BYLokBdKIcR8p3DokUoT416CEUhHxCPZGkg== X-Received: by 2002:a2e:988a:0:b0:2c5:19e9:422c with SMTP id b10-20020a2e988a000000b002c519e9422cmr4563175ljj.24.1699887972646; Mon, 13 Nov 2023 07:06:12 -0800 (PST) Received: from localhost ([2001:8a0:f91e:1a00:8060:1e54:fb28:9635]) by smtp.gmail.com with UTF8SMTPSA id i10-20020a05600c354a00b004053e9276easm14313212wmq.32.2023.11.13.07.06.11 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 13 Nov 2023 07:06:12 -0800 (PST) From: Pedro Alves To: gdb-patches@sourceware.org Cc: Andrew Burgess Subject: [FYI/pushed v4 25/25] Cancel execution command on thread exit, when stepping, nexting, etc. Date: Mon, 13 Nov 2023 15:04:27 +0000 Message-Id: <20231113150427.477431-26-pedro@palves.net> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20231113150427.477431-1-pedro@palves.net> References: <20231113150427.477431-1-pedro@palves.net> MIME-Version: 1.0 X-Spam-Status: No, score=-10.2 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, 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 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org 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) Reviewed-By: Andrew Burgess Change-Id: I901ab64c91d10830590b2dac217b5264635a2b95 --- gdb/infrun.c | 73 ++++++++++++++--- .../gdb.threads/step-over-thread-exit.exp | 81 +++++++++++++------ 2 files changed, 118 insertions(+), 36 deletions(-) diff --git a/gdb/infrun.c b/gdb/infrun.c index 943ea88538c..62b306ff347 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -1956,6 +1956,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. */ @@ -1979,9 +1995,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); } } @@ -2559,12 +2576,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 @@ -5842,6 +5872,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. @@ -5855,6 +5892,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 ()) @@ -9287,7 +9331,8 @@ normal_stop () 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; @@ -9330,7 +9375,8 @@ normal_stop () { 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 ()) { @@ -9346,7 +9392,8 @@ normal_stop () 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; @@ -9354,7 +9401,12 @@ normal_stop () 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"); } } @@ -9437,7 +9489,8 @@ normal_stop () { 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..615bd838763 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,58 @@ 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_multiple $cmd "command aborts when thread exits" { + -re "Command aborted, thread exited\\.\r\n$::gdb_prompt " { + pass $gdb_test_name + } + } + } } 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 {$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 {!${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" + } + + gdb_test_multiple $cmd "command aborts when thread exits" { + -re "Command aborted, thread exited\\.\r\n$::gdb_prompt " { + pass $gdb_test_name } + } + } 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 "thread $thread" "Switching to thread .*" \ - "switch to event thread" + if {${non-stop}} { + gdb_test -nopass "thread $thread" "Switching to thread .*" \ + "switch to event thread" + } } } } @@ -112,13 +139,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 } } }