[PATCH v2 1/1] gdbserver: add process specific thread list and map.
Rohr, Stephan
stephan.rohr@intel.com
Fri Oct 18 11:21:25 GMT 2024
Hi Simon,
Thank you for your feedback. Please see my answers inlined.
I will submit v3 of the patch.
Thanks
Stephan
> -----Original Message-----
> From: Simon Marchi <simark@simark.ca>
> Sent: Tuesday, 8 October 2024 22:13
> To: Rohr, Stephan <stephan.rohr@intel.com>; gdb-patches@sourceware.org
> Cc: aburgess@redhat.com
> Subject: Re: [PATCH v2 1/1] gdbserver: add process specific thread list and
> map.
>
>
> Hi Stephan,
>
> Here's a first pass, non exhaustive, but I feel like there are enough
> comments for now.
>
> On 2024-09-03 07:34, 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.
> >
> > Add the following functions for performance improvements by
> > limiting the iterator range, if applicable:
> >
> > - for_each_thread (ptid_t ptid, Func func)
> > - for_each_thread (process_info *process, Func func)
> > - find_thread_in_random (ptid_t ptid, Func func)
> > ---
> > gdbserver/gdbthread.h | 146 +++++++++++++++++++++++++++++++-----
> -----
> > gdbserver/inferiors.cc | 49 ++++++++++----
> > gdbserver/inferiors.h | 29 +++++++-
> > gdbserver/server.cc | 59 ++++++++++++++---
> > 4 files changed, 226 insertions(+), 57 deletions(-)
> >
> > diff --git a/gdbserver/gdbthread.h b/gdbserver/gdbthread.h
> > index c5a5498088f..f7562ac54a1 100644
> > --- a/gdbserver/gdbthread.h
> > +++ b/gdbserver/gdbthread.h
> > @@ -85,7 +85,7 @@ struct thread_info
> > gdb_thread_options thread_options = 0;
> > };
> >
> > -extern std::list<thread_info *> all_threads;
> > +extern std::list<process_info *> all_processes;
>
> Since all_processes is defined in inferiors.cc, can you puts its
> declaration in inferiors.h?
>
> >
> > void remove_thread (struct thread_info *thread);
> > struct thread_info *add_thread (ptid_t ptid, void *target_data);
> > @@ -100,17 +100,20 @@ struct thread_info *find_thread_ptid (ptid_t
> ptid);
> > found. */
> > struct thread_info *find_any_thread_of_pid (int pid);
> >
> > -/* Find the first thread for which FUNC returns true. Return NULL if no
> thread
> > - satisfying FUNC is found. */
> > +/* Find the first thread for which FUNC returns true, only consider
> > + threads in the thread list of PROCESS. Return NULL if no thread
> > + that satisfies FUNC is found. */
>
> I suggest this more concise formulation:
>
> /* Find the first thread of PROCESS for which FUNC returns true.
>
> Return NULL if no thread that satisfies FUNC is found. */
>
> >
> > template <typename Func>
> > static thread_info *
> > -find_thread (Func func)
> > +find_thread (process_info *process, Func func)
> > {
> > - std::list<thread_info *>::iterator next, cur = all_threads.begin ();
> > + std::list<thread_info *> *thread_list = get_thread_list (process);
> > + std::list<thread_info *>::iterator next, cur = thread_list->begin ();
> >
> > - while (cur != all_threads.end ())
> > + while (cur != thread_list->end ())
> > {
> > + /* FUNC may alter the current iterator. */
> > next = cur;
> > next++;
>
> It seems weird for a find_thread callback to change things, you'd think
> that "find" is a read only operation. Suggestion for subsequent
> cleanup: I think this can be made simple by using basic_safe_iterator.
>
I agree here, I added the 'intrusive_list' and changed to
for (thread_info &thread : *(process.thread_list ()))
{
...
This only applies to 'find_*'. Changing 'for_each:*' similarly causes regressions.
If we want to change to a 'basic_safe_iterator', we need to implement an
'all_threads_iterator' similarly to the implementation on GDB side. This also
relates to the 'abstract_all_threads_iterator' as suggested by Andrew. From my
point of view, we can add an 'all_threads_iterator' in a separate patch. I'm not in
favour of the abstract version - we still need separate implementations of 'advance'
such that we're only adding complexity without real improvements on functional side.
> >
> > @@ -120,7 +123,23 @@ find_thread (Func func)
> > cur = next;
> > }
> >
> > - return NULL;
> > + return nullptr;
> > +}
> > +
> > +/* Like the above, but consider all threads of all processes. */
>
> I would say just "consider threads of all processes".
>
> > +
> > +template <typename Func>
> > +static thread_info *
> > +find_thread (Func func)
> > +{
> > + for (process_info *proc : all_processes)
> > + {
> > + thread_info *thread = find_thread (proc, func);
>
> Should this function (and maybe the other find_thread) take
> `Func &&func`, and should this find_thread std::forward it to the other
> find_thread (perfect forwarding)?
>
> I'm not too sure why this code uses templating anyway, instead of
> gdb::function_view, this seems like an overkill use of templates...
>
I changed to 'gdb::function_view'. This works fine.
> > @@ -142,21 +162,39 @@ template <typename Func>
> > static thread_info *
> > find_thread (ptid_t filter, Func func)
> > {
> > - return find_thread ([&] (thread_info *thread) {
> > - return thread->id.matches (filter) && func (thread);
> > - });
> > + if (filter == minus_one_ptid)
> > + return find_thread (func);
> > +
> > + process_info *process = find_process_pid (filter.pid ());
> > + if (process == nullptr)
> > + return nullptr;
> > +
> > + if (filter.is_pid ())
> > + return find_thread (process, func);
> > +
> > + std::unordered_map<ptid_t, thread_info *> *thread_map
> > + = get_thread_map (process);
> > + std::unordered_map<ptid_t, thread_info *>::iterator it
> > + = thread_map->find (filter);
> > + if (it != thread_map->end () && func (it->second))
> > + return it->second;
> > +
> > + return nullptr;
> > }
> >
> > -/* Invoke FUNC for each thread. */
> > +/* Invoke FUNC for each thread in the thread list of PROCESS. */
>
> I would suggest "for each thread of PROCESS", to be more concise.
>
> > @@ -41,8 +40,15 @@ 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);
> >
> > - all_threads.push_back (new_thread);
> > + gdb_assert (process != nullptr);
> > + /* A thread with this ptid should not exist in the map yet. */
> > + gdb_assert (process->m_ptid_thread_map.find (thread_id)
> > + == process->m_ptid_thread_map.end ());
>
> Please add a blank line before the comment.
>
> > diff --git a/gdbserver/inferiors.h b/gdbserver/inferiors.h
> > index 00e42335014..289954412fe 100644
> > --- a/gdbserver/inferiors.h
> > +++ b/gdbserver/inferiors.h
> > @@ -73,6 +73,13 @@ struct process_info
> > /* DLLs that are loaded for this proc. */
> > std::list<dll_info> all_dlls;
> >
> > + /* This processes' thread list, sorted by creation order. */
>
> Just wondering, is the "sorted by creation order" aspect important to
> any part of gdbserver?
>
The sorting is not relevant from a technical point of view, but still makes sense
as gdbserver would transfer a randomly ordered list to GDB otherwise. This would
be added to GDB's thread list and could cause confusion to the user, e.g., when using
the 'info threads' command.
> > + std::list<thread_info *> m_thread_list;
>
> Could you try to use gdb::intrusive_list for this? It could be as a
> follow-up patch, or in this patch, I don't mind. It will be a bit more
> lightweight than `std::list`. I think that most uses of std::list in
> gdbserver could be replaced with gdb::intrusive_list (or
> gdb::owning_intrusive_list).
>
I added the 'intrusive_list' in a separate patch.
> > +
> > + /* A map of ptid_t to thread_info*, for average O(1) ptid_t lookup.
> > + Exited threads do not appear in the map. */
> > + std::unordered_map<ptid_t, thread_info *> m_ptid_thread_map;
>
> I'm wondering if we could just have the map, and not the list. The
> "downside" is that it won't be sorted by creation order, which is why
> I'm asking above.
>
> > @@ -92,9 +99,25 @@ pid_of (const process_info *proc)
> > return proc->pid;
> > }
> >
> > -/* Return a pointer to the process that corresponds to the current
> > - thread (current_thread). It is an error to call this if there is
> > - no current thread selected. */
> > +/* Accessors for a processes' thread list and map. */
> > +
> > +inline std::list<thread_info *> *
> > +get_thread_list (struct process_info *process)
> > +{
> > + gdb_assert (process != nullptr);
> > + return &(process->m_thread_list);
> > +}
> > +
> > +inline std::unordered_map<ptid_t, thread_info *> *
> > +get_thread_map (struct process_info *process)
> > +{
> > + gdb_assert (process != nullptr);
> > + return &(process->m_ptid_thread_map);
> > +}
>
> Can these be methods of process_info instead? For getter methods, we
> usually
> omit the `get_` prefix. You could then make the `m_*` fields actually
> private.
>
Done.
> > +
> > +/* Return a pointer to the current process. Note that the current
> > + process may be non-null while the current thread (current_thread)
> > + is null. */
>
> Is this comment change related to this patch? If not, put it in a
> separate patch.
>
Yes, the comment is related to this patch.
> > @@ -2509,8 +2506,48 @@ static void
> > handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
> > {
> > client_state &cs = get_client_state ();
> > + static std::list<process_info *>::const_iterator process_iter;
> > static std::list<thread_info *>::const_iterator thread_iter;
> >
> > + auto init_thread_iter = [&] ()
> > + {
> > + process_iter = all_processes.begin ();
> > + std::list<thread_info *> *thread_list;
> > +
> > + for (; process_iter != all_processes.end (); ++process_iter)
> > + {
> > + thread_list = get_thread_list (*process_iter);
> > + thread_iter = thread_list->begin ();
> > + if (thread_iter != thread_list->end ())
> > + break;
> > + }
> > + /* Make sure that there is at least one thread to iterate. */
> > + gdb_assert (process_iter != all_processes.end ());
> > + gdb_assert (thread_iter != thread_list->end ());
> > + };
> > +
> > + auto advance_thread_iter = [&] ()
> > + {
> > + /* The loop below is written in the natural way as-if we'd always
> > + start at the beginning of the inferior list. This fast forwards
> > + the algorithm to the actual current position. */
>
> I was like... this algorithm reminds me of gdb/thread-iter.h... oh, this
> comment is from gdb/thread-iter.h :).
>
> I saw Andrew's comment on v1, about sharing this algorithm in
> gdbsupport. I agree on the principle, but it doesn't sound easy.
>
> 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
More information about the Gdb-patches
mailing list