[lldb/DWARF] Refactor DWARFDIE::Get{Decl,TypeLookup}Context #93291
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
After a bug (the bug is that the functions don't handle DW_AT_signature, aka type units) led me to one of these similar-but-different functions, I started to realize that most of the differences between these two functions are actually bugs. As a first step towards merging them, this patch rewrites both of them to follow the same pattern, while preserving all of their differences. The main change is that GetTypeLookupContext now also uses a `seen` list to avoid reference loops (currently that's not necessary because the function strictly follows parent links, but that will change with DW_AT_signatures). I've also optimized both functions to avoid recursion by contructing starting with the deepest scope first (and then reversing it).
|
@llvm/pr-subscribers-lldb Author: Pavel Labath (labath) ChangesAfter a bug (the bug is that the functions don't handle DW_AT_signature, aka type units) led me to one of these similar-but-different functions, I started to realize that most of the differences between these two functions are actually bugs. As a first step towards merging them, this patch rewrites both of them to follow the same pattern, while preserving all of their differences. The main change is that GetTypeLookupContext now also uses a I've also optimized both functions to avoid recursion by contructing starting with the deepest scope first (and then reversing it). Full diff: https://github.com/llvm/llvm-project/pull/93291.diff 1 Files Affected:
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
index 4884374ef9472..03e289bbf3300 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
@@ -13,6 +13,7 @@
#include "DWARFDebugInfoEntry.h"
#include "DWARFDeclContext.h"
#include "DWARFUnit.h"
+#include "lldb/Symbol/Type.h"
#include "llvm/ADT/iterator.h"
@@ -379,108 +380,118 @@ std::vector<DWARFDIE> DWARFDIE::GetDeclContextDIEs() const {
return result;
}
-static std::vector<lldb_private::CompilerContext>
-GetDeclContextImpl(llvm::SmallSet<lldb::user_id_t, 4> &seen, DWARFDIE die) {
- std::vector<lldb_private::CompilerContext> context;
+static void GetDeclContextImpl(DWARFDIE die,
+ llvm::SmallSet<lldb::user_id_t, 4> &seen,
+ std::vector<CompilerContext> &context) {
// Stop if we hit a cycle.
- if (!die || !seen.insert(die.GetID()).second)
- return context;
-
- // Handle outline member function DIEs by following the specification.
- if (DWARFDIE spec = die.GetReferencedDIE(DW_AT_specification))
- return GetDeclContextImpl(seen, spec);
-
- // Get the parent context chain.
- context = GetDeclContextImpl(seen, die.GetParent());
+ while (die && seen.insert(die.GetID()).second) {
+ // Handle outline member function DIEs by following the specification.
+ if (DWARFDIE spec = die.GetReferencedDIE(DW_AT_specification)) {
+ die = spec;
+ continue;
+ }
- // Add this DIE's contribution at the end of the chain.
- auto push_ctx = [&](CompilerContextKind kind, llvm::StringRef name) {
- context.push_back({kind, ConstString(name)});
- };
- switch (die.Tag()) {
- case DW_TAG_module:
- push_ctx(CompilerContextKind::Module, die.GetName());
- break;
- case DW_TAG_namespace:
- push_ctx(CompilerContextKind::Namespace, die.GetName());
- break;
- case DW_TAG_structure_type:
- push_ctx(CompilerContextKind::Struct, die.GetName());
- break;
- case DW_TAG_union_type:
- push_ctx(CompilerContextKind::Union, die.GetName());
- break;
- case DW_TAG_class_type:
- push_ctx(CompilerContextKind::Class, die.GetName());
- break;
- case DW_TAG_enumeration_type:
- push_ctx(CompilerContextKind::Enum, die.GetName());
- break;
- case DW_TAG_subprogram:
- push_ctx(CompilerContextKind::Function, die.GetName());
- break;
- case DW_TAG_variable:
- push_ctx(CompilerContextKind::Variable, die.GetPubname());
- break;
- case DW_TAG_typedef:
- push_ctx(CompilerContextKind::Typedef, die.GetName());
- break;
- default:
- break;
+ // Add this DIE's contribution at the end of the chain.
+ auto push_ctx = [&](CompilerContextKind kind, llvm::StringRef name) {
+ context.push_back({kind, ConstString(name)});
+ };
+ switch (die.Tag()) {
+ case DW_TAG_module:
+ push_ctx(CompilerContextKind::Module, die.GetName());
+ break;
+ case DW_TAG_namespace:
+ push_ctx(CompilerContextKind::Namespace, die.GetName());
+ break;
+ case DW_TAG_structure_type:
+ push_ctx(CompilerContextKind::Struct, die.GetName());
+ break;
+ case DW_TAG_union_type:
+ push_ctx(CompilerContextKind::Union, die.GetName());
+ break;
+ case DW_TAG_class_type:
+ push_ctx(CompilerContextKind::Class, die.GetName());
+ break;
+ case DW_TAG_enumeration_type:
+ push_ctx(CompilerContextKind::Enum, die.GetName());
+ break;
+ case DW_TAG_subprogram:
+ push_ctx(CompilerContextKind::Function, die.GetName());
+ break;
+ case DW_TAG_variable:
+ push_ctx(CompilerContextKind::Variable, die.GetPubname());
+ break;
+ case DW_TAG_typedef:
+ push_ctx(CompilerContextKind::Typedef, die.GetName());
+ break;
+ default:
+ break;
+ }
+ // Now process the parent.
+ die = die.GetParent();
}
- return context;
}
-std::vector<lldb_private::CompilerContext> DWARFDIE::GetDeclContext() const {
+std::vector<CompilerContext> DWARFDIE::GetDeclContext() const {
llvm::SmallSet<lldb::user_id_t, 4> seen;
- return GetDeclContextImpl(seen, *this);
+ std::vector<CompilerContext> context;
+ GetDeclContextImpl(*this, seen, context);
+ std::reverse(context.begin(), context.end());
+ return context;
}
-std::vector<lldb_private::CompilerContext>
-DWARFDIE::GetTypeLookupContext() const {
- std::vector<lldb_private::CompilerContext> context;
- // If there is no name, then there is no need to look anything up for this
- // DIE.
- const char *name = GetName();
- if (!name || !name[0])
- return context;
- const dw_tag_t tag = Tag();
- if (tag == DW_TAG_compile_unit || tag == DW_TAG_partial_unit)
- return context;
- DWARFDIE parent = GetParent();
- if (parent)
- context = parent.GetTypeLookupContext();
- auto push_ctx = [&](CompilerContextKind kind, llvm::StringRef name) {
- context.push_back({kind, ConstString(name)});
- };
- switch (tag) {
- case DW_TAG_namespace:
- push_ctx(CompilerContextKind::Namespace, name);
- break;
- case DW_TAG_structure_type:
- push_ctx(CompilerContextKind::Struct, name);
- break;
- case DW_TAG_union_type:
- push_ctx(CompilerContextKind::Union, name);
- break;
- case DW_TAG_class_type:
- push_ctx(CompilerContextKind::Class, name);
- break;
- case DW_TAG_enumeration_type:
- push_ctx(CompilerContextKind::Enum, name);
- break;
- case DW_TAG_variable:
- push_ctx(CompilerContextKind::Variable, GetPubname());
- break;
- case DW_TAG_typedef:
- push_ctx(CompilerContextKind::Typedef, name);
- break;
- case DW_TAG_base_type:
- push_ctx(CompilerContextKind::Builtin, name);
- break;
- default:
- break;
+static void GetTypeLookupContextImpl(DWARFDIE die,
+ llvm::SmallSet<lldb::user_id_t, 4> &seen,
+ std::vector<CompilerContext> &context) {
+ // Stop if we hit a cycle.
+ while (die && seen.insert(die.GetID()).second) {
+ // If there is no name, then there is no need to look anything up for this
+ // DIE.
+ const char *name = die.GetName();
+ if (!name || !name[0])
+ return;
+
+ // Add this DIE's contribution at the end of the chain.
+ auto push_ctx = [&](CompilerContextKind kind, llvm::StringRef name) {
+ context.push_back({kind, ConstString(name)});
+ };
+ switch (die.Tag()) {
+ case DW_TAG_namespace:
+ push_ctx(CompilerContextKind::Namespace, die.GetName());
+ break;
+ case DW_TAG_structure_type:
+ push_ctx(CompilerContextKind::Struct, die.GetName());
+ break;
+ case DW_TAG_union_type:
+ push_ctx(CompilerContextKind::Union, die.GetName());
+ break;
+ case DW_TAG_class_type:
+ push_ctx(CompilerContextKind::Class, die.GetName());
+ break;
+ case DW_TAG_enumeration_type:
+ push_ctx(CompilerContextKind::Enum, die.GetName());
+ break;
+ case DW_TAG_variable:
+ push_ctx(CompilerContextKind::Variable, die.GetPubname());
+ break;
+ case DW_TAG_typedef:
+ push_ctx(CompilerContextKind::Typedef, die.GetName());
+ break;
+ case DW_TAG_base_type:
+ push_ctx(CompilerContextKind::Builtin, name);
+ break;
+ default:
+ break;
+ }
+ // Now process the parent.
+ die = die.GetParent();
}
+}
+
+std::vector<CompilerContext> DWARFDIE::GetTypeLookupContext() const {
+ llvm::SmallSet<lldb::user_id_t, 4> seen;
+ std::vector<CompilerContext> context;
+ GetTypeLookupContextImpl(*this, seen, context);
+ std::reverse(context.begin(), context.end());
return context;
}
|
Michael137
approved these changes
May 29, 2024
|
Good idea. I've added one quick test. I'll add more as I work on these functions further. PTAL |
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.
After a bug (the bug is that the functions don't handle DW_AT_signature, aka type units) led me to one of these similar-but-different functions, I started to realize that most of the differences between these two functions are actually bugs.
As a first step towards merging them, this patch rewrites both of them to follow the same pattern, while preserving all of their differences. The main change is that GetTypeLookupContext now also uses a
seenlist to avoid reference loops (currently that's not necessary because the function strictly follows parent links, but that will change with DW_AT_signatures).I've also optimized both functions to avoid recursion by contructing starting with the deepest scope first (and then reversing it).