[lldb][NFCI] Remove duplicated code in DWARFParser

[felipepiovezan]

The method DWARFDebugInfoEntry::Extract needs to skip over all the data in the debug_info / debug_types section for each DIE. It had the logic to do so hardcoded inside a loop, when it already exists in a neatly isolated function.

[llvmbot]

@llvm/pr-subscribers-lldb

@llvm/pr-subscribers-debuginfo

Author: Felipe de Azevedo Piovezan (felipepiovezan)

Changes

The method DWARFDebugInfoEntry::Extract needs to skip over all the data in the debug_info / debug_types section for each DIE. It had the logic to do so hardcoded inside a loop, when it already exists in a neatly isolated function.

---

Full diff: https://github.com/llvm/llvm-project/pull/69531.diff

1 Files Affected:

• (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp (+9-132)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
index a6ab83700904cb9..46e5e209c7e13c6 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
@@ -49,16 +49,12 @@ bool DWARFDebugInfoEntry::Extract(const DWARFDataExtractor &data,
   lldbassert(abbr_idx <= UINT16_MAX);
   m_abbr_idx = abbr_idx;
 
-  // assert (fixed_form_sizes);  // For best performance this should be
-  // specified!
-
   if (m_abbr_idx == 0) {
     m_tag = llvm::dwarf::DW_TAG_null;
     m_has_children = false;
     return true; // NULL debug tag entry
   }
 
-  lldb::offset_t offset = *offset_ptr;
   const auto *abbrevDecl = GetAbbreviationDeclarationPtr(cu);
   if (abbrevDecl == nullptr) {
     cu->GetSymbolFileDWARF().GetObjectFile()->GetModule()->ReportError(
@@ -73,137 +69,18 @@ bool DWARFDebugInfoEntry::Extract(const DWARFDataExtractor &data,
   m_tag = abbrevDecl->getTag();
   m_has_children = abbrevDecl->hasChildren();
   // Skip all data in the .debug_info or .debug_types for the attributes
-  dw_form_t form;
   for (const auto &attribute : abbrevDecl->attributes()) {
-    form = attribute.Form;
-    std::optional<uint8_t> fixed_skip_size =
-        DWARFFormValue::GetFixedSize(form, cu);
-    if (fixed_skip_size)
-      offset += *fixed_skip_size;
-    else {
-      bool form_is_indirect = false;
-      do {
-        form_is_indirect = false;
-        uint32_t form_size = 0;
-        switch (form) {
-        // Blocks if inlined data that have a length field and the data bytes
-        // inlined in the .debug_info/.debug_types
-        case DW_FORM_exprloc:
-        case DW_FORM_block:
-          form_size = data.GetULEB128(&offset);
-          break;
-        case DW_FORM_block1:
-          form_size = data.GetU8_unchecked(&offset);
-          break;
-        case DW_FORM_block2:
-          form_size = data.GetU16_unchecked(&offset);
-          break;
-        case DW_FORM_block4:
-          form_size = data.GetU32_unchecked(&offset);
-          break;
-
-        // Inlined NULL terminated C-strings
-        case DW_FORM_string:
-          data.GetCStr(&offset);
-          break;
-
-        // Compile unit address sized values
-        case DW_FORM_addr:
-          form_size = cu->GetAddressByteSize();
-          break;
-        case DW_FORM_ref_addr:
-          if (cu->GetVersion() <= 2)
-            form_size = cu->GetAddressByteSize();
-          else
-            form_size = 4;
-          break;
+    if (DWARFFormValue::SkipValue(attribute.Form, data, offset_ptr, cu))
+      continue;
 
-        // 0 sized form
-        case DW_FORM_flag_present:
-          form_size = 0;
-          break;
-
-        // 1 byte values
-        case DW_FORM_addrx1:
-        case DW_FORM_data1:
-        case DW_FORM_flag:
-        case DW_FORM_ref1:
-        case DW_FORM_strx1:
-          form_size = 1;
-          break;
-
-        // 2 byte values
-        case DW_FORM_addrx2:
-        case DW_FORM_data2:
-        case DW_FORM_ref2:
-        case DW_FORM_strx2:
-          form_size = 2;
-          break;
-
-        // 3 byte values
-        case DW_FORM_addrx3:
-        case DW_FORM_strx3:
-          form_size = 3;
-          break;
-
-        // 4 byte values
-        case DW_FORM_addrx4:
-        case DW_FORM_data4:
-        case DW_FORM_ref4:
-        case DW_FORM_strx4:
-          form_size = 4;
-          break;
-
-        // 8 byte values
-        case DW_FORM_data8:
-        case DW_FORM_ref8:
-        case DW_FORM_ref_sig8:
-          form_size = 8;
-          break;
-
-        // signed or unsigned LEB 128 values
-        case DW_FORM_addrx:
-        case DW_FORM_loclistx:
-        case DW_FORM_rnglistx:
-        case DW_FORM_sdata:
-        case DW_FORM_udata:
-        case DW_FORM_ref_udata:
-        case DW_FORM_GNU_addr_index:
-        case DW_FORM_GNU_str_index:
-        case DW_FORM_strx:
-          data.Skip_LEB128(&offset);
-          break;
-
-        case DW_FORM_indirect:
-          form_is_indirect = true;
-          form = static_cast<dw_form_t>(data.GetULEB128(&offset));
-          break;
-
-        case DW_FORM_strp:
-        case DW_FORM_line_strp:
-        case DW_FORM_sec_offset:
-          data.GetU32(&offset);
-          break;
-
-        case DW_FORM_implicit_const:
-          form_size = 0;
-          break;
-
-        default:
-          cu->GetSymbolFileDWARF().GetObjectFile()->GetModule()->ReportError(
-              "[{0:x16}]: Unsupported DW_FORM_{1:x}, please file a bug "
-              "and "
-              "attach the file at the start of this error message",
-              (uint64_t)m_offset, (unsigned)form);
-          *offset_ptr = m_offset;
-          return false;
-        }
-        offset += form_size;
-
-      } while (form_is_indirect);
-    }
+    cu->GetSymbolFileDWARF().GetObjectFile()->GetModule()->ReportError(
+        "[{0:x16}]: Unsupported DW_FORM_{1:x}, please file a bug "
+        "and "
+        "attach the file at the start of this error message",
+        (uint64_t)m_offset, (unsigned)attribute.Form);
+    *offset_ptr = m_offset;
+    return false;
   }
-  *offset_ptr = offset;
   return true;
 }
 

[clayborg]

Please manually verify all cases are handled in the DWARFFormValue::SkipValue() function to ensure we are not losing functionality. LGTM.

[felipepiovezan]

Please manually verify all cases are handled in the DWARFFormValue::SkipValue() function to ensure we are not losing functionality. LGTM.

Yup, I've compared the two functions twice, so I'm fairly confident they are identical :)

[bulbazord]

I looked at the implementations and I didn't notice anything too different either. This brings us from 3 implementations of this logic to 2. Hopefully we can make it 1 someday soon. 😄

[JDevlieghere]

🥳