[PATCH v3 3/3] gdbserver: add process specific thread list and map.

[Simon Marchi]

On 2024-10-18 11:20, Stephan Rohr wrote:
> @@ -41,8 +39,16 @@ struct thread_info *
>  add_thread (ptid_t thread_id, void *target_data)
>  {
>    thread_info *new_thread = new thread_info (thread_id, target_data);
> +  process_info *process = get_thread_process (new_thread);
> +
> +  gdb_assert (process != nullptr);
> +
> +  /* A thread with this ptid should not exist in the map yet.  */
> +  gdb_assert (process->thread_map ()->find (thread_id)
> +	      == process->thread_map ()->end ());
>  
> -  all_threads.push_back (*new_thread);
> +  process->thread_list ()->push_back (*new_thread);
> +  process->thread_map ()->insert ({thread_id, new_thread});

A good way to implement you assert without doing an extra lookup is to
check insert's return value.  The second element of the pair is true if
insertion took place, so you can assert that after the fact.

>  struct thread_info *
>  find_thread_ptid (ptid_t ptid)
>  {
> -  return find_thread ([&] (thread_info &thread) {
> -    return thread.id == ptid;
> -  });
> +  process_info *process = find_process_pid (ptid.pid ());
> +  if (process == nullptr)
> +    return nullptr;
> +
> +  std::unordered_map<ptid_t, thread_info *> *thread_map
> +    = process->thread_map ();
> +
> +  std::unordered_map<ptid_t, thread_info *>::iterator it
> +    = thread_map->find (ptid);
> +  if (it != thread_map->end ())
> +    return it->second;

It's subjective, but feel free to use auto here.  In my opinion it makes
the code less cluttered.  And with C++17 we can declare the variable
inside the if (I would suggest using that feature whenever possible).  I
would do either of:

  if (auto it = process->thread_map ()->find (ptid);
      it != process->thread_map ()->end ())
    return it->second;

or

  auto thread_map = process->thread_map ();

  if (auto it = thread_map->find (ptid); it != thread_map->end ())
    return it->second;

... depending on your feeling about calling `process->thread_map ()`
twice.

> @@ -243,12 +271,29 @@ find_process (gdb::function_view<bool (process_info &)> func)
>  
>  /* See gdbthread.h.  */
>  
> +thread_info *
> +find_thread (process_info &process,
> +	     gdb::function_view<bool (thread_info &)> func)

Do you think that this find_thread (and others of this family) should
become a method on process_info?  Keep in mind that all this code was
written before all of this was C++.  If we think a method would make
more sense, I would make the new one a method directly, and eventually
move the existing ones to be methods.

> @@ -82,6 +88,24 @@ 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;
> +
> +  intrusive_list<thread_info>* thread_list ()
> +  {
> +    return &m_thread_list;
> +  }
> +
> +  std::unordered_map<ptid_t, thread_info *>* thread_map ()
> +  {
> +    return &m_ptid_thread_map;
> +  }

I think it would make more sense for these two methods to return
references.

Simon