[lldb/DWARF] Remove duplicate type filtering #116989
Merged
Conversation
This file contains 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
In llvm#108907, the index classes started filtering the DIEs according to the full type query (instead of just the base name). This means that the checks in SymbolFileDWARF are now redundant. I've also moved the non-redundant checks so that now all checking is done in the DWARFIndex class and the caller can expect to get the final filtered list of types.
|
@llvm/pr-subscribers-lldb Author: Pavel Labath (labath) ChangesIn #108907, the index classes started filtering the DIEs according to the full type query (instead of just the base name). This means that the checks in SymbolFileDWARF are now redundant. I've also moved the non-redundant checks so that now all checking is done in the DWARFIndex class and the caller can expect to get the final filtered list of types. Full diff: https://github.com/llvm/llvm-project/pull/116989.diff 2 Files Affected:
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
index c18edd10b96819..30c890d6d01388 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
@@ -137,9 +137,19 @@ void DWARFIndex::GetTypesWithQuery(
bool DWARFIndex::ProcessTypeDIEMatchQuery(
TypeQuery &query, DWARFDIE die,
llvm::function_ref<bool(DWARFDIE die)> callback) {
- // Nothing to match from query
- if (query.GetContextRef().size() <= 1)
+ // Check the language, but only if we have a language filter.
+ if (query.HasLanguage() &&
+ !query.LanguageMatches(SymbolFileDWARF::GetLanguageFamily(*die.GetCU())))
+ return true; // Keep iterating over index types, language mismatch.
+
+ // Since mangled names are unique, we only need to check if the names are
+ // the same.
+ if (query.GetSearchByMangledName()) {
+ if (die.GetMangledName(/*substitute_name_allowed=*/false) !=
+ query.GetTypeBasename().GetStringRef())
+ return true; // Keep iterating over index types, mangled name mismatch.
return callback(die);
+ }
std::vector<lldb_private::CompilerContext> die_context;
if (query.GetModuleSearch())
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 8ce0db4588a46a..c900f330b481bb 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -2726,39 +2726,8 @@ void SymbolFileDWARF::FindTypes(const TypeQuery &query, TypeResults &results) {
TypeQuery query_full(query);
bool have_index_match = false;
m_index->GetTypesWithQuery(query_full, [&](DWARFDIE die) {
- // Check the language, but only if we have a language filter.
- if (query.HasLanguage()) {
- if (!query.LanguageMatches(GetLanguageFamily(*die.GetCU())))
- return true; // Keep iterating over index types, language mismatch.
- }
-
- // Since mangled names are unique, we only need to check if the names are
- // the same.
- if (query.GetSearchByMangledName()) {
- if (die.GetMangledName(/*substitute_name_allowed=*/false) !=
- query.GetTypeBasename().GetStringRef())
- return true; // Keep iterating over index types, mangled name mismatch.
- if (Type *matching_type = ResolveType(die, true, true)) {
- results.InsertUnique(matching_type->shared_from_this());
- return !results.Done(query); // Keep iterating if we aren't done.
- }
- return true; // Keep iterating over index types, weren't able to resolve
- // this type
- }
-
- // Check the context matches
- std::vector<lldb_private::CompilerContext> die_context;
- if (query.GetModuleSearch())
- die_context = die.GetDeclContext();
- else
- die_context = die.GetTypeLookupContext();
- assert(!die_context.empty());
- if (!query.ContextMatches(die_context))
- return true; // Keep iterating over index types, context mismatch.
-
- // Try to resolve the type.
if (Type *matching_type = ResolveType(die, true, true)) {
- if (matching_type->IsTemplateType()) {
+ if (!query.GetSearchByMangledName() && matching_type->IsTemplateType()) {
// We have to watch out for case where we lookup a type by basename and
// it matches a template with simple template names. Like looking up
// "Foo" and if we have simple template names then we will match
@@ -2790,7 +2759,7 @@ void SymbolFileDWARF::FindTypes(const TypeQuery &query, TypeResults &results) {
// With -gsimple-template-names, a templated type's DW_AT_name will not
// contain the template parameters. Try again stripping '<' and anything
// after, filtering out entries with template parameters that don't match.
- if (!have_index_match) {
+ if (!have_index_match && !query.GetSearchByMangledName()) {
// Create a type matcher with a compiler context that is tuned for
// -gsimple-template-names. We will use this for the index lookup and the
// context matching, but will use the original "match" to insert matches
@@ -2804,23 +2773,6 @@ void SymbolFileDWARF::FindTypes(const TypeQuery &query, TypeResults &results) {
// Copy our match's context and update the basename we are looking for
// so we can use this only to compare the context correctly.
m_index->GetTypesWithQuery(query_simple, [&](DWARFDIE die) {
- // Check the language, but only if we have a language filter.
- if (query.HasLanguage()) {
- if (!query.LanguageMatches(GetLanguageFamily(*die.GetCU())))
- return true; // Keep iterating over index types, language mismatch.
- }
-
- // Check the context matches
- std::vector<lldb_private::CompilerContext> die_context;
- if (query.GetModuleSearch())
- die_context = die.GetDeclContext();
- else
- die_context = die.GetTypeLookupContext();
- assert(!die_context.empty());
- if (!query_simple.ContextMatches(die_context))
- return true; // Keep iterating over index types, context mismatch.
-
- // Try to resolve the type.
if (Type *matching_type = ResolveType(die, true, true)) {
ConstString name = matching_type->GetQualifiedName();
// We have found a type that still might not match due to template
|
labath
commented
Nov 20, 2024
Comment on lines
-140
to
-141
| // Nothing to match from query | ||
| if (query.GetContextRef().size() <= 1) |
There was a problem hiding this comment.
This wasn't correct because it wasn't checking the "ComilerContextKind" part of the query. It didn't matter because of the "second line of defense" (which is now gone).
augusto2112
approved these changes
Nov 21, 2024
| die_context = die.GetDeclContext(); | ||
| else | ||
| die_context = die.GetTypeLookupContext(); | ||
| assert(!die_context.empty()); |
There was a problem hiding this comment.
This assert is missing in DWARFIndex::ProcessTypeDIEMatchQuery, maybe add that back?
There was a problem hiding this comment.
I don't feel strongly about this, but I don't think this should be an assert. Like, we can probably reach this code if a bad (corrupted) index points us to an empty DIE. According to the assertion manifesto this shouldn't crash lldb.
|
Apologies for the late ping but looks like this is breaking macOS CI: https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/as-lldb-cmake/16280/execution/node/97/log/?consoleFull Note this is the |
|
I'm looking into this now. |
labath
added a commit
to labath/llvm-project
that referenced
this pull request
Dec 3, 2024
.. in the global namespace The problem was the interaction of llvm#116989 with an optimization in GetTypesWithQuery. The optimization was only correct for non-exact matches, but that didn't matter before this PR due to the "second layer of defense". After that was removed, the query started returning more types than it should.
labath
added a commit
that referenced
this pull request
Dec 3, 2024
.. in the global namespace The problem was the interaction of #116989 with an optimization in GetTypesWithQuery. The optimization was only correct for non-exact matches, but that didn't matter before this PR due to the "second layer of defense". After that was removed, the query started returning more types than it should.
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.
In #108907, the index classes started filtering the DIEs according to the full type query (instead of just the base name). This means that the checks in SymbolFileDWARF are now redundant.
I've also moved the non-redundant checks so that now all checking is done in the DWARFIndex class and the caller can expect to get the final filtered list of types.