Skip to content

Commit f0f0abd

Browse files
committed
Convert ThreadPlanStack's mutex to a shared mutex. (llvm#116438)
I have some reports of A/B inversion deadlocks between the ThreadPlanStack and the StackFrameList accesses. There's a fair bit of reasonable code in lldb that does "While accessing the ThreadPlanStack, look at that threads's StackFrameList", and also plenty of "While accessing the ThreadPlanStack, look at the StackFrameList." In all the cases I've seen so far, there was at most one of the locks taken that were trying to mutate the list, the other three were just reading. So we could solve the deadlock by converting the two mutexes over to shared mutexes. This patch is the easy part, the ThreadPlanStack mutex. The tricky part was because these were originally recursive mutexes, and recursive access to shared mutexes is undefined behavior according to the C++ standard, I had to add a couple NoLock variants to make sure it didn't get used recursively. Then since the only remaining calls are out to ThreadPlans and ThreadPlans don't have access to their containing ThreadPlanStack, converting this to a non-recursive lock should be safe. (cherry picked from commit b42a816)
1 parent d64c6cf commit f0f0abd

File tree

2 files changed

+69
-54
lines changed

2 files changed

+69
-54
lines changed

lldb/include/lldb/Target/ThreadPlanStack.h

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414
#include <unordered_map>
1515
#include <vector>
1616

17+
#include "llvm/Support/RWMutex.h"
18+
1719
#include "lldb/Target/Target.h"
1820
#include "lldb/Target/Thread.h"
1921
#include "lldb/lldb-private-forward.h"
@@ -102,9 +104,12 @@ class ThreadPlanStack {
102104
void SetTID(lldb::tid_t tid);
103105

104106
private:
105-
void PrintOneStack(Stream &s, llvm::StringRef stack_name,
106-
const PlanStack &stack, lldb::DescriptionLevel desc_level,
107-
bool include_internal) const;
107+
lldb::ThreadPlanSP DiscardPlanNoLock();
108+
lldb::ThreadPlanSP GetCurrentPlanNoLock() const;
109+
void PrintOneStackNoLock(Stream &s, llvm::StringRef stack_name,
110+
const PlanStack &stack,
111+
lldb::DescriptionLevel desc_level,
112+
bool include_internal) const;
108113

109114
PlanStack m_plans; ///< The stack of plans this thread is executing.
110115
PlanStack m_completed_plans; ///< Plans that have been completed by this
@@ -116,8 +121,8 @@ class ThreadPlanStack {
116121
size_t m_completed_plan_checkpoint = 0; // Monotonically increasing token for
117122
// completed plan checkpoints.
118123
std::unordered_map<size_t, PlanStack> m_completed_plan_store;
119-
mutable std::recursive_mutex m_stack_mutex;
120-
124+
mutable llvm::sys::RWMutex m_stack_mutex;
125+
121126
// ThreadPlanStacks shouldn't be copied.
122127
ThreadPlanStack(ThreadPlanStack &rhs) = delete;
123128
};

lldb/source/Target/ThreadPlanStack.cpp

Lines changed: 59 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -39,21 +39,21 @@ ThreadPlanStack::ThreadPlanStack(const Thread &thread, bool make_null) {
3939
void ThreadPlanStack::DumpThreadPlans(Stream &s,
4040
lldb::DescriptionLevel desc_level,
4141
bool include_internal) const {
42-
std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
42+
llvm::sys::ScopedReader guard(m_stack_mutex);
4343
s.IndentMore();
44-
PrintOneStack(s, "Active plan stack", m_plans, desc_level, include_internal);
45-
PrintOneStack(s, "Completed plan stack", m_completed_plans, desc_level,
46-
include_internal);
47-
PrintOneStack(s, "Discarded plan stack", m_discarded_plans, desc_level,
48-
include_internal);
44+
PrintOneStackNoLock(s, "Active plan stack", m_plans, desc_level,
45+
include_internal);
46+
PrintOneStackNoLock(s, "Completed plan stack", m_completed_plans, desc_level,
47+
include_internal);
48+
PrintOneStackNoLock(s, "Discarded plan stack", m_discarded_plans, desc_level,
49+
include_internal);
4950
s.IndentLess();
5051
}
5152

52-
void ThreadPlanStack::PrintOneStack(Stream &s, llvm::StringRef stack_name,
53-
const PlanStack &stack,
54-
lldb::DescriptionLevel desc_level,
55-
bool include_internal) const {
56-
std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
53+
void ThreadPlanStack::PrintOneStackNoLock(Stream &s, llvm::StringRef stack_name,
54+
const PlanStack &stack,
55+
lldb::DescriptionLevel desc_level,
56+
bool include_internal) const {
5757
// If the stack is empty, just exit:
5858
if (stack.empty())
5959
return;
@@ -82,15 +82,15 @@ void ThreadPlanStack::PrintOneStack(Stream &s, llvm::StringRef stack_name,
8282
}
8383

8484
size_t ThreadPlanStack::CheckpointCompletedPlans() {
85-
std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
85+
llvm::sys::ScopedWriter guard(m_stack_mutex);
8686
m_completed_plan_checkpoint++;
8787
m_completed_plan_store.insert(
8888
std::make_pair(m_completed_plan_checkpoint, m_completed_plans));
8989
return m_completed_plan_checkpoint;
9090
}
9191

9292
void ThreadPlanStack::RestoreCompletedPlanCheckpoint(size_t checkpoint) {
93-
std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
93+
llvm::sys::ScopedWriter guard(m_stack_mutex);
9494
auto result = m_completed_plan_store.find(checkpoint);
9595
assert(result != m_completed_plan_store.end() &&
9696
"Asked for a checkpoint that didn't exist");
@@ -99,13 +99,13 @@ void ThreadPlanStack::RestoreCompletedPlanCheckpoint(size_t checkpoint) {
9999
}
100100

101101
void ThreadPlanStack::DiscardCompletedPlanCheckpoint(size_t checkpoint) {
102-
std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
102+
llvm::sys::ScopedWriter guard(m_stack_mutex);
103103
m_completed_plan_store.erase(checkpoint);
104104
}
105105

106106
void ThreadPlanStack::ThreadDestroyed(Thread *thread) {
107107
// Tell the plan stacks that this thread is going away:
108-
std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
108+
llvm::sys::ScopedWriter guard(m_stack_mutex);
109109
for (ThreadPlanSP plan : m_plans)
110110
plan->ThreadDestroyed();
111111

@@ -134,20 +134,22 @@ void ThreadPlanStack::PushPlan(lldb::ThreadPlanSP new_plan_sp) {
134134
// If the thread plan doesn't already have a tracer, give it its parent's
135135
// tracer:
136136
// The first plan has to be a base plan:
137-
std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
138-
assert((m_plans.size() > 0 || new_plan_sp->IsBasePlan()) &&
139-
"Zeroth plan must be a base plan");
140-
141-
if (!new_plan_sp->GetThreadPlanTracer()) {
142-
assert(!m_plans.empty());
143-
new_plan_sp->SetThreadPlanTracer(m_plans.back()->GetThreadPlanTracer());
137+
{ // Scope for Lock - DidPush often adds plans to the stack:
138+
llvm::sys::ScopedWriter guard(m_stack_mutex);
139+
assert((m_plans.size() > 0 || new_plan_sp->IsBasePlan()) &&
140+
"Zeroth plan must be a base plan");
141+
142+
if (!new_plan_sp->GetThreadPlanTracer()) {
143+
assert(!m_plans.empty());
144+
new_plan_sp->SetThreadPlanTracer(m_plans.back()->GetThreadPlanTracer());
145+
}
146+
m_plans.push_back(new_plan_sp);
144147
}
145-
m_plans.push_back(new_plan_sp);
146148
new_plan_sp->DidPush();
147149
}
148150

149151
lldb::ThreadPlanSP ThreadPlanStack::PopPlan() {
150-
std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
152+
llvm::sys::ScopedWriter guard(m_stack_mutex);
151153
assert(m_plans.size() > 1 && "Can't pop the base thread plan");
152154

153155
// Note that moving the top element of the vector would leave it in an
@@ -161,7 +163,11 @@ lldb::ThreadPlanSP ThreadPlanStack::PopPlan() {
161163
}
162164

163165
lldb::ThreadPlanSP ThreadPlanStack::DiscardPlan() {
164-
std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
166+
llvm::sys::ScopedWriter guard(m_stack_mutex);
167+
return DiscardPlanNoLock();
168+
}
169+
170+
lldb::ThreadPlanSP ThreadPlanStack::DiscardPlanNoLock() {
165171
assert(m_plans.size() > 1 && "Can't discard the base thread plan");
166172

167173
// Note that moving the top element of the vector would leave it in an
@@ -177,12 +183,12 @@ lldb::ThreadPlanSP ThreadPlanStack::DiscardPlan() {
177183
// If the input plan is nullptr, discard all plans. Otherwise make sure this
178184
// plan is in the stack, and if so discard up to and including it.
179185
void ThreadPlanStack::DiscardPlansUpToPlan(ThreadPlan *up_to_plan_ptr) {
180-
std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
186+
llvm::sys::ScopedWriter guard(m_stack_mutex);
181187
int stack_size = m_plans.size();
182188

183189
if (up_to_plan_ptr == nullptr) {
184190
for (int i = stack_size - 1; i > 0; i--)
185-
DiscardPlan();
191+
DiscardPlanNoLock();
186192
return;
187193
}
188194

@@ -197,23 +203,23 @@ void ThreadPlanStack::DiscardPlansUpToPlan(ThreadPlan *up_to_plan_ptr) {
197203
if (found_it) {
198204
bool last_one = false;
199205
for (int i = stack_size - 1; i > 0 && !last_one; i--) {
200-
if (GetCurrentPlan().get() == up_to_plan_ptr)
206+
if (GetCurrentPlanNoLock().get() == up_to_plan_ptr)
201207
last_one = true;
202-
DiscardPlan();
208+
DiscardPlanNoLock();
203209
}
204210
}
205211
}
206212

207213
void ThreadPlanStack::DiscardAllPlans() {
208-
std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
214+
llvm::sys::ScopedWriter guard(m_stack_mutex);
209215
int stack_size = m_plans.size();
210216
for (int i = stack_size - 1; i > 0; i--) {
211-
DiscardPlan();
217+
DiscardPlanNoLock();
212218
}
213219
}
214220

215221
void ThreadPlanStack::DiscardConsultingControllingPlans() {
216-
std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
222+
llvm::sys::ScopedWriter guard(m_stack_mutex);
217223
while (true) {
218224
int controlling_plan_idx;
219225
bool discard = true;
@@ -234,26 +240,30 @@ void ThreadPlanStack::DiscardConsultingControllingPlans() {
234240

235241
// First pop all the dependent plans:
236242
for (int i = m_plans.size() - 1; i > controlling_plan_idx; i--) {
237-
DiscardPlan();
243+
DiscardPlanNoLock();
238244
}
239245

240246
// Now discard the controlling plan itself.
241247
// The bottom-most plan never gets discarded. "OkayToDiscard" for it
242248
// means discard it's dependent plans, but not it...
243249
if (controlling_plan_idx > 0) {
244-
DiscardPlan();
250+
DiscardPlanNoLock();
245251
}
246252
}
247253
}
248254

249255
lldb::ThreadPlanSP ThreadPlanStack::GetCurrentPlan() const {
250-
std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
256+
llvm::sys::ScopedReader guard(m_stack_mutex);
257+
return GetCurrentPlanNoLock();
258+
}
259+
260+
lldb::ThreadPlanSP ThreadPlanStack::GetCurrentPlanNoLock() const {
251261
assert(m_plans.size() != 0 && "There will always be a base plan.");
252262
return m_plans.back();
253263
}
254264

255265
lldb::ThreadPlanSP ThreadPlanStack::GetCompletedPlan(bool skip_private) const {
256-
std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
266+
llvm::sys::ScopedReader guard(m_stack_mutex);
257267
if (m_completed_plans.empty())
258268
return {};
259269

@@ -271,7 +281,7 @@ lldb::ThreadPlanSP ThreadPlanStack::GetCompletedPlan(bool skip_private) const {
271281

272282
lldb::ThreadPlanSP ThreadPlanStack::GetPlanByIndex(uint32_t plan_idx,
273283
bool skip_private) const {
274-
std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
284+
llvm::sys::ScopedReader guard(m_stack_mutex);
275285
uint32_t idx = 0;
276286

277287
for (lldb::ThreadPlanSP plan_sp : m_plans) {
@@ -286,7 +296,7 @@ lldb::ThreadPlanSP ThreadPlanStack::GetPlanByIndex(uint32_t plan_idx,
286296

287297
lldb::ValueObjectSP
288298
ThreadPlanStack::GetReturnValueObject(bool &is_error) const {
289-
std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
299+
llvm::sys::ScopedReader guard(m_stack_mutex);
290300
if (m_completed_plans.empty())
291301
return {};
292302

@@ -302,7 +312,7 @@ ThreadPlanStack::GetReturnValueObject(bool &is_error) const {
302312
}
303313

304314
lldb::ExpressionVariableSP ThreadPlanStack::GetExpressionVariable() const {
305-
std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
315+
llvm::sys::ScopedReader guard(m_stack_mutex);
306316
if (m_completed_plans.empty())
307317
return {};
308318

@@ -315,23 +325,23 @@ lldb::ExpressionVariableSP ThreadPlanStack::GetExpressionVariable() const {
315325
return {};
316326
}
317327
bool ThreadPlanStack::AnyPlans() const {
318-
std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
328+
llvm::sys::ScopedReader guard(m_stack_mutex);
319329
// There is always a base plan...
320330
return m_plans.size() > 1;
321331
}
322332

323333
bool ThreadPlanStack::AnyCompletedPlans() const {
324-
std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
334+
llvm::sys::ScopedReader guard(m_stack_mutex);
325335
return !m_completed_plans.empty();
326336
}
327337

328338
bool ThreadPlanStack::AnyDiscardedPlans() const {
329-
std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
339+
llvm::sys::ScopedReader guard(m_stack_mutex);
330340
return !m_discarded_plans.empty();
331341
}
332342

333343
bool ThreadPlanStack::IsPlanDone(ThreadPlan *in_plan) const {
334-
std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
344+
llvm::sys::ScopedReader guard(m_stack_mutex);
335345
for (auto plan : m_completed_plans) {
336346
if (plan.get() == in_plan)
337347
return true;
@@ -340,7 +350,7 @@ bool ThreadPlanStack::IsPlanDone(ThreadPlan *in_plan) const {
340350
}
341351

342352
bool ThreadPlanStack::WasPlanDiscarded(ThreadPlan *in_plan) const {
343-
std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
353+
llvm::sys::ScopedReader guard(m_stack_mutex);
344354
for (auto plan : m_discarded_plans) {
345355
if (plan.get() == in_plan)
346356
return true;
@@ -349,7 +359,7 @@ bool ThreadPlanStack::WasPlanDiscarded(ThreadPlan *in_plan) const {
349359
}
350360

351361
ThreadPlan *ThreadPlanStack::GetPreviousPlan(ThreadPlan *current_plan) const {
352-
std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
362+
llvm::sys::ScopedReader guard(m_stack_mutex);
353363
if (current_plan == nullptr)
354364
return nullptr;
355365

@@ -364,7 +374,7 @@ ThreadPlan *ThreadPlanStack::GetPreviousPlan(ThreadPlan *current_plan) const {
364374
// If this is the first completed plan, the previous one is the
365375
// bottom of the regular plan stack.
366376
if (stack_size > 0 && m_completed_plans[0].get() == current_plan) {
367-
return GetCurrentPlan().get();
377+
return GetCurrentPlanNoLock().get();
368378
}
369379

370380
// Otherwise look for it in the regular plans.
@@ -377,7 +387,7 @@ ThreadPlan *ThreadPlanStack::GetPreviousPlan(ThreadPlan *current_plan) const {
377387
}
378388

379389
ThreadPlan *ThreadPlanStack::GetInnermostExpression() const {
380-
std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
390+
llvm::sys::ScopedReader guard(m_stack_mutex);
381391
int stack_size = m_plans.size();
382392

383393
for (int i = stack_size - 1; i > 0; i--) {
@@ -388,13 +398,13 @@ ThreadPlan *ThreadPlanStack::GetInnermostExpression() const {
388398
}
389399

390400
void ThreadPlanStack::ClearThreadCache() {
391-
std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
401+
llvm::sys::ScopedReader guard(m_stack_mutex);
392402
for (lldb::ThreadPlanSP thread_plan_sp : m_plans)
393403
thread_plan_sp->ClearThreadCache();
394404
}
395405

396406
void ThreadPlanStack::WillResume() {
397-
std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
407+
llvm::sys::ScopedWriter guard(m_stack_mutex);
398408
m_completed_plans.clear();
399409
m_discarded_plans.clear();
400410
}

0 commit comments

Comments
 (0)