[Lldb-commits] [PATCH] D34613: Add debug_frame section support

[Greg Clayton via Phabricator via lldb-commits]

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

Glad this is happening. Does this mean we won't see the "bad eh frame" warnings anymore on linux? See inlined comments.



================
Comment at: include/lldb/Symbol/DWARFCallFrameInfo.h:38
   DWARFCallFrameInfo(ObjectFile &objfile, lldb::SectionSP &section,
                      lldb::RegisterKind reg_kind, bool is_eh_frame);
 
----------------
Remove "reg_kind" and "is_eh_frame" and replace with DWARFCallFrameInfo::Type. See inlined comment below by the CFIVersion enum.


================
Comment at: include/lldb/Symbol/DWARFCallFrameInfo.h:77
   enum { CFI_AUG_MAX_SIZE = 8, CFI_HEADER_SIZE = 8 };
+  enum CFIVersion {
+    CFI_VERSION1 = 1, // DWARF v.2
----------------
How about we remove "is_eh_frame" from constructor and make CFIVersion into CFIType and make it public. We went from 2 flavors (DWARF and EH frame) to now 4, so we should just use an enum to define the exact flavor.

```
enum Type {
  EHFrame,
  DWARFv2,
  DWARFv3,
  DWARFv4
};
```

The enum is already in DWARFCallFrameInfo so no need for a prefix.


================
Comment at: source/Symbol/DWARFCallFrameInfo.cpp:306
+    // may ignore these fields?
+    if (!m_is_eh_frame && cie_sp->version >= CFI_VERSION4) {
+      cie_sp->address_size = m_cfi_data.GetU8(&offset);
----------------
Update to use DWARFCallFrameInfo::Type


================
Comment at: source/Symbol/DWARFCallFrameInfo.cpp:315
+    cie_sp->return_addr_reg_num =
+        !m_is_eh_frame && cie_sp->version >= CFI_VERSION3
+            ? static_cast<uint32_t>(m_cfi_data.GetULEB128(&offset))
----------------
Update to use DWARFCallFrameInfo::Type


================
Comment at: source/Symbol/DWARFCallFrameInfo.cpp:485
+    // FDE entries with cie_id == 0 shouldn't be ignored for it.
+    if ((cie_id == 0 && m_is_eh_frame) || cie_id == UINT32_MAX || len == 0) {
+      auto cie_sp = ParseCIE(current_entry);
----------------
Update to use DWARFCallFrameInfo::Type


================
Comment at: source/Symbol/DWARFCallFrameInfo.cpp:499
+
+    if (!m_is_eh_frame)
+      cie_offset = cie_id;
----------------
Update to use DWARFCallFrameInfo::Type


================
Comment at: source/Symbol/DWARFCallFrameInfo.cpp:567
+  // FDE entries with zeroth cie_offset may occur for debug_frame.
+  assert(!(m_is_eh_frame && 0 == cie_offset) && cie_offset != UINT32_MAX);
 
----------------
Update to use DWARFCallFrameInfo::Type


================
Comment at: source/Symbol/FuncUnwinders.cpp:219-221
+  // Only supported on x86 architectures where we get debug_frame from the
+  // compiler that describes the prologue instructions perfectly, and sometimes
+  // the epilogue instructions too.
----------------
What compiler suddenly started including complete unwind info in .debug_frame? They are all supposed, but none ever have. MacOSX x86 and x86_64 has never done this right. I think this if statement is too inclusive. Can you narrow it down? I would love to see an example of this in action. I have never seen the x86 PIC bump code be properly described in any compiler. Are we trying to use this for unwinding frame zero? I would hope not unless we really and carefully know the exact compiler that produced the info. It would be preferable marked with an augmentation code that says "yes, I am really complete everywhere".


================
Comment at: source/Symbol/UnwindTable.cpp:62
+    m_debug_frame_up.reset(
+        new DWARFCallFrameInfo(m_object_file, sect, eRegisterKindDWARF, false));
+  }
----------------
Update to use DWARFCallFrameInfo::Type



https://reviews.llvm.org/D34613