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 1 commit
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

@clayborg clayborg Oct 11, 2024

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

@jeffreytan81 jeffreytan81 Oct 14, 2024

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

@clayborg clayborg Oct 11, 2024

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

@jeffreytan81 jeffreytan81 Oct 14, 2024

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
42 changes: 42 additions & 0 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 @@ -565,6 +565,48 @@ void DebugNamesDWARFIndex::GetTypesWithQuery(
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();
if (parent_contexts.empty())
return GetNamespaces(name, callback);
Copy link
Collaborator

@labath labath Oct 10, 2024

Choose a reason for hiding this comment

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

Is that right? AIUI, GetNamespaces will return all namespaces with the given name, regardless of their nesting level, but parent_contexts.empty() basically means "I'm looking for a top-level namespace".

Copy link
Collaborator

@clayborg clayborg Oct 11, 2024

Choose a reason for hiding this comment

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

Yeah, do we expect if "parent_decl_ctx" is empty that we return every namespace, or just top level ones? Depends on how this API is being used.


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

@labath labath Oct 10, 2024

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

@jeffreytan81 jeffreytan81 Oct 14, 2024

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) &&
!ProcessEntry(entry, [&](DWARFDIE die) {
Copy link
Collaborator

@labath labath Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It may be better to unwrap this into two ifs. The way it is now, it took me a while to figure out that the return only happens when it should (i.e., only when the callback says it is done, and not because WithinParentChain did not find a match).

Copy link
Contributor

@felipepiovezan felipepiovezan Oct 11, 2024

Choose a reason for hiding this comment

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

In the past, we've floated around the idea of swapping the boolean return from the callbacks with some enumeration saying: KeepSearching, StopSearching or something along those lines. This should also improve readability of code invoking the callbacks (and the callbacks themselves).

But I also agree with unwrapping the ifs

Copy link
Collaborator

@clayborg clayborg Oct 11, 2024

Choose a reason for hiding this comment

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

We have a private enum:

enum class IterationAction {
  Continue = 0,
  Stop,
};

// After .debug_names filtering still sending to base class for
// further filtering before calling the callback.
return ProcessNamespaceDieMatchParents(parent_decl_ctx, die,
callback);
}))
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