[lldb] Better matching of types in anonymous namespaces #102111
Conversation
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.
|
@llvm/pr-subscribers-lldb Author: Pavel Labath (labath) ChangesThis 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 ( 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 Full diff: https://github.com/llvm/llvm-project/pull/102111.diff 11 Files Affected:
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) {
|
You can test this locally with the following command:git-clang-format --diff d337f5aa59fecd2413b076ed9573e378c57c1307 77fe1a7bcd61efaf2ff812c89ae20fce25eb77c1 --extensions cpp,h -- lldb/test/API/lang/cpp/dynamic-value/a.h lldb/test/API/lang/cpp/dynamic-value/anonymous-b.cpp 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/pass-to-base.cpp lldb/unittests/Symbol/TestType.cpp lldb/unittests/SymbolFile/DWARF/DWARFDIETest.cppView the diff from clang-format here.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 be763390cc..74bea2214a 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
@@ -3,7 +3,8 @@
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.
+ printf("Also have another A at %p: %d.\n", &anotherA,
+ tmp_value); // Break here in doSomething.
}
class Extra
|
You can test this locally with the following command:darker --check --diff -r d337f5aa59fecd2413b076ed9573e378c57c1307...77fe1a7bcd61efaf2ff812c89ae20fce25eb77c1 lldb/test/API/lang/cpp/dynamic-value/TestDynamicValue.py lldb/test/API/lang/cpp/namespace/TestNamespace.pyView the diff from darker here.--- namespace/TestNamespace.py 2024-08-08 11:41:22.000000 +0000
+++ namespace/TestNamespace.py 2024-08-08 11:45:03.940248 +0000
@@ -209,12 +209,13 @@
)
# 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"])
+ 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.
self.expect_expr("i + j", result_type="int", result_value="7")
|
There was a problem hiding this comment.
This patch relies on "(anonymous namespace)" being removed from the compiler contexts to work. What is the user types any of:
(lldb) type lookup "(anonymous namespace)::A"
(lldb) script lldb.target.FindFirstType("(anonymous namespace)::A");
(lldb) script lldb.target.FindTypes("(anonymous namespace)::A");
Do we correctly set this "(anonymous namespace)" to empty? Or are we expecting these type lookups will always use the looser matching strategy? If not we need to check for both.
We might want to stop passing around std::vector<CompilerContext> and pass around a class that contains the language:
struct CompilerContexts {
lldb::LanguageType language;
std::vector<CompilerContext>;
};
Then we could add a method to check for anonymous namespaces where C++ can check for "(anonymous namespace)".
| // 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; | ||
|
|
There was a problem hiding this comment.
Do we want to only pull out this match if this is a DW_TAG_namespace? Or is it useful to have unnamed structs/unions/classes in the contenxt?
There was a problem hiding this comment.
unnamed structs cannot contain named structs, so the only place these can appear is at the top (bottom?) level. We don't currently have a way to look them up from our APIs taking strings (we'd need something like an (anonymous struct)), but they could be looked up internally by code which constructs CompilerContexts directly. So yeah, I think including them here is the right choice for this function.
| @@ -134,6 +134,20 @@ bool TypeQuery::ContextMatches( | |||
| if (ctx == ctx_end) | |||
| return false; // Pattern too long. | |||
|
|
|||
| if (ctx->kind == CompilerContextKind::Namespace && ctx->name.IsEmpty()) { | |||
There was a problem hiding this comment.
Do we need to check for "(anonymous namespace)" here? Or do we detect this elsewhere and make sure to clear the name? Demangling names will show "(anonymous namespace)"
There was a problem hiding this comment.
The anonymous namespace is detected and nullified on line 815. I'm not aware of such code, but if anyone was constructing these contexts without going through the name parser, then they'd have to make sure they use the expected (empty) encoding for anonymous namespaces. I think that's reasonable, because ensuring this when constructing the names from some kind of a structured source is easy, and noone should be parsing name strings by hand as the process is very nontrivial. They should rather call the function in this file (which will do the nullification for them).
| if (scope_name == "(anonymous namespace)") | ||
| scope_name = ""; |
There was a problem hiding this comment.
So we can catch this here and NULL out the name, but what is the user wants to find something in an anonymous namespace with type lookup "(anonymous namespace)::A". Do we clear the name there as well?
There was a problem hiding this comment.
Yes, that also goes through this piece of code. I'll add a test for that path.
These lookups use the looser matching strategy which means they can to the lookup both with and without the namespace prefix. Matching the exact string in the matching algorighm is not necessary because it gets canonicalized in the parser. I've now added a test for this.
For something like this, I think the bigger question is whether we want our type name syntax to be language specific. If we don't (which is the current state), and we say that we use the same syntax for all language (and the syntax happens to mostly match the c++ language), then I don't think we need this, as we can just canonicalize the type context during parsing (like I did here). OTOH, if we want to have language-specific syntax, then we may need to do something completely different, because it's not even possible to parse the string into the context vector without knowing the language (the type queries don't usually specify a specific language, so we may have to delay the parsing until we know which language we are matching against). |
|
Any more thoughts on this, @clayborg ? |
|
Sorry for the delay, feel free to ping sooner next time! Lets start with this and we can iterate on it if needed. |
|
No worries, I was out a large part of August anyway. |
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 (
::Amatches::(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)::Awould 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.