Add a mutex to protect ThreadPlanStack's from simultaneous access

lldb/include/lldb/Target/ThreadPlan.h

@@ -81,7 +81,7 @@ namespace lldb_private {
 //
 //  Cleaning up after your plans:
 //
-//  When the plan is moved from the plan stack its WillPop method is always
+//  When the plan is moved from the plan stack its DidPop method is always
 //  called, no matter why.  Once it is moved off the plan stack it is done, and
 //  won't get a chance to run again.  So you should undo anything that affects
 //  target state in this method.  But be sure to leave the plan able to
@@ -414,7 +414,7 @@ class ThreadPlan : public std::enable_shared_from_this<ThreadPlan>,
 
   virtual void DidPush();
 
-  virtual void WillPop();
+  virtual void DidPop();
 
   // This pushes a plan onto the plan stack of the current plan's thread.
   // Also sets the plans to private and not master plans.  A plan pushed by 

lldb/include/lldb/Target/ThreadPlanCallFunction.h

@@ -69,10 +69,10 @@ class ThreadPlanCallFunction : public ThreadPlan {
   // been cleaned up.
   lldb::addr_t GetFunctionStackPointer() { return m_function_sp; }
 
-  // Classes that derive from FunctionCaller, and implement their own WillPop
+  // Classes that derive from FunctionCaller, and implement their own DidPop
   // methods should call this so that the thread state gets restored if the
   // plan gets discarded.
-  void WillPop() override;
+  void DidPop() override;
 
   // If the thread plan stops mid-course, this will be the stop reason that
   // interrupted us. Once DoTakedown is called, this will be the real stop

lldb/include/lldb/Target/ThreadPlanCallUserExpression.h

@@ -32,7 +32,7 @@ class ThreadPlanCallUserExpression : public ThreadPlanCallFunction {
 
   void DidPush() override;
 
-  void WillPop() override;
+  void DidPop() override;
 
   lldb::StopInfoSP GetRealStopInfo() override;
 

lldb/include/lldb/Target/ThreadPlanStack.h

@@ -95,7 +95,9 @@ class ThreadPlanStack {
 
   void WillResume();
 
-  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);
 
@@ -116,6 +118,10 @@ class ThreadPlanStack {
   size_t m_completed_plan_checkpoint = 0; // Monotonically increasing token for
                                           // 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 {
@@ -129,15 +135,34 @@ class ThreadPlanStackMap {
 
   void AddThread(Thread &thread) {
     lldb::tid_t tid = thread.GetID();
-    m_plans_list.emplace(tid, thread);
+    std::shared_ptr<ThreadPlanStack> new_elem_sp(new ThreadPlanStack(thread));
+    m_plans_sp_container.push_back(new_elem_sp);
+    m_plans_list.emplace(tid, new_elem_sp.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);
+    // Also remove this from the stack storage:
+    PlansStore::iterator end 
+        = m_plans_sp_container.end();
+    PlansStore::iterator iter; 
+    for (iter = m_plans_sp_container.begin();
+         iter != end; iter++) {
+      if ((*iter)->IsTID(tid)) {
+        break;
+      } 
+    }
+    if (iter == end)
+      return false;
+
+    // Finally, 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_sp_container.erase(iter);
     return true;
   }
 
@@ -146,38 +171,49 @@ class ThreadPlanStackMap {
     if (result == m_plans_list.end())
       return nullptr;
     else
-      return &result->second;
+      return result->second;
   }
 
   // rename to Reactivate?
-  void Activate(ThreadPlanStack &&stack) {
+  void Activate(ThreadPlanStack &stack) {
+    // Remove this from the detached plan list:
+    std::vector<ThreadPlanStack *>::iterator end = m_detached_plans.end();    
+
+    for (std::vector<ThreadPlanStack *>::iterator iter 
+             = m_detached_plans.begin(); iter != end; iter++) {
+      if (*iter == &stack) {
+        m_detached_plans.erase(iter);
+        break;
+      }
+    }
+
     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;
+  }
+
+  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();
   }
 
@@ -194,7 +230,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 separate containers for the two states.
+  // To do that w/o having to spend effort fighting with C++'s desire to invoke
+  // copy constructors, we keep a store for the real object, and then keep
+  // pointers to the objects in the two containers.  The store actually holds
+  // shared_ptrs to the objects, so that the pointers in the containers will stay
+  // valid.
+
+  // Use a shared pointer rather than unique_ptr here to prevent C++ from trying
+  // to use a copy constructor on the stored type.
+  using PlansStore = std::vector<std::shared_ptr<ThreadPlanStack>>;
+  PlansStore m_plans_sp_container;
+  std::vector<ThreadPlanStack *> m_detached_plans;
+
+  using PlansList = std::unordered_map<lldb::tid_t, ThreadPlanStack *>;
   PlansList m_plans_list;
 };
 

lldb/include/lldb/Target/ThreadPlanStepOverBreakpoint.h

@@ -26,7 +26,7 @@ class ThreadPlanStepOverBreakpoint : public ThreadPlan {
   bool StopOthers() override;
   lldb::StateType GetPlanRunState() override;
   bool WillStop() override;
-  void WillPop() override;
+  void DidPop() override;
   bool MischiefManaged() override;
   void ThreadDestroyed() override;
   void SetAutoContinue(bool do_it);

lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntimeNames.cpp

@@ -319,7 +319,7 @@ class ThreadPlanStepInAsync : public ThreadPlan {
 
   bool StopOthers() override { return false; }
 
-  void WillPop() override {
+  void DidPop() override {
     if (m_async_breakpoint_sp)
       m_async_breakpoint_sp->GetTarget().RemoveBreakpointByID(
           m_async_breakpoint_sp->GetID());

lldb/source/Target/Process.cpp

@@ -1453,25 +1453,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/ThreadPlan.cpp

@@ -147,7 +147,7 @@ lldb::user_id_t ThreadPlan::GetNextID() {
 
 void ThreadPlan::DidPush() {}
 
-void ThreadPlan::WillPop() {}
+void ThreadPlan::DidPop() {}
 
 bool ThreadPlan::OkayToDiscard() {
   return IsMasterPlan() ? m_okay_to_discard : true;

lldb/source/Target/ThreadPlanCallFunction.cpp

@@ -215,7 +215,7 @@ void ThreadPlanCallFunction::DoTakedown(bool success) {
   }
 }
 
-void ThreadPlanCallFunction::WillPop() { DoTakedown(PlanSucceeded()); }
+void ThreadPlanCallFunction::DidPop() { DoTakedown(PlanSucceeded()); }
 
 void ThreadPlanCallFunction::GetDescription(Stream *s, DescriptionLevel level) {
   if (level == eDescriptionLevelBrief) {

lldb/source/Target/ThreadPlanCallUserExpression.cpp

@@ -59,8 +59,8 @@ void ThreadPlanCallUserExpression::DidPush() {
     m_user_expression_sp->WillStartExecuting();
 }
 
-void ThreadPlanCallUserExpression::WillPop() {
-  ThreadPlanCallFunction::WillPop();
+void ThreadPlanCallUserExpression::DidPop() {
+  ThreadPlanCallFunction::DidPop();
   if (m_user_expression_sp)
     m_user_expression_sp.reset();
 }