From a66f72981979a1bda60805b8554e0c78c4a39a21 Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Mon, 21 Jun 2021 14:26:36 -0400 Subject: [PATCH] gdb: maintain per-process-target list of resumed threads with pending wait status Looking up threads that are both resumed and have a pending wait status to report is something that we do quite often in the fast path and is expensive if there are many threads, since it currently requires walking whole thread lists. The first instance is in maybe_set_commit_resumed_all_targets. This is called after handling each event in fetch_inferior_event, to see if we should ask targets to commit their resumed threads or not. If at least one thread is resumed but has a pending wait status, we don't ask the targets to commit their resumed threads, because we want to consume and handle the pending wait status first. The second instance is in random_pending_event_thread, where we want to select a random thread among all those that are resumed and have a pending wait status. This is called every time we try to consume events, to see if there are any pending events that we we want to consume, before asking the targets for more events. To allow optimizing these cases, maintain a per-process-target list of threads that are resumed and have a pending wait status. In maybe_set_commit_resumed_all_targets, we'll be able to check in O(1) if there are any such threads simply by checking whether the list is empty. In random_pending_event_thread, we'll be able to use that list, which will be quicker than iterating the list of threads, especially when there are no resumed with pending wait status threads. About implementation details: using the new setters on class thread_info, it's relatively easy to maintain that list. Any time the "resumed" or "pending wait status" property is changed, we check whether that should cause the thread to be added or removed from the list. In set_thread_exited, we try to remove the thread from the list, because keeping an exited thread in that list would make no sense (especially if the thread is freed). My first implementation assumed that a process stratum target was always present when set_thread_exited is called. That's however, not the case: in some cases, targets unpush themselves from an inferior and then call "exit_inferior", which exits all the threads. If the target is unpushed before set_thread_exited is called on the threads, it means we could mistakenly leave some threads in the list. I tried to see how hard it would be to make it such that targets have to exit all threads before unpushing themselves from the inferior (that would seem logical to me, we don't want threads belonging to an inferior that has no process target). That seemed quite difficult and not worth the time at the moment. Instead, I changed inferior::unpush_target to remove all threads of that inferior from the list. As of this patch, the list is not used, this is done in the subsequent patches. The debug messages in process-stratum-target.c need to print some ptids. However, they can't use target_pid_to_str to print them without introducing a dependency on the current inferior (the current inferior is used to get the current target stack). For debug messages, I find it clearer to print the spelled out ptid anyway (the pid, lwp and tid values). Add a ptid_t::to_string method that returns a string representation of the ptid that is meant for debug messages, a bit like we already have frame_id::to_string. Change-Id: Iad8f93db2d13984dd5aa5867db940ed1169dbb67 --- gdb/gdbthread.h | 16 +++++++++++++-- gdb/inferior.c | 21 +++++++++++++++++++ gdb/inferior.h | 3 +-- gdb/process-stratum-target.c | 34 ++++++++++++++++++++++++++++++ gdb/process-stratum-target.h | 21 +++++++++++++++++++ gdb/thread.c | 40 ++++++++++++++++++++++++++++++++++++ gdbsupport/ptid.cc | 8 ++++++++ gdbsupport/ptid.h | 7 +++++++ 8 files changed, 146 insertions(+), 4 deletions(-) diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index e9d1bbedfb9..9c178f531d9 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -296,8 +296,7 @@ public: bool resumed () const { return m_resumed; } - void set_resumed (bool resumed) - { m_resumed = resumed; } + void set_resumed (bool resumed); /* Frontend view of the thread state. Note that the THREAD_RUNNING/ THREAD_STOPPED states are different from EXECUTING. When the @@ -470,6 +469,12 @@ public: linked. */ intrusive_list_node step_over_list_node; + /* Node for list of threads that are resumed and have a pending wait status. + + The list head for this is in process_stratum_target, hence all threads in + this list belong to that process target. */ + intrusive_list_node resumed_with_pending_wait_status_node; + /* Displaced-step state for this thread. */ displaced_step_thread_state displaced_step_state; @@ -488,6 +493,13 @@ private: thread_suspend_state m_suspend; }; +using thread_info_resumed_with_pending_wait_status_node + = intrusive_member_node; +using thread_info_resumed_with_pending_wait_status_list + = intrusive_list; + /* A gdb::ref_ptr pointer to a thread_info. */ using thread_info_ref diff --git a/gdb/inferior.c b/gdb/inferior.c index 9681aaf2cdb..3c569f4f04f 100644 --- a/gdb/inferior.c +++ b/gdb/inferior.c @@ -89,6 +89,27 @@ inferior::inferior (int pid_) m_target_stack.push (get_dummy_target ()); } +/* See inferior.h. */ + +int +inferior::unpush_target (struct target_ops *t) +{ + /* If unpushing the process stratum target from the inferior while threads + exist in the inferior, ensure that we don't leave any threads of the + inferior in the target's "resumed with pending wait status" list. + + See also the comment in set_thread_exited. */ + if (t->stratum () == process_stratum) + { + process_stratum_target *proc_target = as_process_stratum_target (t); + + for (thread_info *thread : this->non_exited_threads ()) + proc_target->maybe_remove_resumed_with_pending_wait_status (thread); + } + + return m_target_stack.unpush (t); +} + void inferior::set_tty (const char *terminal_name) { diff --git a/gdb/inferior.h b/gdb/inferior.h index 830dec3ebba..2bfe29afed3 100644 --- a/gdb/inferior.h +++ b/gdb/inferior.h @@ -362,8 +362,7 @@ public: } /* Unpush T from this inferior's target stack. */ - int unpush_target (struct target_ops *t) - { return m_target_stack.unpush (t); } + int unpush_target (struct target_ops *t); /* Returns true if T is pushed in this inferior's target stack. */ bool target_is_pushed (target_ops *t) diff --git a/gdb/process-stratum-target.c b/gdb/process-stratum-target.c index c851090a7f2..2cb6dfc251a 100644 --- a/gdb/process-stratum-target.c +++ b/gdb/process-stratum-target.c @@ -106,6 +106,40 @@ process_stratum_target::follow_exec (inferior *follow_inf, ptid_t ptid, /* See process-stratum-target.h. */ +void +process_stratum_target::maybe_add_resumed_with_pending_wait_status + (thread_info *thread) +{ + gdb_assert (!thread->resumed_with_pending_wait_status_node.is_linked ()); + + if (thread->resumed () && thread->has_pending_waitstatus ()) + { + infrun_debug_printf ("adding to resumed threads with event list: %s", + thread->ptid.to_string ().c_str ()); + m_resumed_with_pending_wait_status.push_back (*thread); + } +} + +/* See process-stratum-target.h. */ + +void +process_stratum_target::maybe_remove_resumed_with_pending_wait_status + (thread_info *thread) +{ + if (thread->resumed () && thread->has_pending_waitstatus ()) + { + infrun_debug_printf ("removing from resumed threads with event list: %s", + thread->ptid.to_string ().c_str ()); + gdb_assert (thread->resumed_with_pending_wait_status_node.is_linked ()); + auto it = m_resumed_with_pending_wait_status.iterator_to (*thread); + m_resumed_with_pending_wait_status.erase (it); + } + else + gdb_assert (!thread->resumed_with_pending_wait_status_node.is_linked ()); +} + +/* See process-stratum-target.h. */ + std::set all_non_exited_process_targets () { diff --git a/gdb/process-stratum-target.h b/gdb/process-stratum-target.h index 31a97753db9..f03728088d3 100644 --- a/gdb/process-stratum-target.h +++ b/gdb/process-stratum-target.h @@ -22,6 +22,8 @@ #include "target.h" #include +#include "gdbsupport/intrusive_list.h" +#include "gdbthread.h" /* Abstract base class inherited by all process_stratum targets. */ @@ -78,6 +80,14 @@ public: may have spawned new threads we haven't heard of yet. */ bool threads_executing = false; + /* If THREAD is resumed and has a pending wait status, add it to the + target's "resumed with pending wait status" list. */ + void maybe_add_resumed_with_pending_wait_status (thread_info *thread); + + /* If THREAD is resumed and has a pending wait status, remove it from the + target's "resumed with pending wait status" list. */ + void maybe_remove_resumed_with_pending_wait_status (thread_info *thread); + /* The connection number. Visible in "info connections". */ int connection_number = 0; @@ -112,6 +122,17 @@ public: coalesce multiple resumption requests in a single vCont packet. */ bool commit_resumed_state = false; + +private: + /* List of threads managed by this target which simultaneously are resumed + and have a pending wait status. + + This is done for optimization reasons, it would be possible to walk the + inferior thread lists to find these threads. But since this is something + we need to do quite frequently in the hot path, maintaining this list + avoids walking the thread lists repeatedly. */ + thread_info_resumed_with_pending_wait_status_list + m_resumed_with_pending_wait_status; }; /* Downcast TARGET to process_stratum_target. */ diff --git a/gdb/thread.c b/gdb/thread.c index 289d33c74c3..74d6b90923f 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -188,6 +188,17 @@ set_thread_exited (thread_info *tp, bool silent) if (tp->state != THREAD_EXITED) { + process_stratum_target *proc_target = tp->inf->process_target (); + + /* Some targets unpush themselves from the inferior's target stack before + clearing the inferior's thread list (which marks all threads as exited, + and therefore leads to this function). In this case, the inferior's + process target will be nullptr when we arrive here. + + See also the comment in inferior::unpush_target. */ + if (proc_target != nullptr) + proc_target->maybe_remove_resumed_with_pending_wait_status (tp); + gdb::observers::thread_exit.notify (tp, silent); /* Tag it as exited. */ @@ -295,6 +306,29 @@ thread_info::deletable () const /* See gdbthread.h. */ +void +thread_info::set_resumed (bool resumed) +{ + if (resumed == m_resumed) + return; + + process_stratum_target *proc_target = this->inf->process_target (); + + /* If we transition from resumed to not resumed, we might need to remove + the thread from the resumed threads with pending statuses list. */ + if (!resumed) + proc_target->maybe_remove_resumed_with_pending_wait_status (this); + + m_resumed = resumed; + + /* If we transition from not resumed to resumed, we might need to add + the thread to the resumed threads with pending statuses list. */ + if (resumed) + proc_target->maybe_add_resumed_with_pending_wait_status (this); +} + +/* See gdbthread.h. */ + void thread_info::set_pending_waitstatus (const target_waitstatus &ws) { @@ -302,6 +336,9 @@ thread_info::set_pending_waitstatus (const target_waitstatus &ws) m_suspend.waitstatus = ws; m_suspend.waitstatus_pending_p = 1; + + process_stratum_target *proc_target = this->inf->process_target (); + proc_target->maybe_add_resumed_with_pending_wait_status (this); } /* See gdbthread.h. */ @@ -311,6 +348,9 @@ thread_info::clear_pending_waitstatus () { gdb_assert (this->has_pending_waitstatus ()); + process_stratum_target *proc_target = this->inf->process_target (); + proc_target->maybe_remove_resumed_with_pending_wait_status (this); + m_suspend.waitstatus_pending_p = 0; } diff --git a/gdbsupport/ptid.cc b/gdbsupport/ptid.cc index 73e2918cbc5..2417120b1ee 100644 --- a/gdbsupport/ptid.cc +++ b/gdbsupport/ptid.cc @@ -24,3 +24,11 @@ ptid_t const null_ptid = ptid_t::make_null (); ptid_t const minus_one_ptid = ptid_t::make_minus_one (); + +/* See ptid.h. */ + +std::string +ptid_t::to_string () const +{ + return string_printf ("%d.%ld.%ld", m_pid, m_lwp, m_tid); +} diff --git a/gdbsupport/ptid.h b/gdbsupport/ptid.h index a0a91758b37..a2553b29c1a 100644 --- a/gdbsupport/ptid.h +++ b/gdbsupport/ptid.h @@ -33,6 +33,7 @@ */ #include +#include class ptid_t { @@ -124,6 +125,12 @@ public: || *this == filter); } + /* Return a string representation of the ptid. + + This is only meant to be used in debug messages. */ + + std::string to_string () const; + /* Make a null ptid. */ static constexpr ptid_t make_null () -- 2.30.2