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

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

[Simon Marchi]

Fix this:

    /home/simark/src/binutils-gdb/gdbserver/win32-low.cc: In function ‘void child_delete_thread(DWORD, DWORD)’:
    /home/simark/src/binutils-gdb/gdbserver/win32-low.cc:192:7: error: ‘all_threads’ was not declared in this scope; did you mean ‘using_threads’?
      192 |   if (all_threads.size () == 1)
          |       ^~~~~~~~~~~
          |       using_threads

Commit 9f77b3aa0bfc ("gdbserver: change 'all_processes' and
'all_threads' list type") changed the type of `all_thread` to an
intrusive_list, without changing this particular use, which broke the
build because an intrusive_list doesn't know its size (we, could,
Boost's version has an option for that, but we don't need it at the
moment).  The subsequent commit removed `all_threads`, leading to the
error above.

Fix it by using the number of threads of the current process instead.
My rationale: as far as I know, GDBserver on Windows only supports one
process at a time, so there's no need to iterate over all processes.  It
would be nice if someone familiar with the Windows port could confirm
(and test) this.

Change-Id: I84d6226532b887d99248cf3be90f5065fb7a074a
---
 gdbserver/win32-low.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc
index 0174a32e5d19..ef00f3452f31 100644
--- a/gdbserver/win32-low.cc
+++ b/gdbserver/win32-low.cc
@@ -189,7 +189,7 @@ static void
 child_delete_thread (DWORD pid, DWORD tid)
 {
   /* If the last thread is exiting, just return.  */
-  if (all_threads.size () == 1)
+  if (current_process ()->thread_map ().size () == 1)
     return;
 
   thread_info *thread = find_thread_ptid (ptid_t (pid, tid));

base-commit: 3eccfdce99e030be759ea20c66d6f78978b11a25
-- 
2.47.1

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

[Rohr, Stephan]

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.

Stephan

> -----Original Message-----
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Sent: Monday, 2 December 2024 20:47
> To: gdb-patches@sourceware.org
> Cc: Simon Marchi <simon.marchi@polymtl.ca>
> Subject: [PATCH] gdbserver/win32-low.cc: remove use of `all_threads`
> 
> Fix this:
> 
>     /home/simark/src/binutils-gdb/gdbserver/win32-low.cc: In function ‘void
> child_delete_thread(DWORD, DWORD)’:
>     /home/simark/src/binutils-gdb/gdbserver/win32-low.cc:192:7: error:
> ‘all_threads’ was not declared in this scope; did you mean ‘using_threads’?
>       192 |   if (all_threads.size () == 1)
>           |       ^~~~~~~~~~~
>           |       using_threads
> 
> Commit 9f77b3aa0bfc ("gdbserver: change 'all_processes' and
> 'all_threads' list type") changed the type of `all_thread` to an
> intrusive_list, without changing this particular use, which broke the
> build because an intrusive_list doesn't know its size (we, could,
> Boost's version has an option for that, but we don't need it at the
> moment).  The subsequent commit removed `all_threads`, leading to the
> error above.
> 
> Fix it by using the number of threads of the current process instead.
> My rationale: as far as I know, GDBserver on Windows only supports one
> process at a time, so there's no need to iterate over all processes.  It
> would be nice if someone familiar with the Windows port could confirm
> (and test) this.
> 
> Change-Id: I84d6226532b887d99248cf3be90f5065fb7a074a
> ---
>  gdbserver/win32-low.cc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc
> index 0174a32e5d19..ef00f3452f31 100644
> --- a/gdbserver/win32-low.cc
> +++ b/gdbserver/win32-low.cc
> @@ -189,7 +189,7 @@ static void
>  child_delete_thread (DWORD pid, DWORD tid)
>  {
>    /* If the last thread is exiting, just return.  */
> -  if (all_threads.size () == 1)
> +  if (current_process ()->thread_map ().size () == 1)
>      return;
> 
>    thread_info *thread = find_thread_ptid (ptid_t (pid, tid));
> 
> base-commit: 3eccfdce99e030be759ea20c66d6f78978b11a25
> --
> 2.47.1

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

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

[Simon Marchi]

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

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

[Rohr, Stephan]

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

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

[Simon Marchi]

On 12/3/24 4:03 AM, Rohr, Stephan wrote:
> 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;

Yeah, something like this seems even more future proof.  I'll send a v2
that looks like that, thanks.

Simon

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

[Hannes Domani]

 Am Montag, 2. Dezember 2024 um 20:50:34 MEZ hat Simon Marchi <simon.marchi@polymtl.ca> Folgendes geschrieben:

> Fix this:
>
>     /home/simark/src/binutils-gdb/gdbserver/win32-low.cc: In function ‘void child_delete_thread(DWORD, DWORD)’:
>     /home/simark/src/binutils-gdb/gdbserver/win32-low.cc:192:7: error: ‘all_threads’ was not declared in this scope; did you mean ‘using_threads’?
>       192 |  if (all_threads.size () == 1)
>           |      ^~~~~~~~~~~
>           |      using_threads
>
> Commit 9f77b3aa0bfc ("gdbserver: change 'all_processes' and
> 'all_threads' list type") changed the type of `all_thread` to an
> intrusive_list, without changing this particular use, which broke the
> build because an intrusive_list doesn't know its size (we, could,
> Boost's version has an option for that, but we don't need it at the
> moment).  The subsequent commit removed `all_threads`, leading to the
> error above.
>
> Fix it by using the number of threads of the current process instead.
> My rationale: as far as I know, GDBserver on Windows only supports one
> process at a time, so there's no need to iterate over all processes.  It
> would be nice if someone familiar with the Windows port could confirm
> (and test) this.

I tested it now, and there seems to be no problem.

But this if() actually never triggered for me, I never got an
EXIT_THREAD_DEBUG_EVENT for the final thread, only an
EXIT_PROCESS_DEBUG_EVENT.
I tried it out on both Win7 and Win11, but maybe there is some special
constellation where this can happen, or it was different on earlier
Windows versions.

Tested-By: Hannes Domani <ssbssa@yahoo.de>


Hannes


> Change-Id: I84d6226532b887d99248cf3be90f5065fb7a074a
> ---
> gdbserver/win32-low.cc | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc
> index 0174a32e5d19..ef00f3452f31 100644
> --- a/gdbserver/win32-low.cc
> +++ b/gdbserver/win32-low.cc
> @@ -189,7 +189,7 @@ static void
> child_delete_thread (DWORD pid, DWORD tid)
> {
>   /* If the last thread is exiting, just return.  */
> -  if (all_threads.size () == 1)
> +  if (current_process ()->thread_map ().size () == 1)
>     return;
>
>   thread_info *thread = find_thread_ptid (ptid_t (pid, tid));
>
> base-commit: 3eccfdce99e030be759ea20c66d6f78978b11a25
> --
> 2.47.1

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

[Simon Marchi]

On 12/3/24 9:34 AM, Hannes Domani wrote:
>  Am Montag, 2. Dezember 2024 um 20:50:34 MEZ hat Simon Marchi <simon.marchi@polymtl.ca> Folgendes geschrieben:
> 
>> Fix this:
>>
>>      /home/simark/src/binutils-gdb/gdbserver/win32-low.cc: In function ‘void child_delete_thread(DWORD, DWORD)’:
>>      /home/simark/src/binutils-gdb/gdbserver/win32-low.cc:192:7: error: ‘all_threads’ was not declared in this scope; did you mean ‘using_threads’?
>>        192 |  if (all_threads.size () == 1)
>>            |      ^~~~~~~~~~~
>>            |      using_threads
>>
>> Commit 9f77b3aa0bfc ("gdbserver: change 'all_processes' and
>> 'all_threads' list type") changed the type of `all_thread` to an
>> intrusive_list, without changing this particular use, which broke the
>> build because an intrusive_list doesn't know its size (we, could,
>> Boost's version has an option for that, but we don't need it at the
>> moment).  The subsequent commit removed `all_threads`, leading to the
>> error above.
>>
>> Fix it by using the number of threads of the current process instead.
>> My rationale: as far as I know, GDBserver on Windows only supports one
>> process at a time, so there's no need to iterate over all processes.  It
>> would be nice if someone familiar with the Windows port could confirm
>> (and test) this.
> 
> I tested it now, and there seems to be no problem.
> 
> But this if() actually never triggered for me, I never got an
> EXIT_THREAD_DEBUG_EVENT for the final thread, only an
> EXIT_PROCESS_DEBUG_EVENT.
> I tried it out on both Win7 and Win11, but maybe there is some special
> constellation where this can happen, or it was different on earlier
> Windows versions.

Damn, I hoped you would know.  The comment is somewhat useless, it just
literally explains what the code does, much like:

  /* Add 1 and 2 together and assign to x.  */
  x = 1 + 2;

> Tested-By: Hannes Domani <ssbssa@yahoo.de>

Thanks for testing, bug I will send a v2 implementing Stephan's
suggestion.  It's a minor change, but better be safe than sorry.

Simon