Re: [PATCH 07/11] gdb: maintain per-process-target list of resumed threads with pending wait status

[Pedro Alves <pedro@palves.net>]

Hi!

This LGTM.  Some comments, typos and minor things to address below.

On 2021-06-22 5:57 p.m., Simon Marchi via Gdb-patches wrote:

> In set_thread_exited, we try to remove the thread from the list, because
> keeping an exited thread in that list would make no sense (especially if
> the thread is freed).  My first implementation assumed that a process
> stratum target was always present when set_thread_exited is called.
> That's however, not the case: in some cases, targets unpush themselves
> from an inferior and then call "exit_inferior", which exits all the
> threads.  If the target is unpushed before set_thread_exited is called
> on the threads, it means we could mistakenly leave some threads in the
> list.  I tried to see how hard it would be to make it such that targets
> have to exit all threads before unpushing themselves from the inferior
> (that would seem logical to me, we don't want threads belonging to an
> inferior that has no process target).  That seem quite difficult and not
> worth the time.  Instead, I changed inferior::unpush_target to remove an
> threads of that inferior from the list.

"remove an threads" -> "remove all threads" ?

> 
> diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
> index 5ea08a13ee5f..47d7f40eaa08 100644
> --- a/gdb/gdbthread.h
> +++ b/gdb/gdbthread.h
> @@ -296,8 +296,7 @@ class thread_info : public refcounted_object,
>    bool resumed () const
>    { return m_resumed; }
>  
> -  void set_resumed (bool resumed)
> -  { m_resumed = resumed; }
> +  void set_resumed (bool resumed);
>  
>    /* Frontend view of the thread state.  Note that the THREAD_RUNNING/
>       THREAD_STOPPED states are different from EXECUTING.  When the
> @@ -470,6 +469,10 @@ class thread_info : public refcounted_object,
>       linked.  */
>    intrusive_list_node<thread_info> step_over_list_node;
>  
> +  /* Node for list of threads that are resumed and have a pending wait
> +     status.  */

Maybe mention that all threads in list list belong to the same
process_stratum_target ?

> +  intrusive_list_node<thread_info> resumed_with_pending_wait_status_node;
> +


> --- a/gdb/inferior.c
> +++ b/gdb/inferior.c
> @@ -89,6 +89,25 @@ inferior::inferior (int pid_)
>    m_target_stack.push (get_dummy_target ());
>  }
>  
> +/* See inferior.h.  */
> +
> +int
> +inferior::unpush_target (struct target_ops *t)
> +{
> +  /* If unpushing the process stratum target while threads exists, ensure that

"threads exists" -> "threads exist"

> +     we don't leave any threads of this inferior in the target's "resumed with
> +     pending wait status" list.  */
> +  if (t->stratum () == process_stratum)
> +    {
> +      process_stratum_target *proc_target = as_process_stratum_target (t);
> +
> +      for (thread_info *thread : this->non_exited_threads ())
> +	proc_target->maybe_remove_resumed_with_pending_wait_status (thread);

Note the target_pid_to_str call inside maybe_remove_resumed_with_pending_wait_status
adds back a dependency on current_inferior.

> +    }
> +
> +  return m_target_stack.unpush (t);
> +}
> +


> --- a/gdb/process-stratum-target.c
> +++ b/gdb/process-stratum-target.c
> @@ -106,6 +106,40 @@ process_stratum_target::follow_exec (inferior *follow_inf, ptid_t ptid,
>  
>  /* See process-stratum-target.h.  */
>  
> +void
> +process_stratum_target::maybe_add_resumed_with_pending_wait_status
> +  (thread_info *thread)
> +{
> +  gdb_assert (!thread->resumed_with_pending_wait_status_node.is_linked ());
> +
> +  if (thread->resumed () && thread->has_pending_waitstatus ())
> +    {
> +      infrun_debug_printf ("adding to resumed threads with event list: %s",
> +			   target_pid_to_str (thread->ptid).c_str ());

This here too.  Not 100% sure this target call is always done
with the right target stack selected.

> +      m_resumed_with_pending_wait_status.push_back (*thread);
> +    }
> +}
> +
> +/* See process-stratum-target.h.  */
> +

> +
> +private:
> +  /* List of threads managed by this target which simultaneously are resumed
> +     and have a pending wait status.  */

I'd suggest expanding this comment a little to mention this
is done for optimization reasons, to avoid walking
thread lists, something like that.  Or maybe say that in
the thread_info node.  Or both places.

> +  thread_info_resumed_with_pending_wait_status_list
> +    m_resumed_with_pending_wait_status;
>  };
>  
>  /* Downcast TARGET to process_stratum_target.  */
> diff --git a/gdb/thread.c b/gdb/thread.c
> index 289d33c74c3b..26974e1b8cbc 100644
> --- a/gdb/thread.c
> +++ b/gdb/thread.c
> @@ -188,6 +188,10 @@ set_thread_exited (thread_info *tp, bool silent)
>  
>    if (tp->state != THREAD_EXITED)
>      {
> +      process_stratum_target *proc_target = tp->inf->process_target ();
> +      if (proc_target != nullptr)

I think this check needs a comment.

> +	proc_target->maybe_remove_resumed_with_pending_wait_status (tp);
> +
>        gdb::observers::thread_exit.notify (tp, silent);
>  
>        /* Tag it as exited.  */
> @@ -295,6 +299,29 @@ thread_info::deletable () const
>  
>  /* See gdbthread.h.  */
>  
> +void
> +thread_info::set_resumed (bool resumed)
> +{
> +  if (resumed == m_resumed)
> +    return;
> +
> +  process_stratum_target *proc_target = this->inf->process_target ();
> +
> +  /* If we transition from resumed to not resumed, we might need to remove
> +     the thread from the resumed threads with pending statuses list.  */
> +  if (!resumed)
> +    proc_target->maybe_remove_resumed_with_pending_wait_status (this);
> +
> +  m_resumed = resumed;
> +
> +  /* If we transition from not resumed to resumed, we might need to add
> +     the thread to the resumed threads with pending statuses list.  */
> +  if (resumed)
> +    proc_target->maybe_add_resumed_with_pending_wait_status (this);

Longest function name award goes to...  ;-)