[Lldb-commits] [PATCH] D61482: [DWARF] Store compile unit IDs in the DIERef class

[Pavel Labath via Phabricator via lldb-commits]

labath abandoned this revision.
labath added a comment.

Obsoleted by D61908 .


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61482/new/

https://reviews.llvm.org/D61482



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61482: [DWARF] Store compile unit IDs in the DIERef class

[Pavel Labath via Phabricator via lldb-commits]

labath planned changes to this revision.
labath added a comment.

My understanding of the DIERef class has evolved. I'm going to revise this 
approach a bit.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61482/new/

https://reviews.llvm.org/D61482



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61482: [DWARF] Store compile unit IDs in the DIERef class

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Hm... looking at @jankratochvil's patches, I realized that I completely omitted 
HashedNameToDIE from this patch (it used `emplace_back` to construct DIERefs, 
so it snuck past me). Thinking about it, I guess the best way to handle that 
would be to set the cu_idx to the invalid value for DIERefs created there, as 
the offset is sufficient to identify the die in those cases (which I guess is 
also the reason why all tests passed). That shouldn't impact the performance in 
any way as one way or the other one has to do a binary search to convert the 
offset into a DWARFUnit*. I'll try to update that on monday...


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61482/new/

https://reviews.llvm.org/D61482



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61482: [DWARF] Store compile unit IDs in the DIERef class

[Pavel Labath via Phabricator via lldb-commits]

labath updated this revision to Diff 197936.
labath added a comment.

Fix two more things I noticed when looking at the patch in phab:

- incorrect use of unit offset in DIERef constructor in the DebugMap case (I'm 
guessing this didn't cause any test failures because both offsets and IDs are 
generally zero there).
- incorrect local variable name in ManualDWARFIndex (NFC)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61482/new/

https://reviews.llvm.org/D61482

Files:
  source/Plugins/SymbolFile/DWARF/DIERef.cpp
  source/Plugins/SymbolFile/DWARF/DIERef.h
  source/Plugins/SymbolFile/DWARF/DWARFBaseDIE.cpp
  source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
  source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
  source/Plugins/SymbolFile/DWARF/DWARFUnit.h
  source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
  source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
  source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.h
  source/Plugins/SymbolFile/DWARF/NameToDIE.cpp
  source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp

Index: source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
===
--- source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
+++ source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
@@ -23,7 +23,7 @@
DWARFUnit *dwarf_cu)
 : SymbolFileDWARF(objfile.get()), m_obj_file_sp(objfile),
   m_base_dwarf_cu(dwarf_cu) {
-  SetID(((lldb::user_id_t)dwarf_cu->GetOffset()) << 32);
+  SetID(((lldb::user_id_t)dwarf_cu->GetID()) << 32);
 }
 
 void SymbolFileDWARFDwo::LoadSectionData(lldb::SectionType sect_type,
@@ -158,6 +158,6 @@
 
 DWARFDIE
 SymbolFileDWARFDwo::GetDIE(const DIERef &die_ref) {
-  lldbassert(m_base_dwarf_cu->GetOffset() == die_ref.cu_offset);
+  assert(m_base_dwarf_cu->GetID() == die_ref.cu_idx);
   return DebugInfo()->GetDIEForDIEOffset(die_ref.die_offset);
 }
Index: source/Plugins/SymbolFile/DWARF/NameToDIE.cpp
===
--- source/Plugins/SymbolFile/DWARF/NameToDIE.cpp
+++ source/Plugins/SymbolFile/DWARF/NameToDIE.cpp
@@ -38,13 +38,13 @@
   return m_map.GetValues(regex, info_array);
 }
 
-size_t NameToDIE::FindAllEntriesForCompileUnit(dw_offset_t cu_offset,
+size_t NameToDIE::FindAllEntriesForCompileUnit(dw_offset_t cu_idx,
DIEArray &info_array) const {
   const size_t initial_size = info_array.size();
   const uint32_t size = m_map.GetSize();
   for (uint32_t i = 0; i < size; ++i) {
 const DIERef &die_ref = m_map.GetValueAtIndexUnchecked(i);
-if (cu_offset == die_ref.cu_offset)
+if (cu_idx == die_ref.cu_idx)
   info_array.push_back(die_ref);
   }
   return info_array.size() - initial_size;
@@ -56,7 +56,7 @@
 ConstString cstr = m_map.GetCStringAtIndex(i);
 const DIERef &die_ref = m_map.GetValueAtIndexUnchecked(i);
 s->Printf("%p: {0x%8.8x/0x%8.8x} \"%s\"\n", (const void *)cstr.GetCString(),
-  die_ref.cu_offset, die_ref.die_offset, cstr.GetCString());
+  die_ref.cu_idx, die_ref.die_offset, cstr.GetCString());
   }
 }
 
Index: source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.h
===
--- source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.h
+++ source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.h
@@ -60,7 +60,7 @@
   static void
   IndexUnitImpl(DWARFUnit &unit, const lldb::LanguageType cu_language,
 const DWARFFormValue::FixedFormSizes &fixed_form_sizes,
-const dw_offset_t cu_offset, IndexSet &set);
+IndexSet &set);
 
   /// Non-null value means we haven't built the index yet.
   DWARFDebugInfo *m_debug_info;
Index: source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
===
--- source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
+++ source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
@@ -102,19 +102,19 @@
   const LanguageType cu_language = unit.GetLanguageType();
   DWARFFormValue::FixedFormSizes fixed_form_sizes = unit.GetFixedFormSizes();
 
-  IndexUnitImpl(unit, cu_language, fixed_form_sizes, unit.GetOffset(), set);
+  IndexUnitImpl(unit, cu_language, fixed_form_sizes, set);
 
   SymbolFileDWARFDwo *dwo_symbol_file = unit.GetDwoSymbolFile();
   if (dwo_symbol_file && dwo_symbol_file->GetCompileUnit()) {
 IndexUnitImpl(*dwo_symbol_file->GetCompileUnit(), cu_language,
-  fixed_form_sizes, unit.GetOffset(), set);
+  fixed_form_sizes, set);
   }
 }
 
 void ManualDWARFIndex::IndexUnitImpl(
 DWARFUnit &unit, const LanguageType cu_language,
-const DWARFFormValue::FixedFormSizes &fixed_form_sizes,
-const dw_offset_t cu_offset, IndexSet &set) {
+const DWARFFormValue::FixedFormSizes &fixed_form_sizes, IndexSet &set) {
+  user_id_t unit_id = unit.GetID();
   for (const DWARFDebugInfoEntry &die : unit.dies(

[Lldb-commits] [PATCH] D61482: [DWARF] Store compile unit IDs in the DIERef class

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: JDevlieghere, clayborg, aprantl.
Herald added a subscriber: arphaman.

The offset of a compile unit is enough to identify in a section, but
things start to get a lot more complicated when you have multiple object
files, each with potentially several debug-info-carrying sections
floating around.

We were already pushing this concept to its limits split-dwarf, which
required juggling offsets of several units to get things to work.
However, this really become a problem when we tried to introduce
debug_types support, which resulted in a long series of attempts to
"concatenate" various debug info sections so that the unit offset
remains a valid identifier.

Instead of attempting to assign unique offsets to all units that we care
about, this patch does something more fundamental. We just admit that
using offsets as unique identifiers does not scale to all debug info
formats we want to support, and uses a different ID instead.

Decoupling the ID from the offset means the offset can be free to point
to the actual offset of the compile unit in the relevant section,
without any changes to low-level parsing code needed. And using a
surrogate ID means its much easier to map the compile units into a
single address space. At least two strategies are possible after this
patch:
a) map all relevant units into a single continuous sequence of integers
(as if the were all stored in one vector)
b) steal some bits from the cu_idx field and use it as some sort of a
discriminator (essentially creating multiple address spaces)

Additionally, this makes us better prepared for the future, where the
total size of debug info exceeds 4GB. It also simplifies code, as we can
treat different debug info flavours more uniformly.

This patch:

- renames the cu_offset field of DIERef to cu_idx and fixes all uses to treat 
it as such
- removes DWARFUnit::GetBaseObjOffset as it no longer serves any useful 
purpose. Instead the DWO unit is made to share the ID of the "base" unit, as 
they logically represent a single compilation.
- changes SymbolFileDWARFDwo to use the index of the contained unit as its ID 
(this is the same approach taken by SymbolFileDWARFDebugMap).
- has a longer commit message than the code it changes. :)


https://reviews.llvm.org/D61482

Files:
  source/Plugins/SymbolFile/DWARF/DIERef.cpp
  source/Plugins/SymbolFile/DWARF/DIERef.h
  source/Plugins/SymbolFile/DWARF/DWARFBaseDIE.cpp
  source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
  source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
  source/Plugins/SymbolFile/DWARF/DWARFUnit.h
  source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
  source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
  source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.h
  source/Plugins/SymbolFile/DWARF/NameToDIE.cpp
  source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp

Index: source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
===
--- source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
+++ source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
@@ -23,7 +23,7 @@
DWARFUnit *dwarf_cu)
 : SymbolFileDWARF(objfile.get()), m_obj_file_sp(objfile),
   m_base_dwarf_cu(dwarf_cu) {
-  SetID(((lldb::user_id_t)dwarf_cu->GetOffset()) << 32);
+  SetID(((lldb::user_id_t)dwarf_cu->GetID()) << 32);
 }
 
 void SymbolFileDWARFDwo::LoadSectionData(lldb::SectionType sect_type,
@@ -158,6 +158,6 @@
 
 DWARFDIE
 SymbolFileDWARFDwo::GetDIE(const DIERef &die_ref) {
-  lldbassert(m_base_dwarf_cu->GetOffset() == die_ref.cu_offset);
+  assert(m_base_dwarf_cu->GetID() == die_ref.cu_idx);
   return DebugInfo()->GetDIEForDIEOffset(die_ref.die_offset);
 }
Index: source/Plugins/SymbolFile/DWARF/NameToDIE.cpp
===
--- source/Plugins/SymbolFile/DWARF/NameToDIE.cpp
+++ source/Plugins/SymbolFile/DWARF/NameToDIE.cpp
@@ -38,13 +38,13 @@
   return m_map.GetValues(regex, info_array);
 }
 
-size_t NameToDIE::FindAllEntriesForCompileUnit(dw_offset_t cu_offset,
+size_t NameToDIE::FindAllEntriesForCompileUnit(dw_offset_t cu_idx,
DIEArray &info_array) const {
   const size_t initial_size = info_array.size();
   const uint32_t size = m_map.GetSize();
   for (uint32_t i = 0; i < size; ++i) {
 const DIERef &die_ref = m_map.GetValueAtIndexUnchecked(i);
-if (cu_offset == die_ref.cu_offset)
+if (cu_idx == die_ref.cu_idx)
   info_array.push_back(die_ref);
   }
   return info_array.size() - initial_size;
@@ -56,7 +56,7 @@
 ConstString cstr = m_map.GetCStringAtIndex(i);
 const DIERef &die_ref = m_map.GetValueAtIndexUnchecked(i);
 s->Printf("%p: {0x%8.8x/0x%8.8x} \"%s\"\n", (const void *)cstr.GetCString(),
-  die_ref.cu_offset, die_ref.die_offset, cstr.GetCString());
+  die_ref.cu_idx, die_ref.die_offset, cstr.GetCString());