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

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

>> 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" ?

Yes, fixed.

>> @@ -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 ?

Sounds good, changed to:

  /* Node for list of threads that are resumed and have a pending wait status.

     The list head for this is in process_stratum_target, hence all threads in
     this list belong to that process target.  */
  intrusive_list_node<thread_info> resumed_with_pending_wait_status_node;

>> @@ -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"

Done.

>> +     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.

Arggg, we don't want that.  Since that is in a process_stratum_target
method, I'll change the message to call the pid_to_str method of the
current object instead:

      infrun_debug_printf ("removing from resumed threads with event list: %s",
			   this->pid_to_str (thread->ptid).c_str ());

Does that sound good?

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

I'd make the same change to use this->pid_to_str here too.

> 
>> +      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.

Done:

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

     This is done for optimization reasons, it would be possible to walk the
     inferior thread lists to find these threads.  But since this is something
     we need to do quite frequently in the hot path, maintaining this list
     avoids walking the thread lists repeatedly.  */

I would prefer to avoid repeating the same thing at two places, because
it's a recipe for the two places getting out of sync.  Since the node
comment in thread_info now points to here (says the list head is in
process_stratum_target), people looking for more information about this
list should have no problem finding the comment here.

> 
>> +  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.

Done:

      /* Some targets unpush themselves from the inferior's target stack before
         clearing the inferior's thread list (which marks all threads as exited,
         and therefore leads to this function).  In this case, the inferior's
         process target will be nullptr when we arrive here.

         See also the comment in inferior::unpush_target.  */

And I also added a cross-reference to here from the comment in
inferior::unpush_target.

>> +  /* 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...  ;-)

Indeed!  If you have a name that is shorter but just as clear, I'm open
for suggestion.  But I prefer names that are non-ambiguous and use the
right terminology over names that are short just for convenience's sake.

Thanks!

Simon