From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f44.google.com (mail-wr1-f44.google.com [209.85.221.44]) by sourceware.org (Postfix) with ESMTPS id 20958384E7BC for ; Mon, 12 Dec 2022 20:31:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 20958384E7BC 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-f44.google.com with SMTP id bx10so13478346wrb.0 for ; Mon, 12 Dec 2022 12:31:26 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; 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=ZBuH7/rXjMy/kJa4mDbLKZZzHW7OnQciYIB8XLKP5bA=; b=5X7DeLo8HT0fh3WX82v11y9Cb6ri59cnYHpXKS8mWjvIjFvKC67+t6YHBwBhtt9M1e 7T1VFZPOSoq6Gilp3XHUYMxDRO21NrV1cH5pPptl0WOi/1YJNGjJprBi/0NckhIiG4iY Wjlo7szIsrUlphi6sJqW/Ue86GlKxXvf5QXtcKapCeqd3dnxv8pSAVXC7KoCA4AMY4og vzkKvqOgBMN0ru5N1kI+ZpI4W+SJeTd47hB9+tdFUixwN6qCirPk59EIThkV9kekAE6z kC5GnvsSUkswgQgm1/pNuNRqcdlWo22rVS9kkzb4OgpwjgHzvpmqjpjrdsjHrhsgWuDG 15nQ== X-Gm-Message-State: ANoB5plvQM5wiSAWyMFZxqSsqdrNXYztB4SmeuFGYJ4fdtAK+1bFoIcR M8Z8tlaqlkAwLXr0HzAXPfu+ELD+4V1Ekg== X-Google-Smtp-Source: AA0mqf79Fs6jufcLqMTgWU/oOSDoUArA2aVQwKanvytM96pl2Li1JEKOzT6QaRaLDZkPGESpdt/3ow== X-Received: by 2002:a05:6000:98c:b0:24e:b177:ba78 with SMTP id by12-20020a056000098c00b0024eb177ba78mr4900887wrb.51.1670877085423; Mon, 12 Dec 2022 12:31:25 -0800 (PST) Received: from localhost ([2001:8a0:f912:6700:afd9:8b6d:223f:6170]) by smtp.gmail.com with ESMTPSA id l11-20020a5d526b000000b002422202fa7fsm9678579wrc.39.2022.12.12.12.31.24 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 12 Dec 2022 12:31:25 -0800 (PST) From: Pedro Alves To: gdb-patches@sourceware.org Cc: Simon Marchi Subject: [PATCH 20/31] gdb: clear step over information on thread exit (PR gdb/27338) Date: Mon, 12 Dec 2022 20:30:50 +0000 Message-Id: <20221212203101.1034916-21-pedro@palves.net> X-Mailer: git-send-email 2.36.0 In-Reply-To: <20221212203101.1034916-1-pedro@palves.net> References: <20221212203101.1034916-1-pedro@palves.net> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-10.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,SPF_HELO_NONE,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 List-Id: 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: Simon Marchi 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 | 171 ++++++++++++++++++++++++++++++++++---- gdb/thread.c | 3 + 5 files changed, 174 insertions(+), 17 deletions(-) diff --git a/gdb/displaced-stepping.c b/gdb/displaced-stepping.c index 7b5d327008d..aa8571d51e2 100644 --- a/gdb/displaced-stepping.c +++ b/gdb/displaced-stepping.c @@ -254,6 +254,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 5d60f7677f0..875c784dc0f 100644 --- a/gdb/gdbarch-components.py +++ b/gdb/gdbarch-components.py @@ -1826,6 +1826,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 5c9390ea6b3..69212216f03 100644 --- a/gdb/gdbarch-gen.h +++ b/gdb/gdbarch-gen.h @@ -1078,7 +1078,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 e47e3c688e7..2866962d2dc 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -1888,13 +1888,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 { @@ -1950,7 +1952,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--; @@ -4054,6 +4056,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 @@ -5181,6 +5184,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 @@ -5392,7 +5405,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) @@ -5520,6 +5535,125 @@ 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; + + /* Continue handling the event as if we had gotten a + TARGET_WAITKIND_NO_RESUMED. */ + auto handle_as_no_resumed = [ecs] () + { + /* handle_no_resumed doesn't really look at the event kind, but + normal_stop does. */ + ecs->ws.set_no_resumed (); + ecs->event_thread = nullptr; + ecs->ptid = minus_one_ptid; + + /* Re-record the last target status. */ + set_last_target_status (ecs->target, ecs->ptid, ecs->ws); + + 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. @@ -5558,15 +5692,6 @@ handle_inferior_event (struct execution_control_state *ecs) return; } - if (ecs->ws.kind () == TARGET_WAITKIND_THREAD_EXITED) - { - ecs->event_thread = find_thread_ptid (ecs->target, ecs->ptid); - gdb_assert (ecs->event_thread != nullptr); - delete_thread (ecs->event_thread); - prepare_to_wait (ecs); - return; - } - if (ecs->ws.kind () == TARGET_WAITKIND_NO_RESUMED && handle_no_resumed (ecs)) return; @@ -5581,7 +5706,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; } @@ -5730,6 +5854,12 @@ handle_inferior_event (struct execution_control_state *ecs) keep_going (ecs); return; + case TARGET_WAITKIND_THREAD_EXITED: + if (handle_thread_exited (ecs)) + return; + stop_waiting (ecs); + break; + case TARGET_WAITKIND_EXITED: case TARGET_WAITKIND_SIGNALLED: { @@ -6175,7 +6305,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 (); } @@ -6221,6 +6351,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, nullptr); if (pending != nullptr) @@ -8859,6 +8996,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 d607ad9303a..2c45d528bba 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -401,6 +401,9 @@ thread_info::clear_pending_waitstatus () void thread_info::set_thread_options (gdb_thread_options thread_options) { + gdb_assert (this->state != THREAD_EXITED); + gdb_assert (!this->executing ()); + if (m_thread_options == thread_options) return; -- 2.36.0