[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

145

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

146

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

145–147

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/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() {
   assert(m_plans.size() > 1 && "Can't pop the base thread plan");
 
-  lldb::ThreadPlanSP plan_sp = std::move(m_plans.back());
+  lldb::ThreadPlanSP plan_sp = m_plans.back();
+  m_plans.pop_back();
   m_completed_plans.push_back(plan_sp);
   plan_sp->WillPop();
-  m_plans.pop_back();
   return plan_sp;
 }
 
 lldb::ThreadPlanSP ThreadPlanStack::DiscardPlan() {
   assert(m_plans.size() > 1 && "Can't discard the base thread plan");
 
-  lldb::ThreadPlanSP plan_sp = std::move(m_plans.back());
+  lldb::ThreadPlanSP plan_sp = m_plans.back();
+  m_plans.pop_back();
   m_discarded_plans.push_back(plan_sp);
   plan_sp->WillPop();
-  m_plans.pop_back();
   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) {
   int stack_size = m_plans.size();