Add a mutex to protect ThreadPlanStack's from simultaneous access #3164
Conversation
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. Differential Revision: https://reviews.llvm.org/D106171
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. Except for preventing crashes this should be NFC. Differential Revision: https\://reviews.llvm.org/D106122
The swift async code requires us to move ThreadPlanStacks from a TID->ThreadPlanStack map to a vector of "Pending Stacks". But ThreadPlanStacks aren't copyable - and shouldn't be - and the way we were storing them before made it really hard to avoid operations that end up requiring a copy constructor. So I reworked how the stacks are held so that we keep all the stacks in a storage container, and then our organizing containers just refer to the stacks in the storage.
There was a problem hiding this comment.
Generally I'm all in favor of adding the lock. I had a couple of minor questions inline. Github doesn't really show me which commit a patch belongs so if the questions are about the cherry-picked changes, we should of course follow up on LLVM.org instead of here. Otherwise adding the lock sounds good to me!
| m_plans_list.erase(result); | ||
| // Also remove this from the stack storage: | ||
| PlansStore::iterator end |
There was a problem hiding this comment.
is this the same as std::find_if(m_plans_sp_container.begin(), m_plans_sp_container.end(), [](ThreadPlan &p) {return p.IsTID(tid);})?
| // Remove this from the detached plan list: | ||
| std::vector<ThreadPlanStack *>::iterator end = m_detached_plans.end(); | ||
|
|
||
| for (std::vector<ThreadPlanStack *>::iterator iter |
There was a problem hiding this comment.
This looks like another candidtae for find_if or at least for (auto plan : m_detached_plans) ?
| } | ||
|
|
||
| void Clear() { | ||
| for (auto &plan : m_plans_list) | ||
| plan.second.ThreadDestroyed(nullptr); | ||
| plan.second->ThreadDestroyed(nullptr); |
There was a problem hiding this comment.
If this changed from a reference to a pointer, why do we not have to check for nullptr?
| std::vector<ThreadPlanStack *> &detached_plans | ||
| = m_thread_plans.GetDetachedPlanStacks(); | ||
| size_t num_detached_plans = detached_plans.size(); | ||
| for (size_t idx = 0; idx < num_detached_plans; idx++) { |
There was a problem hiding this comment.
why not for (ThreadPlanStack &cur_stack : detached_plans) ?
| @@ -129,15 +135,34 @@ class ThreadPlanStackMap { | |||
|
|
|||
| void AddThread(Thread &thread) { | |||
| lldb::tid_t tid = thread.GetID(); | |||
| m_plans_list.emplace(tid, thread); | |||
| std::shared_ptr<ThreadPlanStack> new_elem_sp(new ThreadPlanStack(thread)); | |||
There was a problem hiding this comment.
auto new_elem_sp = std::make_shared(thread);
| // shared_ptrs to the objects, so that the pointers in the containers will stay | ||
| // valid. | ||
|
|
||
| // Use a shared pointer rather than unique_ptr here to prevent C++ from trying |
There was a problem hiding this comment.
This doesn't really make sense, regardless of whether and object is stored in a shared or unique pointer, if you're moving the smart pointer around, it will copy/move the pointer, not the underlying object. Do you mean here that ThreadPlanStack itself should be copyable?
| @@ -194,7 +230,23 @@ class ThreadPlanStackMap { | |||
|
|
|||
| private: | |||
| Process &m_process; | |||
| using PlansList = std::unordered_map<lldb::tid_t, ThreadPlanStack>; | |||
| // We don't want to make copies of these ThreadPlanStacks, there needs to be | |||
There was a problem hiding this comment.
If you're using shared pointers, I'm not sure why we can't store them directly in the unordered map. The map would only ever be copying shared_pointers around, which won't touch the underlying object.
This patch has two parts. The first is to cherry-pick the code that added a ThreadPlanStack mutex from llvm.org.
But then because swift async support requires us to copy the ThreadPlanStack's from one container to another, we were running into problems with the STL requiring copy constructors (even though we were trying to std::move everything correctly). Rather than fight with STL implementations, the second part of this patch reworks the implementation of ThreadPlanStackMap so that we have one store, and then the organizing containers hold pointers into the store.
Note, the second part of this patch will need to be split into generic & swift specific parts, and by rights should find its way back to swift by merging the generic part from llvm.org, then adding the Swift parts. That's a little tricky because the change is poorly motivated without the swift additions... And I wanted to get the functionality into this release branch, in case the review process drags on. So I'm submitting this version to the release branch.