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

[Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>]

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