Author: Pavel Labath Date: 2024-09-02T08:34:14+02:00 New Revision: dd5d73007240712957f2b633f795d9965afaadd6
URL: https://github.com/llvm/llvm-project/commit/dd5d73007240712957f2b633f795d9965afaadd6 DIFF: https://github.com/llvm/llvm-project/commit/dd5d73007240712957f2b633f795d9965afaadd6.diff LOG: [lldb] Better matching of types in anonymous namespaces (#102111) This patch extends TypeQuery matching to support anonymous namespaces. A new flag is added to control the behavior. In the "strict" mode, the query must match the type exactly -- all anonymous namespaces included. The dynamic type resolver in the itanium abi (the motivating use case for this) uses this flag, as it queries using the name from the demangles, which includes anonymous namespaces. This ensures we don't confuse a type with a same-named type in an anonymous namespace. However, this does *not* ensure we don't confuse two types in anonymous namespacs (in different CUs). To resolve this, we would need to use a completely different lookup algorithm, which probably also requires a DWARF extension. In the "lax" mode (the default), the anonymous namespaces in the query are optional, and this allows one search for the type using the usual language rules (`::A` matches `::(anonymous namespace)::A`). This patch also changes the type context computation algorithm in DWARFDIE, so that it includes anonymous namespace information. This causes a slight change in behavior: the algorithm previously stopped computing the context after encountering an anonymous namespace, which caused the outer namespaces to be ignored. This meant that a type like `NS::(anonymous namespace)::A` would be (incorrectly) recognized as `::A`). This can cause code depending on the old behavior to misbehave. The fix is to specify all the enclosing namespaces in the query, or use a non-exact match. Added: lldb/test/API/lang/cpp/dynamic-value/a.h lldb/test/API/lang/cpp/dynamic-value/anonymous-b.cpp Modified: lldb/include/lldb/Symbol/Type.h lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp lldb/source/Symbol/Type.cpp lldb/test/API/lang/cpp/dynamic-value/Makefile lldb/test/API/lang/cpp/dynamic-value/TestDynamicValue.py lldb/test/API/lang/cpp/dynamic-value/pass-to-base.cpp lldb/test/API/lang/cpp/namespace/TestNamespace.py lldb/unittests/Symbol/TestType.cpp lldb/unittests/SymbolFile/DWARF/DWARFDIETest.cpp Removed: ################################################################################ diff --git a/lldb/include/lldb/Symbol/Type.h b/lldb/include/lldb/Symbol/Type.h index 420307e0dbcf02..03d9f927997476 100644 --- a/lldb/include/lldb/Symbol/Type.h +++ b/lldb/include/lldb/Symbol/Type.h @@ -77,10 +77,13 @@ FLAGS_ENUM(TypeQueryOptions){ /// If set, the query will ignore all Module entries in the type context, /// even for exact matches. e_ignore_modules = (1u << 2), + /// If set, all anonymous namespaces in the context must be matched exactly + /// by the pattern. Otherwise, superfluous namespaces are skipped. + e_strict_namespaces = (1u << 3), /// 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 << 3), + e_find_one = (1u << 4), }; LLDB_MARK_AS_BITMASK_ENUM(TypeQueryOptions) @@ -264,7 +267,22 @@ class TypeQuery { bool GetExactMatch() const { return (m_options & e_exact_match) != 0; } bool GetIgnoreModules() const { return (m_options & e_ignore_modules) != 0; } - void SetIgnoreModules() { m_options &= ~e_ignore_modules; } + void SetIgnoreModules(bool b) { + if (b) + m_options |= e_ignore_modules; + else + m_options &= ~e_ignore_modules; + } + + bool GetStrictNamespaces() const { + return (m_options & e_strict_namespaces) != 0; + } + void SetStrictNamespaces(bool b) { + if (b) + m_options |= e_strict_namespaces; + else + m_options &= ~e_strict_namespaces; + } /// 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 @@ -279,7 +297,7 @@ class TypeQuery { if (b) m_options |= e_find_one; else - m_options &= (e_exact_match | e_find_one); + m_options &= ~e_find_one; } /// Access the internal compiler context array. diff --git a/lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp b/lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp index 7af768aad0bc19..4c547afe30fe81 100644 --- a/lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp +++ b/lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp @@ -90,6 +90,7 @@ TypeAndOrName ItaniumABILanguageRuntime::GetTypeInfo( TypeResults results; TypeQuery query(const_lookup_name.GetStringRef(), TypeQueryOptions::e_exact_match | + TypeQueryOptions::e_strict_namespaces | TypeQueryOptions::e_find_one); if (module_sp) { module_sp->FindTypes(query, results); diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp index fb32e2adeb3fea..0a13c457a307ae 100644 --- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp +++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp @@ -440,12 +440,6 @@ static void GetTypeLookupContextImpl(DWARFDIE die, continue; } - // 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)}); @@ -471,7 +465,7 @@ static void GetTypeLookupContextImpl(DWARFDIE die, push_ctx(CompilerContextKind::Typedef, die.GetName()); break; case DW_TAG_base_type: - push_ctx(CompilerContextKind::Builtin, name); + push_ctx(CompilerContextKind::Builtin, die.GetName()); break; // If any of the tags below appear in the parent chain, stop the decl // context and return. Prior to these being in here, if a type existed in a diff --git a/lldb/source/Symbol/Type.cpp b/lldb/source/Symbol/Type.cpp index eb321407e3734c..f7b44ade0da165 100644 --- a/lldb/source/Symbol/Type.cpp +++ b/lldb/source/Symbol/Type.cpp @@ -134,6 +134,20 @@ bool TypeQuery::ContextMatches( if (ctx == ctx_end) return false; // Pattern too long. + if (ctx->kind == CompilerContextKind::Namespace && ctx->name.IsEmpty()) { + // We're matching an anonymous namespace. These are optional, so we check + // if the pattern expects an anonymous namespace. + if (pat->name.IsEmpty() && (pat->kind & CompilerContextKind::Namespace) == + CompilerContextKind::Namespace) { + // Match, advance both iterators. + ++pat; + } + // Otherwise, only advance the context to skip over the anonymous + // namespace, and try matching again. + ++ctx; + continue; + } + // See if there is a kind mismatch; they should have 1 bit in common. if ((ctx->kind & pat->kind) == CompilerContextKind()) return false; @@ -145,10 +159,16 @@ bool TypeQuery::ContextMatches( ++pat; } - // Skip over any remaining module entries if we were asked to do that. - while (GetIgnoreModules() && ctx != ctx_end && - ctx->kind == CompilerContextKind::Module) - ++ctx; + // Skip over any remaining module and anonymous namespace entries if we were + // asked to do that. + auto should_skip = [this](const CompilerContext &ctx) { + if (ctx.kind == CompilerContextKind::Module) + return GetIgnoreModules(); + if (ctx.kind == CompilerContextKind::Namespace && ctx.name.IsEmpty()) + return !GetStrictNamespaces(); + return false; + }; + ctx = std::find_if_not(ctx, ctx_end, should_skip); // 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. @@ -788,7 +808,13 @@ Type::GetTypeScopeAndBasename(llvm::StringRef name) { switch (pos.value()) { case ':': if (prev_is_colon && template_depth == 0) { - result.scope.push_back(name.slice(name_begin, pos.index() - 1)); + llvm::StringRef scope_name = name.slice(name_begin, pos.index() - 1); + // The itanium demangler uses this string to represent anonymous + // namespaces. Convert it to a more language-agnostic form (which is + // also used in DWARF). + if (scope_name == "(anonymous namespace)") + scope_name = ""; + result.scope.push_back(scope_name); name_begin = pos.index() + 1; } break; diff --git a/lldb/test/API/lang/cpp/dynamic-value/Makefile b/lldb/test/API/lang/cpp/dynamic-value/Makefile index 2bba8e757f79b7..ce91dc63f473f5 100644 --- a/lldb/test/API/lang/cpp/dynamic-value/Makefile +++ b/lldb/test/API/lang/cpp/dynamic-value/Makefile @@ -1,3 +1,3 @@ -CXX_SOURCES := pass-to-base.cpp +CXX_SOURCES := pass-to-base.cpp anonymous-b.cpp include Makefile.rules diff --git a/lldb/test/API/lang/cpp/dynamic-value/TestDynamicValue.py b/lldb/test/API/lang/cpp/dynamic-value/TestDynamicValue.py index 60a2590e1559d3..e016168f047c19 100644 --- a/lldb/test/API/lang/cpp/dynamic-value/TestDynamicValue.py +++ b/lldb/test/API/lang/cpp/dynamic-value/TestDynamicValue.py @@ -170,7 +170,7 @@ def test_get_dynamic_vals(self): self.assertTrue(reallyA_value) reallyA_loc = int(reallyA_value.GetLocation(), 16) - # Finally continue to doSomething again, and make sure we get the right value for anotherA, + # Continue to doSomething again, and make sure we get the right value for anotherA, # which this time around is just an "A". threads = lldbutil.continue_to_breakpoint(process, do_something_bpt) @@ -184,6 +184,19 @@ def test_get_dynamic_vals(self): self.assertEqual(anotherA_loc, reallyA_loc) self.assertEqual(anotherA_value.GetTypeName().find("B"), -1) + # Finally do the same with a B in an anonymous namespace. + threads = lldbutil.continue_to_breakpoint(process, do_something_bpt) + self.assertEqual(len(threads), 1) + thread = threads[0] + + frame = thread.GetFrameAtIndex(0) + anotherA_value = frame.FindVariable("anotherA", use_dynamic) + self.assertTrue(anotherA_value) + self.assertIn("B", anotherA_value.GetTypeName()) + anon_b_value = anotherA_value.GetChildMemberWithName("m_anon_b_value") + self.assertTrue(anon_b_value) + self.assertEqual(anon_b_value.GetValueAsSigned(), 47) + def examine_value_object_of_this_ptr( self, this_static, this_dynamic, dynamic_location ): diff --git a/lldb/test/API/lang/cpp/dynamic-value/a.h b/lldb/test/API/lang/cpp/dynamic-value/a.h new file mode 100644 index 00000000000000..708cbb79fee5cd --- /dev/null +++ b/lldb/test/API/lang/cpp/dynamic-value/a.h @@ -0,0 +1,25 @@ +#ifndef A_H +#define A_H + +#include <cstdio> +#include <memory> + +class A { +public: + A(int value) : m_a_value(value) {} + A(int value, A *client_A) : m_a_value(value), m_client_A(client_A) {} + + virtual ~A() {} + + virtual void doSomething(A &anotherA); + + int Value() { return m_a_value; } + +private: + int m_a_value; + std::auto_ptr<A> m_client_A; +}; + +A *make_anonymous_B(); + +#endif diff --git a/lldb/test/API/lang/cpp/dynamic-value/anonymous-b.cpp b/lldb/test/API/lang/cpp/dynamic-value/anonymous-b.cpp new file mode 100644 index 00000000000000..755afcbf12a988 --- /dev/null +++ b/lldb/test/API/lang/cpp/dynamic-value/anonymous-b.cpp @@ -0,0 +1,13 @@ +#include "a.h" + +namespace { +class B : public A { +public: + B() : A(42) {} + +private: + int m_anon_b_value = 47; +}; +} // namespace + +A *make_anonymous_B() { return new B(); } diff --git a/lldb/test/API/lang/cpp/dynamic-value/pass-to-base.cpp b/lldb/test/API/lang/cpp/dynamic-value/pass-to-base.cpp index 2bccf3303823c1..be763390cc6f90 100644 --- a/lldb/test/API/lang/cpp/dynamic-value/pass-to-base.cpp +++ b/lldb/test/API/lang/cpp/dynamic-value/pass-to-base.cpp @@ -1,5 +1,10 @@ -#include <stdio.h> -#include <memory> +#include "a.h" + +void A::doSomething(A &anotherA) { + printf("In A %p doing something with %d.\n", this, m_a_value); + int tmp_value = anotherA.Value(); + printf("Also have another A at %p: %d.\n", &anotherA, tmp_value); // Break here in doSomething. +} class Extra { @@ -11,33 +16,6 @@ class Extra int m_extra_two; }; -class A -{ -public: - A(int value) : m_a_value (value) {} - A(int value, A* client_A) : m_a_value (value), m_client_A (client_A) {} - - virtual ~A() {} - - virtual void - doSomething (A &anotherA) - { - printf ("In A %p doing something with %d.\n", this, m_a_value); - int tmp_value = anotherA.Value(); - printf ("Also have another A at %p: %d.\n", &anotherA, tmp_value); // Break here in doSomething. - } - - int - Value() - { - return m_a_value; - } - -private: - int m_a_value; - std::auto_ptr<A> m_client_A; -}; - class B : public Extra, public virtual A { public: @@ -65,5 +43,7 @@ main (int argc, char **argv) A reallyA (500); myB.doSomething (reallyA); // Break here and get real address of reallyA. + myB.doSomething(*make_anonymous_B()); + return 0; } diff --git a/lldb/test/API/lang/cpp/namespace/TestNamespace.py b/lldb/test/API/lang/cpp/namespace/TestNamespace.py index 84891b322180c3..8b013d928f9ca5 100644 --- a/lldb/test/API/lang/cpp/namespace/TestNamespace.py +++ b/lldb/test/API/lang/cpp/namespace/TestNamespace.py @@ -208,6 +208,12 @@ def test_with_run_command(self): patterns=[" = 3"], ) + # Search for a type in an anonymous namespace, both with and without the + # namespace prefix. + self.expect("type lookup -- my_uint_t", substrs=["unsigned int"]) + self.expect("type lookup -- (anonymous namespace)::my_uint_t", + substrs=["unsigned int"]) + # rdar://problem/8660275 # test/namespace: 'expression -- i+j' not working # This has been fixed. diff --git a/lldb/unittests/Symbol/TestType.cpp b/lldb/unittests/Symbol/TestType.cpp index e4b56b9ff02f7c..e3bb2cf6e69e2a 100644 --- a/lldb/unittests/Symbol/TestType.cpp +++ b/lldb/unittests/Symbol/TestType.cpp @@ -16,6 +16,7 @@ using namespace lldb; using namespace lldb_private; +using testing::ElementsAre; using testing::Not; TEST(Type, GetTypeScopeAndBasename) { @@ -59,8 +60,33 @@ MATCHER_P(MatchesIgnoringModules, pattern, "") { TypeQuery query(pattern, TypeQueryOptions::e_ignore_modules); return query.ContextMatches(arg); } +MATCHER_P(MatchesWithStrictNamespaces, pattern, "") { + TypeQuery query(pattern, TypeQueryOptions::e_strict_namespaces); + return query.ContextMatches(arg); +} } // namespace +TEST(Type, TypeQueryFlags) { + TypeQuery q("foo", e_none); + auto get = [](const TypeQuery &q) -> std::vector<bool> { + return {q.GetFindOne(), q.GetExactMatch(), q.GetModuleSearch(), + q.GetIgnoreModules(), q.GetStrictNamespaces()}; + }; + EXPECT_THAT(get(q), ElementsAre(false, false, false, false, false)); + + q.SetFindOne(true); + EXPECT_THAT(get(q), ElementsAre(true, false, false, false, false)); + + q.SetIgnoreModules(true); + EXPECT_THAT(get(q), ElementsAre(true, false, false, true, false)); + + q.SetStrictNamespaces(true); + EXPECT_THAT(get(q), ElementsAre(true, false, false, true, true)); + + q.SetIgnoreModules(false); + EXPECT_THAT(get(q), ElementsAre(true, false, false, false, true)); +} + TEST(Type, CompilerContextPattern) { auto make_module = [](llvm::StringRef name) { return CompilerContext(CompilerContextKind::Module, ConstString(name)); @@ -103,6 +129,10 @@ TEST(Type, CompilerContextPattern) { (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_namespace(""), 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"), @@ -111,4 +141,30 @@ TEST(Type, CompilerContextPattern) { Matches(std::vector{make_class("C")})); EXPECT_THAT((std::vector{make_namespace("NS"), make_class("C")}), Not(Matches(std::vector{make_any_type("C")}))); + + EXPECT_THAT((std::vector{make_namespace(""), make_class("C")}), + Matches(std::vector{make_class("C")})); + EXPECT_THAT((std::vector{make_namespace(""), make_class("C")}), + Not(MatchesWithStrictNamespaces(std::vector{make_class("C")}))); + EXPECT_THAT((std::vector{make_namespace(""), make_class("C")}), + Matches(std::vector{make_namespace(""), make_class("C")})); + EXPECT_THAT((std::vector{make_namespace(""), make_class("C")}), + MatchesWithStrictNamespaces( + std::vector{make_namespace(""), make_class("C")})); + EXPECT_THAT((std::vector{make_class("C")}), + Not(Matches(std::vector{make_namespace(""), make_class("C")}))); + EXPECT_THAT((std::vector{make_class("C")}), + Not(MatchesWithStrictNamespaces( + std::vector{make_namespace(""), make_class("C")}))); + EXPECT_THAT((std::vector{make_namespace(""), make_namespace("NS"), + make_namespace(""), make_class("C")}), + Matches(std::vector{make_namespace("NS"), make_class("C")})); + EXPECT_THAT( + (std::vector{make_namespace(""), make_namespace(""), make_namespace("NS"), + make_namespace(""), make_namespace(""), make_class("C")}), + Matches(std::vector{make_namespace("NS"), make_class("C")})); + EXPECT_THAT((std::vector{make_module("A"), make_namespace("NS"), + make_namespace(""), make_class("C")}), + MatchesIgnoringModules( + std::vector{make_namespace("NS"), make_class("C")})); } diff --git a/lldb/unittests/SymbolFile/DWARF/DWARFDIETest.cpp b/lldb/unittests/SymbolFile/DWARF/DWARFDIETest.cpp index 122b7de7516b6d..1e4c8f3ba07787 100644 --- a/lldb/unittests/SymbolFile/DWARF/DWARFDIETest.cpp +++ b/lldb/unittests/SymbolFile/DWARF/DWARFDIETest.cpp @@ -222,6 +222,9 @@ TEST(DWARFDIETest, GetContext) { Attributes: - Attribute: DW_AT_name Form: DW_FORM_string + - Code: 0x4 + Tag: DW_TAG_namespace + Children: DW_CHILDREN_yes debug_info: - Version: 4 AddrSize: 8 @@ -235,6 +238,11 @@ TEST(DWARFDIETest, GetContext) { - AbbrCode: 0x3 Values: - CStr: STRUCT + - AbbrCode: 0x4 + - AbbrCode: 0x3 + Values: + - CStr: STRUCT + - AbbrCode: 0x0 - AbbrCode: 0x0 - AbbrCode: 0x0 )"; @@ -245,15 +253,17 @@ TEST(DWARFDIETest, GetContext) { DWARFUnit *unit = symbol_file->DebugInfo().GetUnitAtIndex(0); ASSERT_TRUE(unit); - auto make_namespace = [](llvm::StringRef name) { + auto make_namespace = [](const char *name) { return CompilerContext(CompilerContextKind::Namespace, ConstString(name)); }; - auto make_struct = [](llvm::StringRef name) { + auto make_struct = [](const char *name) { return CompilerContext(CompilerContextKind::ClassOrStruct, ConstString(name)); }; DWARFDIE struct_die = unit->DIE().GetFirstChild().GetFirstChild(); ASSERT_TRUE(struct_die); + DWARFDIE anon_struct_die = struct_die.GetSibling().GetFirstChild(); + ASSERT_TRUE(anon_struct_die); EXPECT_THAT( struct_die.GetDeclContext(), testing::ElementsAre(make_namespace("NAMESPACE"), make_struct("STRUCT"))); @@ -263,6 +273,18 @@ TEST(DWARFDIETest, GetContext) { EXPECT_THAT(struct_die.GetDWARFDeclContext(), DWARFDeclContext({{DW_TAG_structure_type, "STRUCT"}, {DW_TAG_namespace, "NAMESPACE"}})); + EXPECT_THAT(anon_struct_die.GetDeclContext(), + testing::ElementsAre(make_namespace("NAMESPACE"), + make_namespace(nullptr), + make_struct("STRUCT"))); + EXPECT_THAT(anon_struct_die.GetTypeLookupContext(), + testing::ElementsAre(make_namespace("NAMESPACE"), + make_namespace(nullptr), + make_struct("STRUCT"))); + EXPECT_THAT(anon_struct_die.GetDWARFDeclContext(), + DWARFDeclContext({{DW_TAG_structure_type, "STRUCT"}, + {DW_TAG_namespace, nullptr}, + {DW_TAG_namespace, "NAMESPACE"}})); } TEST(DWARFDIETest, GetContextInFunction) { _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
