Skip to content

Improve namespace lookup using .debug_names parent chain #110062

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 30, 2024
Merged

Improve namespace lookup using .debug_names parent chain #110062

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c5bbc5f
Improve namespace lookup
Sep 24, 2024
3d83ffa
Address review feedback
Oct 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,3 +151,19 @@ bool DWARFIndex::ProcessTypeDIEMatchQuery(
return true;
return callback(die);
}

void DWARFIndex::GetNamespacesWithParents(
ConstString name, const CompilerDeclContext &parent_decl_ctx,
llvm::function_ref<bool(DWARFDIE die)> callback) {
GetNamespaces(name, [&](DWARFDIE die) {
return ProcessNamespaceDieMatchParents(parent_decl_ctx, die, callback);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is die here a DW_AT_namespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

});
}

bool DWARFIndex::ProcessNamespaceDieMatchParents(
const CompilerDeclContext &parent_decl_ctx, DWARFDIE die,
llvm::function_ref<bool(DWARFDIE die)> callback) {
if (!SymbolFileDWARF::DIEInDeclContext(parent_decl_ctx, die))
return true;
return callback(die);
}
11 changes: 11 additions & 0 deletions lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,14 @@ class DWARFIndex {
virtual void
GetTypesWithQuery(TypeQuery &query,
llvm::function_ref<bool(DWARFDIE die)> callback);
/// Get namespace DIEs whose base name match \param name with \param
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is out of date now. Update to mention query

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no query in GetNamespacesWithParents API. Are you reading GetTypesWithQuery() API whose comment has mentioned query above.

/// parent_decl_ctx in its decl parent chain. A base implementation
/// is provided. Specializations should override this if they are able to
/// provide a faster implementation.
virtual void
GetNamespacesWithParents(ConstString name,
const CompilerDeclContext &parent_decl_ctx,
llvm::function_ref<bool(DWARFDIE die)> callback);
virtual void
GetFunctions(const Module::LookupInfo &lookup_info, SymbolFileDWARF &dwarf,
const CompilerDeclContext &parent_decl_ctx,
Expand Down Expand Up @@ -127,6 +135,9 @@ class DWARFIndex {
bool
ProcessTypeDIEMatchQuery(TypeQuery &query, DWARFDIE die,
llvm::function_ref<bool(DWARFDIE die)> callback);
bool ProcessNamespaceDieMatchParents(
const CompilerDeclContext &parent_decl_ctx, DWARFDIE die,
llvm::function_ref<bool(DWARFDIE die)> callback);
};
} // namespace dwarf
} // namespace lldb_private::plugin
Expand Down
64 changes: 54 additions & 10 deletions lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,9 +368,10 @@ void DebugNamesDWARFIndex::GetFullyQualifiedType(
continue;
}

if (SameParentChain(parent_names, *parent_chain) &&
!ProcessEntry(entry, callback))
return;
if (SameParentChain(parent_names, *parent_chain)) {
if (!ProcessEntry(entry, callback))
return;
}
}
m_fallback.GetFullyQualifiedType(context, callback);
}
Expand Down Expand Up @@ -554,17 +555,60 @@ void DebugNamesDWARFIndex::GetTypesWithQuery(
continue;
}

if (WithinParentChain(parent_contexts, *parent_chain) &&
!ProcessEntry(entry, [&](DWARFDIE die) {
// After .debug_names filtering still sending to base class for
// further filtering before calling the callback.
return ProcessTypeDIEMatchQuery(query, die, callback);
}))
return;
if (WithinParentChain(parent_contexts, *parent_chain)) {
if (!ProcessEntry(entry, [&](DWARFDIE die) {
// After .debug_names filtering still sending to base class for
// further filtering before calling the callback.
return ProcessTypeDIEMatchQuery(query, die, callback);
}))
// If the callback returns false, we're done.
return;
}
}
m_fallback.GetTypesWithQuery(query, callback);
}

void DebugNamesDWARFIndex::GetNamespacesWithParents(
ConstString name, const CompilerDeclContext &parent_decl_ctx,
llvm::function_ref<bool(DWARFDIE die)> callback) {
std::vector<lldb_private::CompilerContext> parent_contexts =
parent_decl_ctx.GetCompilerContext();
llvm::SmallVector<CompilerContext> parent_named_contexts;
std::copy_if(parent_contexts.rbegin(), parent_contexts.rend(),
std::back_inserter(parent_named_contexts),
[](const CompilerContext &ctx) { return !ctx.name.IsEmpty(); });
Comment on lines +576 to +579
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you wanted to be really fancy, you could try something like this:

Suggested change
llvm::SmallVector<CompilerContext> parent_named_contexts;
std::copy_if(parent_contexts.rbegin(), parent_contexts.rend(),
std::back_inserter(parent_named_contexts),
[](const CompilerContext &ctx) { return !ctx.name.IsEmpty(); });
llvm::SmallVector<CompilerContext> parent_named_contexts(llvm::make_filter_range(llvm::reverse(parent_contexts),
[](const CompilerContext &ctx) { return !ctx.name.IsEmpty(); }));

:P

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sharing, good to know. Did not find any lldb code using this API, so will keep existing code.

for (const DebugNames::Entry &entry :
m_debug_names_up->equal_range(name.GetStringRef())) {
lldb_private::dwarf::Tag entry_tag = entry.tag();
if (entry_tag == DW_TAG_namespace ||
entry_tag == DW_TAG_imported_declaration) {
std::optional<llvm::SmallVector<Entry, 4>> parent_chain =
getParentChain(entry);
if (!parent_chain) {
// Fallback: use the base class implementation.
if (!ProcessEntry(entry, [&](DWARFDIE die) {
return ProcessNamespaceDieMatchParents(parent_decl_ctx, die,
callback);
}))
return;
continue;
}

if (WithinParentChain(parent_named_contexts, *parent_chain)) {
if (!ProcessEntry(entry, [&](DWARFDIE die) {
// After .debug_names filtering still sending to base class for
// further filtering before calling the callback.
return ProcessNamespaceDieMatchParents(parent_decl_ctx, die,
callback);
}))
// If the callback returns false, we're done.
return;
}
}
}
m_fallback.GetNamespacesWithParents(name, parent_decl_ctx, callback);
}

void DebugNamesDWARFIndex::GetFunctions(
const Module::LookupInfo &lookup_info, SymbolFileDWARF &dwarf,
const CompilerDeclContext &parent_decl_ctx,
Expand Down
4 changes: 3 additions & 1 deletion lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,9 @@ class DebugNamesDWARFIndex : public DWARFIndex {
void
GetTypesWithQuery(TypeQuery &query,
llvm::function_ref<bool(DWARFDIE die)> callback) override;

void GetNamespacesWithParents(
ConstString name, const CompilerDeclContext &parent_decl_ctx,
llvm::function_ref<bool(DWARFDIE die)> callback) override;
void GetFunctions(const Module::LookupInfo &lookup_info,
SymbolFileDWARF &dwarf,
const CompilerDeclContext &parent_decl_ctx,
Expand Down
2 changes: 1 addition & 1 deletion lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2899,7 +2899,7 @@ SymbolFileDWARF::FindNamespace(ConstString name,
if (!DeclContextMatchesThisSymbolFile(parent_decl_ctx))
return namespace_decl_ctx;

m_index->GetNamespaces(name, [&](DWARFDIE die) {
m_index->GetNamespacesWithParents(name, parent_decl_ctx, [&](DWARFDIE die) {
if (!DIEInDeclContext(parent_decl_ctx, die, only_root_namespaces))
return true; // The containing decl contexts don't match

Expand Down
Loading