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

[Pedro Alves <pedro@palves.net>]

On 2021-07-06 10:25 p.m., Simon Marchi wrote:

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

It does not.  That would mean a target higher on the stack wouldn't have a chance
at printing the thread.

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

Looks good.

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

Great.

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

Sure, as a preference, though that shouldn't be a too-strict rule IMO, otherwise
with very long function names we can end up with awkward looking code as soon as we
need to indent a caller a couple levels.  In this case, luckily that didn't happen,
so I'm not really objecting.

"add_resumed_pending_status" or "add_resumed_pending_ws" would work as well for
me, for example.