[v5,1/2] gdbserver: change 'all_processes' and 'all_threads' list type

  This patch replaces the 'std::list' type of 'all_processes' and
'all_threads' with the more lightweight 'owning_intrusive_list'
type.
---
 gdbserver/gdbthread.h  |  8 ++---
 gdbserver/inferiors.cc | 75 +++++++++++++-----------------------------
 gdbserver/inferiors.h  |  7 ++--
 gdbserver/server.cc    | 16 ++++-----
 4 files changed, 37 insertions(+), 69 deletions(-)
  

> @@ -56,7 +55,7 @@ struct thread_info *
>  get_first_thread (void)
>  {
>    if (!all_threads.empty ())
> -    return all_threads.front ();
> +    return &(all_threads.front ());

Unnecessary parenthesis.

> @@ -101,10 +94,9 @@ remove_thread (struct thread_info *thread)
>      target_disable_btrace (thread->btrace);
>  
>    discard_queued_stop_replies (ptid_of (thread));
> -  all_threads.remove (thread);
> +  all_threads.erase (all_threads.iterator_to (*thread));
>    if (current_thread == thread)
>      switch_to_thread (nullptr);
> -  free_one_thread (thread);

switch_to_thread accesses thread, so perhaps move the erase line at the
end?

> @@ -140,11 +131,8 @@ clear_inferiors (void)
>  struct process_info *
>  add_process (int pid, int attached)
>  {
> -  process_info *process = new process_info (pid, attached);
> -
> -  all_processes.push_back (process);
> -
> -  return process;
> +  auto &process = all_processes.emplace_back (pid, attached);
> +  return &process;

  return &all_processes.emplace_back (pid, attached);

> @@ -157,10 +145,9 @@ remove_process (struct process_info *process)
>    clear_symbol_cache (&process->symbol_cache);
>    free_all_breakpoints (process);
>    gdb_assert (find_thread_process (process) == NULL);
> -  all_processes.remove (process);
> +  all_processes.erase (all_processes.iterator_to (*process));
>    if (current_process () == process)
>      switch_to_process (nullptr);
> -  delete process;

switch_to_process does not access `process`, but to be future-proof (if
it ever does), and to mirror the thread removal above, I would move the
erase at the end here as well.

> @@ -177,7 +164,7 @@ process_info *
>  get_first_process (void)
>  {
>    if (!all_processes.empty ())
> -    return all_processes.front ();
> +    return &(all_processes.front ());

Unnecessary parenthesis.

> @@ -236,18 +223,9 @@ for_each_process (gdb::function_view<void (process_info *)> func)
>  process_info *
>  find_process (gdb::function_view<bool (process_info *)> func)
>  {
> -  std::list<process_info *>::iterator next, cur = all_processes.begin ();
> -
> -  while (cur != all_processes.end ())
> -    {
> -      next = cur;
> -      next++;
> -
> -      if (func (*cur))
> -	return *cur;
> -
> -      cur = next;
> -    }
> +  for (process_info &process : all_processes)
> +    if (func (&process))
> +      return &process;

Just to confirm, are we sure that this transformation is safe, nothing
calling `find_process` needed this "safe" iteration?

> @@ -257,18 +235,9 @@ find_process (gdb::function_view<bool (process_info *)> func)
>  thread_info *
>  find_thread (gdb::function_view<bool (thread_info *)> func)
>  {
> -  std::list<thread_info *>::iterator next, cur = all_threads.begin ();
> -
> -  while (cur != all_threads.end ())
> -    {
> -      next = cur;
> -      next++;
> -
> -      if (func (*cur))
> -	return *cur;
> -
> -      cur = next;
> -    }
> +  for (thread_info &thread : all_threads)
> +    if (func (&thread))
> +      return &thread;

Ditto here?

> @@ -2523,7 +2523,7 @@ static void
>  handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
>  {
>    client_state &cs = get_client_state ();
> -  static std::list<thread_info *>::const_iterator thread_iter;
> +  static owning_intrusive_list<thread_info>::iterator thread_iter;

I was skeptical of this change, I don't see why we would need to change
const_iterator to iterator.  I then realized that our const_iterator
implementation is broken.

It shouldn't be

  using const_iterator = const intrusive_list_iterator<T, AsNode>;

but

  using const_iterator = intrusive_list_iterator<const T, AsNode>;

But of course fixing this uncovers some mistakes, not trivial to fix.
I'd like to fix it, but in the mean time I think it's ok for you to
change const_iterator -> iterator, as to not delay this patch series
further.

Simon
  

Hi Simon,

thank you for your feedback.

Regarding the "safe" iteration in 'find_thread' and 'find_process': I ran the testsuite for
the 'native-gdbserver' and 'native-extended-gdbserver' targets on Ubuntu22, I could not
find regressions.

Would this be sufficient to assume the change is safe? I haven't tested on other platforms,
but couldn't find relevant calls to 'find_thread' and 'find_process' that modify the
currently iterated thread / process.

If we're sceptical about breaking something, I also don't object going back to the "safe"
iteration.

Thanks
Stephan

> -----Original Message-----
> From: Simon Marchi <simark@simark.ca>
> Sent: Monday, 28 October 2024 18:16
> To: Rohr, Stephan <stephan.rohr@intel.com>; gdb-patches@sourceware.org
> Cc: aburgess@redhat.com; tom@tromey.com
> Subject: Re: [PATCH v5 1/2] gdbserver: change 'all_processes' and 'all_threads'
> list type
> 
> > @@ -56,7 +55,7 @@ struct thread_info *
> >  get_first_thread (void)
> >  {
> >    if (!all_threads.empty ())
> > -    return all_threads.front ();
> > +    return &(all_threads.front ());
> 
> Unnecessary parenthesis.
> 
> > @@ -101,10 +94,9 @@ remove_thread (struct thread_info *thread)
> >      target_disable_btrace (thread->btrace);
> >
> >    discard_queued_stop_replies (ptid_of (thread));
> > -  all_threads.remove (thread);
> > +  all_threads.erase (all_threads.iterator_to (*thread));
> >    if (current_thread == thread)
> >      switch_to_thread (nullptr);
> > -  free_one_thread (thread);
> 
> switch_to_thread accesses thread, so perhaps move the erase line at the
> end?
> 
> > @@ -140,11 +131,8 @@ clear_inferiors (void)
> >  struct process_info *
> >  add_process (int pid, int attached)
> >  {
> > -  process_info *process = new process_info (pid, attached);
> > -
> > -  all_processes.push_back (process);
> > -
> > -  return process;
> > +  auto &process = all_processes.emplace_back (pid, attached);
> > +  return &process;
> 
>   return &all_processes.emplace_back (pid, attached);
> 
> > @@ -157,10 +145,9 @@ remove_process (struct process_info *process)
> >    clear_symbol_cache (&process->symbol_cache);
> >    free_all_breakpoints (process);
> >    gdb_assert (find_thread_process (process) == NULL);
> > -  all_processes.remove (process);
> > +  all_processes.erase (all_processes.iterator_to (*process));
> >    if (current_process () == process)
> >      switch_to_process (nullptr);
> > -  delete process;
> 
> switch_to_process does not access `process`, but to be future-proof (if
> it ever does), and to mirror the thread removal above, I would move the
> erase at the end here as well.
> 
> > @@ -177,7 +164,7 @@ process_info *
> >  get_first_process (void)
> >  {
> >    if (!all_processes.empty ())
> > -    return all_processes.front ();
> > +    return &(all_processes.front ());
> 
> Unnecessary parenthesis.
> 
> > @@ -236,18 +223,9 @@ for_each_process (gdb::function_view<void
> (process_info *)> func)
> >  process_info *
> >  find_process (gdb::function_view<bool (process_info *)> func)
> >  {
> > -  std::list<process_info *>::iterator next, cur = all_processes.begin ();
> > -
> > -  while (cur != all_processes.end ())
> > -    {
> > -      next = cur;
> > -      next++;
> > -
> > -      if (func (*cur))
> > -	return *cur;
> > -
> > -      cur = next;
> > -    }
> > +  for (process_info &process : all_processes)
> > +    if (func (&process))
> > +      return &process;
> 
> Just to confirm, are we sure that this transformation is safe, nothing
> calling `find_process` needed this "safe" iteration?
> 
> > @@ -257,18 +235,9 @@ find_process (gdb::function_view<bool
> (process_info *)> func)
> >  thread_info *
> >  find_thread (gdb::function_view<bool (thread_info *)> func)
> >  {
> > -  std::list<thread_info *>::iterator next, cur = all_threads.begin ();
> > -
> > -  while (cur != all_threads.end ())
> > -    {
> > -      next = cur;
> > -      next++;
> > -
> > -      if (func (*cur))
> > -	return *cur;
> > -
> > -      cur = next;
> > -    }
> > +  for (thread_info &thread : all_threads)
> > +    if (func (&thread))
> > +      return &thread;
> 
> Ditto here?
> 
> > @@ -2523,7 +2523,7 @@ static void
> >  handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
> >  {
> >    client_state &cs = get_client_state ();
> > -  static std::list<thread_info *>::const_iterator thread_iter;
> > +  static owning_intrusive_list<thread_info>::iterator thread_iter;
> 
> I was skeptical of this change, I don't see why we would need to change
> const_iterator to iterator.  I then realized that our const_iterator
> implementation is broken.
> 
> It shouldn't be
> 
>   using const_iterator = const intrusive_list_iterator<T, AsNode>;
> 
> but
> 
>   using const_iterator = intrusive_list_iterator<const T, AsNode>;
> 
> But of course fixing this uncovers some mistakes, not trivial to fix.
> I'd like to fix it, but in the mean time I think it's ok for you to
> change const_iterator -> iterator, as to not delay this patch series
> further.
> 
> Simon
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-10-29 16:54, Rohr, Stephan wrote:
> Hi Simon,
> 
> thank you for your feedback.
> 
> Regarding the "safe" iteration in 'find_thread' and 'find_process': I ran the testsuite for
> the 'native-gdbserver' and 'native-extended-gdbserver' targets on Ubuntu22, I could not
> find regressions.
> 
> Would this be sufficient to assume the change is safe? I haven't tested on other platforms,
> but couldn't find relevant calls to 'find_thread' and 'find_process' that modify the
> currently iterated thread / process.
> 
> If we're sceptical about breaking something, I also don't object going back to the "safe"
> iteration.

I just did a scan of all uses of find_thread and find_process in
gdbserver, and nothing appeared to need the safe iteration, so I think
we are good.  I think I said it earlier, but it would surprise me if
some thread or process were removed during a find_* operation.

Simon