clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
I see the need for a lot of this, but I feel like there are way too many places
where we do this:
SymbolFileDWARFDwo* dwo_symbol_file = dwarf_cu->GetDwoSymbolFile();
if (dwo_symbol_file)
return dwo_symbol_file->XXXX (...);
I would like us to more cleanly abstract ourselves from SymbolFileDWARFDwo.
Ideally we wouldn't even see "SymbolFileDWARFDwo" anywhere inside
SymbolFileDWARF because it has been abstracted behind DWARFCompileUnit and
DWARFDebugInfoEntry and any other low level classes that need to know about
them.
So I would like to see if the following is possible:
- DWARFCompileUnit should return the DIEs from the DWO file when
DWARFCompileUnit::GetCompileUnitDIEOnly() or DWARFCompileUnit::DIE() is called.
- Any code that was doing the pattern from above that was calling
dwarf_cu->GetDwoSymbolFile() and deferring to the DWO symbol file should just
work normally if the correct DIEs are being handed out.
- All references to SymbolFileDWARFDwo should be removed from SymbolFileDWARF
if we abstract things correctly.
================
Comment at: include/lldb/Symbol/ObjectFile.h:372
@@ -371,3 +371,3 @@
virtual SectionList *
- GetSectionList ();
+ GetSectionList (bool update_module_section_list = true);
----------------
Why is this needed? Can you clarify. I don't like this change and would rather
avoid it if possible.
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h:16
@@ -15,2 +15,3 @@
#include "SymbolFileDWARF.h"
+#include "SymbolFileDWARFDwo.h"
----------------
This shouldn't be needed here, just a forward declaration for
SymbolFileDWARFDwo should do and then include this in DWARFCompileUnit.cpp. No
one outside of this class should need to know anything about
SymbolFileDWARFDwo. It should be an implementation detail that only
DWARFCompileUnit and DWARFDebugInfoEntry need to know about.
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h:208-214
@@ -202,1 +207,9 @@
+
+ SymbolFileDWARFDwo*
+ GetDwoSymbolFile()
+ {
+ // Extract the compile unit DIE as that one contains the dwo symbol
file information
+ ExtractDIEsIfNeeded(true);
+ return m_dwo_symbol_file.get();
+ }
----------------
Make protected and hide this from anyone but DWARFCompileUnit and
DWARFDebugInfoEntry.
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:330
@@ +329,3 @@
+ dw_addr_t base_address =
form_value.Address(dwarf2Data);
+
((DWARFCompileUnit*)cu)->SetBaseAddress(base_address);
+ }
----------------
I don't think the cast is needed anymore now that "cu" isn't const.
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:806-818
@@ -802,10 +805,15 @@
case DW_AT_high_pc:
- hi_pc = form_value.Unsigned();
- if (form_value.Form() != DW_FORM_addr)
+ if (form_value.Form() == DW_FORM_addr ||
+ form_value.Form() == DW_FORM_GNU_addr_index)
{
+ form_value.Address(dwarf2Data);
+ }
+ else
+ {
+ hi_pc = form_value.Unsigned();
if (lo_pc == LLDB_INVALID_ADDRESS)
do_offset = hi_pc != LLDB_INVALID_ADDRESS;
else
hi_pc += lo_pc; // DWARF 4 introduces
<offset-from-lo-pc> to save on relocations
}
break;
----------------
Shouldn't this be:
```
if (form_value.Form() == DW_FORM_addr || form_value.Form() ==
DW_FORM_GNU_addr_index)
hi_pc = form_value.Address(dwarf2Data);
else
hi_pc = form_value.Unsigned();
if (hi_pc != LLDB_INVALID_ADDRESS)
{
if (lo_pc == LLDB_INVALID_ADDRESS)
do_offset = hi_pc != LLDB_INVALID_ADDRESS;
else
hi_pc += lo_pc; // DWARF 4 introduces <offset-from-lo-pc> to save on
relocations
}
```
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:1437-1438
@@ -1384,3 +1436,4 @@
DWARFFormValue form_value;
- if (GetAttributeValue(dwarf2Data, cu, attr, form_value))
+ if (GetAttributeValue(dwarf2Data, cu, attr, form_value) ||
+ GetAttributeValueForDwoCompileUnit(dwarf2Data, cu, attr, form_value))
return form_value.Unsigned();
----------------
GetAttributeValue(...) should just "do the right thing" and look into the DWO
file if needed. Then this change isn't needed.
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:1458-1459
@@ -1404,3 +1457,4 @@
DWARFFormValue form_value;
- if (GetAttributeValue(dwarf2Data, cu, attr, form_value))
+ if (GetAttributeValue(dwarf2Data, cu, attr, form_value) ||
+ GetAttributeValueForDwoCompileUnit(dwarf2Data, cu, attr, form_value))
return form_value.Signed();
----------------
GetAttributeValue(...) should just "do the right thing" and look into the DWO
file if needed. Then this change isn't needed.
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:1480-1481
@@ -1425,3 +1479,4 @@
DWARFFormValue form_value;
- if (GetAttributeValue(dwarf2Data, cu, attr, form_value))
+ if (GetAttributeValue(dwarf2Data, cu, attr, form_value) ||
+ GetAttributeValueForDwoCompileUnit(dwarf2Data, cu, attr, form_value))
return form_value.Reference();
----------------
GetAttributeValue(...) should just "do the right thing" and look into the DWO
file if needed. Then this change isn't needed.
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:1494-1515
@@ +1493,24 @@
+) const
+{
+ DWARFFormValue form_value;
+ if (GetAttributeValue(dwarf2Data, cu, attr, form_value))
+ return form_value.Address(dwarf2Data);
+
+ if (m_tag != DW_TAG_compile_unit)
+ return fail_value;
+
+ SymbolFileDWARFDwo* dwo_sym_file = cu->GetDwoSymbolFile();
+ if (!dwo_sym_file)
+ return fail_value;
+
+ DWARFCompileUnit* dwo_cu = dwo_sym_file->GetCompileUnit();
+ if (!dwo_cu)
+ return fail_value;
+
+ const DWARFDebugInfoEntry* dwo_cu_die = dwo_cu->GetCompileUnitDIEOnly();
+ if (!dwo_cu_die)
+ return fail_value;
+
+ return dwo_cu_die->GetAttributeValueAsAddress(dwo_sym_file, dwo_cu, attr,
fail_value);
+}
+
----------------
GetAttributeValue(...) should just "do the right thing" and look into the DWO
file if needed. This code will change a bit after that, but the check for the
DWO file shouldn't be needed.
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:1535-1536
@@ -1448,3 +1534,4 @@
DWARFFormValue form_value;
- if (GetAttributeValue(dwarf2Data, cu, DW_AT_high_pc, form_value))
+ if (GetAttributeValue(dwarf2Data, cu, DW_AT_high_pc, form_value) ||
+ GetAttributeValueForDwoCompileUnit(dwarf2Data, cu, DW_AT_high_pc,
form_value))
{
----------------
GetAttributeValue(...) should just "do the right thing" and look into the DWO
file if needed. Then this change isn't needed.
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:371-384
@@ -369,7 +370,16 @@
return 0;
- GetTypes (dwarf_cu,
- dwarf_cu->DIE(),
- dwarf_cu->GetOffset(),
- dwarf_cu->GetNextCompileUnitOffset(),
- type_mask,
- type_set);
+
+ SymbolFileDWARFDwo* dwo_symbol_file = dwarf_cu->GetDwoSymbolFile();
+ if (dwo_symbol_file)
+ {
+ dwarf_cu = dwo_symbol_file->GetCompileUnit();
+ dwo_symbol_file->GetTypes (dwarf_cu,
+ dwarf_cu->DIE(),
+ 0,
+ UINT32_MAX,
+ type_mask,
+ type_set);
+ }
+ else
+ {
+ GetTypes (dwarf_cu,
----------------
Hopefully we don't need this if we abstract things correctly so that the
DWARFCompileUnit hands out the correct DIEs from the DWO file and
SymbolFileDWARF shouldn't need to know anything about DWO files.
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:404-417
@@ -387,7 +403,16 @@
{
- GetTypes (dwarf_cu,
- dwarf_cu->DIE(),
- 0,
- UINT32_MAX,
- type_mask,
- type_set);
+ SymbolFileDWARFDwo* dwo_symbol_file =
dwarf_cu->GetDwoSymbolFile();
+ if (dwo_symbol_file)
+ {
+ dwarf_cu = dwo_symbol_file->GetCompileUnit();
+ dwo_symbol_file->GetTypes (dwarf_cu,
+ dwarf_cu->DIE(),
+ 0,
+ UINT32_MAX,
+ type_mask,
+ type_set);
+ }
+ else
+ {
+ GetTypes (dwarf_cu,
+ dwarf_cu->DIE(),
----------------
Hopefully we don't need this if we abstract things correctly so that the
DWARFCompileUnit hands out the correct DIEs from the DWO file and
SymbolFileDWARF shouldn't need to know anything about DWO files.
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1069-1074
@@ -1038,1 +1068,8 @@
+ SymbolFileDWARFDwo* dwo_symbol_file =
dwarf_cu->GetDwoSymbolFile();
+ if (dwo_symbol_file)
+ {
+ DWARFCompileUnit* dwo_dwarf_cu =
dwo_symbol_file->GetCompileUnit();
+ dwo_dwarf_cu->SetUserData(cu_sp.get());
+ }
+
----------------
Shouldn't:
```
const DWARFDebugInfoEntry* cu_die = dwarf_cu->GetCompileUnitDIEOnly ();
```
already return the right cu_die? Should clients of DWARFCompileUnit be required
to do this kind of very specific check? I really want this to be hidden from us
by abstracting this kind of thing inside DWARFCompileUnit.
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1157-1159
@@ -1119,1 +1156,5 @@
{
+ SymbolFileDWARFDwo* dwo_symbol_file = dwarf_cu->GetDwoSymbolFile();
+ if (dwo_symbol_file)
+ return dwo_symbol_file->ParseFunctionBlocks(sc);
+
----------------
Why is this necessary? If we hand out the correct DIE from the DWO file in the
first place, this should just work with no modifications right?
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1626-1631
@@ -1584,1 +1625,8 @@
{
+ // Ask the child dwo files if any of them know about this type
+ for (SymbolFileDWARFDwo* dwo_symbol_file : m_dwo_symbol_files)
+ {
+ if (dwo_symbol_file &&
dwo_symbol_file->HasForwardDeclForClangType(clang_type))
+ return dwo_symbol_file->CompleteType(clang_type);
+ }
+
----------------
It would be nice if this weren't linear. I think we have the same problem int
SymbolFileDWARFDebugMap, but not being linear would be nice at some point.
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1922-1925
@@ -1874,2 +1921,6 @@
{
+ SymbolFileDWARFDwo* dwo_symbol_file =
dwarf_cu->GetDwoSymbolFile();
+ if (dwo_symbol_file)
+ return
dwo_symbol_file->ResolveSymbolContext(so_addr, resolve_scope, sc);
+
resolved |= eSymbolContextCompUnit;
----------------
It would be nice to abstract this behind DWARFCompileUnit. Below we end up
calling dwarf_cu->LookupAddress(...). This seems like a better place to do this
kind of thing...
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2281-2286
@@ -2225,1 +2280,8 @@
+ for (SymbolFileDWARFDwo* dwo_symbol_file : m_dwo_symbol_files)
+ dwo_symbol_file->FindGlobalVariables(name,
+ namespace_decl,
+ true /* append */,
+ max_matches,
+ variables);
+
----------------
Not sure why this is needed? If each DWARFCompileUnit hands out the correct
DIEs, then Index() will point us to the right things and this shouldn't be
needed.
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2405-2410
@@ -2343,1 +2404,8 @@
+
+ for (SymbolFileDWARFDwo* dwo_symbol_file : m_dwo_symbol_files)
+ dwo_symbol_file->FindGlobalVariables(regex,
+ true /* append */,
+ max_matches,
+ variables);
+
m_global_index.Find (regex, die_offsets);
----------------
Not sure why this is needed? If each DWARFCompileUnit hands out the correct
DIEs, then Index() will point us to the right things and this shouldn't be
needed.
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3017-3023
@@ -2949,1 +3016,9 @@
+
+ for (SymbolFileDWARFDwo* dwo_symbol_file : m_dwo_symbol_files)
+ dwo_symbol_file->FindFunctions(name,
+ namespace_decl,
+ name_type_mask,
+ include_inlines,
+ true /* append */,
+ sc_list);
}
----------------
Not sure why this is needed? If each DWARFCompileUnit hands out the correct
DIEs, then Index() will point us to the right things and this shouldn't be
needed.
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3059-3061
@@ -2984,1 +3058,5 @@
+
+ DWARFDebugInfo* info = DebugInfo();
+ if (info == nullptr)
+ return 0;
----------------
If .debug_info wasn't there, then SymbolFileDWARF wouldn't have been
instantiated. Is this truly needed? Did something change when DWO files are now
used?
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3086-3087
@@ -3007,1 +3085,4 @@
+
+ for (SymbolFileDWARFDwo* dwo_symbol_file : m_dwo_symbol_files)
+ dwo_symbol_file->FindFunctions(regex, include_inlines, true,
sc_list);
}
----------------
Not sure why this is needed? If each DWARFCompileUnit hands out the correct
DIEs, then Index() will point us to the right things and this shouldn't be
needed.
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3149-3155
@@ -3066,1 +3148,9 @@
+
+ for (SymbolFileDWARFDwo* dwo_symbol_file : m_dwo_symbol_files)
+ dwo_symbol_file->FindTypes(sc,
+ name,
+ namespace_decl,
+ true /* append */,
+ max_matches,
+ types);
}
----------------
Not sure why this is needed? If each DWARFCompileUnit hands out the correct
DIEs, then Index() will point us to the right things and this shouldn't be
needed.
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3265-3270
@@ -3175,1 +3264,8 @@
+ for (SymbolFileDWARFDwo* dwo_symbol_file : m_dwo_symbol_files)
+ {
+ namespace_decl = dwo_symbol_file->FindNamespace(sc, name,
parent_namespace_decl);
+ if (namespace_decl)
+ return namespace_decl;
+ }
+
----------------
Not sure why this is needed? If each DWARFCompileUnit hands out the correct
DIEs, then Index() will point us to the right things and this shouldn't be
needed.
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3723-3728
@@ -3655,1 +3722,8 @@
+
+ for (SymbolFileDWARFDwo* dwo_symbol_file : m_dwo_symbol_files)
+ {
+ type_sp =
dwo_symbol_file->FindDefinitionTypeForDWARFDeclContext(dwarf_decl_ctx);
+ if (type_sp)
+ return type_sp;
+ }
----------------
Not sure why this is needed? If each DWARFCompileUnit hands out the correct
DIEs, then Index() will point us to the right things and this shouldn't be
needed.
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3909-3911
@@ -3834,1 +3908,5 @@
{
+ SymbolFileDWARFDwo* dwo_symbol_file = dwarf_cu->GetDwoSymbolFile();
+ if (dwo_symbol_file)
+ return dwo_symbol_file->ParseFunctionBlocks(sc);
+
----------------
Not sure why this is needed? If each DWARFCompileUnit hands out the correct
DIEs, then Index() will point us to the right things and this shouldn't be
needed.
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3934-3936
@@ -3855,1 +3933,5 @@
{
+ SymbolFileDWARFDwo* dwo_symbol_file = dwarf_cu->GetDwoSymbolFile();
+ if (dwo_symbol_file)
+ return dwo_symbol_file->ParseTypes(sc);
+
----------------
Not sure why this is needed? If each DWARFCompileUnit hands out the correct
DIEs, then Index() will point us to the right things and this shouldn't be
needed.
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3975-3978
@@ -3894,1 +3974,6 @@
+
+ SymbolFileDWARFDwo* dwo_symbol_file = dwarf_cu->GetDwoSymbolFile();
+ if (dwo_symbol_file)
+ return dwo_symbol_file->ParseVariablesForContext (sc);
+
const DWARFDebugInfoEntry *function_die =
dwarf_cu->GetDIEPtr(sc.function->GetID());
----------------
Not sure why this is needed? If each DWARFCompileUnit hands out the correct
DIEs, then Index() will point us to the right things and this shouldn't be
needed.
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3997-3999
@@ -3912,1 +3996,5 @@
+
+ SymbolFileDWARFDwo* dwo_symbol_file = dwarf_cu->GetDwoSymbolFile();
+ if (dwo_symbol_file)
+ return dwo_symbol_file->ParseVariablesForContext (sc);
----------------
Not sure why this is needed? If each DWARFCompileUnit hands out the correct
DIEs, then Index() will point us to the right things and this shouldn't be
needed.
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h:66-67
@@ -65,2 +65,4 @@
+class SymbolFileDWARFDwo;
+
class SymbolFileDWARF : public lldb_private::SymbolFile, public
lldb_private::UserID
----------------
Hopefully we don't need this if we abstract things correctly so that the
DWARFCompileUnit hands out the correct DIEs from the DWO file and
SymbolFileDWARF shouldn't need to know anything about DWO files.
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h:419-423
@@ -418,1 +418,7 @@
+
+ void
+ AddDwoSymbolFile(SymbolFileDWARFDwo* dwo_symbol_file)
+ {
+ m_dwo_symbol_files.push_back(dwo_symbol_file);
+ }
----------------
Hopefully we don't need this if we abstract things correctly so that the
DWARFCompileUnit hands out the correct DIEs from the DWO file and
SymbolFileDWARF shouldn't need to know anything about DWO files.
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h:478
@@ -471,2 +477,3 @@
ClangTypeToDIE m_forward_decl_clang_type_to_die;
+ std::vector<SymbolFileDWARFDwo*> m_dwo_symbol_files;
};
----------------
Hopefully we don't need this if we abstract things correctly so that the
DWARFCompileUnit hands out the correct DIEs from the DWO file and
SymbolFileDWARF shouldn't need to know anything about DWO files.
================
Comment at: source/Symbol/ObjectFile.cpp:604-625
@@ -603,15 +603,23 @@
SectionList *
-ObjectFile::GetSectionList()
+ObjectFile::GetSectionList(bool update_module_section_list)
{
if (m_sections_ap.get() == nullptr)
{
- ModuleSP module_sp(GetModule());
- if (module_sp)
+ if (update_module_section_list)
{
- lldb_private::Mutex::Locker locker(module_sp->GetMutex());
- CreateSections(*module_sp->GetUnifiedSectionList());
+ ModuleSP module_sp(GetModule());
+ if (module_sp)
+ {
+ lldb_private::Mutex::Locker locker(module_sp->GetMutex());
+ CreateSections(*module_sp->GetUnifiedSectionList());
+ }
+ }
+ else
+ {
+ SectionList unified_section_list;
+ CreateSections(unified_section_list);
}
}
return m_sections_ap.get();
}
----------------
Can you explain why this is needed?
http://reviews.llvm.org/D12291
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
