Fix FDE indexing while scan debug_info section

[tatyana-krasnukha]

Summary

There are some differences between eh_frame and debug_frame formats that are not considered by DWARFCallFrameInfo::GetFDEIndex.
An FDE entry contains CIE_pointer in debug_frame in same place as cie_id in eh_frame. As described in dwarf standard (section 6.4.1), CIE_pointer is an "offset into the .debug_frame section". So, variable cie_offset should be equal cie_id for debug_frame.
FDE entries with zeroth CIE pointer (which is actually placed in cie_id variable) shouldn't be ignored also.

I had same issue as described here, and these changes have fixed it for me (with "m_is_eh_frame" set to false, of course).

I have also added a little change which allow to use debug_info section when eh_frame is absent. This case really can take place on some platforms.

[abidh]

@tatyana-krasnukha Please make sure that patch is generated from the top level lldb directory.

Committed in r303847.

[labath]

Guys, this breaks a whole bunch of unwinding tests on android: http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-android/builds/4243

[labath]

I have reverted this commit, because we are still parsing debug_frame entries incorrectly. The first issue I ran into was parsing of CIE entries (which have a somewhat different layout in the debug_frame sections, and we're not taking that into account), but there may be others. We cannot enable debug_frame parsing until we can parse it properly. The issue is not android specific, as the earlier comment might have suggested, it just so happens that the android clang is the only compiler in our test cluster that produces debug_frame sections. Nonetheless, I can provide you with some example android binaries if you want to look at them more closely.

[tatyana-krasnukha]

Yes, give those binaries, please.

[tatyana-krasnukha]

Added support of DWARF <= 5. For unsupported versions GetUnwindPlan will return false and next behavior should be like without debug_frame at all. Checked it on binaries with v.2 and v.4 on x86-linux target, but have not a chance to run tests for arm-android and other. I'd appreciate it if someone, who has adjusted environment, would check it.

[labath]

Thank you for adding the support for dwarf 4 CIE. I think it's a good start, but I believe the version check needs to be done in a smarter way.

On the flip side, I tried running this through the test suite on android, and I noticed only one test failing for arm64. I am going to try digging into this more later.

source/Symbol/DWARFCallFrameInfo.cpp

482

Unprintable character? Same thing two lines below.

source/Symbol/UnwindTable.cpp

70

So, this checks the version number of the *first* CIE in the debug_frame section right. That is incorrect, as the same section could have CIEs with different version numbers (In fact, that may be quite likely, as a module will usually contain some startup code, which will probably have been built with a different compiler than the user code). I think the "supported" decision should be made per-CIE, not for the whole section (or at the very least, check that *all* CIEs in the section have supported version numbers). This means the will need to be made in DWARFCallFrameInfo.cpp, which is good anyway, as the fact that you are reaching inside the section from UnwindTable.cpp was my other issue with this patch.

71

The spec says "This number is specific to the call frame information and is independent of the DWARF version number. ", so it's probably not right to call this "DwarfVersion"

[tatyana-krasnukha]

source/Symbol/DWARFCallFrameInfo.cpp

482

Hm... It's strange, I formatted it with clang-format tool.

source/Symbol/UnwindTable.cpp

70

Not exactly true. This code checks DWARF version in debug_info section, if it exists. Its only purpose is to avoid extra actions if it is already known that the version is unsupported.

Version number of each CIE is checked in ParseCIE method.

71

It is not same, but it is not independent too. Look on comments for enum CFIVersion in DWARFCallFrameInfo.h

[labath]

source/Symbol/DWARFCallFrameInfo.cpp

482

clang-format does not remove non-ascii characters. You need to do that yourself.

source/Symbol/UnwindTable.cpp

70

Ok, so you're checking the version of the first compile unit. It still sounds wrong to me. What if the second compile unit has unsupported version?

If this is just an attempt at a performance optimization, then I say we just remove it. We can attempt to optimize performance differently if it ever becomes an issue.

71

I didn't notice that you are checking the debug_info section. You can ignore this comment.

[tatyana-krasnukha]

Removed non-printable characters from DWARFCallFrameInfo::GetFDEIndex and redundant version checks from UnwindTable::Initialize.

[jasonmolenda]

I don't have any comments on this - it seems like a good addition. We've almost entirely stopped using eh_frame/debug_frame on Darwin so I don't have any way to test this. When we were adding the eh_frame/debug_frame parser, clang (and I think gcc) were both generating the same thing for both sections, so there was no benefit to reading debug_frame. Greg wrote it with the idea that the code could process either source of information, but we never actually did that with it. I'm sure there are targets where a binary might have debug_frame but not eh_frame, and this change could be a big help there.

[labath]

I've looked into the other test failures I was seeing on arm64 after merging this in. I have a fix for this up in D34199.

As for the testing situation, theoretically we could claim this is tested, as android arm targets will use this code, but probably nobody except our bots will be able to exercise it. One possible way to test this in a more targeted way would be to go with something similar to https://reviews.llvm.org/D32434. I.e.,

• build an tiny executable with a .debug_frame section
• run obj2yaml on it
• strip down the resulting yaml file (you probably only need the .text and .debug_frame section)
• manually create a Module corresponding to the yaml file (as in the test)
• construct a DWARFCallFrameInfo based on the Module
• ask the call frame info for an unwind plan an verify it

I haven't tried constructing a DWARFCallFrameInfo in a unit test, but given that it only takes an ObjectFile in it's constructor, it looks like it should be possible to do it without having to instantiate the whole world.

include/lldb/Symbol/DWARFCallFrameInfo.h

   DWARFCallFrameInfo(ObjectFile &objfile, lldb::SectionSP &section,
                      lldb::RegisterKind reg_kind, bool is_eh_frame);
 
-  ~DWARFCallFrameInfo();
+  ~DWARFCallFrameInfo() = default;
 
   // Locate an AddressRange that includes the provided Address in this
   // object's eh_frame/debug_info

 
 private:
   enum { CFI_AUG_MAX_SIZE = 8, CFI_HEADER_SIZE = 8 };
+  enum CFIVersion {
+    CFI_VERSION1 = 1, // DWARF v.2
+    CFI_VERSION3 = 3, // DWARF v.3
+    CFI_VERSION4 = 4  // DWARF v.4, v.5
+  };
 
   struct CIE {
     dw_offset_t cie_offset;

     uint8_t version;
     char augmentation[CFI_AUG_MAX_SIZE]; // This is typically empty or very
                                          // short.
+    uint8_t address_size = sizeof(uint32_t); // The size of a target address.
+    uint8_t segment_size = 0;                // The size of a segment selector.
+
     uint32_t code_align;
     int32_t data_align;
     uint32_t return_addr_reg_num;

source/Symbol/DWARFCallFrameInfo.cpp

       m_fde_index(), m_fde_index_initialized(false),
       m_is_eh_frame(is_eh_frame) {}
 
-DWARFCallFrameInfo::~DWARFCallFrameInfo() {}
-
 bool DWARFCallFrameInfo::GetUnwindPlan(Address addr, UnwindPlan &unwind_plan) {
   FDEEntryMap::Entry fde_entry;
 

     //    cie.cieID = cieID;
     cie_sp->ptr_encoding = DW_EH_PE_absptr; // default
     cie_sp->version = m_cfi_data.GetU8(&offset);
+    if (cie_sp->version > CFI_VERSION4) {
+      Host::SystemLog(Host::eSystemLogError,
+                      "CIE parse error: CFI version %d is not supported\n",
+                      cie_sp->version);
+      return nullptr;
+    }
 
     for (i = 0; i < CFI_AUG_MAX_SIZE; ++i) {
       cie_sp->augmentation[i] = m_cfi_data.GetU8(&offset);

                       "CIE parse error: CIE augmentation string was too large "
                       "for the fixed sized buffer of %d bytes.\n",
                       CFI_AUG_MAX_SIZE);
-      return cie_sp;
+      return nullptr;
     }
+
+    // m_cfi_data uses address size from target architecture of the process
+    // may ignore these fields?
+    if (!m_is_eh_frame && cie_sp->version >= CFI_VERSION4) {
+      cie_sp->address_size = m_cfi_data.GetU8(&offset);
+      cie_sp->segment_size = m_cfi_data.GetU8(&offset);
+    }
+
     cie_sp->code_align = (uint32_t)m_cfi_data.GetULEB128(&offset);
     cie_sp->data_align = (int32_t)m_cfi_data.GetSLEB128(&offset);
-    cie_sp->return_addr_reg_num = m_cfi_data.GetU8(&offset);
 
+    cie_sp->return_addr_reg_num =
+        !m_is_eh_frame && cie_sp->version >= CFI_VERSION3
+            ? static_cast<uint32_t>(m_cfi_data.GetULEB128(&offset))
+            : m_cfi_data.GetU8(&offset);
+
     if (cie_sp->augmentation[0]) {
       // Get the length of the eh_frame augmentation data
       // which starts with a ULEB128 length in bytes

       m_fde_index_initialized = true;
       return;
     }
+
+    // An FDE entry contains CIE_pointer in debug_frame in same place as cie_id
+    // in eh_frame. CIE_pointer is an offset into the .debug_frame section.
+    // So, variable cie_offset should be equal to cie_id for debug_frame.
+    // 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);
+      if (!cie_sp) {
+        // Cannot parse, the reason is already logged
+        m_fde_index.Clear();
+        m_fde_index_initialized = true;
+        return;
+      }
+
+      m_cie_map[current_entry] = std::move(cie_sp);
+      offset = next_entry;
+      continue;
+    }
+
+    if (!m_is_eh_frame)
+      cie_offset = cie_id;
+
     if (cie_offset > m_cfi_data.GetByteSize()) {
-      Host::SystemLog(
-          Host::eSystemLogError,
-          "error: Invalid cie offset of 0x%x found in cie/fde at 0x%x\n",
+      Host::SystemLog(Host::eSystemLogError,
+                      "error: Invalid cie offset of 0x%x "
+                      "found in cie/fde at 0x%x\n",
                       cie_offset, current_entry);
       // Don't trust anything in this eh_frame section if we find blatantly
       // invalid data.
       m_fde_index.Clear();

       return;
     }
 
-    if (cie_id == 0 || cie_id == UINT32_MAX || len == 0) {
-      m_cie_map[current_entry] = ParseCIE(current_entry);
-      offset = next_entry;
-      continue;
-    }
-
     const CIE *cie = GetCIE(cie_offset);
     if (cie) {
       const lldb::addr_t pc_rel_addr = m_section_sp->GetFileAddress();

     cie_offset = m_cfi_data.GetU32(&offset);
   }
 
-  assert(cie_offset != 0 && cie_offset != UINT32_MAX);
+  // FDE entries with zeroth cie_offset may occur for debug_frame.
+  assert(!(m_is_eh_frame && 0 == cie_offset) && cie_offset != UINT32_MAX);
 
   // Translate the CIE_id from the eh_frame format, which
   // is relative to the FDE offset, into a __eh_frame section

source/Symbol/UnwindTable.cpp

     if (sect.get()) {
       m_eh_frame_up.reset(new DWARFCallFrameInfo(m_object_file, sect,
                                                  eRegisterKindEHFrame, true));
+    } else {
+      // Try to find .debug_frame section even if .eh_frame doesn't exist.
+      sect = sl->FindSectionByType(eSectionTypeDWARFDebugFrame, true);
+      if (sect) {
+        m_eh_frame_up.reset(new DWARFCallFrameInfo(m_object_file, sect,
+                                                   eRegisterKindDWARF, false));
+      }
     }
     sect = sl->FindSectionByType(eSectionTypeCompactUnwind, true);
     if (sect.get()) {