Re: [PATCH 20/31] gdb: clear step over information on thread exit (PR gdb/27338)

Andrew Burgess via Gdb-patches
2023-06-08T15:30:02Z

Pedro Alves <pedro@palves.net> writes:

> 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 <Thread ID 2> 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.

LGTM.

Reviewed-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

>
> Co-authored-by: Simon Marchi <simon.marchi@efficios.com>
> 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