[lldb][NFCI] Apply IndexEntry to DWARFUnitHeader outside of extraction

[bulbazord]

Summary

I plan on replacing LLDB's DWARFUnitHeader implementation with LLVM's.
LLVM's DWARFUnitHeader::extract applies the DWARFUnitIndex::Entry to a
given DWARFUnitHeader outside of the extraction because the index entry
is only relevant to one place where we may parse DWARFUnitHeaders
(specifically when we're creating a DWARFUnit in a DWO context). To ease
the transition, I've reshaped LLDB's implementation to look closer to
LLVM's.

[aprantl]

lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

891

I know this was also in the original code, so you may not know either: Should these error messages be limited to dwarf packages (.dwp) or should they be more generic?

982

llvm coding style would delete most of these {} ?

[bulbazord]

lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

891

I'm not sure, but my understanding is that we only apply the index entry to the header in DWO contexts, so I assume DWP is involved.

982

Sounds good, I'll update.

[fdeazeve]

I'm not exactly familiar with DWOs, but the code motions LGTM!

lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

884

If LLVM's function is like this as well, we shouldn't do anything now and just refactor it later once we make the switch. But a pointer parameter in a function that asserts not null immediately is more expressive as a reference. Note that, in the code where this is called, we have a:

if (ptr)
  ApplyIndex(ptr)

So the assert is not needed, as the callee respects the contract and can do a *ptr

[bulbazord]

lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

884

Yes, this was my thought process as well. I wrote it this way because LLVM's function is just like this. I'm also touching that equivalent function in LLVM in a different way (adding error handling, see https://reviews.llvm.org/D151933). Let's rework this in a follow-up?

[bulbazord]

Changed a bit of formatting to follow LLVM style more closely.

lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h

public:
   dw_offset_t GetTypeOffset() const { return m_type_offset; }
   std::optional<uint64_t> GetDWOId() const { return m_dwo_id; }
   bool IsTypeUnit() const {
     return m_unit_type == llvm::dwarf::DW_UT_type ||
            m_unit_type == llvm::dwarf::DW_UT_split_type;
   }
   uint32_t GetNextUnitOffset() const { return m_offset + m_length + 4; }
 
+  llvm::Error ApplyIndexEntry(const llvm::DWARFUnitIndex::Entry *index_entry);
+
   static llvm::Expected<DWARFUnitHeader>
   extract(const lldb_private::DWARFDataExtractor &data, DIERef::Section section,
           lldb_private::DWARFContext &dwarf_context,
           lldb::offset_t *offset_ptr);
 };
 
 class DWARFUnit : public lldb_private::UserID {
   using die_iterator_range =

lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

if (m_func_aranges_up == nullptr) {
     }
 
     const bool minimize = false;
     m_func_aranges_up->Sort(minimize);
   }
   return *m_func_aranges_up;
 }
 
-llvm::Expected<DWARFUnitHeader>
-DWARFUnitHeader::extract(const DWARFDataExtractor &data,
-                         DIERef::Section section,
-                         lldb_private::DWARFContext &context,
-                         lldb::offset_t *offset_ptr) {
+llvm::Error DWARFUnitHeader::ApplyIndexEntry(
+    const llvm::DWARFUnitIndex::Entry *index_entry) {
+  // We should only be calling this function when the index entry is not set and
+  // we have a valid one to set it to.
+  assert(index_entry);
+  assert(!m_index_entry);
+
+  if (m_abbr_offset)
+    return llvm::createStringError(
+        llvm::inconvertibleErrorCode(),
+        "Package unit with a non-zero abbreviation offset");
+
+  auto *unit_contrib = index_entry->getContribution();
+  if (!unit_contrib || unit_contrib->getLength32() != m_length + 4)
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "Inconsistent DWARF package unit index");
+
+  auto *abbr_entry = index_entry->getContribution(llvm::DW_SECT_ABBREV);
+  if (!abbr_entry)
+    return llvm::createStringError(
+        llvm::inconvertibleErrorCode(),
+        "DWARF package index missing abbreviation column");
+
+  m_abbr_offset = abbr_entry->getOffset();
+  m_index_entry = index_entry;
+  return llvm::Error::success();
+}
+
+llvm::Expected<DWARFUnitHeader> DWARFUnitHeader::extract(
+    const DWARFDataExtractor &data, DIERef::Section section,
+    lldb_private::DWARFContext &context, lldb::offset_t *offset_ptr) {
   DWARFUnitHeader header;
   header.m_offset = *offset_ptr;
   header.m_length = data.GetDWARFInitialLength(offset_ptr);
   header.m_version = data.GetU16(offset_ptr);
   if (header.m_version == 5) {
     header.m_unit_type = data.GetU8(offset_ptr);
     header.m_addr_size = data.GetU8(offset_ptr);
     header.m_abbr_offset = data.GetDWARFOffset(offset_ptr);
     if (header.m_unit_type == llvm::dwarf::DW_UT_skeleton ||
         header.m_unit_type == llvm::dwarf::DW_UT_split_compile)
       header.m_dwo_id = data.GetU64(offset_ptr);
   } else {
     header.m_abbr_offset = data.GetDWARFOffset(offset_ptr);
     header.m_addr_size = data.GetU8(offset_ptr);
     header.m_unit_type =
         section == DIERef::Section::DebugTypes ? DW_UT_type : DW_UT_compile;
   }
 
   if (header.IsTypeUnit()) {
     header.m_type_hash = data.GetU64(offset_ptr);
     header.m_type_offset = data.GetDWARFOffset(offset_ptr);
   }
 
-  if (context.isDwo()) {
-    const llvm::DWARFUnitIndex *Index;
-    if (header.IsTypeUnit()) {
-      Index = &context.GetAsLLVM().getTUIndex();
-      if (*Index)
-        header.m_index_entry = Index->getFromHash(header.m_type_hash);
-    } else {
-      Index = &context.GetAsLLVM().getCUIndex();
-      if (*Index && header.m_version >= 5 && header.m_dwo_id)
-        header.m_index_entry = Index->getFromHash(*header.m_dwo_id);
-    }
-    if (!header.m_index_entry)
-      header.m_index_entry = Index->getFromOffset(header.m_offset);
-  }
-
-  if (header.m_index_entry) {
-    if (header.m_abbr_offset) {
-      return llvm::createStringError(
-          llvm::inconvertibleErrorCode(),
-          "Package unit with a non-zero abbreviation offset");
-    }
-    auto *unit_contrib = header.m_index_entry->getContribution();
-    if (!unit_contrib || unit_contrib->getLength32() != header.m_length + 4) {
-      return llvm::createStringError(llvm::inconvertibleErrorCode(),
-                                     "Inconsistent DWARF package unit index");
-    }
-    auto *abbr_entry =
-        header.m_index_entry->getContribution(llvm::DW_SECT_ABBREV);
-    if (!abbr_entry) {
-      return llvm::createStringError(
-          llvm::inconvertibleErrorCode(),
-          "DWARF package index missing abbreviation column");
-    }
-    header.m_abbr_offset = abbr_entry->getOffset();
-  }
-
   bool length_OK = data.ValidOffset(header.GetNextUnitOffset() - 1);
   bool version_OK = SymbolFileDWARF::SupportedVersion(header.m_version);
   bool addr_size_OK = (header.m_addr_size == 2) || (header.m_addr_size == 4) ||
                       (header.m_addr_size == 8);
   bool type_offset_OK =
       !header.IsTypeUnit() || (header.m_type_offset <= header.GetLength());
 
   if (!length_OK)

 }
 
 llvm::Expected<DWARFUnitSP>
 DWARFUnit::extract(SymbolFileDWARF &dwarf, user_id_t uid,
                    const DWARFDataExtractor &debug_info,
                    DIERef::Section section, lldb::offset_t *offset_ptr) {
   assert(debug_info.ValidOffset(*offset_ptr));
 
-  auto expected_header = DWARFUnitHeader::extract(
-      debug_info, section, dwarf.GetDWARFContext(), offset_ptr);
+  DWARFContext &context = dwarf.GetDWARFContext();
+  auto expected_header =
+      DWARFUnitHeader::extract(debug_info, section, context, offset_ptr);
   if (!expected_header)
     return expected_header.takeError();
 
+  if (context.isDwo()) {
+    const llvm::DWARFUnitIndex::Entry *entry = nullptr;
+    const llvm::DWARFUnitIndex &index = expected_header->IsTypeUnit()
+                                            ? context.GetAsLLVM().getTUIndex()
+                                            : context.GetAsLLVM().getCUIndex();
+    if (index) {
+      if (expected_header->IsTypeUnit())
+        entry = index.getFromHash(expected_header->GetTypeHash());
+      else if (auto dwo_id = expected_header->GetDWOId())
+        entry = index.getFromHash(*dwo_id);
+    }
+    if (!entry)
+      entry = index.getFromOffset(expected_header->GetOffset());
+    if (entry)
+      if (llvm::Error err = expected_header->ApplyIndexEntry(entry))
+        return err;
+  }
+
   const llvm::DWARFDebugAbbrev *abbr = dwarf.DebugAbbrev();
   if (!abbr)
     return llvm::make_error<llvm::object::GenericBinaryError>(
         "No debug_abbrev data");
 
   bool abbr_offset_OK =
       dwarf.GetDWARFContext().getOrLoadAbbrevData().ValidOffset(
           expected_header->GetAbbrOffset());