[lldb] Avoid moving ThreadPlanSP from plans vector

[kastiglione]

Summary

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

1. Move the last plan, leaving a moved ThreadPlanSP in the plans vector
2. Call out to WillPop which can't guarantee that m_plans isn't accessed in its invalid state
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, leaving a null ThreadPlanSP in the container. There are asserts in place to prevent null ThreadPlanSP instances from being pushed on to the stack, and so this could break that invariant.

From discussion, it seems the use of std::move wasn't the result of a measured performance problem.

[kastiglione]

Also call pop_back just after retrieving back.

[aprantl]

lldb/source/Target/ThreadPlanStack.cpp

152–153

Should we comment that we are intentionally making a copy of the SP to avoid a race condition with std::move()?

153

It's counterintuitive that WillPop() is being called *after* the value is popped now.

[kastiglione]

Rename WillPop to DidPop, and comment on m_plans invariant.

[kastiglione]

@aprantl I renamed WillPop to accurately reflect that it's DidPop. I also added a comment about the invariant that is broken when std::move is used on elements of m_plans.

[kastiglione]

Re-simplify following discussion with Adrian and Jim.

[JDevlieghere]

IIRC the fact that the shared pointer is null is implementation defined and the standard just says that a moved-from object is in an undefined state.

lldb/source/Target/ThreadPlanStack.cpp

152–153

Note that moving the top element of the vector leaves it in an undefined state, which breaks the thread plan stack guarantee that the last element is valid.

[kastiglione]

In D106171#2910693, @JDevlieghere wrote:

IIRC the fact that the shared pointer is null is implementation defined and the standard just says that a moved-from object is in an undefined state.

According to cppreference, the move constructor says:

After the construction, *this contains a copy of the previous state of r, r is empty and its stored pointer is null.

[kastiglione]

• Reword comment using Jonas's suggestion
• Pop the last plan right away, and thus change from WillPop to DidPop

[jingham]

LGTM

[JDevlieghere]

Thanks!

lldb/include/lldb/Target/ThreadPlan.h

 //  Your plan may also get discarded, i.e. moved from the plan stack to the
 //  "discarded plan stack".  This can happen, for instance, if the plan is
 //  calling a function and the function call crashes and you want to unwind the
 //  attempt to call.  So don't assume that your plan will always successfully
 //  stop.  Which leads to:
 //
 //  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
 //  correctly fill the StopInfo, however.  N.B. Don't wait to do clean up
 //  target state till the destructor, since that will usually get called when
 //  the target resumes, and you want to leave the target state correct for new
 //  plans in the time between when your plan gets unshipped and the next
 //  resume.
public:
   }
 
   bool GetPrivate() { return m_plan_private; }
 
   void SetPrivate(bool input) { m_plan_private = input; }
 
   virtual void DidPush();
 
-  virtual void WillPop();
+  virtual void DidPop();
 
   ThreadPlanKind GetKind() const { return m_kind; }
 
   bool IsPlanComplete();
 
   void SetPlanComplete(bool success = true);
 
   virtual bool IsPlanStale() { return false; }

lldb/include/lldb/Target/ThreadPlanCallFunction.h

lldb::ValueObjectSP GetReturnValueObject() override {
     return m_return_valobj_sp;
   }
 
   // Return the stack pointer that the function received on entry.  Any stack
   // address below this should be considered invalid after the function has
   // 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
   // reason at the end of the function call. If it hasn't been set for one or
   // the other of these reasons, we'll return the PrivateStopReason. This is
   // needed because we want the CallFunction thread plans not to show up as the
   // stop reason. But if something bad goes wrong, it is nice to be able to
   // tell the user what really happened.

lldb/include/lldb/Target/ThreadPlanCallUserExpression.h

ThreadPlanCallUserExpression(Thread &thread, Address &function,
                                lldb::UserExpressionSP &user_expression_sp);
 
   ~ThreadPlanCallUserExpression() override;
 
   void GetDescription(Stream *s, lldb::DescriptionLevel level) override;
 
   void DidPush() override;
 
-  void WillPop() override;
+  void DidPop() override;
 
   lldb::StopInfoSP GetRealStopInfo() override;
 
   bool MischiefManaged() override;
 
   void TransferExpressionOwnership() { m_manage_materialization = true; }
 
   lldb::ExpressionVariableSP GetExpressionVariable() override {

lldb/include/lldb/Target/ThreadPlanStepOverBreakpoint.h

public:
   ~ThreadPlanStepOverBreakpoint() override;
 
   void GetDescription(Stream *s, lldb::DescriptionLevel level) override;
   bool ValidatePlan(Stream *error) override;
   bool ShouldStop(Event *event_ptr) override;
   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);
   bool ShouldAutoContinue(Event *event_ptr) override;
   bool IsPlanStale() override;
 
   lldb::addr_t GetBreakpointLoadAddress() const { return m_breakpoint_addr; }
 

lldb/source/Target/ThreadPlan.cpp

 
 lldb::user_id_t ThreadPlan::GetNextID() {
   static uint32_t g_nextPlanID = 0;
   return ++g_nextPlanID;
 }
 
 void ThreadPlan::DidPush() {}
 
-void ThreadPlan::WillPop() {}
+void ThreadPlan::DidPop() {}
 
 bool ThreadPlan::OkayToDiscard() {
   return IsMasterPlan() ? m_okay_to_discard : true;
 }
 
 lldb::StateType ThreadPlan::RunState() {
   if (m_tracer_sp && m_tracer_sp->TracingEnabled())
     return eStateStepping;

lldb/source/Target/ThreadPlanCallFunction.cpp

void ThreadPlanCallFunction::DoTakedown(bool success) {
   } else {
     LLDB_LOGF(log,
               "ThreadPlanCallFunction(%p): DoTakedown called as no-op for "
               "thread 0x%4.4" PRIx64 ", m_valid: %d complete: %d.\n",
               static_cast<void *>(this), m_tid, m_valid, IsPlanComplete());
   }
 }
 
-void ThreadPlanCallFunction::WillPop() { DoTakedown(PlanSucceeded()); }
+void ThreadPlanCallFunction::DidPop() { DoTakedown(PlanSucceeded()); }
 
 void ThreadPlanCallFunction::GetDescription(Stream *s, DescriptionLevel level) {
   if (level == eDescriptionLevelBrief) {
     s->Printf("Function call thread plan");
   } else {
     s->Printf("Thread plan to call 0x%" PRIx64,
               m_function_addr.GetLoadAddress(&GetTarget()));
   }

lldb/source/Target/ThreadPlanCallUserExpression.cpp

 }
 
 void ThreadPlanCallUserExpression::DidPush() {
   ThreadPlanCallFunction::DidPush();
   if (m_user_expression_sp)
     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();
 }
 
 bool ThreadPlanCallUserExpression::MischiefManaged() {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_STEP));
 
   if (IsPlanComplete()) {

lldb/source/Target/ThreadPlanStack.cpp

void ThreadPlanStack::PushPlan(lldb::ThreadPlanSP new_plan_sp) {
   }
   m_plans.push_back(new_plan_sp);
   new_plan_sp->DidPush();
 }
 
 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;
 }
 
 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;
 }
 
 // If the input plan is nullptr, discard all plans.  Otherwise make sure this
 // plan is in the stack, and if so discard up to and including it.
 void ThreadPlanStack::DiscardPlansUpToPlan(ThreadPlan *up_to_plan_ptr) {
   std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
   int stack_size = m_plans.size();

lldb/source/Target/ThreadPlanStepOverBreakpoint.cpp

bool ThreadPlanStepOverBreakpoint::DoWillResume(StateType resume_state,
   return true;
 }
 
 bool ThreadPlanStepOverBreakpoint::WillStop() {
   ReenableBreakpointSite();
   return true;
 }
 
-void ThreadPlanStepOverBreakpoint::WillPop() {
-  ReenableBreakpointSite();
-}
+void ThreadPlanStepOverBreakpoint::DidPop() { ReenableBreakpointSite(); }
 
 bool ThreadPlanStepOverBreakpoint::MischiefManaged() {
   lldb::addr_t pc_addr = GetThread().GetRegisterContext()->GetPC();
 
   if (pc_addr == m_breakpoint_addr) {
     // If we are still at the PC of our breakpoint, then for some reason we
     // didn't get a chance to run.
     return false;