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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: source/Symbol/FuncUnwinders.cpp:63-64
     return plan_sp;
   if (UnwindPlanSP plan_sp = GetDebugFrameUnwindPlan(target))
     return plan_sp;
   if (UnwindPlanSP plan_sp = GetCompactUnwindUnwindPlan(target))
----------------
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.


================
Comment at: source/Symbol/FuncUnwinders.cpp:69-70
     return plan_sp;
+  if (UnwindPlanSP plan_sp = GetSymbolFileUnwindPlan(thread))
+    return plan_sp;
 
----------------
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 at: source/Symbol/FuncUnwinders.cpp:70
+  if (UnwindPlanSP plan_sp = GetSymbolFileUnwindPlan(thread))
+    return plan_sp;
 
----------------
That is fine for now. Good to get things in and we can iterate later. 


================
Comment at: source/Symbol/FuncUnwinders.cpp:365-366
     return plan_sp;
   if (UnwindPlanSP plan_sp = GetDebugFrameAugmentedUnwindPlan(target, thread))
     return plan_sp;
+  if (UnwindPlanSP plan_sp = GetSymbolFileUnwindPlan(thread))
----------------
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.


================
Comment at: source/Symbol/FuncUnwinders.cpp:367-368
     return plan_sp;
+  if (UnwindPlanSP plan_sp = GetSymbolFileUnwindPlan(thread))
+    return plan_sp;
 
----------------
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?


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

https://reviews.llvm.org/D61853