From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f45.google.com (mail-wr1-f45.google.com [209.85.221.45]) by sourceware.org (Postfix) with ESMTPS id 421073897796 for ; Wed, 13 Jul 2022 22:25:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 421073897796 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wr1-f45.google.com with SMTP id r14so52990wrg.1 for ; Wed, 13 Jul 2022 15:25:16 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=BbWLKtiT5N5WSiqXOhKMDTzDuroQlIzhIQRONKZl5zg=; b=YEPNBbZibozkSOMtOgoR+qOTFnFDEqBJMOAlTfqElbEpMChQzd75Eoeuza1SLPMiMf 0iCG3yu4G98pU1nZIItXaxK29sazUAsZaCzcfjffDr8jBeVIfaBO58yBP3C1nLkfpikC t5ljMomDofS2iyF+gjBJHC+95VeCIfHhc5+wz+Y70l7HFBgT9WaC11Sr5Y6kdfH95XR3 W2KI5DUo5zcddUX8eMi5K0iITrv1HWqQZ/ANxB5+FPjbulikLmyO+Xkwcf8nNWpJhVg7 B4vXqZ5HSTtL3Vzfu8uovS6j1tqCVQtws+YFU9qCRxR4sDydVL3dMx7vRryDh+RDU4P3 ngvA== X-Gm-Message-State: AJIora+Tqr7gNogbY7p2ggYOZQ7UlZnXX24J9BWYkyG/moUW+iPDBDuU S8Xt9USev7NiJBcRogKw/k3b6q328A8= X-Google-Smtp-Source: AGRyM1t4KiPF4Umy4fDjPrR6FrGw3Bw2NR04npTYRGgjQw8mPSi8JNQgM7GPMtjoPOBo5KNkwlQHiQ== X-Received: by 2002:a05:6000:186f:b0:21d:969e:c80e with SMTP id d15-20020a056000186f00b0021d969ec80emr5080683wri.129.1657751114859; Wed, 13 Jul 2022 15:25:14 -0700 (PDT) Received: from localhost ([2001:8a0:f924:2600:209d:85e2:409e:8726]) by smtp.gmail.com with ESMTPSA id z1-20020a7bc7c1000000b003a2e27fc275sm3309675wmk.12.2022.07.13.15.25.13 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 13 Jul 2022 15:25:13 -0700 (PDT) From: Pedro Alves To: gdb-patches@sourceware.org Cc: Pedro Alves Subject: [PATCH v2 18/29] gdb: clear step over information on thread exit (PR gdb/27338) Date: Wed, 13 Jul 2022 23:24:22 +0100 Message-Id: <20220713222433.374898-19-pedro@palves.net> X-Mailer: git-send-email 2.36.0 In-Reply-To: <20220713222433.374898-1-pedro@palves.net> References: <20220713222433.374898-1-pedro@palves.net> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-9.4 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, RISK_FREE, 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.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 13 Jul 2022 22:25:19 -0000 From: Simon Marchi GDB doesn't handle correctly the case where a thread steps over a breakpoint (using either in-line or displaced stepping), and the executed instruction causes the thread to exit. Using the test program included later in the series, this is what it looks like with displaced-stepping, on x86-64 Linux, where we have two displaced-step buffers: $ ./gdb -q -nx --data-directory=data-directory build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-over-thread-exit/step-over-thread-exit -ex "b my_exit_syscall" -ex r Reading symbols from build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-over-thread-exit/step-over-thread-exit... Breakpoint 1 at 0x123c: file src/binutils-gdb/gdb/testsuite/lib/my-syscalls.S, line 68. Starting program: build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-over-thread-exit/step-over-thread-exit [Thread debugging using libthread_db enabled] Using host libthread_db library "/usr/lib/../lib/libthread_db.so.1". [New Thread 0x7ffff7c5f640 (LWP 2915510)] [Switching to Thread 0x7ffff7c5f640 (LWP 2915510)] Thread 2 "step-over-threa" hit Breakpoint 1, my_exit_syscall () at src/binutils-gdb/gdb/testsuite/lib/my-syscalls.S:68 68 syscall (gdb) c Continuing. [New Thread 0x7ffff7c5f640 (LWP 2915524)] [Thread 0x7ffff7c5f640 (LWP 2915510) exited] [Switching to Thread 0x7ffff7c5f640 (LWP 2915524)] Thread 3 "step-over-threa" hit Breakpoint 1, my_exit_syscall () at src/binutils-gdb/gdb/testsuite/lib/my-syscalls.S:68 68 syscall (gdb) c Continuing. [New Thread 0x7ffff7c5f640 (LWP 2915616)] [Thread 0x7ffff7c5f640 (LWP 2915524) exited] [Switching to Thread 0x7ffff7c5f640 (LWP 2915616)] Thread 4 "step-over-threa" hit Breakpoint 1, my_exit_syscall () at src/binutils-gdb/gdb/testsuite/lib/my-syscalls.S:68 68 syscall (gdb) c Continuing. ... hangs ... The first two times we do "continue", we displaced-step the syscall instruction that causes the thread to exit. When the thread exits, the main thread, waiting on pthread_join, is unblocked. It spawns a new thread, which hits the breakpoint on the syscall again. However, infrun was never notified that the displaced-stepping threads are done using the displaced-step buffer, so now both buffers are marked as used. So when we do the third continue, there are no buffers available to displaced-step the syscall, so the thread waits forever for its turn. When trying the same but with in-line step over (displaced-stepping disabled): $ ./gdb -q -nx --data-directory=data-directory \ build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-over-thread-exit/step-over-thread-exit \ -ex "b my_exit_syscall" -ex "set displaced-stepping off" -ex r Reading symbols from build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-over-thread-exit/step-over-thread-exit... Breakpoint 1 at 0x123c: file src/binutils-gdb/gdb/testsuite/lib/my-syscalls.S, line 68. Starting program: build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-over-thread-exit/step-over-thread-exit [Thread debugging using libthread_db enabled] Using host libthread_db library "/usr/lib/../lib/libthread_db.so.1". [New Thread 0x7ffff7c5f640 (LWP 2928290)] [Switching to Thread 0x7ffff7c5f640 (LWP 2928290)] Thread 2 "step-over-threa" hit Breakpoint 1, my_exit_syscall () at src/binutils-gdb/gdb/testsuite/lib/my-syscalls.S:68 68 syscall (gdb) c Continuing. [Thread 0x7ffff7c5f640 (LWP 2928290) exited] No unwaited-for children left. (gdb) i th Id Target Id Frame 1 Thread 0x7ffff7c60740 (LWP 2928285) "step-over-threa" 0x00007ffff7f7c9b7 in __pthread_clockjoin_ex () from /usr/lib/libpthread.so.0 The current thread has terminated. See `help thread'. (gdb) thread 1 [Switching to thread 1 (Thread 0x7ffff7c60740 (LWP 2928285))] #0 0x00007ffff7f7c9b7 in __pthread_clockjoin_ex () from /usr/lib/libpthread.so.0 (gdb) c Continuing. ^C^C ... hangs ... The "continue" causes an in-line step to occur, meaning the main thread is stopped while we step the syscall. The stepped thread exits when executing the syscall, the linux-nat target notices there are no more resumed threads to be waited for, so returns TARGET_WAITKIND_NO_RESUMED, which causes the prompt to return. But infrun never clears the in-line step over info. So if we try continuing the main thread, GDB doesn't resume it, because it thinks there's an in-line step in progress that we need to wait for to finish, and we are stuck there. To fix this, infrun needs to be informed when a thread doing a displaced or in-line step over exits. We can do that with the new target_set_thread_options mechanism which is optimal for only enabling exit events of the thread that needs it; or, if that is not supported, by using target_thread_events, which enables thread exit events for all threads. This is done by this commit. This patch then modifies handle_inferior_event in infrun.c to clean up any step-over the exiting thread might have been doing at the time of the exit. The cases to consider are: - the exiting thread was doing an in-line step-over with an all-stop target - the exiting thread was doing an in-line step-over with a non-stop target - the exiting thread was doing a displaced step-over with a non-stop target The displaced-stepping buffer implementation in displaced-stepping.c is modified to account for the fact that it's possible that we "finish" a displaced step after a thread exit event. The buffer that the exiting thread was using is marked as available again and the original instructions under the scratch pad are restored. However, it skips applying the fixup, which wouldn't make sense since the thread does not exist anymore. Another case that needs handling is if a displaced-stepping thread exits, and the event is reported while we are in stop_all_threads. We should call displaced_step_finish in the handle_one function, in that case. It was already called in other code paths, just not the "thread exited" path. This commit doesn't make infrun ask the target to report the TARGET_WAITKIND_THREAD_EXITED events yet, that'll be done later in the series. Note that "stop_print_frame = false;" line is moved to normal_stop, because TARGET_WAITKIND_THREAD_EXITED can also end up with the event transmorphed into TARGET_WAITKIND_NO_RESUMED. Moving it to normal_stop keeps it centralized. Co-authored-by: Pedro Alves Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27338 Change-Id: I745c6955d7ef90beb83bcf0ff1d1ac8b9b6285a5 --- gdb/displaced-stepping.c | 7 ++ gdb/gdbarch-components.py | 4 + gdb/gdbarch-gen.h | 6 +- gdb/infrun.c | 163 +++++++++++++++++++++++++++++++++++--- gdb/thread.c | 2 + 5 files changed, 168 insertions(+), 14 deletions(-) diff --git a/gdb/displaced-stepping.c b/gdb/displaced-stepping.c index 83080cf6bdd..96d4732b7dc 100644 --- a/gdb/displaced-stepping.c +++ b/gdb/displaced-stepping.c @@ -236,6 +236,13 @@ displaced_step_buffers::finish (gdbarch *arch, thread_info *thread, thread->ptid.to_string ().c_str (), paddress (arch, buffer->addr)); + /* If the thread exited while stepping, we are done. The code above + made the buffer available again, and we restored the bytes in the + buffer. We don't want to run the fixup: since the thread is now + dead there's nothing to adjust. */ + if (status.kind () == TARGET_WAITKIND_THREAD_EXITED) + return DISPLACED_STEP_FINISH_STATUS_OK; + regcache *rc = get_thread_regcache (thread); bool instruction_executed_successfully diff --git a/gdb/gdbarch-components.py b/gdb/gdbarch-components.py index 5d391aa7dc0..d4c6c3bc94e 100644 --- a/gdb/gdbarch-components.py +++ b/gdb/gdbarch-components.py @@ -1740,6 +1740,10 @@ Throw an exception if any unexpected error happens. Method( comment=""" Clean up after a displaced step of THREAD. + +It is possible for the displaced-stepped instruction to have caused +the thread to exit. The implementation can detect this case by +checking if WS.kind is TARGET_WAITKIND_THREAD_EXITED. """, type="displaced_step_finish_status", name="displaced_step_finish", diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h index 221dd84008c..b0643616561 100644 --- a/gdb/gdbarch-gen.h +++ b/gdb/gdbarch-gen.h @@ -1030,7 +1030,11 @@ typedef displaced_step_prepare_status (gdbarch_displaced_step_prepare_ftype) (st extern displaced_step_prepare_status gdbarch_displaced_step_prepare (struct gdbarch *gdbarch, thread_info *thread, CORE_ADDR &displaced_pc); extern void set_gdbarch_displaced_step_prepare (struct gdbarch *gdbarch, gdbarch_displaced_step_prepare_ftype *displaced_step_prepare); -/* Clean up after a displaced step of THREAD. */ +/* Clean up after a displaced step of THREAD. + + It is possible for the displaced-stepped instruction to have caused + the thread to exit. The implementation can detect this case by + checking if WS.kind is TARGET_WAITKIND_THREAD_EXITED. */ typedef displaced_step_finish_status (gdbarch_displaced_step_finish_ftype) (struct gdbarch *gdbarch, thread_info *thread, const target_waitstatus &ws); extern displaced_step_finish_status gdbarch_displaced_step_finish (struct gdbarch *gdbarch, thread_info *thread, const target_waitstatus &ws); diff --git a/gdb/infrun.c b/gdb/infrun.c index bc4accc0fd3..a382782e005 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -1820,13 +1820,15 @@ displaced_step_prepare (thread_info *thread) a step-over (either in-line or displaced) finishes. */ static void -update_thread_events_after_step_over (thread_info *event_thread) +update_thread_events_after_step_over (thread_info *event_thread, + const target_waitstatus &event_status) { if (target_supports_set_thread_options (0)) { /* We can control per-thread options. Disable events for the - event thread. */ - event_thread->set_thread_options (0); + event thread, unless the thread is gone. */ + if (event_status.kind () != TARGET_WAITKIND_THREAD_EXITED) + event_thread->set_thread_options (0); } else { @@ -1882,7 +1884,7 @@ displaced_step_finish (thread_info *event_thread, if (!displaced->in_progress ()) return DISPLACED_STEP_FINISH_STATUS_OK; - update_thread_events_after_step_over (event_thread); + update_thread_events_after_step_over (event_thread, event_status); gdb_assert (event_thread->inf->displaced_step_state.in_progress_count > 0); event_thread->inf->displaced_step_state.in_progress_count--; @@ -3994,6 +3996,7 @@ struct wait_one_event }; static bool handle_one (const wait_one_event &event); +static int finish_step_over (struct execution_control_state *ecs); /* Prepare and stabilize the inferior for detaching it. E.g., detaching while a thread is displaced stepping is a recipe for @@ -5091,6 +5094,16 @@ handle_one (const wait_one_event &event) event.ws); save_waitstatus (t, event.ws); t->stop_requested = false; + + if (event.ws.kind () == TARGET_WAITKIND_THREAD_EXITED) + { + if (displaced_step_finish (t, event.ws) + != DISPLACED_STEP_FINISH_STATUS_OK) + { + gdb_assert_not_reached ("displaced_step_finish on " + "exited thread failed"); + } + } } } else @@ -5300,7 +5313,9 @@ stop_all_threads (const char *reason, inferior *inf) } } -/* Handle a TARGET_WAITKIND_NO_RESUMED event. */ +/* Handle a TARGET_WAITKIND_NO_RESUMED event. Return true if we + handled the event and should continue waiting. Return false if we + should stop and report the event to the user. */ static bool handle_no_resumed (struct execution_control_state *ecs) @@ -5428,6 +5443,117 @@ handle_no_resumed (struct execution_control_state *ecs) return false; } +/* Handle a TARGET_WAITKIND_THREAD_EXITED event. Return true if we + handled the event and should continue waiting. Return false if we + should stop and report the event to the user. */ + +static bool +handle_thread_exited (execution_control_state *ecs) +{ + context_switch (ecs); + + /* Clear these so we don't re-start the thread stepping over a + breakpoint/watchpoint. */ + ecs->event_thread->stepping_over_breakpoint = 0; + ecs->event_thread->stepping_over_watchpoint = 0; + + /* Maybe the thread was doing a step-over, if so release + resources and start any further pending step-overs. + + If we are on a non-stop target and the thread was doing an + in-line step, this also restarts the other threads. */ + int ret = finish_step_over (ecs); + + /* finish_step_over returns true if it moves ecs' wait status + back into the thread, so that we go handle another pending + event before this one. But we know it never does that if + the event thread has exited. */ + gdb_assert (ret == 0); + + /* If finish_step_over started a new in-line step-over, don't + try to restart anything else. */ + if (step_over_info_valid_p ()) + { + delete_thread (ecs->event_thread); + return true; + } + + /* Maybe we are on an all-stop target and we got this event + while doing a step-like command on another thread. If so, + go back to doing that. If this thread was stepping, + switch_back_to_stepped_thread will consider that the thread + was interrupted mid-step and will try keep stepping it. We + don't want that, the thread is gone. So clear the proceed + status so it doesn't do that. */ + clear_proceed_status_thread (ecs->event_thread); + if (switch_back_to_stepped_thread (ecs)) + { + delete_thread (ecs->event_thread); + return true; + } + + inferior *inf = ecs->event_thread->inf; + bool slock_applies = schedlock_applies (ecs->event_thread); + + delete_thread (ecs->event_thread); + ecs->event_thread = nullptr; + + auto handle_as_no_resumed = [ecs] () + { + ecs->ws.set_no_resumed (); + ecs->event_thread = nullptr; + ecs->ptid = minus_one_ptid; + return handle_no_resumed (ecs); + }; + + /* If we are on an all-stop target, the target has stopped all + threads to report the event. We don't actually want to + stop, so restart the threads. */ + if (!target_is_non_stop_p ()) + { + if (slock_applies) + { + /* Since the target is !non-stop, then everything is stopped + at this point, and we can't assume we'll get further + events until we resume the target again. Handle this + event like if it were a TARGET_WAITKIND_NO_RESUMED. Note + this refreshes the thread list and checks whether there + are other resumed threads before deciding whether to + print "no-unwaited-for left". This is important because + the user could have done: + + (gdb) set scheduler-locking on + (gdb) thread 1 + (gdb) c& + (gdb) thread 2 + (gdb) c + + ... and only one of the threads exited. */ + return handle_as_no_resumed (); + } + else + { + /* Switch to the first non-exited thread we can find, and + resume. */ + auto range = inf->non_exited_threads (); + if (range.begin () == range.end ()) + { + /* Looks like the target reported a + TARGET_WAITKIND_THREAD_EXITED for its last known + thread. */ + return handle_as_no_resumed (); + } + thread_info *non_exited_thread = *range.begin (); + switch_to_thread (non_exited_thread); + insert_breakpoints (); + resume (GDB_SIGNAL_0); + } + } + + prepare_to_wait (ecs); + return true; +} + /* Given an execution control state that has been freshly filled in by an event from the inferior, figure out what it means and take appropriate action. @@ -5466,12 +5592,6 @@ handle_inferior_event (struct execution_control_state *ecs) return; } - if (ecs->ws.kind () == TARGET_WAITKIND_THREAD_EXITED) - { - prepare_to_wait (ecs); - return; - } - if (ecs->ws.kind () == TARGET_WAITKIND_NO_RESUMED && handle_no_resumed (ecs)) return; @@ -5486,7 +5606,6 @@ handle_inferior_event (struct execution_control_state *ecs) { /* No unwaited-for children left. IOW, all resumed children have exited. */ - stop_print_frame = false; stop_waiting (ecs); return; } @@ -5636,6 +5755,15 @@ handle_inferior_event (struct execution_control_state *ecs) keep_going (ecs); return; + case TARGET_WAITKIND_THREAD_EXITED: + if (handle_thread_exited (ecs)) + return; + /* Need to re-record the last target status because the waitkind + may have changed to TARGET_WAITKIND_NO_RESUMED. */ + set_last_target_status (ecs->target, ecs->ptid, ecs->ws); + stop_waiting (ecs); + break; + case TARGET_WAITKIND_EXITED: case TARGET_WAITKIND_SIGNALLED: { @@ -6085,7 +6213,7 @@ finish_step_over (struct execution_control_state *ecs) back an event. */ gdb_assert (ecs->event_thread->control.trap_expected); - update_thread_events_after_step_over (ecs->event_thread); + update_thread_events_after_step_over (ecs->event_thread, ecs->ws); clear_step_over_info (); } @@ -6131,6 +6259,13 @@ finish_step_over (struct execution_control_state *ecs) if (ecs->event_thread->stepping_over_watchpoint) return 0; + /* The code below is meant to avoid one thread hogging the event + loop by doing constant in-line step overs. If the stepping + thread exited, there's no risk for this to happen, so we can + safely let our caller process the event immediately. */ + if (ecs->ws.kind () == TARGET_WAITKIND_THREAD_EXITED) + return 0; + pending = iterate_over_threads (resumed_thread_with_pending_status, NULL); if (pending != NULL) @@ -8772,6 +8907,8 @@ normal_stop (void) if (last.kind () == TARGET_WAITKIND_NO_RESUMED) { + stop_print_frame = false; + SWITCH_THRU_ALL_UIS () if (current_ui->prompt_state == PROMPT_BLOCKED) { diff --git a/gdb/thread.c b/gdb/thread.c index 6ea05f70a41..a83db6b07fd 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -401,6 +401,8 @@ thread_info::clear_pending_waitstatus () void thread_info::set_thread_options (gdb_thread_options thread_options) { + gdb_assert (this->state != THREAD_EXITED && !this->executing ()); + if (m_thread_options == thread_options) return; -- 2.36.0