Re: [PATCH 22/31] gdbserver: Queue no-resumed event after thread exit

[Andrew Burgess <aburgess@redhat.com>]

Pedro Alves <pedro@palves.net> writes:

> Normally, if the last thread resumed thread on the target exits, the

"thread resumed thread" .... I think there's an extra 'thread' in there.

Otherwise, LGTM.

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

Thanks,
Andrew

> server sends a no-resumed event to GDB.  If however, GDB enables the
> GDB_THREAD_OPTION_EXIT option on a thread, and, that thread exits, the
> server sends a thread exit event for that thread instead.
>
> In all-stop RSP mode, since events can only be forwarded to GDB one at
> a time, and the whole target stops whenever an event is reported, GDB
> resumes the target again after getting a THREAD_EXITED event, and then
> the server finally reports back a no-resumed event if/when
> appropriate.
>
> For non-stop RSP though, events are asynchronous, and if the server
> sends a thread-exit event for the last resumed thread, the no-resumed
> event is never sent.  This patch makes sure that in non-stop mode, the
> server queues a no-resumed event after the thread-exit event if it was
> the last resumed thread that exited.
>
> Without this, we'd see failures in step-over-thread-exit testcases
> added later in the series, like so:
>
>    continue
>    Continuing.
>  - No unwaited-for children left.
>  - (gdb) PASS: gdb.threads/step-over-thread-exit.exp: displaced-stepping=off: non-stop=on: target-non-stop=on: schedlock=off: ns_stop_all=1: continue stops when thread exits
>  + FAIL: gdb.threads/step-over-thread-exit.exp: displaced-stepping=off: non-stop=on: target-non-stop=on: schedlock=off: ns_stop_all=1: continue stops when thread exits (timeout)
>
> (and other similar ones)
>
> Change-Id: I927d78b30f88236dbd5634b051a716f72420e7c7
> ---
>  gdbserver/linux-low.cc | 47 +++++++++++++++++++++++++-----------------
>  gdbserver/linux-low.h  |  2 ++
>  gdbserver/server.cc    | 12 ++++++++++-
>  gdbserver/target.cc    |  6 ++++++
>  gdbserver/target.h     |  6 ++++++
>  5 files changed, 53 insertions(+), 20 deletions(-)
>
> diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
> index ebc3bf34127..bf6dc1d995a 100644
> --- a/gdbserver/linux-low.cc
> +++ b/gdbserver/linux-low.cc
> @@ -2963,7 +2963,6 @@ linux_process_target::wait_1 (ptid_t ptid, target_waitstatus *ourstatus,
>    int report_to_gdb;
>    int trace_event;
>    int in_step_range;
> -  int any_resumed;
>  
>    threads_debug_printf ("[%s]", target_pid_to_str (ptid).c_str ());
>  
> @@ -2977,23 +2976,7 @@ linux_process_target::wait_1 (ptid_t ptid, target_waitstatus *ourstatus,
>    in_step_range = 0;
>    ourstatus->set_ignore ();
>  
> -  auto status_pending_p_any = [&] (thread_info *thread)
> -    {
> -      return status_pending_p_callback (thread, minus_one_ptid);
> -    };
> -
> -  auto not_stopped = [&] (thread_info *thread)
> -    {
> -      return not_stopped_callback (thread, minus_one_ptid);
> -    };
> -
> -  /* Find a resumed LWP, if any.  */
> -  if (find_thread (status_pending_p_any) != NULL)
> -    any_resumed = 1;
> -  else if (find_thread (not_stopped) != NULL)
> -    any_resumed = 1;
> -  else
> -    any_resumed = 0;
> +  bool was_any_resumed = any_resumed ();
>  
>    if (step_over_bkpt == null_ptid)
>      pid = wait_for_event (ptid, &w, options);
> @@ -3004,7 +2987,7 @@ linux_process_target::wait_1 (ptid_t ptid, target_waitstatus *ourstatus,
>        pid = wait_for_event (step_over_bkpt, &w, options & ~WNOHANG);
>      }
>  
> -  if (pid == 0 || (pid == -1 && !any_resumed))
> +  if (pid == 0 || (pid == -1 && !was_any_resumed))
>      {
>        gdb_assert (target_options & TARGET_WNOHANG);
>  
> @@ -6166,6 +6149,32 @@ linux_process_target::thread_stopped (thread_info *thread)
>    return get_thread_lwp (thread)->stopped;
>  }
>  
> +bool
> +linux_process_target::any_resumed ()
> +{
> +  bool any_resumed;
> +
> +  auto status_pending_p_any = [&] (thread_info *thread)
> +    {
> +      return status_pending_p_callback (thread, minus_one_ptid);
> +    };
> +
> +  auto not_stopped = [&] (thread_info *thread)
> +    {
> +      return not_stopped_callback (thread, minus_one_ptid);
> +    };
> +
> +  /* Find a resumed LWP, if any.  */
> +  if (find_thread (status_pending_p_any) != NULL)
> +    any_resumed = 1;
> +  else if (find_thread (not_stopped) != NULL)
> +    any_resumed = 1;
> +  else
> +    any_resumed = 0;
> +
> +  return any_resumed;
> +}
> +
>  /* This exposes stop-all-threads functionality to other modules.  */
>  
>  void
> diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
> index 1c1754d2b59..33a14e15173 100644
> --- a/gdbserver/linux-low.h
> +++ b/gdbserver/linux-low.h
> @@ -259,6 +259,8 @@ class linux_process_target : public process_stratum_target
>  
>    bool thread_stopped (thread_info *thread) override;
>  
> +  bool any_resumed () override;
> +
>    void pause_all (bool freeze) override;
>  
>    void unpause_all (bool unfreeze) override;
> diff --git a/gdbserver/server.cc b/gdbserver/server.cc
> index 5099db1ee31..77b4b57466d 100644
> --- a/gdbserver/server.cc
> +++ b/gdbserver/server.cc
> @@ -4740,7 +4740,17 @@ handle_target_event (int err, gdb_client_data client_data)
>  	    }
>  	}
>        else
> -	push_stop_notification (cs.last_ptid, cs.last_status);
> +	{
> +	  push_stop_notification (cs.last_ptid, cs.last_status);
> +
> +	  if (cs.last_status.kind () == TARGET_WAITKIND_THREAD_EXITED
> +	      && !target_any_resumed ())
> +	    {
> +	      target_waitstatus ws;
> +	      ws.set_no_resumed ();
> +	      push_stop_notification (null_ptid, ws);
> +	    }
> +	}
>      }
>  
>    /* Be sure to not change the selected thread behind GDB's back.
> diff --git a/gdbserver/target.cc b/gdbserver/target.cc
> index 4584e9b3a8e..833e32a4830 100644
> --- a/gdbserver/target.cc
> +++ b/gdbserver/target.cc
> @@ -612,6 +612,12 @@ process_stratum_target::thread_stopped (thread_info *thread)
>    gdb_assert_not_reached ("target op thread_stopped not supported");
>  }
>  
> +bool
> +process_stratum_target::any_resumed ()
> +{
> +  return true;
> +}
> +
>  bool
>  process_stratum_target::supports_get_tib_address ()
>  {
> diff --git a/gdbserver/target.h b/gdbserver/target.h
> index e2e818b130b..c3345fc67e8 100644
> --- a/gdbserver/target.h
> +++ b/gdbserver/target.h
> @@ -320,6 +320,9 @@ class process_stratum_target
>    /* Return true if THREAD is known to be stopped now.  */
>    virtual bool thread_stopped (thread_info *thread);
>  
> +  /* Return true if any thread is known to be resumed.  */
> +  virtual bool any_resumed ();
> +
>    /* Return true if the get_tib_address op is supported.  */
>    virtual bool supports_get_tib_address ();
>  
> @@ -683,6 +686,9 @@ target_read_btrace_conf (struct btrace_target_info *tinfo,
>  #define target_supports_software_single_step() \
>    the_target->supports_software_single_step ()
>  
> +#define target_any_resumed() \
> +  the_target->any_resumed ()
> +
>  ptid_t mywait (ptid_t ptid, struct target_waitstatus *ourstatus,
>  	       target_wait_flags options, int connected_wait);
>  
> -- 
> 2.36.0