[lldb/DWARF] Bypass the compres^Wconstruction of DIERefs in debug_names #93296
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DebugNamesDWARFIndex was jumping through hoops to construct a DIERef from an index entry only to jump through them back a short while later to construct a DWARFDIE. This used to be necessary as the index lookup was a two stage process, where we first enumerated all matches, and then examined them (so it was important that the enumeration was cheap -- does not trigger unnecessary parsing). However, now that the processing is callback based, we are always immediately examening the DWARFDIE right after finding the entry, and the DIERef just gets in the way.
labath
requested review from
felipepiovezan and
JDevlieghere
and removed request for
JDevlieghere
|
@llvm/pr-subscribers-lldb Author: Pavel Labath (labath) ChangesDebugNamesDWARFIndex was jumping through hoops to construct a DIERef from an index entry only to jump through them back a short while later to construct a DWARFDIE. This used to be necessary as the index lookup was a two stage process, where we first enumerated all matches, and then examined them (so it was important that the enumeration was cheap -- does not trigger unnecessary parsing). However, now that the processing is callback based, we are always immediately examening the DWARFDIE right after finding the entry, and the DIERef just gets in the way. Full diff: https://github.com/llvm/llvm-project/pull/93296.diff 5 Files Affected:
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp b/lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp
index 33537df4f5076..1703597a7cd2f 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp
@@ -284,8 +284,12 @@ void AppleDWARFIndex::GetFunctions(
for (const auto &entry : m_apple_names_up->equal_range(name)) {
DIERef die_ref(std::nullopt, DIERef::Section::DebugInfo,
*entry.getDIESectionOffset());
- if (!ProcessFunctionDIE(lookup_info, die_ref, dwarf, parent_decl_ctx,
- callback))
+ DWARFDIE die = dwarf.GetDIE(die_ref);
+ if (!die) {
+ ReportInvalidDIERef(die_ref, name);
+ continue;
+ }
+ if (!ProcessFunctionDIE(lookup_info, die, parent_decl_ctx, callback))
return;
}
}
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
index 20c07a94b5076..30fb5d5ebdb0d 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
@@ -24,16 +24,11 @@ using namespace lldb_private::plugin::dwarf;
DWARFIndex::~DWARFIndex() = default;
bool DWARFIndex::ProcessFunctionDIE(
- const Module::LookupInfo &lookup_info, DIERef ref, SymbolFileDWARF &dwarf,
+ const Module::LookupInfo &lookup_info, DWARFDIE die,
const CompilerDeclContext &parent_decl_ctx,
llvm::function_ref<bool(DWARFDIE die)> callback) {
llvm::StringRef name = lookup_info.GetLookupName().GetStringRef();
FunctionNameType name_type_mask = lookup_info.GetNameTypeMask();
- DWARFDIE die = dwarf.GetDIE(ref);
- if (!die) {
- ReportInvalidDIERef(ref, name);
- return true;
- }
if (!(name_type_mask & eFunctionNameTypeFull)) {
ConstString name_to_match_against;
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.h
index 0551b07100a96..cb3ae8a06d788 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.h
@@ -81,11 +81,10 @@ class DWARFIndex {
StatsDuration m_index_time;
/// Helper function implementing common logic for processing function dies. If
- /// the function given by "ref" matches search criteria given by
- /// "parent_decl_ctx" and "name_type_mask", it is inserted into the "dies"
- /// vector.
- bool ProcessFunctionDIE(const Module::LookupInfo &lookup_info, DIERef ref,
- SymbolFileDWARF &dwarf,
+ /// the function given by "die" matches search criteria given by
+ /// "parent_decl_ctx" and "name_type_mask", it calls the callback with the
+ /// given die.
+ bool ProcessFunctionDIE(const Module::LookupInfo &lookup_info, DWARFDIE die,
const CompilerDeclContext &parent_decl_ctx,
llvm::function_ref<bool(DWARFDIE die)> callback);
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
index 79400e36e04f3..90e42be7202d8 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
@@ -64,27 +64,25 @@ DebugNamesDWARFIndex::GetNonSkeletonUnit(const DebugNames::Entry &entry) const {
return cu ? &cu->GetNonSkeletonUnit() : nullptr;
}
-std::optional<DIERef>
-DebugNamesDWARFIndex::ToDIERef(const DebugNames::Entry &entry) const {
+DWARFDIE DebugNamesDWARFIndex::GetDIE(const DebugNames::Entry &entry) const {
DWARFUnit *unit = GetNonSkeletonUnit(entry);
- if (!unit)
- return std::nullopt;
- if (std::optional<uint64_t> die_offset = entry.getDIEUnitOffset())
- return DIERef(unit->GetSymbolFileDWARF().GetFileIndex(),
- DIERef::Section::DebugInfo, unit->GetOffset() + *die_offset);
-
- return std::nullopt;
+ std::optional<uint64_t> die_offset = entry.getDIEUnitOffset();
+ if (!unit || !die_offset)
+ return DWARFDIE();
+ if (DWARFDIE die = unit->GetDIE(unit->GetOffset() + *die_offset))
+ return die;
+
+ m_module.ReportErrorIfModifyDetected(
+ "the DWARF debug information has been modified (bad offset {0:x} in "
+ "debug_names section)\n",
+ *die_offset);
+ return DWARFDIE();
}
bool DebugNamesDWARFIndex::ProcessEntry(
const DebugNames::Entry &entry,
llvm::function_ref<bool(DWARFDIE die)> callback) {
- std::optional<DIERef> ref = ToDIERef(entry);
- if (!ref)
- return true;
- SymbolFileDWARF &dwarf = *llvm::cast<SymbolFileDWARF>(
- m_module.GetSymbolFile()->GetBackingSymbolFile());
- DWARFDIE die = dwarf.GetDIE(*ref);
+ DWARFDIE die = GetDIE(entry);
if (!die)
return true;
return callback(die);
@@ -183,7 +181,7 @@ void DebugNamesDWARFIndex::GetCompleteObjCClass(
llvm::function_ref<bool(DWARFDIE die)> callback) {
// Keep a list of incomplete types as fallback for when we don't find the
// complete type.
- DIEArray incomplete_types;
+ std::vector<DWARFDIE> incomplete_types;
for (const DebugNames::Entry &entry :
m_debug_names_up->equal_range(class_name.GetStringRef())) {
@@ -191,19 +189,14 @@ void DebugNamesDWARFIndex::GetCompleteObjCClass(
entry.tag() != DW_TAG_class_type)
continue;
- std::optional<DIERef> ref = ToDIERef(entry);
- if (!ref)
- continue;
-
- DWARFUnit *cu = m_debug_info.GetUnit(*ref);
- if (!cu || !cu->Supports_DW_AT_APPLE_objc_complete_type()) {
- incomplete_types.push_back(*ref);
+ DWARFDIE die = GetDIE(entry);
+ if (!die) {
+ // Report invalid
continue;
}
-
- DWARFDIE die = m_debug_info.GetDIE(*ref);
- if (!die) {
- ReportInvalidDIERef(*ref, class_name.GetStringRef());
+ DWARFUnit *cu = die.GetCU();
+ if (!cu->Supports_DW_AT_APPLE_objc_complete_type()) {
+ incomplete_types.push_back(die);
continue;
}
@@ -212,12 +205,11 @@ void DebugNamesDWARFIndex::GetCompleteObjCClass(
callback(die);
return;
}
- incomplete_types.push_back(*ref);
+ incomplete_types.push_back(die);
}
- auto dierefcallback = DIERefCallback(callback, class_name.GetStringRef());
- for (DIERef ref : incomplete_types)
- if (!dierefcallback(ref))
+ for (DWARFDIE die : incomplete_types)
+ if (!callback(die))
return;
m_fallback.GetCompleteObjCClass(class_name, must_be_implementation, callback);
@@ -379,8 +371,8 @@ void DebugNamesDWARFIndex::GetFunctions(
if (tag != DW_TAG_subprogram && tag != DW_TAG_inlined_subroutine)
continue;
- if (std::optional<DIERef> ref = ToDIERef(entry)) {
- if (!ProcessFunctionDIE(lookup_info, *ref, dwarf, parent_decl_ctx,
+ if (DWARFDIE die = GetDIE(entry)) {
+ if (!ProcessFunctionDIE(lookup_info, die, parent_decl_ctx,
[&](DWARFDIE die) {
if (!seen.insert(die.GetDIE()).second)
return true;
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h
index 81fb8f88b805a..a27a414ecdd19 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h
@@ -84,7 +84,7 @@ class DebugNamesDWARFIndex : public DWARFIndex {
ManualDWARFIndex m_fallback;
DWARFUnit *GetNonSkeletonUnit(const DebugNames::Entry &entry) const;
- std::optional<DIERef> ToDIERef(const DebugNames::Entry &entry) const;
+ DWARFDIE GetDIE(const DebugNames::Entry &entry) const;
bool ProcessEntry(const DebugNames::Entry &entry,
llvm::function_ref<bool(DWARFDIE die)> callback);
|
labath
commented
May 24, 2024
| @@ -183,27 +181,22 @@ void DebugNamesDWARFIndex::GetCompleteObjCClass( | |||
| llvm::function_ref<bool(DWARFDIE die)> callback) { | |||
| // Keep a list of incomplete types as fallback for when we don't find the | |||
| // complete type. | |||
| DIEArray incomplete_types; | |||
| std::vector<DWARFDIE> incomplete_types; | |||
There was a problem hiding this comment.
This can force the parsing early parsing of some units (e.g., if we run into many incomplete types, followed by a complete definition). I don't know how often this happens, but if you think that's an issue, we can fix it by storing the incomplete types as a (DWARFUnit *, die_offset) pair here.
There was a problem hiding this comment.
Let me try to run at least one basic experiment here
felipepiovezan
approved these changes
May 28, 2024
|
LGTM as well. Avoiding bouncing between two different DIE representations is a good thing. |
vg0204
pushed a commit
to vg0204/llvm-project
that referenced
this pull request
May 29, 2024
…es (llvm#93296) DebugNamesDWARFIndex was jumping through hoops to construct a DIERef from an index entry only to jump through them back a short while later to construct a DWARFDIE. This used to be necessary as the index lookup was a two stage process, where we first enumerated all matches, and then examined them (so it was important that the enumeration was cheap -- does not trigger unnecessary parsing). However, now that the processing is callback based, we are always immediately examining the DWARFDIE right after finding the entry, and the DIERef just gets in the way.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
DebugNamesDWARFIndex was jumping through hoops to construct a DIERef from an index entry only to jump through them back a short while later to construct a DWARFDIE.
This used to be necessary as the index lookup was a two stage process, where we first enumerated all matches, and then examined them (so it was important that the enumeration was cheap -- does not trigger unnecessary parsing). However, now that the processing is callback based, we are always immediately examening the DWARFDIE right after finding the entry, and the DIERef just gets in the way.