[lldb] Refactor TypeQuery::ContextMatches, take 2 #101333
Merged
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
This is an alternative, much simpler implementation of llvm#99305. In this version I replace the AnyModule wildcard match with a special TypeQuery flag which achieves (mostly) the same thing. It is a preparatory step for teaching ContextMatches about anonymous namespaces. It started out as a way to remove the assumption that the pattern and target contexts must be of the same length -- that's will not be correct with anonymous namespaces, and probably isn't even correct right now for AnyModule matches.
|
@llvm/pr-subscribers-lldb Author: Pavel Labath (labath) ChangesThis is an alternative, much simpler implementation of #99305. In this version I replace the AnyModule wildcard match with a special TypeQuery flag which achieves (mostly) the same thing. It is a preparatory step for teaching ContextMatches about anonymous namespaces. It started out as a way to remove the assumption that the pattern and target contexts must be of the same length -- that's will not be correct with anonymous namespaces, and probably isn't even correct right now for AnyModule matches. Full diff: https://github.com/llvm/llvm-project/pull/101333.diff 7 Files Affected:
diff --git a/lldb/include/lldb/Symbol/Type.h b/lldb/include/lldb/Symbol/Type.h
index c6f30cde81867..a35f9974fa39a 100644
--- a/lldb/include/lldb/Symbol/Type.h
+++ b/lldb/include/lldb/Symbol/Type.h
@@ -65,11 +65,6 @@ struct CompilerContext {
llvm::raw_ostream &operator<<(llvm::raw_ostream &os,
const CompilerContext &rhs);
-/// Match \p context_chain against \p pattern, which may contain "Any"
-/// kinds. The \p context_chain should *not* contain any "Any" kinds.
-bool contextMatches(llvm::ArrayRef<CompilerContext> context_chain,
- llvm::ArrayRef<CompilerContext> pattern);
-
FLAGS_ENUM(TypeQueryOptions){
e_none = 0u,
/// If set, TypeQuery::m_context contains an exact context that must match
@@ -79,10 +74,13 @@ FLAGS_ENUM(TypeQueryOptions){
/// If set, TypeQuery::m_context is a clang module compiler context. If not
/// set TypeQuery::m_context is normal type lookup context.
e_module_search = (1u << 1),
+ /// If set, the query will ignore all Module entries in the type context,
+ /// even for exact matches.
+ e_ignore_modules = (1u << 2),
/// When true, the find types call should stop the query as soon as a single
/// matching type is found. When false, the type query should find all
/// matching types.
- e_find_one = (1u << 2),
+ e_find_one = (1u << 3),
};
LLDB_MARK_AS_BITMASK_ENUM(TypeQueryOptions)
@@ -264,6 +262,9 @@ class TypeQuery {
bool LanguageMatches(lldb::LanguageType language) const;
bool GetExactMatch() const { return (m_options & e_exact_match) != 0; }
+
+ bool GetIgnoreModules() const { return (m_options & e_ignore_modules) != 0; }
+
/// The \a m_context can be used in two ways: normal types searching with
/// the context containing a stanadard declaration context for a type, or
/// with the context being more complete for exact matches in clang modules.
diff --git a/lldb/include/lldb/lldb-private-enumerations.h b/lldb/include/lldb/lldb-private-enumerations.h
index 9d18316dcea25..c24a3538f58da 100644
--- a/lldb/include/lldb/lldb-private-enumerations.h
+++ b/lldb/include/lldb/lldb-private-enumerations.h
@@ -207,8 +207,6 @@ enum class CompilerContextKind : uint16_t {
Builtin = 1 << 10,
Any = 1 << 15,
- /// Match 0..n nested modules.
- AnyModule = Any | Module,
/// Match any type.
AnyType = Any | ClassOrStruct | Union | Enum | Typedef | Builtin,
/// Math any declaration context.
diff --git a/lldb/source/Symbol/Type.cpp b/lldb/source/Symbol/Type.cpp
index e76574795733f..eb321407e3734 100644
--- a/lldb/source/Symbol/Type.cpp
+++ b/lldb/source/Symbol/Type.cpp
@@ -6,7 +6,9 @@
//
//===----------------------------------------------------------------------===//
+#include <algorithm>
#include <cstdio>
+#include <iterator>
#include <optional>
#include "lldb/Core/Module.h"
@@ -30,6 +32,7 @@
#include "lldb/Target/Process.h"
#include "lldb/Target/Target.h"
#include "lldb/lldb-enumerations.h"
+#include "lldb/lldb-private-enumerations.h"
#include "llvm/ADT/StringRef.h"
@@ -43,35 +46,6 @@ llvm::raw_ostream &lldb_private::operator<<(llvm::raw_ostream &os,
return os << lldb_stream.GetString();
}
-bool lldb_private::contextMatches(llvm::ArrayRef<CompilerContext> context_chain,
- llvm::ArrayRef<CompilerContext> pattern) {
- auto ctx = context_chain.begin();
- auto ctx_end = context_chain.end();
- for (const CompilerContext &pat : pattern) {
- // Early exit if the pattern is too long.
- if (ctx == ctx_end)
- return false;
- if (*ctx != pat) {
- // Skip any number of module matches.
- if (pat.kind == CompilerContextKind::AnyModule) {
- // Greedily match 0..n modules.
- ctx = std::find_if(ctx, ctx_end, [](const CompilerContext &ctx) {
- return ctx.kind != CompilerContextKind::Module;
- });
- continue;
- }
- // See if there is a kind mismatch; they should have 1 bit in common.
- if (((uint16_t)ctx->kind & (uint16_t)pat.kind) == 0)
- return false;
- // The name is ignored for AnyModule, but not for AnyType.
- if (pat.kind != CompilerContextKind::AnyModule && ctx->name != pat.name)
- return false;
- }
- ++ctx;
- }
- return true;
-}
-
static CompilerContextKind ConvertTypeClass(lldb::TypeClass type_class) {
if (type_class == eTypeClassAny)
return CompilerContextKind::AnyType;
@@ -153,19 +127,36 @@ void TypeQuery::SetLanguages(LanguageSet languages) {
bool TypeQuery::ContextMatches(
llvm::ArrayRef<CompilerContext> context_chain) const {
- if (GetExactMatch() || context_chain.size() == m_context.size())
- return ::contextMatches(context_chain, m_context);
+ auto ctx = context_chain.rbegin(), ctx_end = context_chain.rend();
+ for (auto pat = m_context.rbegin(), pat_end = m_context.rend();
+ pat != pat_end;) {
+
+ if (ctx == ctx_end)
+ return false; // Pattern too long.
+
+ // See if there is a kind mismatch; they should have 1 bit in common.
+ if ((ctx->kind & pat->kind) == CompilerContextKind())
+ return false;
+
+ if (ctx->name != pat->name)
+ return false;
+
+ ++ctx;
+ ++pat;
+ }
+
+ // Skip over any remaining module entries if we were asked to do that.
+ while (GetIgnoreModules() && ctx != ctx_end &&
+ ctx->kind == CompilerContextKind::Module)
+ ++ctx;
- // We don't have an exact match, we need to bottom m_context.size() items to
- // match for a successful lookup.
- if (context_chain.size() < m_context.size())
- return false; // Not enough items in context_chain to allow for a match.
+ // At this point, we have exhausted the pattern and we have a partial match at
+ // least. If that's all we're looking for, we're done.
+ if (!GetExactMatch())
+ return true;
- size_t compare_count = context_chain.size() - m_context.size();
- return ::contextMatches(
- llvm::ArrayRef<CompilerContext>(context_chain.data() + compare_count,
- m_context.size()),
- m_context);
+ // We have an exact match if we've exhausted the target context as well.
+ return ctx == ctx_end;
}
bool TypeQuery::LanguageMatches(lldb::LanguageType language) const {
@@ -223,9 +214,6 @@ void CompilerContext::Dump(Stream &s) const {
case CompilerContextKind::Typedef:
s << "Typedef";
break;
- case CompilerContextKind::AnyModule:
- s << "AnyModule";
- break;
case CompilerContextKind::AnyType:
s << "AnyType";
break;
diff --git a/lldb/test/Shell/SymbolFile/DWARF/clang-gmodules-type-lookup.c b/lldb/test/Shell/SymbolFile/DWARF/clang-gmodules-type-lookup.c
index a8da692e57b1f..4e5f8905adecf 100644
--- a/lldb/test/Shell/SymbolFile/DWARF/clang-gmodules-type-lookup.c
+++ b/lldb/test/Shell/SymbolFile/DWARF/clang-gmodules-type-lookup.c
@@ -6,8 +6,9 @@
// RUN: %clangxx_host -g -gmodules -fmodules -std=c99 -x c-header %S/Inputs/pch.h -g -c -o %t.pch
// RUN: %clangxx_host -g -gmodules -fmodules -std=c99 -x c -include-pch %t.pch %s -c -o %t.o
// RUN: %clangxx_host %t.o -o %t.exe
-// RUN: lldb-test symbols -dump-clang-ast -find type --language=C99 \
-// RUN: -compiler-context 'AnyModule:*,ClassOrStruct:TypeFromPCH' %t.exe | FileCheck %s
+// RUN: lldb-test symbols -dump-clang-ast -find type --find-in-any-module \
+// RUN: --language=C99 -compiler-context 'ClassOrStruct:TypeFromPCH' \
+// RUN: %t.exe | FileCheck %s
anchor_t anchor;
diff --git a/lldb/test/Shell/SymbolFile/DWARF/x86/compilercontext.ll b/lldb/test/Shell/SymbolFile/DWARF/x86/compilercontext.ll
index 8fa45e955abf1..f64b363b1a002 100644
--- a/lldb/test/Shell/SymbolFile/DWARF/x86/compilercontext.ll
+++ b/lldb/test/Shell/SymbolFile/DWARF/x86/compilercontext.ll
@@ -9,11 +9,8 @@
; RUN: lldb-test symbols %t.o -find=type --language=C99 \
; RUN: -compiler-context="Module:CModule,Module:SubModule,ClassOrStruct:FromSubmodule" \
; RUN: | FileCheck %s
-; RUN: lldb-test symbols %t.o -find=type --language=C99 \
-; RUN: -compiler-context="Module:CModule,AnyModule:*,ClassOrStruct:FromSubmodule" \
-; RUN: | FileCheck %s
-; RUN: lldb-test symbols %t.o -find=type --language=C99 \
-; RUN: -compiler-context="AnyModule:*,ClassOrStruct:FromSubmodule" \
+; RUN: lldb-test symbols %t.o -find=type -find-in-any-module --language=C99 \
+; RUN: -compiler-context="ClassOrStruct:FromSubmodule" \
; RUN: | FileCheck %s
; RUN: lldb-test symbols %t.o -find=type --language=C99 \
; RUN: -compiler-context="Module:CModule,Module:SubModule,AnyType:FromSubmodule" \
diff --git a/lldb/tools/lldb-test/lldb-test.cpp b/lldb/tools/lldb-test/lldb-test.cpp
index da3822393dad1..535422a6d827b 100644
--- a/lldb/tools/lldb-test/lldb-test.cpp
+++ b/lldb/tools/lldb-test/lldb-test.cpp
@@ -194,6 +194,12 @@ static cl::opt<std::string> CompilerContext(
cl::desc("Specify a compiler context as \"kind:name,...\"."),
cl::value_desc("context"), cl::sub(SymbolsSubcommand));
+static cl::opt<bool> FindInAnyModule(
+ "find-in-any-module",
+ cl::desc("If true, the type will be searched for in all modules. Otherwise "
+ "the modules must be provided in -compiler-context"),
+ cl::sub(SymbolsSubcommand));
+
static cl::opt<std::string>
Language("language", cl::desc("Specify a language type, like C99."),
cl::value_desc("language"), cl::sub(SymbolsSubcommand));
@@ -312,7 +318,6 @@ llvm::SmallVector<CompilerContext, 4> parseCompilerContext() {
.Case("Variable", CompilerContextKind::Variable)
.Case("Enum", CompilerContextKind::Enum)
.Case("Typedef", CompilerContextKind::Typedef)
- .Case("AnyModule", CompilerContextKind::AnyModule)
.Case("AnyType", CompilerContextKind::AnyType)
.Default(CompilerContextKind::Invalid);
if (value.empty()) {
@@ -581,11 +586,13 @@ Error opts::symbols::findTypes(lldb_private::Module &Module) {
if (!ContextOr)
return ContextOr.takeError();
+ TypeQueryOptions Opts = TypeQueryOptions::e_module_search;
+ if (FindInAnyModule)
+ Opts |= TypeQueryOptions::e_ignore_modules;
TypeResults results;
if (!Name.empty()) {
if (ContextOr->IsValid()) {
- TypeQuery query(*ContextOr, ConstString(Name),
- TypeQueryOptions::e_module_search);
+ TypeQuery query(*ContextOr, ConstString(Name), Opts);
if (!Language.empty())
query.AddLanguage(Language::GetLanguageTypeFromString(Language));
Symfile.FindTypes(query, results);
@@ -596,7 +603,7 @@ Error opts::symbols::findTypes(lldb_private::Module &Module) {
Symfile.FindTypes(query, results);
}
} else {
- TypeQuery query(parseCompilerContext(), TypeQueryOptions::e_module_search);
+ TypeQuery query(parseCompilerContext(), Opts);
if (!Language.empty())
query.AddLanguage(Language::GetLanguageTypeFromString(Language));
Symfile.FindTypes(query, results);
@@ -816,6 +823,9 @@ Expected<Error (*)(lldb_private::Module &)> opts::symbols::getAction() {
if (Regex && Name.empty())
return make_string_error("-regex used without a -name");
+ if (FindInAnyModule && (Find != FindType::Type))
+ return make_string_error("-find-in-any-module only works with -find=type");
+
switch (Find) {
case FindType::None:
if (!Context.empty() || !Name.empty() || !File.empty() || Line != 0)
diff --git a/lldb/unittests/Symbol/TestType.cpp b/lldb/unittests/Symbol/TestType.cpp
index 79201d6ba2e59..438f141ff3c1c 100644
--- a/lldb/unittests/Symbol/TestType.cpp
+++ b/lldb/unittests/Symbol/TestType.cpp
@@ -7,13 +7,16 @@
//
//===----------------------------------------------------------------------===//
+#include "gmock/gmock.h"
#include "gtest/gtest.h"
#include "lldb/Symbol/Type.h"
#include "lldb/lldb-enumerations.h"
+#include "lldb/lldb-private-enumerations.h"
using namespace lldb;
using namespace lldb_private;
+using testing::Not;
TEST(Type, GetTypeScopeAndBasename) {
EXPECT_EQ(Type::GetTypeScopeAndBasename("int"),
@@ -47,40 +50,64 @@ TEST(Type, GetTypeScopeAndBasename) {
EXPECT_EQ(Type::GetTypeScopeAndBasename("foo<::bar"), std::nullopt);
}
+namespace {
+MATCHER_P(Matches, pattern, "") {
+ TypeQuery query(pattern, TypeQueryOptions::e_none);
+ return query.ContextMatches(arg);
+}
+MATCHER_P(MatchesIgnoringModules, pattern, "") {
+ TypeQuery query(pattern, TypeQueryOptions::e_ignore_modules);
+ return query.ContextMatches(arg);
+}
+} // namespace
+
TEST(Type, CompilerContextPattern) {
- std::vector<CompilerContext> mmc = {
- {CompilerContextKind::Module, ConstString("A")},
- {CompilerContextKind::Module, ConstString("B")},
- {CompilerContextKind::ClassOrStruct, ConstString("S")}};
- std::vector<CompilerContext> mc = {
- {CompilerContextKind::Module, ConstString("A")},
- {CompilerContextKind::ClassOrStruct, ConstString("S")}};
- std::vector<CompilerContext> mac = {
- {CompilerContextKind::Module, ConstString("A")},
- {CompilerContextKind::AnyModule, ConstString("*")},
- {CompilerContextKind::ClassOrStruct, ConstString("S")}};
- EXPECT_TRUE(contextMatches(mmc, mac));
- EXPECT_TRUE(contextMatches(mc, mac));
- EXPECT_FALSE(contextMatches(mac, mc));
- std::vector<CompilerContext> mmmc = {
- {CompilerContextKind::Module, ConstString("A")},
- {CompilerContextKind::Module, ConstString("B")},
- {CompilerContextKind::Module, ConstString("C")},
- {CompilerContextKind::ClassOrStruct, ConstString("S")}};
- EXPECT_TRUE(contextMatches(mmmc, mac));
- std::vector<CompilerContext> mme = {
- {CompilerContextKind::Module, ConstString("A")},
- {CompilerContextKind::Module, ConstString("B")},
- {CompilerContextKind::Enum, ConstString("S")}};
- std::vector<CompilerContext> mma = {
- {CompilerContextKind::Module, ConstString("A")},
- {CompilerContextKind::Module, ConstString("B")},
- {CompilerContextKind::AnyType, ConstString("S")}};
- EXPECT_TRUE(contextMatches(mme, mma));
- EXPECT_TRUE(contextMatches(mmc, mma));
- std::vector<CompilerContext> mme2 = {
- {CompilerContextKind::Module, ConstString("A")},
- {CompilerContextKind::Module, ConstString("B")},
- {CompilerContextKind::Enum, ConstString("S2")}};
- EXPECT_FALSE(contextMatches(mme2, mma));
+ auto make_module = [](llvm::StringRef name) {
+ return CompilerContext(CompilerContextKind::Module, ConstString(name));
+ };
+ auto make_class = [](llvm::StringRef name) {
+ return CompilerContext(CompilerContextKind::ClassOrStruct,
+ ConstString(name));
+ };
+ auto make_any_type = [](llvm::StringRef name) {
+ return CompilerContext(CompilerContextKind::AnyType, ConstString(name));
+ };
+ auto make_enum = [](llvm::StringRef name) {
+ return CompilerContext(CompilerContextKind::Enum, ConstString(name));
+ };
+ auto make_namespace = [](llvm::StringRef name) {
+ return CompilerContext(CompilerContextKind::Namespace, ConstString(name));
+ };
+
+ EXPECT_THAT(
+ (std::vector{make_module("A"), make_module("B"), make_class("C")}),
+ Matches(
+ std::vector{make_module("A"), make_module("B"), make_class("C")}));
+ EXPECT_THAT(
+ (std::vector{make_module("A"), make_module("B"), make_class("C")}),
+ Not(Matches(std::vector{make_class("C")})));
+ EXPECT_THAT(
+ (std::vector{make_module("A"), make_module("B"), make_class("C")}),
+ MatchesIgnoringModules(std::vector{make_class("C")}));
+ EXPECT_THAT(
+ (std::vector{make_module("A"), make_module("B"), make_class("C")}),
+ MatchesIgnoringModules(std::vector{make_module("B"), make_class("C")}));
+ EXPECT_THAT(
+ (std::vector{make_module("A"), make_module("B"), make_class("C")}),
+ Not(MatchesIgnoringModules(std::vector{make_module("A"), make_class("C")})));
+ EXPECT_THAT((std::vector{make_module("A"), make_module("B"), make_enum("C")}),
+ Matches(std::vector{make_module("A"), make_module("B"),
+ make_any_type("C")}));
+ EXPECT_THAT(
+ (std::vector{make_module("A"), make_module("B"), make_class("C")}),
+ Matches(
+ std::vector{make_module("A"), make_module("B"), make_any_type("C")}));
+ EXPECT_THAT(
+ (std::vector{make_module("A"), make_module("B"), make_enum("C2")}),
+ Not(Matches(std::vector{make_module("A"), make_module("B"),
+ make_any_type("C")})));
+ EXPECT_THAT((std::vector{make_class("C")}),
+ Matches(std::vector{make_class("C")}));
+ EXPECT_THAT((std::vector{make_namespace("NS"), make_class("C")}),
+ Not(Matches(std::vector{make_any_type("C")})));
}
|
|
@Michael137, I'd appreciate it if you could give this a spin. If https://github.com/swiftlang/llvm-project/blob/ee8bc8b8d30eb99807adbcd3c1f044e00af66cdd/lldb/source/Plugins/TypeSystem/Swift/TypeSystemSwiftTypeRef.cpp#L219-L225 is the only use of |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Michael137
reviewed
Jul 31, 2024
| if (GetExactMatch() || context_chain.size() == m_context.size()) | ||
| return ::contextMatches(context_chain, m_context); | ||
| auto ctx = context_chain.rbegin(), ctx_end = context_chain.rend(); | ||
| for (auto pat = m_context.rbegin(), pat_end = m_context.rend(); |
There was a problem hiding this comment.
Could this loop be just a std::search?
There was a problem hiding this comment.
With the modules out of the way, I guess it could be, but then I'd have to rewrite it again to this form for anonymous namespaces.
| } | ||
|
|
||
| // Skip over any remaining module entries if we were asked to do that. | ||
| while (GetIgnoreModules() && ctx != ctx_end && |
There was a problem hiding this comment.
Since the m_options won't change, we could lift this condition like so:
Suggested change
| while (GetIgnoreModules() && ctx != ctx_end && | |
| if (GetIgnoreModules()) | |
| while (ctx != ctx_end && |
But feel free to ignore
There was a problem hiding this comment.
Yeah, I've thought about that when writing this, but I figured that we shouldn't make the compiler's job too easy :)
I also tried a version with std::find, but that ended up even longer.
|
@Michael137 Have you tried applying the patch to swift-lldb and running the tests? |
Not yet, but planning to do so sometime this week |
Michael137
approved these changes
Aug 1, 2024
I can add that here. Thanks for checking it out. |
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.
This is an alternative, much simpler implementation of #99305. In this version I replace the AnyModule wildcard match with a special TypeQuery flag which achieves (mostly) the same thing.
It is a preparatory step for teaching ContextMatches about anonymous namespaces. It started out as a way to remove the assumption that the pattern and target contexts must be of the same length -- that's will not be correct with anonymous namespaces, and probably isn't even correct right now for AnyModule matches.