[Lldb-commits] [PATCH] D106171: [lldb] Avoid moving ThreadPlanSP from plans vector

[Dave Lee via Phabricator via lldb-commits]

kastiglione created this revision.
kastiglione added reviewers: jingham, aprantl.
kastiglione requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106171

Files:
  lldb/source/Target/ThreadPlanStack.cpp


Index: lldb/source/Target/ThreadPlanStack.cpp
===================================================================
--- lldb/source/Target/ThreadPlanStack.cpp
+++ lldb/source/Target/ThreadPlanStack.cpp
@@ -142,7 +142,7 @@
 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_completed_plans.push_back(plan_sp);
   plan_sp->WillPop();
   m_plans.pop_back();
@@ -152,7 +152,7 @@
 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_discarded_plans.push_back(plan_sp);
   plan_sp->WillPop();
   m_plans.pop_back();


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D106171.359396.patch
Type: text/x-patch
Size: 872 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20210716/3e6f5265/attachment.bin>