Previous patch (r360409) introduced the "symbol file unwind plan"
concept, but that plan wasn't used for unwinding yet. With this patch,
we start to consider the new plan as a possible strategy for both
synchronous and asynchronous unwinding. I also add a test that asserts
that unwinding via breakpad STACK CFI info works end-to-end.
Details
Details
Diff Detail
Diff Detail
- Repository
- rLLDB LLDB
Event Timeline
| source/Symbol/FuncUnwinders.cpp | ||
|---|---|---|
| 63–71 | I would love to at least make this entire function just be: std::lock_guard<std::recursive_mutex> guard(m_mutex);
if (UnwindPlanSP plan_sp = GetSymbolFileUnwindPlan(thread))
return plan_sp;
return nullptr;and let the symbol files do all of the work as they will know if one plan is better than another? Or do we need to add an ObjectFile component?: std::lock_guard<std::recursive_mutex> guard(m_mutex);
if (UnwindPlanSP plan_sp = GetSymbolFileUnwindPlan(thread))
return plan_sp;
if (UnwindPlanSP plan_sp = GetObjectFileUnwindPlan(thread))
return plan_sp;
return nullptr;Since macho will always check EH frame, then .debug_frame, then compact unwind and never ARM. ELF would check EH frame, then .debug_frame, then ARM unwind. If we split it up GetObjectFileUnwindPlan would check: and GetSymbolFileUnwindPlan would check: I might also argue the ordering is wrong in the original function. The .debug_frame should come first since it can be more complete than EH frame. Granted most times the .debug_frame is the same as the EH frame, but if it isn't we should be using that first. Then EH frame, then apple compact unwind, then ARM unwind. That is why I put the symbol file check first in the example code where we check the symbol file, then the object file. | |
| source/Symbol/FuncUnwinders.cpp | ||
|---|---|---|
| 63–71 | I can see some appeal in this idea, but is there any practical benefit to doing that now? Doing it might be useful if we plan to customize the plan order in different symbol file plugins, but i don't think we want to do that now. So all of this code will end up in the base SymbolFile class anyway, and I don't see much difference between this being in Symbol/FuncUnwinders and Symbol/SymbolFile. In fact it might be better to keep it in a separate class, as symbol files have a lot of other responsibilities anyway. Given a choice, I'd much rather work on being able to avoid passing the Target and Thread pointers into the unwindplan generation code, because I think it has more practical benefits. In fact if I moved everything over to SymbolFile right now, it would mean the plugin would have to start operating with Targets and Threads, which is something we've managed to avoid so far. Moving stuff to symbol file might be a smaller project than the other one, but it is too not without it's pitfalls. There are places now where we directly ask for a specific (usually eh-frame) unwind plan, which kind of works now, but it won't be possible if that is all hidden behind a "symbol file" facade. | |
| source/Symbol/FuncUnwinders.cpp | ||
|---|---|---|
| 65–66 | We should move this before EH frame. Doesn't need to be in this patch, but it should be done. .debug_frame will be as good if not better than EH frame. | |
| 71–72 | The question then becomes: if a symbol file can return an unwind plan, where should be it in terms of trust? I would almost argue it should be above EH frame, and possibly even .debug_frame? | |
| 72 | That is fine for now. Good to get things in and we can iterate later. | |
| 367–368 | We should move this before EH frame. Doesn't need to be in this patch, but it should be done. .debug_frame will be as good if not better than EH frame. | |
| 369–370 | The question then becomes: if a symbol file can return an unwind plan, where should be it in terms of trust? I would almost argue it should be above EH frame, and possibly even .debug_frame? | |
Comment Actions
- use (reduced) yaml form of the minidump now that yaml2obj supports memory regions
- move the symbol file unwind plan to the top of the list. I agree with the reasoning behind this and with the debug_frame>eh_frame (which I'll create a separate patch for) idea, though I think this is mostly academic, as we're unlikely to ever have two of these plans describing the same function.
We should move this before EH frame. Doesn't need to be in this patch, but it should be done. .debug_frame will be as good if not better than EH frame.