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

["Rohr, Stephan" <stephan.rohr@intel.com>]

> -----Original Message-----
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Sent: Tuesday, 3 December 2024 03:48
> To: Rohr, Stephan <stephan.rohr@intel.com>; gdb-patches@sourceware.org
> Subject: Re: [PATCH] gdbserver/win32-low.cc: remove use of `all_threads`
> 
> 
> 
> 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?

This seems reasonable.  We could also use

process_info *process = find_process_pid (pid);

if (process == NULL)
  return;

if (process->thread_map ().size () == 1)
  return;

> 
> Simon

Stephan
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928