01/06: Merge GetCompileUnit + GetCompileUnitContainingDIEOffset

[jankratochvil]

Summary

These two methods are very similar and various refactorizations need to modify both similar ways. Merge them as a template.

One could also just remove GetCompileUnit and make GetCompileUnitContainingDIEOffset to also accept offset of the CU itself (currently it accepts only DIE offsets after the CU header). But that would be less safe regarding some internal sanity checking.

The whole patchset available for testing as: git clone -b debugtypes git://git.jankratochvil.net/lldb

[labath]

What you could do instead is have a single function which returns the CU if it is in the range cu.GetOffset() <= input < cu.GetNextUnitOffset(), then the wrapper functions could just perform a more stricter check on the returned cu (I.e. no templates or callbacks).

This may lower the need for this refactoring, but independently of that, I don't believe we should touch this function without simplifying it into something more like

uint32_t index = DW_INVALID_INDEX;
DWARFUnit result = nullptr;
auto pos = llvm::lower_bound(m_compile_units, offset, OffsetLessThanCompileUnitOffset);
if (pos != m_compile_units.end() && ???) {
  index = std::distance(...);
  result = ...
}
if (idx_ptr) *idx_ptr = index;
return result;

as the current implementation is hard to understand for no good reason.

[jankratochvil]

In D61498#1489682, @labath wrote:

What you could do instead is have a single function which returns the CU if it is in the range cu.GetOffset() <= input < cu.GetNextUnitOffset(), then the wrapper functions could just perform a more stricter check on the returned cu (I.e. no templates or callbacks).

OK, I agree; originally I did not want to touch more code than what I needed for .debug_types.

[labath]

Thanks, this looks much better.

In D61498#1490885, @jankratochvil wrote:

OK, I agree; originally I did not want to touch more code than what I needed for .debug_types.

I think that "not wanting to touch more code than needed" was the reason why all previous attemtps at debug_types were hacky in one way or another. :)

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

129–130 ↗

(On Diff #198148)

Ah, I think I finally understand the --pos thingy. :)

lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h

protected:
   std::unique_ptr<DWARFDebugAranges>
       m_cu_aranges_up; // A quick address to compile unit table
 
 private:
   // All parsing needs to be done partially any managed by this class as
   // accessors are called.
   void ParseCompileUnitHeadersIfNeeded();
 
+  uint32_t FindCompileUnitIndex(dw_offset_t offset);
+
   DISALLOW_COPY_AND_ASSIGN(DWARFDebugInfo);
 };
 
 #endif // SymbolFileDWARF_DWARFDebugInfo_h_

lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp

DWARFUnit *DWARFDebugInfo::GetCompileUnitAtIndex(user_id_t idx) {
   return cu;
 }
 
 bool DWARFDebugInfo::OffsetLessThanCompileUnitOffset(
     dw_offset_t offset, const DWARFUnitSP &cu_sp) {
   return offset < cu_sp->GetOffset();
 }
 
-DWARFUnit *DWARFDebugInfo::GetCompileUnitAtOffset(dw_offset_t cu_offset,
-                                                  uint32_t *idx_ptr) {
-  DWARFUnitSP cu_sp;
-  uint32_t cu_idx = DW_INVALID_INDEX;
-  if (cu_offset != DW_INVALID_OFFSET) {
-    ParseCompileUnitHeadersIfNeeded();
+uint32_t DWARFDebugInfo::FindCompileUnitIndex(dw_offset_t offset) {
 
-    // Watch out for single compile unit executable as they are pretty common
-    const size_t num_cus = m_compile_units.size();
-    if (num_cus == 1) {
-      if (m_compile_units[0]->GetOffset() == cu_offset) {
-        cu_sp = m_compile_units[0];
-        cu_idx = 0;
-      }
-    } else if (num_cus) {
-      CompileUnitColl::const_iterator end_pos = m_compile_units.end();
-      CompileUnitColl::const_iterator begin_pos = m_compile_units.begin();
-      CompileUnitColl::const_iterator pos = std::upper_bound(
-          begin_pos, end_pos, cu_offset, OffsetLessThanCompileUnitOffset);
-      if (pos != begin_pos) {
-        --pos;
-        if ((*pos)->GetOffset() == cu_offset) {
-          cu_sp = *pos;
-          cu_idx = std::distance(begin_pos, pos);
-        }
-      }
-    }
+  // llvm::lower_bound is not used as for DIE offsets it would still return
+  // index +1 and GetOffset() returning index itself would be a special case.
+  auto pos = llvm::upper_bound(m_compile_units, offset,
+                               OffsetLessThanCompileUnitOffset);
+  uint32_t idx = std::distance(m_compile_units.begin(), pos);
+  if (idx == 0)
+    return DW_INVALID_OFFSET;
+  return idx - 1;
+
+DWARFUnit *DWARFDebugInfo::GetCompileUnitAtOffset(dw_offset_t cu_offset,
+                                                  uint32_t *idx_ptr) {
+  uint32_t idx = FindCompileUnitIndex(cu_offset);
+  DWARFUnit *result = GetCompileUnitAtIndex(idx);
+  if (result && result->GetOffset() != cu_offset) {
+    result = nullptr;
+    idx = DW_INVALID_INDEX;
   }
   if (idx_ptr)
-    *idx_ptr = cu_idx;
-  return cu_sp.get();
+    *idx_ptr = idx;
+  return result;
 }
 
 DWARFUnit *DWARFDebugInfo::GetCompileUnit(const DIERef &die_ref) {
   if (die_ref.cu_offset == DW_INVALID_OFFSET)
     return GetCompileUnitContainingDIEOffset(die_ref.die_offset);
   else
     return GetCompileUnitAtOffset(die_ref.cu_offset);
 }
 
 DWARFUnit *
 DWARFDebugInfo::GetCompileUnitContainingDIEOffset(dw_offset_t die_offset) {
-  ParseCompileUnitHeadersIfNeeded();
-
-  DWARFUnitSP cu_sp;
-
-  // Watch out for single compile unit executable as they are pretty common
-  const size_t num_cus = m_compile_units.size();
-  if (num_cus == 1) {
-    if (m_compile_units[0]->ContainsDIEOffset(die_offset))
-      return m_compile_units[0].get();
-  } else if (num_cus) {
-    CompileUnitColl::const_iterator end_pos = m_compile_units.end();
-    CompileUnitColl::const_iterator begin_pos = m_compile_units.begin();
-    CompileUnitColl::const_iterator pos = std::upper_bound(
-        begin_pos, end_pos, die_offset, OffsetLessThanCompileUnitOffset);
-    if (pos != begin_pos) {
-      --pos;
-      if ((*pos)->ContainsDIEOffset(die_offset))
-        return (*pos).get();
-    }
-  }
-
+  uint32_t idx = FindCompileUnitIndex(die_offset);
+  DWARFUnit *result = GetCompileUnitAtIndex(idx);
+  if (result && !result->ContainsDIEOffset(die_offset))
     return nullptr;
+  return result;
 }
 
 DWARFDIE
 DWARFDebugInfo::GetDIEForDIEOffset(dw_offset_t die_offset) {
   DWARFUnit *cu = GetCompileUnitContainingDIEOffset(die_offset);
   if (cu)
     return cu->GetDIE(die_offset);
   return DWARFDIE();