[PATCH v6 2/2] gdbserver: add process specific thread list and map

[Simon Marchi]

On 2024-11-06 04:23, Stephan Rohr wrote:
> Replace the servers global thread list with a process specific thread
> list and a ptid -> thread map similar to 'inferior::ptid_thread_map' on
> GDB side.  Optimize the 'find_thread' and 'find_thread_ptid' functions
> to use std::unordered_map::find for faster lookup of threads without
> iterating over all processes and threads, if applicable.  This becomes
> important when debugging applications with a large thread count, e.g.,
> in the context of GPU debugging.

Just noting that this would be a good use case for gdb::unordered_map,
which I'm trying to add here:

  https://inbox.sourceware.org/gdb-patches/20240823184910.883268-1-simon.marchi@efficios.com/T/#t

It should be a trivial change to do later.

I noted some nits below, but other thank that it looks good to me.  So
with those fixed:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

> @@ -122,12 +120,22 @@ void for_each_thread (gdb::function_view<void (thread_info *)> func);
>  
>  void for_each_thread (int pid, gdb::function_view<void (thread_info *)> func);
>  
> -/* Find the a random thread for which FUNC (THREAD) returns true.  If
> -   no entry is found then return NULL.  */
> +/* Like the above, but only consider threads matching PTID.  */
> +
> +void
> +for_each_thread (ptid_t ptid, gdb::function_view<void (thread_info *)> func);

I don't know if it's proper GNU style, but I would format it like this,
in this case:

void for_each_thread
  (ptid_t ptid, gdb::function_view<void (thread_info *)> func);

> @@ -42,7 +41,15 @@ static std::string current_inferior_cwd;
>  struct thread_info *
>  add_thread (ptid_t thread_id, void *target_data)
>  {
> -  auto &new_thread = all_threads.emplace_back (thread_id, target_data);
> +  process_info *process = find_process_pid (thread_id.pid ());
> +  gdb_assert (process != nullptr);
> +
> +  auto &new_thread = process->thread_list ().emplace_back (thread_id,
> +							   target_data);
> +  auto map_elem = process->thread_map ().insert ({thread_id, &new_thread});
> +
> +  /* A thread with this ptid should not exist in the map yet.  */
> +  gdb_assert (map_elem.second);

I would find it slightly clearer like this:

  bool inserted
    = process->thread_map ().insert ({ thread_id, &new_thread }).second;

  /* A thread with this ptid should not exist in the map yet.  */
  gdb_assert (inserted);

It might not be obvious at first sight what `second` means (I hate C++
pairs for that), so giving the variable the name `inserted` makes it a
bit clearer.

> @@ -122,7 +144,11 @@ set_thread_regcache_data (struct thread_info *thread, struct regcache *data)
>  void
>  clear_inferiors (void)
>  {
> -  all_threads.clear ();
> +  for_each_process ([&] (process_info *process)

No need to capture in this lambda.

> @@ -83,6 +88,33 @@ struct process_info : public intrusive_list_node<process_info>
>       not access inferior memory or registers, as we haven't determined
>       the target architecture/description.  */
>    bool starting_up = false;
> +
> +  /* Return a reference to the private thread list.  */
> +  owning_intrusive_list<thread_info> &thread_list ()
> +  {
> +    return m_thread_list;
> +  }

Let's format this like this:

  owning_intrusive_list<thread_info> &thread_list ()
  { return m_thread_list; }

> +
> +  /* Return a reference to the private thread map.  */
> +  std::unordered_map<ptid_t, thread_info *> &thread_map ()
> +  {
> +    return m_ptid_thread_map;
> +  }

Likewise here.

> @@ -2596,24 +2632,26 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
>        if (strcmp ("qfThreadInfo", own_buf) == 0)
>  	{
>  	  require_running_or_return (own_buf);
> -	  thread_iter = all_threads.begin ();
> +	  init_thread_iter ();
>  
>  	  *own_buf++ = 'm';
>  	  ptid_t ptid = thread_iter->id;
>  	  write_ptid (own_buf, ptid);
> -	  thread_iter++;
> +	  advance_thread_iter ();
>  	  return;
>  	}
>  
>        if (strcmp ("qsThreadInfo", own_buf) == 0)
>  	{
>  	  require_running_or_return (own_buf);
> -	  if (thread_iter != all_threads.end ())
> +	  /* We're done if the process iterator hit the end of the

hit -> hits

Simon