[lldb] Introduce backtracing of Swift Tasks #9787
Conversation
kastiglione
requested review from
adrian-prantl,
felipepiovezan and
jasonmolenda
| // Print a backtrace of the Task to stdout. | ||
| ExecutionContext exe_ctx{m_backend.GetExecutionContextRef()}; | ||
| auto tt = std::make_shared<ThreadTask>( | ||
| 3000, task_info->resumeAsyncContext, exe_ctx); | ||
| StreamString ss; | ||
| tt->GetStatus(ss, 0, 100, 0, false, true); | ||
| auto desc = ss.GetString(); | ||
| printf("%.*s\n", (int)desc.size(), desc.data()); |
There was a problem hiding this comment.
This part is the reason this PR is in draft state. This code prints the backtrace of a task to stdout, but that will be migrated to a separate command. Initially something like language swift task backtrace.
| ExecutionContext &exe_ctx) | ||
| : Thread(exe_ctx.GetProcessRef(), tid, true), | ||
| m_concrete_reg_ctx_sp(exe_ctx.GetFrameSP()->GetRegisterContext()) { | ||
| m_async_ctx = async_ctx; |
There was a problem hiding this comment.
we can initialize m_sync_ctx in the member initializer list?
There was a problem hiding this comment.
I wanted like with like: to place the logic for initializing the two register values side by side.
| /// The Task's async context. | ||
| addr_t m_async_ctx = LLDB_INVALID_ADDRESS; | ||
| /// The address of the async context's resume function. | ||
| addr_t m_pc = LLDB_INVALID_ADDRESS; |
There was a problem hiding this comment.
The name pc here is a bit misleading: it would be natural to think this contains the PC of the top frame of the thread, which is not what this variable contains.
Maybe it would be better to call this m_continuation_address?
When we are unwinding, we say "pc" because we are producing the register context of the frame above the current frame. Here, we just have a Thread class with a member pc, so I don't think the same interpretation would apply.
There was a problem hiding this comment.
Actually I am confused now, is this really printing frame 0 correctly?
In the register context class, there is this code:
if (reg_info->kinds[eRegisterKindGeneric] == LLDB_REGNUM_GENERIC_PC) {
reg_value = m_pc;
return true;
}
I could be missing something, but this looks like it prints the PC of frame 1 instead of frame 0.
There was a problem hiding this comment.
maybe m_resume_address? since "resume" is the term used in AsyncContext.
I'm not sure where the confusion stems from, but yes this prints "frame 0" correctly.
There was a problem hiding this comment.
I changed it to m_resume_fn, how does that sound?
There was a problem hiding this comment.
I think I understand 2024 Felipe's confusion now. Usually, the "resume address" (to use the terminology of the code here) means the PC of the "frame above" or the "next funclet to execute after the current frame". However, here we have Tasks which are suspended, so there is no "current frame"; in other words, the "next funclet to execute" is going to be frame zero.
There was a problem hiding this comment.
we have Tasks which are suspended, so there is no "current frame"; in other words, the "next funclet to execute" is going to be frame zero.
👍
| auto resume_offset = ptr_size; // offsetof(AsyncContext, ResumeParent) | ||
| auto resume_ptr = async_ctx + resume_offset; | ||
| Status status; | ||
| m_pc = exe_ctx.GetProcessRef().ReadPointerFromMemory(resume_ptr, status); |
There was a problem hiding this comment.
I'm moderately concerned that this ctor may fail.
Should we make the constructor take the continuation ptr directly and instead create a "make thread function" that can fail?
There was a problem hiding this comment.
That seems fine to me. Note that if ReadPointerFromMemory fails, the resulting address is LLDB_INVALID_ADDRESS, which subsequent code should handle.
There was a problem hiding this comment.
Looking at the code where ThreadTask's are created, I don't see any handling of invalid pcs. IIUC we will just print a thread with a weird PC?
There was a problem hiding this comment.
there's no handling of invalid pcs, what do you think should be done? I assumed the async context's resume function can be trusted to be valid, but you must see some ways in which it would be invalid?
| uint32_t resume_ptr = async_ctx + resume_offset; | ||
| Status status; | ||
| m_resume_fn = | ||
| exe_ctx.GetProcessRef().ReadPointerFromMemory(resume_ptr, status); |
There was a problem hiding this comment.
IIUC to apply the skip prologue trick, you would need to factor out the fragment below of SwiftLanguageRuntime::TrySkipVirtualParentProlog into a separate function, and say that pc_value = m_resume_fn:
Address pc;
Target &target = process.GetTarget();
pc.SetLoadAddress(pc_value, &target);
if (!pc.IsValid())
return {};
SymbolContext sc;
if (!pc.CalculateSymbolContext(&sc,
eSymbolContextFunction | eSymbolContextSymbol))
return {};
if (!sc.symbol && !sc.function)
return {};
auto prologue_size = sc.symbol ? sc.symbol->GetPrologueByteSize()
: sc.function->GetPrologueByteSize();
return pc_value + prologue_size;
| // TaskContinuationFunction *ResumeParent; // offset 8 | ||
| // }; | ||
| uint32_t resume_offset = ptr_size; // offsetof(AsyncContext, ResumeParent) | ||
| uint32_t resume_ptr = async_ctx + resume_offset; |
There was a problem hiding this comment.
Given that you show the offsets of Parent and ResumeParent in the comment, is it clarifying to have resume_offset. Why not just addr_t resume_ptr = async_ctx + ptr_size. (also why is resume_ptr uint32_t, it's adding an addr_t and an offset isn't it?)
There was a problem hiding this comment.
also why is resume_ptr uint32_t, it's adding an addr_t and an offset isn't it?
yep you caught a bug, it was after I switched these variables away from auto types.
There was a problem hiding this comment.
That's one of the reasons I will die on the proverbial auto hill 👀
No description provided.