[Lldb-commits] [PATCH] D58129: Move UnwindTable from ObjectFile to Module

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

You changed the API to return a pointer which can be NULL so we either need to check all returns for this or change the API to return a reference to a default constructed UnwindTable. I like the latter option, but I will leave that up to you. Since we are moving functionality into symbol files, we might want to opt for the latter to allow us to populate the UnwindTable as needed from any source when ever that source becomes available (like adding a symbol file to a module after the fact).



================
Comment at: include/lldb/Core/Module.h:709
+  //------------------------------------------------------------------
+  UnwindTable *GetUnwindTable();
+
----------------
I would vote to return a "UnwindTable&" and make a const and non const version of this. Otherwise we have to NULL check any call to this API.


================
Comment at: include/lldb/Core/Module.h:1108-1110
+  llvm::Optional<UnwindTable> m_unwind_table; /// < Table of FuncUnwinders
+                                              /// objects created for this
+                                              /// Module's functions
----------------
Why not just make this an instance to avoid having to null check all accessor calls?:

```
UnwindTable m_unwind_table;
```


================
Comment at: source/Commands/CommandObjectTarget.cpp:3536-3537
       FuncUnwindersSP func_unwinders_sp(
-          sc.module_sp->GetObjectFile()
-              ->GetUnwindTable()
-              .GetUncachedFuncUnwindersContainingAddress(start_addr, sc));
+          sc.module_sp->GetUnwindTable()
+              ->GetUncachedFuncUnwindersContainingAddress(start_addr, sc));
       if (!func_unwinders_sp)
----------------
Either NULL check or change the API?


================
Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2865-2866
 
-    DWARFCallFrameInfo *eh_frame = GetUnwindTable().GetEHFrameInfo();
-    if (eh_frame) {
+    if (DWARFCallFrameInfo *eh_frame =
+            GetModule()->GetUnwindTable()->GetEHFrameInfo()) {
       if (m_symtab_ap == nullptr)
----------------
Either NULL check or change the API?


================
Comment at: source/Plugins/Process/Utility/RegisterContextLLDB.cpp:242-243
       func_unwinders_sp =
-          pc_module_sp->GetObjectFile()
-              ->GetUnwindTable()
-              .GetFuncUnwindersContainingAddress(m_current_pc, m_sym_ctx);
+          pc_module_sp->GetUnwindTable()->GetFuncUnwindersContainingAddress(
+              m_current_pc, m_sym_ctx);
     }
----------------
Either NULL check or change the API?


================
Comment at: source/Plugins/Process/Utility/RegisterContextLLDB.cpp:674-675
   FuncUnwindersSP func_unwinders_sp(
-      pc_module_sp->GetObjectFile()
-          ->GetUnwindTable()
-          .GetFuncUnwindersContainingAddress(m_current_pc, m_sym_ctx));
+      pc_module_sp->GetUnwindTable()->GetFuncUnwindersContainingAddress(
+          m_current_pc, m_sym_ctx));
   if (!func_unwinders_sp)
----------------
Either NULL check or change the API?


================
Comment at: source/Plugins/Process/Utility/RegisterContextLLDB.cpp:776-777
     func_unwinders_sp =
-        pc_module_sp->GetObjectFile()
-            ->GetUnwindTable()
-            .GetFuncUnwindersContainingAddress(m_current_pc, m_sym_ctx);
+        pc_module_sp->GetUnwindTable()->GetFuncUnwindersContainingAddress(
+            m_current_pc, m_sym_ctx);
   }
----------------
Either NULL check or change the API?


================
Comment at: source/Plugins/Process/Utility/RegisterContextLLDB.cpp:795
     DWARFCallFrameInfo *eh_frame =
-        pc_module_sp->GetObjectFile()->GetUnwindTable().GetEHFrameInfo();
+        pc_module_sp->GetUnwindTable()->GetEHFrameInfo();
     if (eh_frame) {
----------------
Either NULL check or change the API?


================
Comment at: source/Plugins/Process/Utility/RegisterContextLLDB.cpp:805
     ArmUnwindInfo *arm_exidx =
-        pc_module_sp->GetObjectFile()->GetUnwindTable().GetArmUnwindInfo();
+        pc_module_sp->GetUnwindTable()->GetArmUnwindInfo();
     if (arm_exidx) {
----------------
Either NULL check or change the API?


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

https://reviews.llvm.org/D58129