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

  From: Simon Marchi <simon.marchi@polymtl.ca>

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 concerned 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.  If
we made GDBserver for Windows support multiple processes, my intuition
is that we'd want this check to use the number of threads of the
concerned process, not the number of threads overall.

Add the method `process_info::thread_count`, to get the number of
threads of the process.

I'm not really sure what this check is for in the first place, Hannes
Domani said that this check didn't seem to trigger on Windows 7 and 11.
Perhaps it was necessary before.

Change-Id: I84d6226532b887d99248cf3be90f5065fb7a074a
Tested-By: Hannes Domani <ssbssa@yahoo.de>
---
 gdbserver/inferiors.h  |  4 ++++
 gdbserver/win32-low.cc | 11 ++++++++---
 2 files changed, 12 insertions(+), 3 deletions(-)
  

Hi Simon,

please see my comments below.

> -----Original Message-----
> From: simark@simark.ca <simark@simark.ca>
> Sent: Wednesday, 4 December 2024 17:35
> To: gdb-patches@sourceware.org
> Cc: Simon Marchi <simon.marchi@polymtl.ca>; Hannes Domani
> <ssbssa@yahoo.de>
> Subject: [PATCH 2/3] gdbserver/win32-low.cc: remove use of `all_threads`
> 
> From: Simon Marchi <simon.marchi@polymtl.ca>
> 
> 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
> 

Could you shorten that error message, e.g., we don't need the full path.

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

I'd suggest to remove the comment on boost as it's not needed at the moment.

> Fix it by using the number of threads of the concerned 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.  If
> we made GDBserver for Windows support multiple processes, my intuition
> is that we'd want this check to use the number of threads of the
> concerned process, not the number of threads overall.
> 
> Add the method `process_info::thread_count`, to get the number of
> threads of the process.
> 
> I'm not really sure what this check is for in the first place, Hannes
> Domani said that this check didn't seem to trigger on Windows 7 and 11.
> Perhaps it was necessary before.
> 
> Change-Id: I84d6226532b887d99248cf3be90f5065fb7a074a
> Tested-By: Hannes Domani <ssbssa@yahoo.de>
> ---
>  gdbserver/inferiors.h  |  4 ++++
>  gdbserver/win32-low.cc | 11 ++++++++---
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/gdbserver/inferiors.h b/gdbserver/inferiors.h
> index 728b043f3944..a197654e8556 100644
> --- a/gdbserver/inferiors.h
> +++ b/gdbserver/inferiors.h
> @@ -97,6 +97,10 @@ struct process_info : public
> intrusive_list_node<process_info>
>    std::unordered_map<ptid_t, thread_info *> &thread_map ()
>    { return m_ptid_thread_map; }
> 
> +  /* Return the number of threads in this process.  */
> +  unsigned int thread_count () const
> +  { return m_ptid_thread_map.size (); }
> +
>    /* Return the thread with ptid PTID, or nullptr if no such thread is
>       found.  */
>    thread_info *find_thread (ptid_t ptid);
> diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc
> index 0174a32e5d19..e36bcc4f44a9 100644
> --- a/gdbserver/win32-low.cc
> +++ b/gdbserver/win32-low.cc
> @@ -188,12 +188,17 @@ delete_thread_info (thread_info *thread)
>  static void
>  child_delete_thread (DWORD pid, DWORD tid)
>  {
> +  process_info *process = find_process_pid (pid);
> +
> +  if (process == nullptr)
> +    return;
> +
>    /* If the last thread is exiting, just return.  */
> -  if (all_threads.size () == 1)
> +  if (process->thread_count () == 1)
>      return;
> 
> -  thread_info *thread = find_thread_ptid (ptid_t (pid, tid));
> -  if (thread == NULL)
> +  thread_info *thread = process.find_thread (ptid_t (pid, tid));

I think there is an error here, shouldn't this be

  process->find_thread (ptid_t (pid, tid))

instead?

> +  if (thread == nullptr)
>      return;
> 
>    delete_thread_info (thread);
> --
> 2.47.1

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
  

On 2024-12-04 14:31, Rohr, Stephan wrote:
> Hi Simon,
> 
> please see my comments below.
> 
>> -----Original Message-----
>> From: simark@simark.ca <simark@simark.ca>
>> Sent: Wednesday, 4 December 2024 17:35
>> To: gdb-patches@sourceware.org
>> Cc: Simon Marchi <simon.marchi@polymtl.ca>; Hannes Domani
>> <ssbssa@yahoo.de>
>> Subject: [PATCH 2/3] gdbserver/win32-low.cc: remove use of `all_threads`
>>
>> From: Simon Marchi <simon.marchi@polymtl.ca>
>>
>> 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
>>
> 
> Could you shorten that error message, e.g., we don't need the full path.

I will, but I don't think that having that path really hinders
readability.

>> 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.
>>
> 
> I'd suggest to remove the comment on boost as it's not needed at the moment.

Ack.

>> Fix it by using the number of threads of the concerned 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.  If
>> we made GDBserver for Windows support multiple processes, my intuition
>> is that we'd want this check to use the number of threads of the
>> concerned process, not the number of threads overall.
>>
>> Add the method `process_info::thread_count`, to get the number of
>> threads of the process.
>>
>> I'm not really sure what this check is for in the first place, Hannes
>> Domani said that this check didn't seem to trigger on Windows 7 and 11.
>> Perhaps it was necessary before.
>>
>> Change-Id: I84d6226532b887d99248cf3be90f5065fb7a074a
>> Tested-By: Hannes Domani <ssbssa@yahoo.de>
>> ---
>>  gdbserver/inferiors.h  |  4 ++++
>>  gdbserver/win32-low.cc | 11 ++++++++---
>>  2 files changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/gdbserver/inferiors.h b/gdbserver/inferiors.h
>> index 728b043f3944..a197654e8556 100644
>> --- a/gdbserver/inferiors.h
>> +++ b/gdbserver/inferiors.h
>> @@ -97,6 +97,10 @@ struct process_info : public
>> intrusive_list_node<process_info>
>>    std::unordered_map<ptid_t, thread_info *> &thread_map ()
>>    { return m_ptid_thread_map; }
>>
>> +  /* Return the number of threads in this process.  */
>> +  unsigned int thread_count () const
>> +  { return m_ptid_thread_map.size (); }
>> +
>>    /* Return the thread with ptid PTID, or nullptr if no such thread is
>>       found.  */
>>    thread_info *find_thread (ptid_t ptid);
>> diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc
>> index 0174a32e5d19..e36bcc4f44a9 100644
>> --- a/gdbserver/win32-low.cc
>> +++ b/gdbserver/win32-low.cc
>> @@ -188,12 +188,17 @@ delete_thread_info (thread_info *thread)
>>  static void
>>  child_delete_thread (DWORD pid, DWORD tid)
>>  {
>> +  process_info *process = find_process_pid (pid);
>> +
>> +  if (process == nullptr)
>> +    return;
>> +
>>    /* If the last thread is exiting, just return.  */
>> -  if (all_threads.size () == 1)
>> +  if (process->thread_count () == 1)
>>      return;
>>
>> -  thread_info *thread = find_thread_ptid (ptid_t (pid, tid));
>> -  if (thread == NULL)
>> +  thread_info *thread = process.find_thread (ptid_t (pid, tid));
> 
> I think there is an error here, shouldn't this be
> 
>   process->find_thread (ptid_t (pid, tid))
> 
> instead?

Heh, you're right, that means I didn't build-test properly.

I fixed all those locally, I'll wait to see if Kevin or Hannes have
anything to say about this version.

Thanks!

Simon
  

Simon Marchi <simark@simark.ca> writes:

>>> --- a/gdbserver/win32-low.cc
>>> +++ b/gdbserver/win32-low.cc
>>> @@ -188,12 +188,17 @@ delete_thread_info (thread_info *thread)
>>>  static void
>>>  child_delete_thread (DWORD pid, DWORD tid)
>>>  {
>>> +  process_info *process = find_process_pid (pid);
>>> +
>>> +  if (process == nullptr)
>>> +    return;
>>> +
>>>    /* If the last thread is exiting, just return.  */
>>> -  if (all_threads.size () == 1)
>>> +  if (process->thread_count () == 1)
>>>      return;
>>>
>>> -  thread_info *thread = find_thread_ptid (ptid_t (pid, tid));
>>> -  if (thread == NULL)
>>> +  thread_info *thread = process.find_thread (ptid_t (pid, tid));
>> 
>> I think there is an error here, shouldn't this be
>> 
>>   process->find_thread (ptid_t (pid, tid))
>> 
>> instead?
>
> Heh, you're right, that means I didn't build-test properly.
>
> I fixed all those locally, I'll wait to see if Kevin or Hannes have
> anything to say about this version.

FWIW, took your series + that fixup for a spin on our native Windows
testing and it reports no regressions.  Thanks for the nice cleanup!
  

On 2024-12-05 04:08, Kévin Le Gouguec wrote:
> Simon Marchi <simark@simark.ca> writes:
> 
>>>> --- a/gdbserver/win32-low.cc
>>>> +++ b/gdbserver/win32-low.cc
>>>> @@ -188,12 +188,17 @@ delete_thread_info (thread_info *thread)
>>>>  static void
>>>>  child_delete_thread (DWORD pid, DWORD tid)
>>>>  {
>>>> +  process_info *process = find_process_pid (pid);
>>>> +
>>>> +  if (process == nullptr)
>>>> +    return;
>>>> +
>>>>    /* If the last thread is exiting, just return.  */
>>>> -  if (all_threads.size () == 1)
>>>> +  if (process->thread_count () == 1)
>>>>      return;
>>>>
>>>> -  thread_info *thread = find_thread_ptid (ptid_t (pid, tid));
>>>> -  if (thread == NULL)
>>>> +  thread_info *thread = process.find_thread (ptid_t (pid, tid));
>>>
>>> I think there is an error here, shouldn't this be
>>>
>>>   process->find_thread (ptid_t (pid, tid))
>>>
>>> instead?
>>
>> Heh, you're right, that means I didn't build-test properly.
>>
>> I fixed all those locally, I'll wait to see if Kevin or Hannes have
>> anything to say about this version.
> 
> FWIW, took your series + that fixup for a spin on our native Windows
> testing and it reports no regressions.  Thanks for the nice cleanup!

Thanks for testing!

I will push the first two patches right now to un-break the build, and
wait for maybe more feedback from Hannes form the third one.

Simon