[lldb] Fix thread plan to step through swift_task_switch #9452
Conversation
This thread plan attempts to identify whether a funclet contains a single line in the line table. If it does, it considers such a funclet to contain no user code and then attempts to step through it, reaching the next funclet. This is problematic, as some funclets _can_ have a single line for legitimate reasons. Recent improvements are taking LLDB closer to being able to step from any funclet to any funclet. As such, if a `thread step-*` command takes us to one "single-line"-funclet, this should not be problematic. LLDB must know how to move from this funclet to wherever the next `thread step-*` requires. The other evidence this plan is not really needed is that no tests fail when it is removed. It seems that `API/lang/swift/async/stepping/step-in/task-switch` was originally intended to test this step-through behavior, but the async function of that test had a **single** funclet (the entry funclet). Stepping through that would have taken us back to caller, which would have been incorrect. With or without this patch, we are correctly stopping at the entry funclet. In fact, the test is even checking we do so: ``` self.assertEqual(frame.name, "a.f() async -> Swift.Int") ``` Note that the demangled name doesn't contain a number prior to `a.f()`, which is how non-entry funclets can be identified. With all that in mind, it doesn't seem like this test was exercising ThreadPlanStepInAsync. A follow up commit will use one idea from ThreadPlanStepInAsync in order to step through *swift_task_switch* (as opposed to through funclets) to the target funclet of swift_task_switch. (cherry picked from commit 68ada77c6a087e4d26dac3b315df84ca2b072233)
…plugin
The symbol for swift_task_switch is marked as a trampoline, which triggers a
generic handling of it through DynamicLoaderDarwin. This makes sense, as it is
usually just a symbol stub that we jump through to get to the real task_switch:
```
...
0x1000031bc <+112>: b 0x100003d20 ; symbol stub for: swift_task_switch
...
testt.out`swift_task_switch:
swift_task_switch
-> 0x100003d20 <+0>: adrp x16, 1
0x100003d24 <+4>: ldr x16, [x16, #0x10]
0x100003d28 <+8>: br x16 ; <<< real task_switch_addr
```
DynamicLoaderDarwin will take the thread all the way to the target of the
`br x16` instruction, which is the real task switch.
However, once that's done, there are no thread plans attempting to take us to to
target funclet. Recall that a task_switch is a request to run a given funclet
inside _some_ executor/thread. It's like a function call, but with some possible
scheduling in the middle.
This commit adds a new handling of task_switch inside SwiftLanguageRuntime; it
is a simplified version of the (now removed) ThreadPlanStepInAsync: a breakpoint
is created to the target funclet.
Note: we could remove the handling inside DynamicLoaderDarwin with some kind of
"if (symbol == swift_task_switch) don't do anything". The new thread plan would
work even on the stub.
(cherry picked from commit 44c94449a1965a901fec42d2d2f02409af79dcab)
felipepiovezan
requested review from
kastiglione,
adrian-prantl and
jimingham
|
@jimingham I am particularly interested in your thoughts about the second commit message / implementation. |
|
Ah, I forgot to include the test for the second commit. Will update the PR momentarily. |
| constexpr unsigned resume_fn_regnum = LLDB_REGNUM_GENERIC_ARG1; | ||
| unsigned resume_fn_reg = reg_ctx->ConvertRegisterKindToRegisterNumber( | ||
| RegisterKind::eRegisterKindGeneric, resume_fn_regnum); | ||
| uint64_t resume_fn_ptr = reg_ctx->ReadRegisterAsUnsigned(resume_fn_reg, 0); |
There was a problem hiding this comment.
ConvertRegisterKindToRegisterNumber could fail (on an architecture that passes ALL arguments on the stack, so not likely, but formally it could.)
|
The previous plan handled the case where the task switch ended up with the resume code called on another thread. I didn't see how your patch handled that? |
Mmmm I see. I need to fix that. Let me put this PR in draft mode for now |
There was a problem hiding this comment.
Apologies if I've overlooked the details, but can you provide explanation of how this handles multiple tasks going through the same codepath? One of the responsibilities of ThreadPlanStepInAsync is to capture the async context before the step, and when its internal breakpoint hits, it checks that the async context is the same. When the async context is different, execution continues. In other words, stepping from one task shouldn't stop at the same resume function for a different task.
|
ah, Jim has said the same… |
|
Replaced by #9546 |
This PR removes ThreadPlanStepInAsync and replaces it with a simpler plan that only handles swift_task_switch.
Please review each commit individually.