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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
