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
@@ -84,8 +84,6 @@ struct thread_info : public intrusive_list_node<thread_info>
gdb_thread_options thread_options = 0;
};
-extern intrusive_list<thread_info> all_threads;
-
void remove_thread (struct thread_info *thread);
struct thread_info *add_thread (ptid_t ptid, void *target_data);
@@ -99,8 +97,16 @@ 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 of PROCESS for which FUNC returns true.
+
+ Return NULL if no thread satisfying FUNC is found. */
+
+thread_info *
+find_thread (process_info &process,
+ gdb::function_view<bool (thread_info &)> func);
+
+
+/* Like the above, but consider threads of all processes. */
thread_info *
find_thread (gdb::function_view<bool (thread_info &)> func);
@@ -116,6 +122,12 @@ find_thread (int pid, gdb::function_view<bool (thread_info &)> func);
thread_info *
find_thread (ptid_t filter, gdb::function_view<bool (thread_info &)> func);
+/* Invoke FUNC for each thread of PROCESS. */
+
+void
+for_each_thread (process_info &process,
+ gdb::function_view<void (thread_info &)> func);
+
/* Invoke FUNC for each thread. */
void
@@ -126,6 +138,18 @@ for_each_thread (gdb::function_view<void (thread_info &)> func);
void
for_each_thread (int pid, gdb::function_view<void (thread_info &)> func);
+/* Like the above, but only consider threads matching PTID. */
+
+void
+for_each_thread (ptid_t ptid, gdb::function_view<void (thread_info &)> func);
+
+/* Find a random thread that matches PTID and FUNC (THREAD)
+ returns true. If no entry is found then return NULL. */
+
+thread_info *
+find_thread_in_random (ptid_t ptid,
+ gdb::function_view<bool (thread_info &)> func);
+
/* Find the a random thread for which FUNC (THREAD) returns true. If
no entry is found then return NULL. */
@@ -23,8 +23,6 @@
#include "dll.h"
intrusive_list<process_info> all_processes;
-intrusive_list<thread_info> all_threads;
-
/* The current process. */
static process_info *current_process_;
@@ -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});
if (current_thread == NULL)
switch_to_thread (new_thread);
@@ -55,18 +61,28 @@ add_thread (ptid_t thread_id, void *target_data)
struct thread_info *
get_first_thread (void)
{
- if (!all_threads.empty ())
- return &(all_threads.front ());
- else
- return NULL;
+ return find_thread ([] (thread_info &thread)
+ {
+ return true;
+ });
}
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;
+
+ return nullptr;
}
/* Find a thread associated with the given PROCESS, or NULL if no
@@ -95,8 +111,16 @@ remove_thread (struct thread_info *thread)
target_disable_btrace (thread->btrace);
discard_queued_stop_replies (ptid_of (thread));
- auto it = all_threads.iterator_to (*thread);
- all_threads.erase (it);
+ process_info *process = get_thread_process (thread);
+ gdb_assert (process != nullptr);
+
+ /* We should not try to remove a thread that was not added. */
+ int num_erased = process->thread_map ()->erase (thread->id);
+ gdb_assert (num_erased > 0);
+
+ auto it = process->thread_list ()->iterator_to (*thread);
+ process->thread_list ()->erase (it);
+
if (current_thread == thread)
switch_to_thread (nullptr);
delete thread;
@@ -123,9 +147,13 @@ set_thread_regcache_data (struct thread_info *thread, struct regcache *data)
void
clear_inferiors (void)
{
- all_threads.clear_and_dispose ([=] (thread_info *thr)
+ for_each_process ([&] (process_info &process)
{
- delete thr;
+ process.thread_list ()->clear_and_dispose ([=] (thread_info *thr)
+ {
+ delete thr;
+ });
+ process.thread_map ()->clear ();
});
clear_dlls ();
@@ -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)
+{
+ for (thread_info &thread : *(process.thread_list ()))
+ {
+ if (func (thread))
+ return &thread;
+ }
+
+ return nullptr;
+}
+/* See gdbthread.h. */
+
thread_info *
find_thread (gdb::function_view<bool (thread_info &)> func)
{
- for (thread_info &thread : all_threads)
- if (func (thread))
- return &thread;
+ for (process_info &process : all_processes)
+ {
+ thread_info *thread = find_thread (process, func);
+ if (thread != nullptr)
+ return thread;
+ }
return NULL;
}
@@ -269,20 +314,39 @@ find_thread (int pid, gdb::function_view<bool (thread_info &)> func)
thread_info *
find_thread (ptid_t filter, gdb::function_view<bool (thread_info &)> 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
+ = process->thread_map ();
+ 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;
}
/* See gdbthread.h. */
void
-for_each_thread (gdb::function_view<void (thread_info &)> func)
+for_each_thread (process_info &process,
+ gdb::function_view<void (thread_info &)> func)
{
- intrusive_list<thread_info>::iterator next, cur = all_threads.begin ();
+ intrusive_list<thread_info> *thread_list = process.thread_list ();
+ intrusive_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++;
func (*cur);
@@ -293,45 +357,84 @@ for_each_thread (gdb::function_view<void (thread_info &)> func)
/* See gdbthread.h. */
void
-for_each_thread (int pid, gdb::function_view<void (thread_info &)> func)
+for_each_thread (gdb::function_view<void (thread_info &)> func)
{
- for_each_thread ([&] (thread_info &thread)
+ for_each_process ([&] (process_info &proc)
{
- if (pid == thread.id.pid ())
- func (thread);
+ for_each_thread (proc, func);
});
}
/* See gdbthread.h. */
+void
+for_each_thread (int pid, gdb::function_view<void (thread_info &)> func)
+{
+ process_info *process = find_process_pid (pid);
+ if (process == nullptr)
+ return;
+
+ for_each_thread (*process, func);
+}
+
+/* See gdbthread.h. */
+
+void
+for_each_thread (ptid_t ptid, gdb::function_view<void (thread_info &)> func)
+{
+ if (ptid == minus_one_ptid)
+ for_each_thread (func);
+ else if (ptid.is_pid ())
+ for_each_thread (ptid.pid (), func);
+ else
+ find_thread (ptid, [func] (thread_info &thread)
+ {
+ func (thread);
+ return false;
+ });
+}
+
+/* See gdbthread.h. */
+
thread_info *
-find_thread_in_random (gdb::function_view<bool (thread_info &)> func)
+find_thread_in_random (ptid_t ptid,
+ gdb::function_view<bool (thread_info &)> func)
{
int count = 0;
int random_selector;
/* First count how many interesting entries we have. */
- for_each_thread ([&] (thread_info &thread) {
- if (func (thread))
- count++;
- });
+ for_each_thread (ptid, [&] (thread_info &thread)
+ {
+ if (func (thread))
+ count++;
+ });
if (count == 0)
- return NULL;
+ return nullptr;
/* Now randomly pick an entry out of those. */
random_selector = (int)
((count * (double) rand ()) / (RAND_MAX + 1.0));
- thread_info *thread = find_thread ([&] (thread_info &thr_arg) {
- return func (thr_arg) && (random_selector-- == 0);
- });
+ thread_info *thread = find_thread (ptid, [&] (thread_info &thr_arg)
+ {
+ return func (thr_arg) && (random_selector-- == 0);
+ });
gdb_assert (thread != NULL);
return thread;
}
+/* See gdbthread.h. */
+
+thread_info *
+find_thread_in_random (gdb::function_view<bool (thread_info &)> func)
+{
+ return find_thread_in_random (minus_one_ptid, func);
+}
+
/* See gdbsupport/common-gdbthread.h. */
@@ -23,6 +23,8 @@
#include "gdbsupport/intrusive_list.h"
#include "dll.h"
+#include <unordered_map>
+
struct thread_info;
struct regcache;
struct target_desc;
@@ -31,9 +33,13 @@ struct breakpoint;
struct raw_breakpoint;
struct fast_tracepoint_jump;
struct process_info_private;
+struct process_info;
+
+extern intrusive_list<process_info> all_processes;
-struct process_info : public intrusive_list_node<process_info>
+class process_info : public intrusive_list_node<process_info>
{
+public:
process_info (int pid_, int attached_)
: pid (pid_), attached (attached_)
{}
@@ -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;
+ }
+
+private:
+ /* This processes' thread list, sorted by creation order. */
+ intrusive_list<thread_info> m_thread_list;
+
+ /* 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;
};
/* Get the pid of PROC. */
@@ -92,9 +116,9 @@ 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. */
+/* Return a pointer to the current process. Note that the current
+ process may be non-null while the current thread (current_thread)
+ is null. */
struct process_info *current_process (void);
struct process_info *get_thread_process (const struct thread_info *);
@@ -1355,7 +1355,7 @@ handle_detach (char *own_buf)
another process might delete the next thread in the iteration, which is
the one saved by the safe iterator. We will never delete the currently
iterated on thread, so standard iteration should be safe. */
- for (thread_info &thread : all_threads)
+ for (thread_info &thread : *(process->thread_list ()))
{
/* Only threads that are of the process we are detaching. */
if (thread.id.pid () != pid)
@@ -2523,8 +2523,48 @@ static void
handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
{
client_state &cs = get_client_state ();
+ static intrusive_list<process_info>::iterator process_iter;
static intrusive_list<thread_info>::iterator thread_iter;
+ auto init_thread_iter = [&] ()
+ {
+ process_iter = all_processes.begin ();
+ intrusive_list<thread_info> *thread_list;
+
+ for (; process_iter != all_processes.end (); ++process_iter)
+ {
+ thread_list = process_iter->thread_list ();
+ 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. */
+ intrusive_list<thread_info> *thread_list
+ = process_iter->thread_list ();
+ goto start;
+
+ for (; process_iter != all_processes.end (); ++process_iter)
+ {
+ thread_list = process_iter->thread_list ();
+ thread_iter = thread_list->begin ();
+ while (thread_iter != thread_list->end ())
+ {
+ return;
+ start:
+ ++thread_iter;
+ }
+ }
+ };
+
/* Reply the current thread id. */
if (strcmp ("qC", own_buf) == 0 && !disable_packet_qC)
{
@@ -2535,7 +2575,7 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
ptid = cs.general_thread;
else
{
- thread_iter = all_threads.begin ();
+ init_thread_iter ();
ptid = thread_iter->id;
}
@@ -2596,24 +2636,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
+ process list. */
+ if (process_iter != all_processes.end ())
{
*own_buf++ = 'm';
ptid_t ptid = thread_iter->id;
write_ptid (own_buf, ptid);
- thread_iter++;
+ advance_thread_iter ();
return;
}
else