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

  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(-)
  

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
  

On 2023-06-07 18:52, Andrew Burgess wrote:
> 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?

THREAD_EXITED stop replies can't carry expedited registers, they're not
a kind of T stop reply, they're their own stop reply kind.  They're just:

 @anchor{thread exit event}
 @cindex thread exit event, remote reply
 @item w @var{AA} ; @var{tid}

 The thread exited, and @var{AA} is the exit status.  This response
 should not be sent by default; @value{GDBN} requests it with the
 @ref{QThreadEvents} packet.  See also @ref{thread create event} above.
 @var{AA} is formatted as a big-endian hex string.

But even if a remote server included them somehow, we wouldn't parse them:

    case 'w':		/* Thread exited.  */
      {
	ULONGEST value;

	p = unpack_varlen_hex (&buf[1], &value);
	event->ws.set_thread_exited (value);
	if (*p != ';')
	  error (_("stop reply packet badly formatted: %s"), buf);
	event->ptid = read_ptid (++p, NULL);
	break;
      }

I don't remember exactly why I added the "status->kind () != TARGET_WAITKIND_THREAD_EXITED"
check, I think it was just because I was reading the code and noticed it doesn't make sense
to process expected registers for that event.

> 
> 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);
> 
>       ...
> 

Makes sense, I did this.

> But maybe that's bad because we'd be making assert based on state that
> arrives from gdbserver.  

As explained above, that can't happen, so an assertion is fine.

> In which case, should we perform some action to
> ensure that the regcache data is correctly freed?

I wrote a couple patches to improve ownership around this, using
unique_ptr more.  I will post them after this series (need to write
commit logs, etc.)
  

Hi!

On 2023-11-13 14:11, Pedro Alves wrote:

> On 2023-06-07 18:52, Andrew Burgess wrote:
>> In which case, should we perform some action to
>> ensure that the regcache data is correctly freed?
> 
> I wrote a couple patches to improve ownership around this, using
> unique_ptr more.  I will post them after this series (need to write
> commit logs, etc.)
> 

I finally sent patches for this:

 [PATCH 0/2] Use unique_ptr more in the remote target
 https://sourceware.org/pipermail/gdb-patches/2023-December/205179.html

Pedro Alves