[lldb] Avoid moving ThreadPlanSP from plans vector

Change `ThreadPlanStack::PopPlan` and `::DiscardPlan` to not do the following:

1. Move the last plan, leaving a moved `ThreadPlanSP` in the plans vector
2. Operate on the last plan
3. Pop the last plan off the plans vector

This leaves a period of time where the last element in the plans vector has been moved. I am not sure what, if any, guarantees there are when doing this, but it seems like it would/could leave a null `ThreadPlanSP` in the container. There are asserts in place to prevent empty/null `ThreadPlanSP` instances from being pushed on to the stack, and so this could break that invariant during multithreaded access to the thread plan stack.

An open question is whether this use of `std::move` was the result of a measure performance problem.

Differential Revision: https://reviews.llvm.org/D106171

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
@@ -413,7 +413,7 @@ public:
 
   virtual void DidPush();
 
-  virtual void WillPop();
+  virtual void DidPop();
 
   ThreadPlanKind GetKind() const { return m_kind; }
 

lldb/include/lldb/Target/ThreadPlanCallFunction.h

@@ -68,10 +68,10 @@ public:
   // 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 @@ public:
 
   void DidPush() override;
 
-  void WillPop() override;
+  void DidPop() override;
 
   lldb::StopInfoSP GetRealStopInfo() override;
 

lldb/include/lldb/Target/ThreadPlanStepOverBreakpoint.h

@@ -26,7 +26,7 @@ public:
   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/Target/ThreadPlan.cpp

@@ -149,7 +149,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

@@ -209,7 +209,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();
 }

lldb/source/Target/ThreadPlanStack.cpp

@@ -150,10 +150,13 @@ lldb::ThreadPlanSP ThreadPlanStack::PopPlan() {
   std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
   assert(m_plans.size() > 1 && "Can't pop the base thread plan");
 
-  lldb::ThreadPlanSP plan_sp = std::move(m_plans.back());
-  m_completed_plans.push_back(plan_sp);
-  plan_sp->WillPop();
+  // Note that moving the top element of the vector would leave it in an
+  // undefined state, and break the guarantee that the stack's thread plans are
+  // all valid.
+  lldb::ThreadPlanSP plan_sp = m_plans.back();
   m_plans.pop_back();
+  m_completed_plans.push_back(plan_sp);
+  plan_sp->DidPop();
   return plan_sp;
 }
 
@@ -161,10 +164,13 @@ lldb::ThreadPlanSP ThreadPlanStack::DiscardPlan() {
   std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
   assert(m_plans.size() > 1 && "Can't discard the base thread plan");
 
-  lldb::ThreadPlanSP plan_sp = std::move(m_plans.back());
-  m_discarded_plans.push_back(plan_sp);
-  plan_sp->WillPop();
+  // Note that moving the top element of the vector would leave it in an
+  // undefined state, and break the guarantee that the stack's thread plans are
+  // all valid.
+  lldb::ThreadPlanSP plan_sp = m_plans.back();
   m_plans.pop_back();
+  m_discarded_plans.push_back(plan_sp);
+  plan_sp->DidPop();
   return plan_sp;
 }
 

lldb/source/Target/ThreadPlanStepOverBreakpoint.cpp

@@ -124,9 +124,7 @@ bool ThreadPlanStepOverBreakpoint::WillStop() {
   return true;
 }
 
-void ThreadPlanStepOverBreakpoint::WillPop() {
-  ReenableBreakpointSite();
-}
+void ThreadPlanStepOverBreakpoint::DidPop() { ReenableBreakpointSite(); }
 
 bool ThreadPlanStepOverBreakpoint::MischiefManaged() {
   lldb::addr_t pc_addr = GetThread().GetRegisterContext()->GetPC();