Re: [PATCH 17/31] Introduce GDB_THREAD_OPTION_EXIT thread option, fix step-over-thread-exit

Andrew Burgess via Gdb-patches
2023-06-08T13:18:54Z

Pedro Alves <pedro@palves.net> writes:

> When stepping over a breakpoint with displaced stepping, GDB needs to
> be informed if the stepped thread exits, otherwise the displaced
> stepping buffer that was allocated to that thread leaks, and this can
> result in deadlock, with other threads waiting for their turn to
> displaced step, but their turn never comes.
>
> Similarly, when stepping over a breakpoint in line, GDB also needs to
> be informed if the stepped thread exits, so that is can clear the step
> over state and re-resume threads.
>
> This commit makes it possible for GDB to ask the target to report
> thread exit events for a given thread, using the new "thread options"
> mechanism introduced by a previous patch.
>
> This only adds the core bits. Following patches in the series will
> teach the Linux backends (native & gdbserver) to handle the
> GDB_THREAD_OPTION_EXIT option, and then a later patch will make use of
> these thread exit events to clean up displaced stepping and inline
> stepping state properly.

LGTM.

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

Thanks,
Andrew

>
> Change-Id: I96b719fdf7fee94709e98bb3a90751d8134f3a38
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27338
> ---
> gdb/infrun.c | 15 ++++++++++-----
> gdb/remote.c | 9 +++++++++
> gdb/target/target.c | 1 +
> gdb/target/target.h | 4 ++++
> 4 files changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 6fdffb31884..e47e3c688e7 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -2445,24 +2445,29 @@ do_target_resume (ptid_t resume_ptid, bool step, enum gdb_signal sig)
> else
> target_pass_signals (signal_pass);
>
> - /* Request that the target report thread-{created,cloned} events in
> - the following situations:
> + /* Request that the target report thread-{created,cloned,exited}
> + events in the following situations:
>
> - If we are performing an in-line step-over-breakpoint, then we
> will remove a breakpoint from the target and only run the
> current thread. We don't want any new thread (spawned by the
> - step) to start running, as it might miss the breakpoint.
> + step) to start running, as it might miss the breakpoint. We
> + need to clear the step-over state if the stepped thread exits,
> + so we also enable thread-exit events.
>
> - If we are stepping over a breakpoint out of line (displaced
> stepping) then we won't remove a breakpoint from the target,
> but, if the step spawns a new clone thread, then we will need
> to fixup the $pc address in the clone child too, so we need it
> - to start stopped.
> + to start stopped. We need to release the displaced stepping
> + buffer if the stepped thread exits, so we also enable
> + thread-exit events.
> */
> if (step_over_info_valid_p ()
> || displaced_step_in_progress_thread (tp))
> {
> - gdb_thread_options options = GDB_THREAD_OPTION_CLONE;
> + gdb_thread_options options
> + = GDB_THREAD_OPTION_CLONE | GDB_THREAD_OPTION_EXIT;
> if (target_supports_set_thread_options (options))
> tp->set_thread_options (options);
> else
> diff --git a/gdb/remote.c b/gdb/remote.c
> index f7ab8523fd5..0d2b7c09a07 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -3998,6 +3998,15 @@ remote_target::update_thread_list ()
> if (has_single_non_exited_thread (tp->inf))
> continue;
>
> + /* Do not remove the thread if we've requested to be
> + notified of its exit. For example, the thread may be
> + displaced stepping, infrun will need to handle the
> + exit event, and displaced stepping info is recorded
> + in the thread object. If we deleted the thread now,
> + we'd lose that info. */
> + if ((tp->thread_options () & GDB_THREAD_OPTION_EXIT) != 0)
> + continue;
> +
> /* Not found. */
> delete_thread (tp);
> }
> diff --git a/gdb/target/target.c b/gdb/target/target.c
> index 453167a2ad4..2dc033e63b7 100644
> --- a/gdb/target/target.c
> +++ b/gdb/target/target.c
> @@ -196,6 +196,7 @@ to_string (gdb_thread_options options)
> {
> static constexpr gdb_thread_options::string_mapping mapping[] = {
> MAP_ENUM_FLAG (GDB_THREAD_OPTION_CLONE),
> + MAP_ENUM_FLAG (GDB_THREAD_OPTION_EXIT),
> };
> return options.to_string (mapping);
> }
> diff --git a/gdb/target/target.h b/gdb/target/target.h
> index 139e371e8d8..4e8839c5667 100644
> --- a/gdb/target/target.h
> +++ b/gdb/target/target.h
> @@ -34,6 +34,10 @@ enum gdb_thread_option : unsigned
> /* Tell the target to report TARGET_WAITKIND_THREAD_CLONED events
> for the thread. */
> GDB_THREAD_OPTION_CLONE = 1 << 0,
> +
> + /* Tell the target to report TARGET_WAITKIND_THREAD_EXIT events for
> + the thread. */
> + GDB_THREAD_OPTION_EXIT = 1 << 1,
> };
>
> DEF_ENUM_FLAGS_TYPE (enum gdb_thread_option, gdb_thread_options);
> --
> 2.36.0