[Lldb-commits] [PATCH] D12291: Add split dwarf support to SymbolFileDWARF
Greg Clayton via lldb-commits
lldb-commits at lists.llvm.org
Tue Sep 1 10:56:19 PDT 2015
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
See inlined comments.
Main fix themes:
- It would be great to try and avoid any manual function calls where we supply the compile unit and a DIE offset that we assume comes from that compile unit. I identified many places where this was happening and we might have more. It is OK in DWARFDebugInfoEntry in places, but everywhere else we should try to avoid. Also anywhere where we grab an attribute, we should store a FormValue instead of a dw_offset_t (or a DIERef) and use that.
- Modify DWARFDebugInfoEntry::GetAttributeValue() to handle looking through the DW_AT_specification and DW_AT_abstract_origin for attributes with a new default parameter "bool check_specification_or_abstract_origin = false". Add this parameter to all DWARFDebugInfoEntry::GetAttributeValueAs...() functions.
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:218
@@ -217,3 +217,3 @@
case DW_AT_encoding: encoding = form_value.Unsigned(); break;
- case DW_AT_type: encoding_uid = form_value.Reference(); break;
+ case DW_AT_type: encoding_uid = DIERef(die.GetCU()->GetOffset(), form_value.Reference()).GetUID(); break;
default:
----------------
DIERef can and should be initialized with only the "form_value". It contains enough info to extract the correct reference. No need to pass in the die.GetCU()->GetOffset().
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1148
@@ -1149,3 +1147,3 @@
// made with the specification and not with this die.
- DWARFDIE spec_die = dwarf->DebugInfo()->GetDIE(specification_die_offset);
+ DWARFDIE spec_die = dwarf->DebugInfo()->GetDIE(DIERef(die.GetCU()->GetOffset(), specification_die_offset));
clang::DeclContext *spec_clang_decl_ctx = GetClangDeclContextForDIE (spec_die);
----------------
We should store "specification_die_offset" as a FormValue so we don't possibly use the wrong compile unit here. FormValue contains all that is needed to decode a value. If the DW_AT_specification was actually from a DW_AT_abstract_origin where the abstract origin DIE contained a DW_AT_specification, this would use the wrong compile unit and be an invalid DIERef (or point to the wrong thing). In general, we need to avoid this kind of manual compile unit + offset to avoid these issues everywhere.
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1170
@@ -1171,3 +1169,3 @@
- DWARFDIE abs_die = dwarf->DebugInfo()->GetDIE (abstract_origin_die_offset);
+ DWARFDIE abs_die = dwarf->DebugInfo()->GetDIE (DIERef(die.GetCU()->GetOffset(), abstract_origin_die_offset));
clang::DeclContext *abs_clang_decl_ctx = GetClangDeclContextForDIE (abs_die);
----------------
Store abstract_origin_die_offset as a FormValue to avoid issues as mentioned above. No manual CU + offset anywhere.
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp:316-328
@@ +315,15 @@
+
+ const char* dwo_name = cu_die.GetAttributeValueAsString(m_dwarf2Data,
+ this,
+ DW_AT_GNU_dwo_name,
+ nullptr);
+ if (!dwo_name)
+ return;
+
+ const char* comp_dir = cu_die.GetAttributeValueAsString(m_dwarf2Data,
+ this,
+ DW_AT_comp_dir,
+ nullptr);
+ if (!comp_dir)
+ return;
+
----------------
Is DW_AT_GNU_dwo_name always a relative path?
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp:660
@@ -564,3 +659,3 @@
{
- return m_dwarf2Data->DebugInfo()->GetDIE (die_offset);
+ return m_dwarf2Data->DebugInfo()->GetDIE (DIERef(die_offset));
}
----------------
Don't we need the compile unit to be encoded into the DIERef here?
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp:161
@@ -145,3 +160,3 @@
else
- return DWARFDIE(dwarf->DebugInfo()->GetCompileUnitContainingDIE(block_die->GetOffset()), block_die);
+ return DWARFDIE(dwarf->DebugInfo()->GetCompileUnitContainingDIE(DIERef(cu->GetOffset(), block_die->GetOffset())), block_die);
}
----------------
We probably should swift m_die->LookupAddress to take a "DWARFDIE *function_die, DWARFDIE *block_die" instead of "DWARFDebugInfoEntry* function_die, DWARFDebugInfoEntry* block_die". Then we can avoid doing the manual combination of CU + offset here.
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp:184
@@ +183,3 @@
+ assert ((id&0xffffffff00000000ull) == 0 || m_cu->GetID() == 0);
+ id |= ((lldb::user_id_t)m_cu->GetID()) << 32;
+ }
----------------
The DWARFCompileUnit::GetID() already shifts the cu_offset over by 32, so we will just lose the compile unit bits here.
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:610
@@ -588,3 +609,3 @@
{
- DWARFDIE die = dwarf2Data->DebugInfo()->GetDIE(die_offset);
+ DWARFDIE die = dwarf2Data->DebugInfo()->GetDIE(DIERef(cu->GetOffset(), die_offset));
if (die)
----------------
I would like to avoid the manual CU + offset here if possible. We could make die_offsets into an array of DIERef objects.
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:1250
@@ -1180,2 +1249,3 @@
dw_offset_t end_addr_offset = DW_INVALID_OFFSET;
- const dw_offset_t attr_offset = GetAttributeValue(dwarf2Data, cu, attr, form_value, &end_addr_offset);
+ SymbolFileDWARF* attribute_symbol_file = nullptr;
+ const dw_offset_t attr_offset = GetAttributeValue(dwarf2Data,
----------------
Not sure what you were thinking here? You init this with nullptr and then try to use it below? Did you mean to init this with dwarf2Data? If so then we don't need this? Otherwise, please fix
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:1262
@@ -1187,3 +1261,3 @@
// We have an inlined location list in the .debug_info section
- const DWARFDataExtractor& debug_info = dwarf2Data->get_debug_info_data();
+ const DWARFDataExtractor& debug_info = attribute_symbol_file->get_debug_info_data();
dw_offset_t block_offset = blockData - debug_info.GetDataStart();
----------------
This will crash due to attribute_symbol_file always being nullptr.
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:1273-1279
@@ -1198,7 +1272,9 @@
lldb::offset_t debug_loc_offset = form_value.Unsigned();
- if (dwarf2Data)
+ if (attribute_symbol_file)
{
- assert(dwarf2Data->get_debug_loc_data().GetAddressByteSize() == cu->GetAddressByteSize());
- return DWARFLocationList::Extract(dwarf2Data->get_debug_loc_data(), &debug_loc_offset, location_data);
+ assert(attribute_symbol_file->get_debug_loc_data().GetAddressByteSize() == cu->GetAddressByteSize());
+ return DWARFLocationList::Extract(attribute_symbol_file->get_debug_loc_data(),
+ &debug_loc_offset,
+ location_data);
}
}
----------------
This is currently always nullptr, so this is dead code
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:1298
@@ -1221,1 +1297,3 @@
{
+ const char* dw_at_name = GetAttributeValueAsString(dwarf2Data, cu, DW_AT_name, nullptr);
+ if (dw_at_name)
----------------
We should probably just modify DWARFDebugInfoEntry::GetAttributeValue() to take an extra parameter like:
bool check_specification_or_abstract_origin
If this is true DWARFDebugInfoEntry::GetAttributeValue() should look through the DW_AT_specification or DW_AT_abstract_origin. I think we have many many places that do this sort of:
- check this die for the attribute
- check the DW_AT_specification for the attribute
- check the DW_AT_abstract_origin for the attribute
If you do do this, we could make the extra parameter default to "bool check_specification_or_abstract_origin = false" so all code continues to behave as it currently does, then opt it where we need to. The more smarts we put into DWARFDebugInfoEntry::GetAttributeValue() the better off we will be seeing as the compile unit in the main executable file when using DWO files has some attributes in the main executable DW_TAG_compile_unit and some in the other. I don't believe we need anything from the other, but again, the more we centralize this in DWARFDebugInfoEntry::GetAttributeValue() the less problems we will have in the future.
We would also need to add this "bool check_specification_or_abstract_origin" parameter to all of the DWARFDebugInfoEntry::GetAttributeValueAsXXX() functions.
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:1336-1347
@@ -1257,13 +1335,14 @@
- if (GetAttributeValue(dwarf2Data, cu, DW_AT_MIPS_linkage_name, form_value))
- name = form_value.AsCString();
+ name = GetAttributeValueAsString(dwarf2Data, cu, DW_AT_MIPS_linkage_name, nullptr);
+ if (name)
+ return name;
- if (GetAttributeValue(dwarf2Data, cu, DW_AT_linkage_name, form_value))
- name = form_value.AsCString();
+ name = GetAttributeValueAsString(dwarf2Data, cu, DW_AT_linkage_name, nullptr);
+ if (name)
+ return name;
- if (substitute_name_allowed && name == nullptr)
- {
- if (GetAttributeValue(dwarf2Data, cu, DW_AT_name, form_value))
- name = form_value.AsCString();
- }
+ if (!substitute_name_allowed)
+ return nullptr;
+
+ name = GetAttributeValueAsString(dwarf2Data, cu, DW_AT_name, nullptr);
return name;
----------------
not sure if we are OK with not checking the DW_AT_specification or DW_AT_abstract_origin here? Seems like a bug that would be fixed with our new DWARFDebugInfoEntry::GetAttributeValue() with the "bool check_specification_or_abstract_origin" parameter...
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:1365-1387
@@ -1285,20 +1364,25 @@
{
- const char* name = NULL;
+ const char* name = nullptr;
if (!dwarf2Data)
return name;
-
- DWARFFormValue form_value;
- if (GetAttributeValue(dwarf2Data, cu, DW_AT_MIPS_linkage_name, form_value))
- name = form_value.AsCString();
- else if (GetAttributeValue(dwarf2Data, cu, DW_AT_linkage_name, form_value))
- name = form_value.AsCString();
- else if (GetAttributeValue(dwarf2Data, cu, DW_AT_name, form_value))
- name = form_value.AsCString();
- else if (GetAttributeValue(dwarf2Data, cu, DW_AT_specification, form_value))
+ name = GetAttributeValueAsString(dwarf2Data, cu, DW_AT_MIPS_linkage_name, nullptr);
+ if (name)
+ return name;
+
+ name = GetAttributeValueAsString(dwarf2Data, cu, DW_AT_linkage_name, nullptr);
+ if (name)
+ return name;
+
+ name = GetAttributeValueAsString(dwarf2Data, cu, DW_AT_name, nullptr);
+ if (name)
+ return name;
+
+ DWARFFormValue form_value;
+ if (GetAttributeValue(dwarf2Data, cu, DW_AT_specification, form_value))
{
DWARFDIE die = const_cast<DWARFCompileUnit*>(cu)->GetDIE(form_value.Reference());
if (die)
name = die.GetPubname();
}
return name;
----------------
not sure if we are OK with not checking the DW_AT_specification or DW_AT_abstract_origin here? Seems like a bug that would be fixed with our new DWARFDebugInfoEntry::GetAttributeValue() with the "bool check_specification_or_abstract_origin" parameter...
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:1427
@@ -1342,3 +1426,3 @@
{
- DWARFFormValue form_value;
- if (die.GetAttributeValue(dwarf2Data, cu, DW_AT_name, form_value))
+ const char* name = die.GetAttributeValueAsString(dwarf2Data, cu, DW_AT_name, nullptr);
+ if (name)
----------------
We don't check DW_AT_specification or DW_AT_abstract_origin...
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3732
@@ -3761,3 +3731,3 @@
uint32_t i;
- lldb::user_id_t type_uid = LLDB_INVALID_UID;
+ DIERef type_die_ref;
DWARFExpression location(die.GetCU());
----------------
Please store as a FormValue. We have a branch where we need this as a FormValue so this will ease merging.
http://reviews.llvm.org/D12291
More information about the lldb-commits
mailing list