[lldb] Update async step-in implementation #2742

kastiglione · 2021-03-23T20:13:23Z

This followup to #2537 is to support the following:

The new async unwinding implemented across a few other commits (see Add UnwindPlans that can follow an AsyncContext chain #2617 Don't create a swift async unwind plan when in a function prologue #2645 [lldb] Swift async: Hopefully correct unwinder #2663)
The initial implementation set a breakpoint on the async resume function, but the breakpoint was not conditional and would stop even if another async task (ie "thread") was the caller. This change implements the conditional breakpoint
Handle the updated swift_task_switch ABI changes (see Change the async ABI to not pass the active task and executor swift#36437)

Conditional breakpoints need some explanation. This change introduces a new mode for thread plans which has been discussed with @jimingham. As of this change, thread plans are no longer bound to a thread, they can be detached from an initial thread, and connected to a new thread. This is to support Swift Concurrency, where stepping into an async function can not assume that function will be called on the same thread, and cannot assume the state of the OS native stack.

To support this, the Process now manages new list of thread plans that have been detached from a thread (m_async_thread_plans), and has supporting code to move thread plans back and forth between m_thread_plans and m_async_thread_plans.

The new function FindDetachedPlanExplainingStop is used to determine if a detached thread plan explains a current stop, and if so that thread plan is attached to the current thread, in order to continue carrying out its work.

kastiglione · 2021-03-23T20:15:19Z

TODO:

Write API tests
Figure out long term StackID comparison for async frames
Prevent step-over from stepping in
Attempt to get rid of the duct tape solutions

lldb/source/Target/SwiftLanguageRuntimeNames.cpp

adrian-prantl

Thanks! I have a bunch of mostly non-substantial comments inline.

adrian-prantl · 2021-03-23T21:48:21Z

lldb/include/lldb/Target/Process.h

@@ -2256,6 +2256,10 @@ bool PruneThreadPlansForTID(lldb::tid_t tid);
 /// Prune ThreadPlanStacks for all unreported threads.
 void PruneThreadPlans();

+  void SynchronizeThreadPlans();


Nit: It would be nice to have Doxygen comment here.

I agree. FindDetachedPlanExplainingStop doesn't indicate that the function also reinstates that stack on the thread that found it relevant. So either a comment or something in the name to make that clear is important.

great points. fwiw I would not have committed without comments, but I also didn't want to write comments only to find out code review obviated them :)

lldb/include/lldb/Target/ThreadPlan.h

adrian-prantl · 2021-03-23T21:50:49Z

lldb/include/lldb/Target/ThreadPlan.h

+  void SetTID(lldb::tid_t tid) { m_tid = tid; }
+
+  friend lldb::ThreadPlanSP
+  Process::FindDetachedPlanExplainingStop(Event *event_ptr);


Nit: Doxygen comment explaining what this function does?

adrian-prantl · 2021-03-23T21:54:28Z

lldb/include/lldb/Target/ThreadPlanStack.h

@@ -145,8 +149,36 @@ class ThreadPlanStackMap {
      return &result->second;
  }

+  // rename to Reactivate?
+  void Activate(ThreadPlanStack &&stack) {
+    if (m_plans_list.find(stack.GetTID()) == m_plans_list.end()) {


Nit: LLVM coding style omits {} on single statements.

Reactivate is better than Activate. But the operation is more that the Plan Stack gets Detached from the current thread, and then Reattached to the thread the work lands on, than that the stack is somehow deactivated & reactivated. Using that sort of language might make more sense?

I haven't felt a strong attachment to which terms to use for having a thread vs no thread. Detached and Reattached sounds good to me. I've also used dissociate and associate, which have the benefit of not being overloaded.

adrian-prantl · 2021-03-23T21:55:01Z

lldb/include/lldb/Target/ThreadPlanStack.h

+    if (m_plans_list.find(stack.GetTID()) == m_plans_list.end()) {
+      m_plans_list.emplace(stack.GetTID(), std::move(stack));
+    } else {
+      m_plans_list.at(stack.GetTID()) = std::move(stack);


Is this equivalent to m_plans_list[stack.GetTID()] = std::move(stack); or better in some way?

kind of equivalent. To use operator[], the value type needs to be default constructible, and ThreadPlanStack is not. Maybe the better thing to do here is make it default constructible.

Prior to this change, it didn't make sense to add ThreadPlanStacks that didn't have a TID associated with them, so a default constructor wouldn't have made sense either. That's no longer the case, so you could make them default constructible. But the other requirement for ThreadPlanStacks is that they have a base plan, either the ThreadPlanBase, or ThreadPlanNull. The latter handles the case where we're shutting down and want to have a do-nothing ThreadPlanStack in case anybody asks it questions.

Since ThreadPlanStacks are only made in one or two places, that's just handled making a stack and immediately pushing on the proper base plan. But if we are going to make ThreadPlanStacks default constructible, we should probably do that bit of business in the constructor to keep us forgetting to do this.

lldb/source/Target/SwiftLanguageRuntimeNames.cpp

adrian-prantl · 2021-03-23T22:04:16Z

lldb/source/Target/SwiftLanguageRuntimeNames.cpp

+    auto arch = reg_ctx_sp->CalculateTarget()->GetArchitecture();
+    int async_context_regnum = 0;
+    if (arch.GetMachine() == llvm::Triple::x86_64) {
+      async_context_regnum = dwarf_r14_x86_64;


It would be nice to consolidate the knowledge about r14/r22 into a single place, too.

We have way too many Swift specific files in Target now. There really all belong in the Swift LanguageRuntime Plugin directory.

lldb/source/Target/SwiftLanguageRuntimeNames.cpp

lldb/source/Target/Thread.cpp

kastiglione · 2021-03-23T22:14:34Z

Thanks for all the comments @adrian-prantl

jimingham

I kind of want to figure out why getting the context didn't work in the AsyncThreadPlan's ShouldStop, but did work in the ThreadPlanBase's ShouldStop? That really shouldn't make any difference, and the code is complicated by having that not work. It would be nice to clear that up before moving forward.

It also seems weird to me that you are disassociating the whole plan stack from the thread when you see an async event. I was imagining that when you detect that an async dispatch is about to start, you would make a disassociated ThreadPlan stack to catch the async when it resumed, and push a step-out plan onto the current stack.

jimingham · 2021-03-23T22:39:39Z

lldb/include/lldb/Target/Process.h

@@ -2256,6 +2256,10 @@ bool PruneThreadPlansForTID(lldb::tid_t tid);
 /// Prune ThreadPlanStacks for all unreported threads.
 void PruneThreadPlans();

+  void SynchronizeThreadPlans();


I agree. FindDetachedPlanExplainingStop doesn't indicate that the function also reinstates that stack on the thread that found it relevant. So either a comment or something in the name to make that clear is important.

jimingham · 2021-03-23T22:42:54Z

lldb/include/lldb/Target/ThreadPlanStack.h

@@ -145,8 +149,36 @@ class ThreadPlanStackMap {
      return &result->second;
  }

+  // rename to Reactivate?
+  void Activate(ThreadPlanStack &&stack) {
+    if (m_plans_list.find(stack.GetTID()) == m_plans_list.end()) {


Reactivate is better than Activate. But the operation is more that the Plan Stack gets Detached from the current thread, and then Reattached to the thread the work lands on, than that the stack is somehow deactivated & reactivated. Using that sort of language might make more sense?

lldb/source/Target/SwiftLanguageRuntimeNames.cpp

jimingham · 2021-03-23T23:12:13Z

lldb/source/Target/SwiftLanguageRuntimeNames.cpp

+    auto arch = reg_ctx_sp->CalculateTarget()->GetArchitecture();
+    int async_context_regnum = 0;
+    if (arch.GetMachine() == llvm::Triple::x86_64) {
+      async_context_regnum = dwarf_r14_x86_64;


We have way too many Swift specific files in Target now. There really all belong in the Swift LanguageRuntime Plugin directory.

jimingham · 2021-03-23T23:17:27Z

lldb/source/Target/Process.cpp

        should_resume = !m_thread_list.ShouldStop(event_ptr);
+        SynchronizeThreadPlans();


Why do you only do this if we aren't restarted? That's going to happen if for instance there was another breakpoint that was hit at same time as when you were catching the async work getting detached, and that breakpoint auto-continued. We still need to make a plan stack to catch the async execution when it starts up onto the unassociated thread plan stack list? And this is just house-keeping, we don't need to ask the process questions to do that since we're just looking at the TIDs as marked in the stacks in CleanUp, right?

@jimingham the placement isn't relative to was_restarted, it's that SynchronizeThreadPlans should be run after ThreadList::ShouldStop. The idea is that ThreadPlan::ShouldStop is where a thread plan could choose to dissociate from its current thread.

Normally it is the ThreadList::ShouldStop that runs actions that might restart the target. Synchronous breakpoint callbacks have run before this, but they should never actually restart, they should return a ShouldStop. So maybe this check is over-defensive. If there were a legit reason why we might have actually restarted here, we would need to think about what work we needed to do.

Note also that ShouldStop CAN restart the process (somebody could have a breakpoint command that is just "continue"... So you need to make sure the SynchronizeThreadPlans can get called when the target is running (or maybe even ran & stopped again in the middle of SynchronizeThreadPlan.

You also need to call SynchronizeThreadPlans above in the Interrupted branch's ShouldStop. It's highly unlikely that somebody will interrupt and stop just as async work is being scheduled, but not impossible.

kastiglione · 2021-03-31T23:53:20Z

@jimingham I spent time to remove the use of the breakpoint callback to capture info. It's all done from the ThreadPlan now. See 2a4d4ac.

kastiglione · 2021-04-01T20:52:54Z

Left for a followup:

Function renames and add docstrings
Removing StackID::operator< heuristic

kastiglione · 2021-04-01T20:53:08Z

@swift-ci test

jimingham

Most of the comments are about naming or smaller refinements. Overall this looks pretty good.

jimingham · 2021-04-01T19:16:21Z

lldb/include/lldb/Target/Process.h

@@ -2796,6 +2800,7 @@ void PruneThreadPlans();
                                     /// threads in m_thread_list, as well as
                                     /// threads we knew existed, but haven't
                                     /// determined that they have died yet.
+  std::vector<ThreadPlanStack> m_async_thread_plans;


Maybe m_async_plan_stacks would be a better name here? This isn't a collection of plans it's a collection of plan stacks.

jimingham · 2021-04-01T19:18:44Z

lldb/include/lldb/Target/ThreadPlan.h

@@ -507,6 +507,14 @@ class ThreadPlan : public std::enable_shared_from_this<ThreadPlan>,
      return m_iteration_count;
  }

+  bool HasTID() { return m_tid != LLDB_INVALID_THREAD_ID; }
+  void ClearTID() { m_tid = LLDB_INVALID_THREAD_ID; }


Maybe ClearTID & SetTID should be private and friended to ThreadPlanStack. I don't think there's any case where we would want somebody to change the TID for one plan in a stack, but not the others.

jimingham · 2021-04-01T19:55:49Z

lldb/include/lldb/Target/ThreadPlanStack.h

+    if (m_plans_list.find(stack.GetTID()) == m_plans_list.end()) {
+      m_plans_list.emplace(stack.GetTID(), std::move(stack));
+    } else {
+      m_plans_list.at(stack.GetTID()) = std::move(stack);


Prior to this change, it didn't make sense to add ThreadPlanStacks that didn't have a TID associated with them, so a default constructor wouldn't have made sense either. That's no longer the case, so you could make them default constructible. But the other requirement for ThreadPlanStacks is that they have a base plan, either the ThreadPlanBase, or ThreadPlanNull. The latter handles the case where we're shutting down and want to have a do-nothing ThreadPlanStack in case anybody asks it questions.

Since ThreadPlanStacks are only made in one or two places, that's just handled making a stack and immediately pushing on the proper base plan. But if we are going to make ThreadPlanStacks default constructible, we should probably do that bit of business in the constructor to keep us forgetting to do this.

jimingham · 2021-04-01T21:28:30Z

lldb/include/lldb/Target/ThreadPlanStack.h

+  std::vector<ThreadPlanStack> CleanUp() {
+    llvm::SmallVector<lldb::tid_t, 2> invalidated_tids;
+    for (auto &pair : m_plans_list)
+      if (pair.second.GetTID() != pair.first)


I think it would be more obvious - since when a thread plan stack is invalidated its TID gets set to LLDB_INVALID_THREAD_ID, that you explicitly check for that value. If the two key and the stack's TID are different but the Stack is not LLDB_INVALID_THREAD_ID, we probably want to assert, since that shouldn't happen.

jimingham · 2021-04-01T21:45:51Z

lldb/include/lldb/Target/ThreadPlanStack.h

+  }
+
+  // rename to ...?
+  std::vector<ThreadPlanStack> CleanUp() {


CleanUp seems too generic a name for this.

I agree, it was a placeholder, to defer thinking of a better name until the dust settled.

jimingham · 2021-04-01T21:52:24Z

lldb/source/Target/Process.cpp

+    m_async_thread_plans.emplace_back(std::move(stack));
+}
+
+ThreadPlanSP Process::FindDetachedPlanExplainingStop(Thread &thread,


We need to have a single name for these Detached/Deactivated/etc. Stacks. Also since this routine doesn't just find the Plan, it also Reattaches/Reactivates the Stack, "Find" isn't a good verb for this function.

jimingham · 2021-04-01T22:05:24Z

lldb/source/Target/Process.cpp

        should_resume = !m_thread_list.ShouldStop(event_ptr);
+        SynchronizeThreadPlans();


Normally it is the ThreadList::ShouldStop that runs actions that might restart the target. Synchronous breakpoint callbacks have run before this, but they should never actually restart, they should return a ShouldStop. So maybe this check is over-defensive. If there were a legit reason why we might have actually restarted here, we would need to think about what work we needed to do.

Note also that ShouldStop CAN restart the process (somebody could have a breakpoint command that is just "continue"... So you need to make sure the SynchronizeThreadPlans can get called when the target is running (or maybe even ran & stopped again in the middle of SynchronizeThreadPlan.

You also need to call SynchronizeThreadPlans above in the Interrupted branch's ShouldStop. It's highly unlikely that somebody will interrupt and stop just as async work is being scheduled, but not impossible.

jimingham · 2021-04-01T22:11:49Z

lldb/source/Target/StackID.cpp

@@ -61,6 +61,13 @@ bool lldb_private::operator<(const StackID &lhs, const StackID &rhs) {
  const lldb::addr_t lhs_cfa = lhs.GetCallFrameAddress();
  const lldb::addr_t rhs_cfa = rhs.GetCallFrameAddress();

+  // FIXME: rdar://76119439


Can a async function call another async function that also gets detached? In that case, there's really no natural ordering since they would both be in the heap and where is subject to the whims of malloc...

jimingham · 2021-04-01T22:13:23Z

lldb/source/Target/SwiftLanguageRuntimeNames.cpp


          auto line_start = line_entry.range.GetBaseAddress().GetFileAddress();
-          if (line_start >= fn_start && line_start < fn_end) {
-            if (++line_entry_count > 1) {
+          if (line_start >= fn_start && line_start < fn_end)


In general it's better not to mix this sort of cleanup into a big patch like this.

my bad, Adrian had commented on the use of curly braces so I did a pass on all of the code I had added since there was a bunch. It could also have been something I did earlier before the size had increased.

jimingham · 2021-04-01T22:25:11Z

lldb/source/Target/SwiftLanguageRuntimeNames.cpp

+  }
+
+  bool ShouldStop(Event *event) override {
+    if (!m_async_breakpoint_sp)


It looks like if GetBreakpointAsyncContext matches this context the plan is complete. So if GetBreakpointAsyncContext is expensive, you can do the SetPlanComplete in DoPlanExplainsStop, and then have ShouldStop check IsPlanComplete and use that to decide what to return. That way you only have to do the work once.

There's one slightly non-obvious bit to that which is that is it possible that your plan wasn't the one that explained the stop. Rather, an earlier plan explained the stop, but then that planwas finished, and and got popped off the stack. We don't do another round of ExplainsStop at that point (*) but just ask the next plan on the stack if it ShouldStop.

In that case, your ShouldStop will get called, without having previously called PlanExplainsStop. The way to finesse that is to have ShouldStop call DoPlanExplainsStop and then return IsPlanComplete. The up side is no matter how you get to your ShouldStop, you only do the work to check what to do once, and all in one place.

(*) It might be more regular to redo the "PlanExplainsStop" every time we pop a plan to figure out the next plan we should ask ShouldStop of. However, that would require other changes, for instance plans that only use other plans would have to start responding themselves to ExplainsStop, which they don't at present.

kastiglione · 2021-04-02T01:14:18Z

@swift-ci test macOS Platform

kastiglione · 2021-04-02T02:58:55Z

@swift-ci test macOS Platform

kastiglione · 2021-04-02T15:52:53Z

@swift-ci test macOS Platform

kastiglione · 2021-04-02T18:36:25Z

@swift-ci test macOS Platform

(cherry picked from #2742)

[lldb] Update async step-in implementation

a7be84e

kastiglione requested review from jimingham, adrian-prantl and fredriss March 23, 2021 20:21

kastiglione commented Mar 23, 2021

View reviewed changes

lldb/source/Target/SwiftLanguageRuntimeNames.cpp Outdated Show resolved Hide resolved

adrian-prantl reviewed Mar 23, 2021

View reviewed changes

jimingham reviewed Mar 24, 2021

View reviewed changes

kastiglione added 10 commits March 30, 2021 11:30

remove optional curly braces, and other formatting

0306157

use linear search instead of binary search for GetFrameWithStackID

cb7a68b

don't step through AsyncSuspendResumePartialFunction

512faeb

remove incorrectly written check for being at function start

e7429bb

return early in case of !(arm64 || x86_64)

40b38fc

reuse IsSwiftAsyncAwaitResumePartialFunctionSymbol

0a333e4

replace IsTID with comparison

d14684e

use SmallVector over vector

3aee29a

remove superfluous conditional

bbefe62

remove duct tape and fix a bug or two in the process

2a4d4ac

kastiglione added 2 commits April 1, 2021 13:41

add step in tests

50b9531

add comment and radar number for StackID heuristic

9849007

kastiglione marked this pull request as ready for review April 1, 2021 20:52

jimingham reviewed Apr 1, 2021

View reviewed changes

kastiglione merged commit e092783 into swift/main Apr 2, 2021

kastiglione deleted the lldb-Update-async-step-in-implementation branch April 2, 2021 22:33

kastiglione mentioned this pull request Apr 13, 2021

[lldb] Update async step-in implementation #2835

Merged

kastiglione added a commit that referenced this pull request Apr 13, 2021

[lldb] Update async step-in implementation (#2835)

e0d865a

(cherry picked from #2742)

		should_resume = !m_thread_list.ShouldStop(event_ptr);
		SynchronizeThreadPlans();

[lldb] Update async step-in implementation #2742

[lldb] Update async step-in implementation #2742

Uh oh!

Conversation

kastiglione commented Mar 23, 2021

Uh oh!

kastiglione commented Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

adrian-prantl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kastiglione commented Mar 23, 2021

Uh oh!

jimingham left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kastiglione commented Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kastiglione commented Apr 1, 2021

Uh oh!

kastiglione commented Apr 1, 2021

Uh oh!

jimingham left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kastiglione commented Mar 23, 2021 •

edited

Loading

kastiglione commented Mar 31, 2021 •

edited

Loading