Re: [PATCH 14/31] all-stop/synchronous RSP support thread-exit events

[Andrew Burgess <aburgess@redhat.com>]

Pedro Alves <pedro@palves.net> writes:

> Currently, GDB does not understand the THREAD_EXITED stop reply in
> remote all-stop mode.  There's no good reason for this, it just
> happened that THREAD_EXITED was only ever reported in non-stop mode so
> far.  This patch teaches GDB to parse that event in all-stop RSP too.
> There is no need to add a qSupported feature for this, because the
> server won't send a THREAD_EXITED event unless GDB explicitly asks for
> it, with QThreadEvents, or with the GDB_THREAD_OPTION_EXIT
> QThreadOptions option added in the next patch.
>
> Change-Id: Ide5d12391adf432779fe4c79526801c4a5630966
> ---
>  gdb/remote.c        | 5 +++--
>  gdbserver/server.cc | 1 +
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 9de8ed8a068..f7ab8523fd5 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -8172,7 +8172,8 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply,
>        && status->kind () != TARGET_WAITKIND_NO_RESUMED)
>      {
>        /* Expedited registers.  */
> -      if (!stop_reply->regcache.empty ())
> +      if (status->kind () != TARGET_WAITKIND_THREAD_EXITED
> +	  && !stop_reply->regcache.empty ())

I haven't investigated if this crops up, but, inside the if block we
call xfree to release some of the regcache data.

If it was ever the case the status->kind() was THREAD_EXITED, and the
regcache was not empty, would we then leak memory?

If THREAD_EXITED implies that the regcache is empty could we use an
assert here instead of adding the kind check to the if?  Maybe:

  if (!stop_reply->regcache.empty ())
    {
      gdb_assert (status->kind () != TARGET_WAITKIND_THREAD_EXITED);

      ...

But maybe that's bad because we'd be making assert based on state that
arrives from gdbserver.  In which case, should we perform some action to
ensure that the regcache data is correctly freed?

Thanks,
Andrew

>  	{
>  	  struct regcache *regcache
>  	    = get_thread_arch_regcache (this, ptid, stop_reply->arch);
> @@ -8358,7 +8359,7 @@ remote_target::wait_as (ptid_t ptid, target_waitstatus *status,
>  	     again.  Keep waiting for events.  */
>  	  rs->waiting_for_stop_reply = 1;
>  	  break;
> -	case 'N': case 'T': case 'S': case 'X': case 'W':
> +	case 'N': case 'T': case 'S': case 'X': case 'W': case 'w':
>  	  {
>  	    /* There is a stop reply to handle.  */
>  	    rs->waiting_for_stop_reply = 0;
> diff --git a/gdbserver/server.cc b/gdbserver/server.cc
> index 07a3319d114..5099db1ee31 100644
> --- a/gdbserver/server.cc
> +++ b/gdbserver/server.cc
> @@ -3061,6 +3061,7 @@ resume (struct thread_resume *actions, size_t num_actions)
>  
>        if (cs.last_status.kind () != TARGET_WAITKIND_EXITED
>  	  && cs.last_status.kind () != TARGET_WAITKIND_SIGNALLED
> +	  && cs.last_status.kind () != TARGET_WAITKIND_THREAD_EXITED
>  	  && cs.last_status.kind () != TARGET_WAITKIND_NO_RESUMED)
>  	current_thread->last_status = cs.last_status;
>  
> -- 
> 2.36.0