Re: [PATCH] gdbserver/win32-low.cc: remove use of `all_threads`

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2024-12-02 15:48, Rohr, Stephan wrote:
> Hi Simon,
> 
> thanks for fixing this.  I missed to check the windows build when submitting the
> thread map patch for GDBserver.  The fix itself looks ok, but I also cannot judge if
> GDBserver only supports a single process at a time.  We could also use something
> like
> 
>   int num_threads = 0;
>   for_each_process ([&num_threads] (process_info *process)
>     {
>       num_threads += process->thread_map ().size ();
>     });
> 
>   if (num_threads == 0)
>     return;
> 
> to preserve the same behaviour as implemented now.

Even though it looks like preserving the same behaviour in surface, I
don't think it makes sense.  Let's saywe did the work to make GDBserver
for Windows multi-process, what would this check become?  My intuition
is that this would be changed to check the number of threads in the
process the thread being deleted belongs to, not the overall number of
threads.  So adding that loop wouldn't go in the right direction.  I
don't know the exact reason for this check to exist in the first place,
my guess is that we must always have at least one thread in the process
for things not to explode, and if this is indeed the last thread
existing, then an "exit process" event will likely follow.

However, the patch could probably be improved by not using
`current_process ()`.  The caller of this function,
`get_child_debug_event ()`, would likely not have set the current
process to the right one.  So it would perhaps be better to write the
function like this instead:

  static void
  child_delete_thread (DWORD pid, DWORD tid)
  {
    thread_info *thread = find_thread_ptid (ptid_t (pid, tid));
    if (thread == NULL)
      return;

    /* If the last thread is exiting, just return.  */
    if (thread->process ()->thread_map ().size () == 1)
      return;

    delete_thread_info (thread);
  }

Would that make sense?

Simon