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

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

lldb/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();
 
+  template<class Contains>
+  DWARFUnit *GetCompileUnitTmpl(
+      Contains contains, dw_offset_t offset, uint32_t *idx_ptr = NULL);
+
   DISALLOW_COPY_AND_ASSIGN(DWARFDebugInfo);
 };
 
+template<class Contains>
+DWARFUnit *DWARFDebugInfo::GetCompileUnitTmpl(
+    Contains contains, dw_offset_t offset, uint32_t *idx_ptr) {
+  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 (contains(*m_compile_units[0], offset)) {
+      if (idx_ptr)
+        *idx_ptr = 0;
+      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, offset, OffsetLessThanCompileUnitOffset);
+    if (pos != begin_pos) {
+      --pos;
+      if (contains(**pos, offset)) {
+        if (idx_ptr)
+          *idx_ptr = pos - begin_pos;
+        return (*pos).get();
+      }
+    }
+  }
+
+  if (idx_ptr)
+    *idx_ptr = DW_INVALID_INDEX;
+  return nullptr;
+}
+
 #endif // SymbolFileDWARF_DWARFDebugInfo_h_

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

 
 bool DWARFDebugInfo::OffsetLessThanCompileUnitOffset(
     dw_offset_t offset, const DWARFUnitSP &cu_sp) {
   return offset < cu_sp->GetOffset();
 }
 
 DWARFUnit *DWARFDebugInfo::GetCompileUnit(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();
-
-    // 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);
-        }
-      }
-    }
-  }
-  if (idx_ptr)
-    *idx_ptr = cu_idx;
-  return cu_sp.get();
+  return GetCompileUnitTmpl(
+      [](const DWARFUnit &unit, dw_offset_t cu_offset) {
+        return unit.GetOffset() == cu_offset;
+      }, cu_offset, idx_ptr);
 }
 
 DWARFUnit *DWARFDebugInfo::GetCompileUnit(const DIERef &die_ref) {
   if (die_ref.cu_offset == DW_INVALID_OFFSET)
     return GetCompileUnitContainingDIEOffset(die_ref.die_offset);
   else
     return GetCompileUnit(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();
-    }
-  }
-
-  return nullptr;
+  return GetCompileUnitTmpl(
+      [](const DWARFUnit &unit, dw_offset_t die_offset) {
+        return unit.ContainsDIEOffset(die_offset);
+      }, die_offset);
 }
 
 DWARFDIE
 DWARFDebugInfo::GetDIEForDIEOffset(dw_offset_t die_offset) {
   DWARFUnit *cu = GetCompileUnitContainingDIEOffset(die_offset);
   if (cu)
     return cu->GetDIE(die_offset);
   return DWARFDIE();