[Lldb-commits] [PATCH] D61853: [FuncUnwinders] Use "symbol file" unwind plans for unwinding

[Pavel Labath via Phabricator via lldb-commits]

labath marked an inline comment as done.
labath added inline comments.


================
Comment at: source/Symbol/FuncUnwinders.cpp:61-70
   if (UnwindPlanSP plan_sp = GetEHFrameUnwindPlan(target))
     return plan_sp;
   if (UnwindPlanSP plan_sp = GetDebugFrameUnwindPlan(target))
     return plan_sp;
   if (UnwindPlanSP plan_sp = GetCompactUnwindUnwindPlan(target))
     return plan_sp;
   if (UnwindPlanSP plan_sp = GetArmUnwindUnwindPlan(target))
----------------
clayborg wrote:
> 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:
> EH frame
> Apple compact unwind
> ARM compact unwind
> 
> and GetSymbolFileUnwindPlan would check:
> .debug_frame
> breakpad
> 
> 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.
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. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61853/new/

https://reviews.llvm.org/D61853