Rework the ThreadPlanStackMap class so we can move ThreadPlanStacks #3172
Conversation
from the TID->Plans map to the "Detached plan" vector without having to tiptoe around no invoking the copy constructor.
| if (m_plans_list.find(stack.GetTID()) == m_plans_list.end()) | ||
| m_plans_list.emplace(stack.GetTID(), std::move(stack)); | ||
| m_plans_list.emplace(stack.GetTID(), &stack); |
There was a problem hiding this comment.
stack is a unique_ptr, right? Don't we want to call ::get here, or is the & overloaded for unique pointers?
There was a problem hiding this comment.
"stack" is actually a ThreadPlanStack & passed into the function, not one we've pulled out of the container, where it would be a UP. So I think this is okay.
There was a problem hiding this comment.
Got it, and the address is held by the unique_ptr in the store, so it's stable which is why it's safe to take its address. That's why it caught my attention initially. Maybe it's worth adding a comment saying that (or changing the interface to take a pointer to be more explicit about it).
There was a problem hiding this comment.
I don't want this to be a pointer, since I don't want to deal with getting passed a null, that's not allowed.
| // Then tell the stack its thread has been destroyed: | ||
| removed_stack->ThreadDestroyed(nullptr); | ||
| // And then remove it from the container so it goes away. | ||
| m_plans_up_container.erase(iter); |
There was a problem hiding this comment.
FYI: If haven't discovered it yet, there is also std::remove_if (https://en.cppreference.com/w/cpp/algorithm/remove). It works by moving the to-be-removed element to the end so you can std::erase it there (see the example).
There was a problem hiding this comment.
Thanks, I'll keep that in mind. ! I don't think that would make this code any clearer.
| PlansStore m_plans_up_container; | ||
| std::vector<ThreadPlanStack *> m_detached_plans; | ||
|
|
||
| using PlansList = std::unordered_map<lldb::tid_t, ThreadPlanStack *>; |
There was a problem hiding this comment.
I think I mentioned that in another review already, but if we store ThreadPlanStack * instead of ThreadPlanStack & shouldn't we check for nullptr at each use? In other words, could we store ThreadPlanStack & here?
There was a problem hiding this comment.
Yeah, I tried making these & not * but it seemed like that involved a lot more fighting with the compiler. This one is more straightforward. We will never have nullptr's in this list or return them so I'll add a comment to that effect.
There was a problem hiding this comment.
I added a comment to the effect that the vector you get of ThreadPlanStack's will never have any nullptr entries. That is true by the way we deal with them.
|
I fixed the auto's and added a comment that the pended stacks vector won't have nullptr's in them. |
Unintentionally deleted in #3172
This call to `ThreadPlanStack::SetTID` seems to have been unintentionally deleted in #3172. This call sets the TID of all thread plans in the stack, and must be called before calling `Activate`. This resulted in downstream misbehavior, one example of which is in `ThreadPlanStackMap::Update` where a base plan was being queued multiple times on to the same plan stack because the TID wasn't correct. https://github.com/apple/llvm-project/blob/be3823b722c461f7c11f81601e33a3e53ea76565/lldb/source/Target/ThreadPlanStack.cpp#L418-L422 rdar://96534399 (cherry-picked from commit 24f4737)
This call to `ThreadPlanStack::SetTID` seems to have been unintentionally deleted in #3172. This call sets the TID of all thread plans in the stack, and must be called before calling `Activate`. This resulted in downstream misbehavior, one example of which is in `ThreadPlanStackMap::Update` where a base plan was being queued multiple times on to the same plan stack because the TID wasn't correct. https://github.com/apple/llvm-project/blob/be3823b722c461f7c11f81601e33a3e53ea76565/lldb/source/Target/ThreadPlanStack.cpp#L418-L422 rdar://96534399 (cherry-picked from commit 24f4737) (cherry-picked from commit 3348866)
Supporting swift async requires we move non-copyable ThreadPlanStacks from the TID->Plans map to the "Detached plan" vector. That's easier to do if we keep the stacks in a central store, and use references in the organizing containers.
This version of the patch uses unique_ptr's - I was getting "no copy constructor" build failures with an earlier version that used UP's but this version is fine.
I wanted folks to get a change to look at this. I also had "no copy constructor" failures when the "organizing containers" were ThreadPlanStack &'s rather than ThreadPlanStack *'s. I'm going to try converting them back - since we never put nullptr elements into the organizing containers so that's more correct. Otherwise I'll add a comment to the effect that these will never be null.
If that works, I'll update the patch with that fix, but I wanted to give folks a chance to look at the other changes.