Skip to content

Commit dd5d730

Browse files
authored
[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.
1 parent da13754 commit dd5d730

File tree

12 files changed

+202
-48
lines changed

12 files changed

+202
-48
lines changed

lldb/include/lldb/Symbol/Type.h

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -77,10 +77,13 @@ FLAGS_ENUM(TypeQueryOptions){
7777
/// If set, the query will ignore all Module entries in the type context,
7878
/// even for exact matches.
7979
e_ignore_modules = (1u << 2),
80+
/// If set, all anonymous namespaces in the context must be matched exactly
81+
/// by the pattern. Otherwise, superfluous namespaces are skipped.
82+
e_strict_namespaces = (1u << 3),
8083
/// When true, the find types call should stop the query as soon as a single
8184
/// matching type is found. When false, the type query should find all
8285
/// matching types.
83-
e_find_one = (1u << 3),
86+
e_find_one = (1u << 4),
8487
};
8588
LLDB_MARK_AS_BITMASK_ENUM(TypeQueryOptions)
8689

@@ -264,7 +267,22 @@ class TypeQuery {
264267
bool GetExactMatch() const { return (m_options & e_exact_match) != 0; }
265268

266269
bool GetIgnoreModules() const { return (m_options & e_ignore_modules) != 0; }
267-
void SetIgnoreModules() { m_options &= ~e_ignore_modules; }
270+
void SetIgnoreModules(bool b) {
271+
if (b)
272+
m_options |= e_ignore_modules;
273+
else
274+
m_options &= ~e_ignore_modules;
275+
}
276+
277+
bool GetStrictNamespaces() const {
278+
return (m_options & e_strict_namespaces) != 0;
279+
}
280+
void SetStrictNamespaces(bool b) {
281+
if (b)
282+
m_options |= e_strict_namespaces;
283+
else
284+
m_options &= ~e_strict_namespaces;
285+
}
268286

269287
/// The \a m_context can be used in two ways: normal types searching with
270288
/// the context containing a stanadard declaration context for a type, or
@@ -279,7 +297,7 @@ class TypeQuery {
279297
if (b)
280298
m_options |= e_find_one;
281299
else
282-
m_options &= (e_exact_match | e_find_one);
300+
m_options &= ~e_find_one;
283301
}
284302

285303
/// Access the internal compiler context array.

lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@ TypeAndOrName ItaniumABILanguageRuntime::GetTypeInfo(
9090
TypeResults results;
9191
TypeQuery query(const_lookup_name.GetStringRef(),
9292
TypeQueryOptions::e_exact_match |
93+
TypeQueryOptions::e_strict_namespaces |
9394
TypeQueryOptions::e_find_one);
9495
if (module_sp) {
9596
module_sp->FindTypes(query, results);

lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -440,12 +440,6 @@ static void GetTypeLookupContextImpl(DWARFDIE die,
440440
continue;
441441
}
442442

443-
// If there is no name, then there is no need to look anything up for this
444-
// DIE.
445-
const char *name = die.GetName();
446-
if (!name || !name[0])
447-
return;
448-
449443
// Add this DIE's contribution at the end of the chain.
450444
auto push_ctx = [&](CompilerContextKind kind, llvm::StringRef name) {
451445
context.push_back({kind, ConstString(name)});
@@ -471,7 +465,7 @@ static void GetTypeLookupContextImpl(DWARFDIE die,
471465
push_ctx(CompilerContextKind::Typedef, die.GetName());
472466
break;
473467
case DW_TAG_base_type:
474-
push_ctx(CompilerContextKind::Builtin, name);
468+
push_ctx(CompilerContextKind::Builtin, die.GetName());
475469
break;
476470
// If any of the tags below appear in the parent chain, stop the decl
477471
// context and return. Prior to these being in here, if a type existed in a

lldb/source/Symbol/Type.cpp

Lines changed: 31 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,20 @@ bool TypeQuery::ContextMatches(
134134
if (ctx == ctx_end)
135135
return false; // Pattern too long.
136136

137+
if (ctx->kind == CompilerContextKind::Namespace && ctx->name.IsEmpty()) {
138+
// We're matching an anonymous namespace. These are optional, so we check
139+
// if the pattern expects an anonymous namespace.
140+
if (pat->name.IsEmpty() && (pat->kind & CompilerContextKind::Namespace) ==
141+
CompilerContextKind::Namespace) {
142+
// Match, advance both iterators.
143+
++pat;
144+
}
145+
// Otherwise, only advance the context to skip over the anonymous
146+
// namespace, and try matching again.
147+
++ctx;
148+
continue;
149+
}
150+
137151
// See if there is a kind mismatch; they should have 1 bit in common.
138152
if ((ctx->kind & pat->kind) == CompilerContextKind())
139153
return false;
@@ -145,10 +159,16 @@ bool TypeQuery::ContextMatches(
145159
++pat;
146160
}
147161

148-
// Skip over any remaining module entries if we were asked to do that.
149-
while (GetIgnoreModules() && ctx != ctx_end &&
150-
ctx->kind == CompilerContextKind::Module)
151-
++ctx;
162+
// Skip over any remaining module and anonymous namespace entries if we were
163+
// asked to do that.
164+
auto should_skip = [this](const CompilerContext &ctx) {
165+
if (ctx.kind == CompilerContextKind::Module)
166+
return GetIgnoreModules();
167+
if (ctx.kind == CompilerContextKind::Namespace && ctx.name.IsEmpty())
168+
return !GetStrictNamespaces();
169+
return false;
170+
};
171+
ctx = std::find_if_not(ctx, ctx_end, should_skip);
152172

153173
// At this point, we have exhausted the pattern and we have a partial match at
154174
// least. If that's all we're looking for, we're done.
@@ -788,7 +808,13 @@ Type::GetTypeScopeAndBasename(llvm::StringRef name) {
788808
switch (pos.value()) {
789809
case ':':
790810
if (prev_is_colon && template_depth == 0) {
791-
result.scope.push_back(name.slice(name_begin, pos.index() - 1));
811+
llvm::StringRef scope_name = name.slice(name_begin, pos.index() - 1);
812+
// The itanium demangler uses this string to represent anonymous
813+
// namespaces. Convert it to a more language-agnostic form (which is
814+
// also used in DWARF).
815+
if (scope_name == "(anonymous namespace)")
816+
scope_name = "";
817+
result.scope.push_back(scope_name);
792818
name_begin = pos.index() + 1;
793819
}
794820
break;
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
1-
CXX_SOURCES := pass-to-base.cpp
1+
CXX_SOURCES := pass-to-base.cpp anonymous-b.cpp
22

33
include Makefile.rules

lldb/test/API/lang/cpp/dynamic-value/TestDynamicValue.py

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,7 @@ def test_get_dynamic_vals(self):
170170
self.assertTrue(reallyA_value)
171171
reallyA_loc = int(reallyA_value.GetLocation(), 16)
172172

173-
# Finally continue to doSomething again, and make sure we get the right value for anotherA,
173+
# Continue to doSomething again, and make sure we get the right value for anotherA,
174174
# which this time around is just an "A".
175175

176176
threads = lldbutil.continue_to_breakpoint(process, do_something_bpt)
@@ -184,6 +184,19 @@ def test_get_dynamic_vals(self):
184184
self.assertEqual(anotherA_loc, reallyA_loc)
185185
self.assertEqual(anotherA_value.GetTypeName().find("B"), -1)
186186

187+
# Finally do the same with a B in an anonymous namespace.
188+
threads = lldbutil.continue_to_breakpoint(process, do_something_bpt)
189+
self.assertEqual(len(threads), 1)
190+
thread = threads[0]
191+
192+
frame = thread.GetFrameAtIndex(0)
193+
anotherA_value = frame.FindVariable("anotherA", use_dynamic)
194+
self.assertTrue(anotherA_value)
195+
self.assertIn("B", anotherA_value.GetTypeName())
196+
anon_b_value = anotherA_value.GetChildMemberWithName("m_anon_b_value")
197+
self.assertTrue(anon_b_value)
198+
self.assertEqual(anon_b_value.GetValueAsSigned(), 47)
199+
187200
def examine_value_object_of_this_ptr(
188201
self, this_static, this_dynamic, dynamic_location
189202
):
Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
#ifndef A_H
2+
#define A_H
3+
4+
#include <cstdio>
5+
#include <memory>
6+
7+
class A {
8+
public:
9+
A(int value) : m_a_value(value) {}
10+
A(int value, A *client_A) : m_a_value(value), m_client_A(client_A) {}
11+
12+
virtual ~A() {}
13+
14+
virtual void doSomething(A &anotherA);
15+
16+
int Value() { return m_a_value; }
17+
18+
private:
19+
int m_a_value;
20+
std::auto_ptr<A> m_client_A;
21+
};
22+
23+
A *make_anonymous_B();
24+
25+
#endif
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
#include "a.h"
2+
3+
namespace {
4+
class B : public A {
5+
public:
6+
B() : A(42) {}
7+
8+
private:
9+
int m_anon_b_value = 47;
10+
};
11+
} // namespace
12+
13+
A *make_anonymous_B() { return new B(); }
Lines changed: 9 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,10 @@
1-
#include <stdio.h>
2-
#include <memory>
1+
#include "a.h"
2+
3+
void A::doSomething(A &anotherA) {
4+
printf("In A %p doing something with %d.\n", this, m_a_value);
5+
int tmp_value = anotherA.Value();
6+
printf("Also have another A at %p: %d.\n", &anotherA, tmp_value); // Break here in doSomething.
7+
}
38

49
class Extra
510
{
@@ -11,33 +16,6 @@ class Extra
1116
int m_extra_two;
1217
};
1318

14-
class A
15-
{
16-
public:
17-
A(int value) : m_a_value (value) {}
18-
A(int value, A* client_A) : m_a_value (value), m_client_A (client_A) {}
19-
20-
virtual ~A() {}
21-
22-
virtual void
23-
doSomething (A &anotherA)
24-
{
25-
printf ("In A %p doing something with %d.\n", this, m_a_value);
26-
int tmp_value = anotherA.Value();
27-
printf ("Also have another A at %p: %d.\n", &anotherA, tmp_value); // Break here in doSomething.
28-
}
29-
30-
int
31-
Value()
32-
{
33-
return m_a_value;
34-
}
35-
36-
private:
37-
int m_a_value;
38-
std::auto_ptr<A> m_client_A;
39-
};
40-
4119
class B : public Extra, public virtual A
4220
{
4321
public:
@@ -65,5 +43,7 @@ main (int argc, char **argv)
6543
A reallyA (500);
6644
myB.doSomething (reallyA); // Break here and get real address of reallyA.
6745

46+
myB.doSomething(*make_anonymous_B());
47+
6848
return 0;
6949
}

lldb/test/API/lang/cpp/namespace/TestNamespace.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,12 @@ def test_with_run_command(self):
208208
patterns=[" = 3"],
209209
)
210210

211+
# Search for a type in an anonymous namespace, both with and without the
212+
# namespace prefix.
213+
self.expect("type lookup -- my_uint_t", substrs=["unsigned int"])
214+
self.expect("type lookup -- (anonymous namespace)::my_uint_t",
215+
substrs=["unsigned int"])
216+
211217
# rdar://problem/8660275
212218
# test/namespace: 'expression -- i+j' not working
213219
# This has been fixed.

lldb/unittests/Symbol/TestType.cpp

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
using namespace lldb;
1818
using namespace lldb_private;
19+
using testing::ElementsAre;
1920
using testing::Not;
2021

2122
TEST(Type, GetTypeScopeAndBasename) {
@@ -59,8 +60,33 @@ MATCHER_P(MatchesIgnoringModules, pattern, "") {
5960
TypeQuery query(pattern, TypeQueryOptions::e_ignore_modules);
6061
return query.ContextMatches(arg);
6162
}
63+
MATCHER_P(MatchesWithStrictNamespaces, pattern, "") {
64+
TypeQuery query(pattern, TypeQueryOptions::e_strict_namespaces);
65+
return query.ContextMatches(arg);
66+
}
6267
} // namespace
6368

69+
TEST(Type, TypeQueryFlags) {
70+
TypeQuery q("foo", e_none);
71+
auto get = [](const TypeQuery &q) -> std::vector<bool> {
72+
return {q.GetFindOne(), q.GetExactMatch(), q.GetModuleSearch(),
73+
q.GetIgnoreModules(), q.GetStrictNamespaces()};
74+
};
75+
EXPECT_THAT(get(q), ElementsAre(false, false, false, false, false));
76+
77+
q.SetFindOne(true);
78+
EXPECT_THAT(get(q), ElementsAre(true, false, false, false, false));
79+
80+
q.SetIgnoreModules(true);
81+
EXPECT_THAT(get(q), ElementsAre(true, false, false, true, false));
82+
83+
q.SetStrictNamespaces(true);
84+
EXPECT_THAT(get(q), ElementsAre(true, false, false, true, true));
85+
86+
q.SetIgnoreModules(false);
87+
EXPECT_THAT(get(q), ElementsAre(true, false, false, false, true));
88+
}
89+
6490
TEST(Type, CompilerContextPattern) {
6591
auto make_module = [](llvm::StringRef name) {
6692
return CompilerContext(CompilerContextKind::Module, ConstString(name));
@@ -103,6 +129,10 @@ TEST(Type, CompilerContextPattern) {
103129
(std::vector{make_module("A"), make_module("B"), make_class("C")}),
104130
Matches(
105131
std::vector{make_module("A"), make_module("B"), make_any_type("C")}));
132+
EXPECT_THAT((std::vector{make_module("A"), make_module("B"),
133+
make_namespace(""), make_class("C")}),
134+
Matches(std::vector{make_module("A"), make_module("B"),
135+
make_any_type("C")}));
106136
EXPECT_THAT(
107137
(std::vector{make_module("A"), make_module("B"), make_enum("C2")}),
108138
Not(Matches(std::vector{make_module("A"), make_module("B"),
@@ -111,4 +141,30 @@ TEST(Type, CompilerContextPattern) {
111141
Matches(std::vector{make_class("C")}));
112142
EXPECT_THAT((std::vector{make_namespace("NS"), make_class("C")}),
113143
Not(Matches(std::vector{make_any_type("C")})));
144+
145+
EXPECT_THAT((std::vector{make_namespace(""), make_class("C")}),
146+
Matches(std::vector{make_class("C")}));
147+
EXPECT_THAT((std::vector{make_namespace(""), make_class("C")}),
148+
Not(MatchesWithStrictNamespaces(std::vector{make_class("C")})));
149+
EXPECT_THAT((std::vector{make_namespace(""), make_class("C")}),
150+
Matches(std::vector{make_namespace(""), make_class("C")}));
151+
EXPECT_THAT((std::vector{make_namespace(""), make_class("C")}),
152+
MatchesWithStrictNamespaces(
153+
std::vector{make_namespace(""), make_class("C")}));
154+
EXPECT_THAT((std::vector{make_class("C")}),
155+
Not(Matches(std::vector{make_namespace(""), make_class("C")})));
156+
EXPECT_THAT((std::vector{make_class("C")}),
157+
Not(MatchesWithStrictNamespaces(
158+
std::vector{make_namespace(""), make_class("C")})));
159+
EXPECT_THAT((std::vector{make_namespace(""), make_namespace("NS"),
160+
make_namespace(""), make_class("C")}),
161+
Matches(std::vector{make_namespace("NS"), make_class("C")}));
162+
EXPECT_THAT(
163+
(std::vector{make_namespace(""), make_namespace(""), make_namespace("NS"),
164+
make_namespace(""), make_namespace(""), make_class("C")}),
165+
Matches(std::vector{make_namespace("NS"), make_class("C")}));
166+
EXPECT_THAT((std::vector{make_module("A"), make_namespace("NS"),
167+
make_namespace(""), make_class("C")}),
168+
MatchesIgnoringModules(
169+
std::vector{make_namespace("NS"), make_class("C")}));
114170
}

0 commit comments

Comments
 (0)