Merge pull request #3172 from jimingham/stack-map-using-unique-ptr

Rework the ThreadPlanStackMap class so we can move ThreadPlanStacks

Author: jimingham

lldb/include/lldb/Target/ThreadPlanStack.h

@@ -95,7 +95,9 @@ class ThreadPlanStack {
   /// generated.
   void ClearThreadCache();
 
-  bool IsTID(lldb::tid_t tid);
+  bool IsTID(lldb::tid_t tid) {
+    return GetTID() == tid;
+  }
   lldb::tid_t GetTID();
   void SetTID(lldb::tid_t tid);
 
@@ -115,6 +117,9 @@ class ThreadPlanStack {
                                           // completed plan checkpoints.
   std::unordered_map<size_t, PlanStack> m_completed_plan_store;
   mutable std::recursive_mutex m_stack_mutex;
+  
+  // ThreadPlanStacks shouldn't be copied.
+  ThreadPlanStack(ThreadPlanStack &rhs) = delete;
 };
 
 class ThreadPlanStackMap {
@@ -128,15 +133,34 @@ class ThreadPlanStackMap {
 
   void AddThread(Thread &thread) {
     lldb::tid_t tid = thread.GetID();
-    m_plans_list.emplace(tid, thread);
+    // If we already have a ThreadPlanStack for this thread, use it.
+    if (m_plans_list.find(tid) != m_plans_list.end())
+      return;
+
+    m_plans_up_container.emplace_back(
+        std::make_unique<ThreadPlanStack>(thread));
+    m_plans_list.emplace(tid, m_plans_up_container.back().get());
   }
 
   bool RemoveTID(lldb::tid_t tid) {
     auto result = m_plans_list.find(tid);
     if (result == m_plans_list.end())
       return false;
-    result->second.ThreadDestroyed(nullptr);
+    ThreadPlanStack *removed_stack = result->second;
     m_plans_list.erase(result);
+    // Now find it in the stack storage:
+    auto end = m_plans_up_container.end();
+    auto iter = std::find_if(m_plans_up_container.begin(), end,
+        [&] (std::unique_ptr<ThreadPlanStack> &stack) {
+          return stack->IsTID(tid);
+        });
+    if (iter == end)
+      return false;
+
+    // Then tell the stack its thread has been destroyed:
+    removed_stack->ThreadDestroyed(nullptr);
+    // And then remove it from the container so it goes away.
+    m_plans_up_container.erase(iter);
     return true;
   }
 
@@ -145,7 +169,7 @@ class ThreadPlanStackMap {
     if (result == m_plans_list.end())
       return nullptr;
     else
-      return &result->second;
+      return result->second;
   }
 
   /// Clear the Thread* cache that each ThreadPlan contains.
@@ -154,38 +178,51 @@ class ThreadPlanStackMap {
   /// generated.
   void ClearThreadCache() {
     for (auto &plan_list : m_plans_list)
-      plan_list.second.ClearThreadCache();
+      plan_list.second->ClearThreadCache();
   }
 
   // rename to Reactivate?
-  void Activate(ThreadPlanStack &&stack) {
+  void Activate(ThreadPlanStack &stack) {
+    // Remove this from the detached plan list:
+    auto end = m_detached_plans.end();    
+    auto iter = std::find_if(m_detached_plans.begin(), end, 
+        [&] (ThreadPlanStack *elem) {
+          return elem == &stack; });
+    if (iter != end)
+      m_detached_plans.erase(iter);
+
     if (m_plans_list.find(stack.GetTID()) == m_plans_list.end())
-      m_plans_list.emplace(stack.GetTID(), std::move(stack));
+      m_plans_list.emplace(stack.GetTID(), &stack);
     else
-      m_plans_list.at(stack.GetTID()) = std::move(stack);
+      m_plans_list.at(stack.GetTID()) = &stack;
   }
 
-  // rename to ...?
-  std::vector<ThreadPlanStack> CleanUp() {
+  void ScanForDetachedPlanStacks() {
     llvm::SmallVector<lldb::tid_t, 2> invalidated_tids;
     for (auto &pair : m_plans_list)
-      if (pair.second.GetTID() != pair.first)
+      if (pair.second->GetTID() != pair.first)
         invalidated_tids.push_back(pair.first);
 
-    std::vector<ThreadPlanStack> detached_stacks;
-    detached_stacks.reserve(invalidated_tids.size());
     for (auto tid : invalidated_tids) {
       auto it = m_plans_list.find(tid);
-      auto stack = std::move(it->second);
+      ThreadPlanStack *stack = it->second;
       m_plans_list.erase(it);
-      detached_stacks.emplace_back(std::move(stack));
+      m_detached_plans.push_back(stack);
     }
-    return detached_stacks;
   }
 
+  // This gets the vector of pointers to thread plans that aren't
+  // currently running on a thread.  This is generally for thread
+  // plans that represent asynchronous operations waiting to be
+  // scheduled.
+  // The vector will never have null ThreadPlanStacks in it.
+  std::vector<ThreadPlanStack *> &GetDetachedPlanStacks() {
+    return m_detached_plans;
+  }
+  
   void Clear() {
     for (auto &plan : m_plans_list)
-      plan.second.ThreadDestroyed(nullptr);
+      plan.second->ThreadDestroyed(nullptr);
     m_plans_list.clear();
   }
 
@@ -202,7 +239,23 @@ class ThreadPlanStackMap {
 
 private:
   Process &m_process;
-  using PlansList = std::unordered_map<lldb::tid_t, ThreadPlanStack>;
+  // We don't want to make copies of these ThreadPlanStacks, there needs to be
+  // just one of these tracking each piece of work.  But we need to move the
+  // work from "attached to a TID" state to "detached" state, which is most
+  // conveniently done by having organizing containers for each of the two 
+  // states.
+  // To make it easy to move these non-copyable entities in and out of the
+  // organizing containers, we make the ThreadPlanStacks into unique_ptr's in a 
+  // storage container - m_plans_up_container.  Storing unique_ptrs means we
+  // can then use the pointer to the ThreadPlanStack in the "organizing"
+  // containers, the TID->Stack map m_plans_list, and the detached plans
+  // vector m_detached_plans.
+  
+  using PlansStore = std::vector<std::unique_ptr<ThreadPlanStack>>;
+  PlansStore m_plans_up_container;
+  std::vector<ThreadPlanStack *> m_detached_plans;
+  
+  using PlansList = std::unordered_map<lldb::tid_t, ThreadPlanStack *>;
   PlansList m_plans_list;
 };
 

lldb/source/Target/Process.cpp

@@ -1286,25 +1286,23 @@ void Process::UpdateThreadListIfNeeded() {
 }
 
 void Process::SynchronizeThreadPlans() {
-  for (auto &stack : m_thread_plans.CleanUp())
-    m_async_thread_plans.emplace_back(std::move(stack));
+  m_thread_plans.ScanForDetachedPlanStacks();
 }
 
 ThreadPlanSP Process::FindDetachedPlanExplainingStop(Thread &thread,
                                                      Event *event_ptr) {
-  auto end = m_async_thread_plans.end();
-  for (auto it = m_async_thread_plans.begin(); it != end; ++it) {
-    auto plan_sp = it->GetCurrentPlan();
+  std::vector<ThreadPlanStack *> &detached_plans 
+      = m_thread_plans.GetDetachedPlanStacks();
+  size_t num_detached_plans = detached_plans.size();
+  for (size_t idx = 0; idx < num_detached_plans; idx++) {
+    ThreadPlanStack *cur_stack = detached_plans[idx];
+    ThreadPlanSP plan_sp = cur_stack->GetCurrentPlan();
     plan_sp->SetTID(thread.GetID());
     if (!plan_sp->DoPlanExplainsStop(event_ptr)) {
       plan_sp->ClearTID();
       continue;
     }
-
-    auto stack = std::move(*it);
-    m_async_thread_plans.erase(it);
-    stack.SetTID(plan_sp->GetTID());
-    m_thread_plans.Activate(std::move(stack));
+    m_thread_plans.Activate(*cur_stack);
     return plan_sp;
   }
   return {};

lldb/source/Target/ThreadPlanStack.cpp

@@ -452,8 +452,8 @@ void ThreadPlanStackMap::DumpPlans(Stream &strm,
       index_id = thread_sp->GetIndexID();
 
     if (condense_if_trivial) {
-      if (!elem.second.AnyPlans() && !elem.second.AnyCompletedPlans() &&
-          !elem.second.AnyDiscardedPlans()) {
+      if (!elem.second->AnyPlans() && !elem.second->AnyCompletedPlans() &&
+          !elem.second->AnyDiscardedPlans()) {
         strm.Printf("thread #%u: tid = 0x%4.4" PRIx64 "\n", index_id, tid);
         strm.IndentMore();
         strm.Indent();
@@ -466,7 +466,7 @@ void ThreadPlanStackMap::DumpPlans(Stream &strm,
     strm.Indent();
     strm.Printf("thread #%u: tid = 0x%4.4" PRIx64 ":\n", index_id, tid);
 
-    elem.second.DumpThreadPlans(strm, desc_level, internal);
+    elem.second->DumpThreadPlans(strm, desc_level, internal);
   }
 }