Add a mutex to guard access to the ThreadPlanStack class

[jingham]

Summary

   
We've seen reports of crashes (none we've been able to reproduce
locally) that look like they are caused by concurrent access to a
thread plan stack.  It looks like there are error paths when an
interrupt request to debugserver times out that cause this problem.

The thread plan stack access is never in a hot loop, and there
aren't enough of them for the extra data member to matter, so
there's really no good reason not to protect the access.

Adding the mutex revealed a couple of places where we were
using "auto" in an iteration when we should have been using
"auto &" - we didn't intend to copy the stack - and I fixed
those as well.

[kastiglione]

I've also opened D106171 as a potential fix. It could be that we merge both diffs, they aren't mutually exclusive.

lldb/include/lldb/Target/ThreadPlanStack.h

157

lldb/source/Target/ThreadPlanStack.cpp

417

432

[JDevlieghere]

lldb/include/lldb/Target/ThreadPlanStack.h

113

Does this actually have to be a recursive mutex?

[kastiglione]

I believe it does need to be a reclusive mutex.

[kastiglione]

lldb/include/lldb/Target/ThreadPlanStack.h

113

For example DiscardAllPlans calls DiscardPlan both of which declare a lock_guard.

[jingham]

In D106122#2884543, @kastiglione wrote:

I believe it does need to be a reclusive mutex.

Yes, I could refactor this to have a no-lock version, but I don't think it's worth complicating things.

[JDevlieghere]

Fair enough. LGTM!

lldb/include/lldb/Target/ThreadPlanStack.h

 //===-- ThreadPlanStack.h ---------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_TARGET_THREADPLANSTACK_H
 #define LLDB_TARGET_THREADPLANSTACK_H
 
 #include <mutex>
 #include <string>
 #include <unordered_map>
 #include <vector>
 
 #include "lldb/Target/Target.h"
 #include "lldb/Target/Thread.h"
 #include "lldb/lldb-private-forward.h"
 #include "lldb/lldb-private.h"
 
 namespace lldb_private {
 
 // The ThreadPlans have a thread for use when they are asked all the ThreadPlan
 // state machine questions, but they should never cache any pointers from their
PlanStack m_completed_plans; ///< Plans that have been completed by this
                                /// stop.  They get deleted when the thread
                                /// resumes.
   PlanStack m_discarded_plans; ///< Plans that have been discarded by this
                                /// stop.  They get deleted when the thread
                                /// resumes.
   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;
 };
 
 class ThreadPlanStackMap {
 public:
   ThreadPlanStackMap(Process &process) : m_process(process) {}
   ~ThreadPlanStackMap() = default;
 
   // Prune the map using the current_threads list.
public:
   /// This is useful in situations like when a new Thread list is being
   /// generated.
   void ClearThreadCache() {
     for (auto &plan_list : m_plans_list)
       plan_list.second.ClearThreadCache();
   }
 
   void Clear() {
-    for (auto plan : m_plans_list)
+    for (auto &plan : m_plans_list)
       plan.second.ThreadDestroyed(nullptr);
     m_plans_list.clear();
   }
 
   // Implements Process::DumpThreadPlans
   void DumpPlans(Stream &strm, lldb::DescriptionLevel desc_level, bool internal,
                  bool ignore_boring, bool skip_unreported);
 

lldb/source/Target/ThreadPlanStack.cpp

 //===-- ThreadPlanStack.cpp -------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Target/ThreadPlanStack.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/Thread.h"
 #include "lldb/Target/ThreadPlan.h"
 #include "lldb/Utility/Log.h"
 
 using namespace lldb;
 using namespace lldb_private;
if (make_null) {
     m_plans.push_back(
         ThreadPlanSP(new ThreadPlanNull(const_cast<Thread &>(thread))));
   }
 }
 
 void ThreadPlanStack::DumpThreadPlans(Stream &s,
                                       lldb::DescriptionLevel desc_level,
                                       bool include_internal) const {
+  std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
   s.IndentMore();
   PrintOneStack(s, "Active plan stack", m_plans, desc_level, include_internal);
   PrintOneStack(s, "Completed plan stack", m_completed_plans, desc_level,
                 include_internal);
   PrintOneStack(s, "Discarded plan stack", m_discarded_plans, desc_level,
                 include_internal);
   s.IndentLess();
 }
 
 void ThreadPlanStack::PrintOneStack(Stream &s, llvm::StringRef stack_name,
                                     const PlanStack &stack,
                                     lldb::DescriptionLevel desc_level,
                                     bool include_internal) const {
+  std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
   // If the stack is empty, just exit:
   if (stack.empty())
     return;
 
   // Make sure there are public completed plans:
   bool any_public = false;
   if (!include_internal) {
     for (auto plan : stack) {
for (auto plan : stack) {
       if (!include_internal && plan->GetPrivate())
         continue;
       PrintPlanElement(s, plan, desc_level, print_idx++);
     }
   }
 }
 
 size_t ThreadPlanStack::CheckpointCompletedPlans() {
+  std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
   m_completed_plan_checkpoint++;
   m_completed_plan_store.insert(
       std::make_pair(m_completed_plan_checkpoint, m_completed_plans));
   return m_completed_plan_checkpoint;
 }
 
 void ThreadPlanStack::RestoreCompletedPlanCheckpoint(size_t checkpoint) {
+  std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
   auto result = m_completed_plan_store.find(checkpoint);
   assert(result != m_completed_plan_store.end() &&
          "Asked for a checkpoint that didn't exist");
   m_completed_plans.swap((*result).second);
   m_completed_plan_store.erase(result);
 }
 
 void ThreadPlanStack::DiscardCompletedPlanCheckpoint(size_t checkpoint) {
+  std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
   m_completed_plan_store.erase(checkpoint);
 }
 
 void ThreadPlanStack::ThreadDestroyed(Thread *thread) {
   // Tell the plan stacks that this thread is going away:
+  std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
   for (ThreadPlanSP plan : m_plans)
     plan->ThreadDestroyed();
 
   for (ThreadPlanSP plan : m_discarded_plans)
     plan->ThreadDestroyed();
 
   for (ThreadPlanSP plan : m_completed_plans)
     plan->ThreadDestroyed();
if (thread != nullptr) {
     m_plans.push_back(null_plan_sp);
   }
 }
 
 void ThreadPlanStack::PushPlan(lldb::ThreadPlanSP new_plan_sp) {
   // If the thread plan doesn't already have a tracer, give it its parent's
   // tracer:
   // The first plan has to be a base plan:
+  std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
   assert((m_plans.size() > 0 || new_plan_sp->IsBasePlan()) &&
          "Zeroth plan must be a base plan");
 
   if (!new_plan_sp->GetThreadPlanTracer()) {
     assert(!m_plans.empty());
     new_plan_sp->SetThreadPlanTracer(m_plans.back()->GetThreadPlanTracer());
   }
   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();
   m_plans.pop_back();
   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();
   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) {
+  std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
   int stack_size = m_plans.size();
 
   if (up_to_plan_ptr == nullptr) {
     for (int i = stack_size - 1; i > 0; i--)
       DiscardPlan();
     return;
   }
 
for (int i = stack_size - 1; i > 0 && !last_one; i--) {
       if (GetCurrentPlan().get() == up_to_plan_ptr)
         last_one = true;
       DiscardPlan();
     }
   }
 }
 
 void ThreadPlanStack::DiscardAllPlans() {
+  std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
   int stack_size = m_plans.size();
   for (int i = stack_size - 1; i > 0; i--) {
     DiscardPlan();
   }
   return;
 }
 
 void ThreadPlanStack::DiscardConsultingMasterPlans() {
+  std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
   while (true) {
     int master_plan_idx;
     bool discard = true;
 
     // Find the first master plan, see if it wants discarding, and if yes
     // discard up to it.
     for (master_plan_idx = m_plans.size() - 1; master_plan_idx >= 0;
          master_plan_idx--) {
while (true) {
     // means discard it's dependent plans, but not it...
     if (master_plan_idx > 0) {
       DiscardPlan();
     }
   }
 }
 
 lldb::ThreadPlanSP ThreadPlanStack::GetCurrentPlan() const {
+  std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
   assert(m_plans.size() != 0 && "There will always be a base plan.");
   return m_plans.back();
 }
 
 lldb::ThreadPlanSP ThreadPlanStack::GetCompletedPlan(bool skip_private) const {
+  std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
   if (m_completed_plans.empty())
     return {};
 
   if (!skip_private)
     return m_completed_plans.back();
 
   for (int i = m_completed_plans.size() - 1; i >= 0; i--) {
     lldb::ThreadPlanSP completed_plan_sp;
     completed_plan_sp = m_completed_plans[i];
     if (!completed_plan_sp->GetPrivate())
       return completed_plan_sp;
   }
   return {};
 }
 
 lldb::ThreadPlanSP ThreadPlanStack::GetPlanByIndex(uint32_t plan_idx,
                                                    bool skip_private) const {
+  std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
   uint32_t idx = 0;
 
   for (lldb::ThreadPlanSP plan_sp : m_plans) {
     if (skip_private && plan_sp->GetPrivate())
       continue;
     if (idx == plan_idx)
       return plan_sp;
     idx++;
   }
   return {};
 }
 
 lldb::ValueObjectSP ThreadPlanStack::GetReturnValueObject() const {
+  std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
   if (m_completed_plans.empty())
     return {};
 
   for (int i = m_completed_plans.size() - 1; i >= 0; i--) {
     lldb::ValueObjectSP return_valobj_sp;
     return_valobj_sp = m_completed_plans[i]->GetReturnValueObject();
     if (return_valobj_sp)
       return return_valobj_sp;
   }
   return {};
 }
 
 lldb::ExpressionVariableSP ThreadPlanStack::GetExpressionVariable() const {
+  std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
   if (m_completed_plans.empty())
     return {};
 
   for (int i = m_completed_plans.size() - 1; i >= 0; i--) {
     lldb::ExpressionVariableSP expression_variable_sp;
     expression_variable_sp = m_completed_plans[i]->GetExpressionVariable();
     if (expression_variable_sp)
       return expression_variable_sp;
   }
   return {};
 }
 bool ThreadPlanStack::AnyPlans() const {
+  std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
   // There is always a base plan...
   return m_plans.size() > 1;
 }
 
 bool ThreadPlanStack::AnyCompletedPlans() const {
+  std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
   return !m_completed_plans.empty();
 }
 
 bool ThreadPlanStack::AnyDiscardedPlans() const {
+  std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
   return !m_discarded_plans.empty();
 }
 
 bool ThreadPlanStack::IsPlanDone(ThreadPlan *in_plan) const {
+  std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
   for (auto plan : m_completed_plans) {
     if (plan.get() == in_plan)
       return true;
   }
   return false;
 }
 
 bool ThreadPlanStack::WasPlanDiscarded(ThreadPlan *in_plan) const {
+  std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
   for (auto plan : m_discarded_plans) {
     if (plan.get() == in_plan)
       return true;
   }
   return false;
 }
 
 ThreadPlan *ThreadPlanStack::GetPreviousPlan(ThreadPlan *current_plan) const {
+  std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
   if (current_plan == nullptr)
     return nullptr;
 
   // Look first in the completed plans, if the plan is here and there is
   // a completed plan above it, return that.
   int stack_size = m_completed_plans.size();
   for (int i = stack_size - 1; i > 0; i--) {
     if (current_plan == m_completed_plans[i].get())
ThreadPlan *ThreadPlanStack::GetPreviousPlan(ThreadPlan *current_plan) const {
   for (int i = stack_size - 1; i > 0; i--) {
     if (current_plan == m_plans[i].get())
       return m_plans[i - 1].get();
   }
   return nullptr;
 }
 
 ThreadPlan *ThreadPlanStack::GetInnermostExpression() const {
+  std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
   int stack_size = m_plans.size();
 
   for (int i = stack_size - 1; i > 0; i--) {
     if (m_plans[i]->GetKind() == ThreadPlan::eKindCallFunction)
       return m_plans[i].get();
   }
   return nullptr;
 }
 
 void ThreadPlanStack::ClearThreadCache() {
+  std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
   for (lldb::ThreadPlanSP thread_plan_sp : m_plans)
     thread_plan_sp->ClearThreadCache();
 }
 
 void ThreadPlanStack::WillResume() {
+  std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
   m_completed_plans.clear();
   m_discarded_plans.clear();
 }
 
 void ThreadPlanStackMap::Update(ThreadList &current_threads,
                                 bool delete_missing,
                                 bool check_for_new) {
 
void ThreadPlanStackMap::Update(ThreadList &current_threads,
   // If we aren't reaping missing threads at this point,
   // we are done.
   if (!delete_missing)
     return;
   // Otherwise scan for absent TID's.
   std::vector<lldb::tid_t> missing_threads;
   // If we are going to delete plans from the plan stack,
   // then scan for absent TID's:
-  for (auto thread_plans : m_plans_list) {
+  for (auto &thread_plans : m_plans_list) {
     lldb::tid_t cur_tid = thread_plans.first;
     ThreadSP thread_sp = current_threads.FindThreadByID(cur_tid);
     if (!thread_sp)
       missing_threads.push_back(cur_tid);
   }
   for (lldb::tid_t tid : missing_threads) {
     RemoveTID(tid);
   }
 }
 
 void ThreadPlanStackMap::DumpPlans(Stream &strm,
                                    lldb::DescriptionLevel desc_level,
                                    bool internal, bool condense_if_trivial,
                                    bool skip_unreported) {
-  for (auto elem : m_plans_list) {
+  for (auto &elem : m_plans_list) {
     lldb::tid_t tid = elem.first;
     uint32_t index_id = 0;
     ThreadSP thread_sp = m_process.GetThreadList().FindThreadByID(tid);
 
     if (skip_unreported) {
       if (!thread_sp)
         continue;
     }