[Lldb-commits] [lldb] [lldb] Better matching of types in anonymous namespaces (PR #102111)
Pavel Labath via lldb-commits
lldb-commits at lists.llvm.org
Tue Aug 6 01:50:34 PDT 2024
https://github.com/labath updated https://github.com/llvm/llvm-project/pull/102111
>From c6936f3d5d0592babe9082e867b179af594c447b Mon Sep 17 00:00:00 2001
From: Pavel Labath <pavel at labath.sk>
Date: Tue, 6 Aug 2024 09:51:20 +0200
Subject: [PATCH 1/2] [lldb] Better matching of types in anonymous namespaces
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.
---
lldb/include/lldb/Symbol/Type.h | 10 ++++-
.../ItaniumABI/ItaniumABILanguageRuntime.cpp | 1 +
.../Plugins/SymbolFile/DWARF/DWARFDIE.cpp | 8 +---
lldb/source/Symbol/Type.cpp | 33 +++++++++++++---
lldb/test/API/lang/cpp/dynamic-value/Makefile | 2 +-
.../cpp/dynamic-value/TestDynamicValue.py | 17 ++++++++-
lldb/test/API/lang/cpp/dynamic-value/a.h | 25 ++++++++++++
.../lang/cpp/dynamic-value/anonymous-b.cpp | 15 ++++++++
.../lang/cpp/dynamic-value/pass-to-base.cpp | 38 +++++--------------
lldb/unittests/Symbol/TestType.cpp | 22 +++++++++++
.../SymbolFile/DWARF/DWARFDIETest.cpp | 24 +++++++++++-
11 files changed, 149 insertions(+), 46 deletions(-)
create mode 100644 lldb/test/API/lang/cpp/dynamic-value/a.h
create mode 100644 lldb/test/API/lang/cpp/dynamic-value/anonymous-b.cpp
diff --git a/lldb/include/lldb/Symbol/Type.h b/lldb/include/lldb/Symbol/Type.h
index 420307e0dbcf0..021e27b52fca7 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)
@@ -266,6 +269,11 @@ class TypeQuery {
bool GetIgnoreModules() const { return (m_options & e_ignore_modules) != 0; }
void SetIgnoreModules() { m_options &= ~e_ignore_modules; }
+ bool GetStrictNamespaces() const {
+ return (m_options & e_strict_namespaces) != 0;
+ }
+ void SetStrictNamespaces() { 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
/// with the context being more complete for exact matches in clang modules.
diff --git a/lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp b/lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
index 7af768aad0bc1..4c547afe30fe8 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 fb32e2adeb3fe..0a13c457a307a 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 eb321407e3734..ce6099acd55f8 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,10 @@ 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);
+ 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 2bba8e757f79b..ce91dc63f473f 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 60a2590e1559d..fda353df2cdfb 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,21 @@ 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 0000000000000..708cbb79fee5c
--- /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 0000000000000..3bfd4f4b96d98
--- /dev/null
+++ b/lldb/test/API/lang/cpp/dynamic-value/anonymous-b.cpp
@@ -0,0 +1,15 @@
+#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 2bccf3303823c..be763390cc6f9 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/unittests/Symbol/TestType.cpp b/lldb/unittests/Symbol/TestType.cpp
index e4b56b9ff02f7..f4c5f1e5964da 100644
--- a/lldb/unittests/Symbol/TestType.cpp
+++ b/lldb/unittests/Symbol/TestType.cpp
@@ -59,6 +59,10 @@ 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, CompilerContextPattern) {
@@ -111,4 +115,22 @@ 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")}));
}
diff --git a/lldb/unittests/SymbolFile/DWARF/DWARFDIETest.cpp b/lldb/unittests/SymbolFile/DWARF/DWARFDIETest.cpp
index 122b7de7516b6..07e6cf78b7adf 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,16 @@ 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) {
>From 800ef4b76d35ac60b34ada066b79f66818f3475c Mon Sep 17 00:00:00 2001
From: Pavel Labath <pavel at labath.sk>
Date: Tue, 6 Aug 2024 10:48:37 +0200
Subject: [PATCH 2/2] reformat
---
lldb/test/API/lang/cpp/dynamic-value/TestDynamicValue.py | 2 --
lldb/test/API/lang/cpp/dynamic-value/anonymous-b.cpp | 4 +---
lldb/unittests/SymbolFile/DWARF/DWARFDIETest.cpp | 6 ++++--
3 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/lldb/test/API/lang/cpp/dynamic-value/TestDynamicValue.py b/lldb/test/API/lang/cpp/dynamic-value/TestDynamicValue.py
index fda353df2cdfb..e016168f047c1 100644
--- a/lldb/test/API/lang/cpp/dynamic-value/TestDynamicValue.py
+++ b/lldb/test/API/lang/cpp/dynamic-value/TestDynamicValue.py
@@ -184,7 +184,6 @@ 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)
@@ -198,7 +197,6 @@ def test_get_dynamic_vals(self):
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/anonymous-b.cpp b/lldb/test/API/lang/cpp/dynamic-value/anonymous-b.cpp
index 3bfd4f4b96d98..755afcbf12a98 100644
--- a/lldb/test/API/lang/cpp/dynamic-value/anonymous-b.cpp
+++ b/lldb/test/API/lang/cpp/dynamic-value/anonymous-b.cpp
@@ -10,6 +10,4 @@ class B : public A {
};
} // namespace
-A *make_anonymous_B() {
- return new B();
-}
+A *make_anonymous_B() { return new B(); }
diff --git a/lldb/unittests/SymbolFile/DWARF/DWARFDIETest.cpp b/lldb/unittests/SymbolFile/DWARF/DWARFDIETest.cpp
index 07e6cf78b7adf..1e4c8f3ba0778 100644
--- a/lldb/unittests/SymbolFile/DWARF/DWARFDIETest.cpp
+++ b/lldb/unittests/SymbolFile/DWARF/DWARFDIETest.cpp
@@ -275,10 +275,12 @@ TEST(DWARFDIETest, GetContext) {
{DW_TAG_namespace, "NAMESPACE"}}));
EXPECT_THAT(anon_struct_die.GetDeclContext(),
testing::ElementsAre(make_namespace("NAMESPACE"),
- make_namespace(nullptr), make_struct("STRUCT")));
+ make_namespace(nullptr),
+ make_struct("STRUCT")));
EXPECT_THAT(anon_struct_die.GetTypeLookupContext(),
testing::ElementsAre(make_namespace("NAMESPACE"),
- make_namespace(nullptr), make_struct("STRUCT")));
+ make_namespace(nullptr),
+ make_struct("STRUCT")));
EXPECT_THAT(anon_struct_die.GetDWARFDeclContext(),
DWARFDeclContext({{DW_TAG_structure_type, "STRUCT"},
{DW_TAG_namespace, nullptr},
More information about the lldb-commits
mailing list