clayborg 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))
----------------
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.


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

Reply via email to