Skip to content

[LLDB][NativePDB] Allow type lookup in namespaces #149876

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Michael137 merged 7 commits into from
Aug 4, 2025
Merged

[LLDB][NativePDB] Allow type lookup in namespaces #149876

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fa3c96b
[LLDB][NativePDB] Allow type lookup in namespaces
Nerixyz Jul 21, 2025
288b5f7
refactor: move basename discovery to `BuildParentMap`
Nerixyz Jul 22, 2025
1dbde10
refactor: convert test to use split-file
Nerixyz Jul 28, 2025
8344a98
refactor: create context in symbol file plugin
Nerixyz Jul 28, 2025
9694e94
fix: add braces to `if`
Nerixyz Jul 29, 2025
fb5bce0
fix: comment and move up sorting of map
Nerixyz Jul 29, 2025
85c2c42
fix: assert kind is union
Nerixyz Jul 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 54 additions & 4 deletions lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1630,6 +1630,53 @@ size_t SymbolFileNativePDB::ParseSymbolArrayInScope(
return count;
}

void SymbolFileNativePDB::CacheTypeNames() {
Copy link
Contributor

@ZequanWu ZequanWu Jul 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, SymbolFileNativePDB::BuildParentMap() already does the tpi stream iteration and it's called at NativePDB plugin initial setup. We could just cache the those base names there instead of iterating it the second time.

if (!m_type_base_names.IsEmpty())
return;

LazyRandomTypeCollection &types = m_index->tpi().typeCollection();
for (auto ti = types.getFirst(); ti; ti = types.getNext(*ti)) {
CVType cvt = types.getType(*ti);
llvm::StringRef name;
// We are only interested in records, unions, and enums.
// We aren't interested in forward references as we'll visit the actual
// type later anyway.
switch (cvt.kind()) {
case LF_STRUCTURE:
case LF_CLASS: {
ClassRecord cr;
llvm::cantFail(TypeDeserializer::deserializeAs<ClassRecord>(cvt, cr));
if (cr.isForwardRef())
continue;
name = cr.Name;
} break;
case LF_UNION: {
UnionRecord ur;
llvm::cantFail(TypeDeserializer::deserializeAs<UnionRecord>(cvt, ur));
if (ur.isForwardRef())
continue;
name = ur.Name;
} break;
case LF_ENUM: {
EnumRecord er;
llvm::cantFail(TypeDeserializer::deserializeAs<EnumRecord>(cvt, er));
if (er.isForwardRef())
continue;
name = er.Name;
} break;
default:
continue;
}
if (name.empty())
continue;

auto base_name = MSVCUndecoratedNameParser::DropScope(name);
m_type_base_names.Append(ConstString(base_name), ti->getIndex());
}

m_type_base_names.Sort();
}

void SymbolFileNativePDB::DumpClangAST(Stream &s, llvm::StringRef filter) {
auto ts_or_err = GetTypeSystemForLanguage(eLanguageTypeC_plus_plus);
if (!ts_or_err)
Expand Down Expand Up @@ -1720,11 +1767,14 @@ void SymbolFileNativePDB::FindTypes(const lldb_private::TypeQuery &query,

std::lock_guard<std::recursive_mutex> guard(GetModuleMutex());

std::vector<TypeIndex> matches =
m_index->tpi().findRecordsByName(query.GetTypeBasename().GetStringRef());
// We can't query for the basename or full name because the type might reside
// in an anonymous namespace. Cache the basenames first.
CacheTypeNames();
std::vector<uint32_t> matches;
m_type_base_names.GetValues(query.GetTypeBasename(), matches);

for (TypeIndex type_idx : matches) {
TypeSP type_sp = GetOrCreateType(type_idx);
for (uint32_t match_idx : matches) {
TypeSP type_sp = GetOrCreateType(TypeIndex(match_idx));
if (!type_sp)
continue;

Expand Down
4 changes: 4 additions & 0 deletions lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,8 @@ class SymbolFileNativePDB : public SymbolFileCommon {

void ParseInlineSite(PdbCompilandSymId inline_site_id, Address func_addr);

void CacheTypeNames();

llvm::BumpPtrAllocator m_allocator;

lldb::addr_t m_obj_load_address = 0;
Expand All @@ -278,6 +280,8 @@ class SymbolFileNativePDB : public SymbolFileCommon {
llvm::DenseMap<lldb::user_id_t, std::shared_ptr<InlineSite>> m_inline_sites;
llvm::DenseMap<llvm::codeview::TypeIndex, llvm::codeview::TypeIndex>
m_parent_types;

lldb_private::UniqueCStringMap<uint32_t> m_type_base_names;
};

} // namespace npdb
Expand Down
8 changes: 6 additions & 2 deletions lldb/source/Symbol/Type.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,9 @@ bool TypeQuery::ContextMatches(
if (ctx == ctx_end)
return false; // Pattern too long.

if (ctx->kind == CompilerContextKind::Namespace && ctx->name.IsEmpty()) {
if ((ctx->kind & CompilerContextKind::Namespace) ==
Copy link
Member

@Michael137 Michael137 Jul 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@labath since you looked at this some time ago, do you see a concern with this? I guess the alternative is to make sure we don't construct the TypeQuery with a AnyDeclContext in PDB-land. @Nerixyz why can't we do that? Is there some PDB limitation?

Copy link
Contributor Author

@Nerixyz Nerixyz Jul 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why can't we do that? Is there some PDB limitation?

We could do that. It would probably replicate the TypeQuery constructor, though (which arguably isn't that large).

Copy link
Collaborator

@labath labath Jul 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, so the current assumption is that when matching types, the pattern can contain wildcards (you can say "i'm looking for a type or a namespace at a given position"), but the thing you're matching is always precise (it will say "i'm a type" or "I'm a namespace", but it can't say "I don't know"). For DWARF this isn't an issue because we always have that information. I don't know if that's the case for PDB, or if it's just a side-effect of how it performs the query (*). If it's not possible to do it the other way, then we may have to change the way we do matching, but I think it'd be better to not do that.

(*) Currently, the PDB code kind of cheats by taking the type name and parsing it as a type query (SymbolFileNativePDB.cpp:1736 in this patch), but then using the result of that parsing as the thing to match. This is bad for two reasons:

  • if you're parsing from a name, it's clear that you won't be able to see whether enclosing scopes are a type or a namespace. The information just isn't there --Foo::Bar looks the same regardless of whether Foo is a namespace or not.
  • It's going to be slow because this approach requires you to fully materialize the type (and everything it depends on) before knowing if this is even the type you're looking for. This may not be as big of an issue for PDB as it is for DWARF because (so I hear) in doesn't have template information (which is the big source of fanout in this process) and it may have less of these searches internally (because it's already linked, in DWARF we need to do this just to search for a definition DIE), but it still means that a search for foo::iterator will materialize every iterator out there.

For this reason it would be better type context was created differently, without relying on a Type object. For DWARF this happens in DWARFDIE::GetTypeLookupContext, but I don't know if something like that works in PDB. The patch description seems to indicate that this information does not exist, but I don't understand how anything could work in that case. E.g. I don't know what kind of Clang AST would we created for these types. Are they properly nested in their expected namespace? If so how do you figure out that namespace?

Copy link
Contributor Author

@Nerixyz Nerixyz Jul 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. I'll create the context in the PDB plugin. To avoid materializing all types that match the basename, I can see two approaches:

  1. Naively use the type's name (PDB stores the demangled name there). This would essentially do the same as the TypeQuery constructor where it tries to separate the scopes. Every scope/context but the last one would be a namespace. For the last one, we can find the exact type.
  2. PDB also contains a UniqueName field which can contain a mangled type name. Similar to CreateDeclInfoForType, the name could be demangled and each scope could be checked.

In both cases, it's possible to try and get the parent to handle nested structs/enums/unions like in CreateDeclInfoForType.

Copy link
Collaborator

@labath labath Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know which approach is better, but I like the direction of this. I'll just say that if you find yourself needing to parse a type name, don't reimplement that code. I'd rather take the parsing code out of the TypeQuery constructor, and factor it into function that can be called independently.

How expensive is this "get the parent to handle nested structs/enums/unions" operation? I'm asking because I'm wondering if we can just do it all the time, or if we should try to avoid it. If it's cheap, we can just construct the precise context (one where every scope knows whether it's a type or a namespace) every time. The advantage of that is that we can reuse the existing infrastructure and the overall flow will be very similar to how things work in DWARF. However, if it's expensive, then we may want to do something like you've done here, where we first do a "fuzzy" match (just checking whether the names match, not looking at the kinds), and then we confirm the match (if needed) by looking at the kinds.

Copy link
Contributor Author

@Nerixyz Nerixyz Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How expensive is this "get the parent to handle nested structs/enums/unions" operation?

This is only a lookup in a map that we created. But this only works for tag types, not for namespaces. My approach now uses Type::GetTypeScopeAndBasename to split the undecorated name. At first, it assumes everything is a namespace and then walks backwards to find parent types.

I'm not sure what the best way to test this is.

Copy link
Collaborator

@labath labath Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can identify tag types, then I think it's safe to assume that the rest are namespaces (particularly if that's the same approach we use when constructing the clang AST for these types).

Your test looks reasonable to me. I'd consider using lldb-test (like lldb/test/Shell/SymbolFile/DWARF/x86/find-basic-type.cpp does) instead, mainly because it lets you distinguish multiple instances of S better, but I wouldn't say that's required.

Copy link
Contributor Author

@Nerixyz Nerixyz Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd consider using lldb-test (like lldb/test/Shell/SymbolFile/DWARF/x86/find-basic-type.cpp does) instead, mainly because it lets you distinguish multiple instances of S better

Looks like this doesn't work with the native PDB plugin yet because it's missing the decl:

> lldb-test symbols --name=::S --find=type main.exe
Module: main.exe
Found 1 types:
000001F4D1CB2DC0: Type{0x00024884} , name = "S", size = 1, compiler_type = 0x000001f4d4283d00 struct S {
    char a[1];
}

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) ==
Expand Down Expand Up @@ -164,7 +166,9 @@ bool TypeQuery::ContextMatches(
auto should_skip = [this](const CompilerContext &ctx) {
if (ctx.kind == CompilerContextKind::Module)
return GetIgnoreModules();
if (ctx.kind == CompilerContextKind::Namespace && ctx.name.IsEmpty())
if ((ctx.kind & CompilerContextKind::Namespace) ==
CompilerContextKind::Namespace &&
ctx.name.IsEmpty())
return !GetStrictNamespaces();
return false;
};
Expand Down
18 changes: 18 additions & 0 deletions lldb/test/Shell/SymbolFile/NativePDB/Inputs/namespace-access.lldbinit
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
b main
r

type lookup S
type lookup ::S
type lookup Outer::S
type lookup Outer::Inner1::S
type lookup Inner1::S
type lookup Outer::Inner1::Inner2::S
type lookup Inner2::S
type lookup Outer::Inner2::S
type lookup Outer::A
type lookup A
type lookup ::A
expr sizeof(S)
expr sizeof(A)

quit
Copy link
Member

@Michael137 Michael137 Jul 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets put these commands into the test file itself. You can use split-file for this. For example:

llvm-project/lldb/test/Shell/Settings/TestFrameFormatFunctionScopeObjC.test

Line 4 in 81651e9

# RUN: split-file %s %t

114 changes: 114 additions & 0 deletions lldb/test/Shell/SymbolFile/NativePDB/namespace-access.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
// clang-format off
// REQUIRES: target-windows

// Test namespace lookup.
// RUN: %build --nodefaultlib -o %t.exe -- %s
// RUN: %lldb -f %t.exe -s \
// RUN: %p/Inputs/namespace-access.lldbinit 2>&1 | FileCheck %s

struct S {
char a[1];
};

namespace Outer {

struct S {
char a[2];
};

namespace Inner1 {
struct S {
char a[3];
};

namespace Inner2 {
struct S {
char a[4];
};
} // namespace Inner2
} // namespace Inner1

namespace Inner2 {
struct S {
char a[5];
};
} // namespace Inner2

namespace {
struct A {
char a[6];
};
} // namespace

} // namespace Outer

namespace {
struct A {
char a[7];
};
} // namespace

int main(int argc, char **argv) {
S s;
Outer::S os;
Outer::Inner1::S oi1s;
Outer::Inner1::Inner2::S oi1i2s;
Outer::Inner2::S oi2s;
A a1;
Outer::A a2;
return sizeof(s) + sizeof(os) + sizeof(oi1s) + sizeof(oi1i2s) + sizeof(oi2s) + sizeof(a1) + sizeof(a2);
}



// CHECK: (lldb) type lookup S
// CHECK: struct S {
// CHECK: struct S {
// CHECK: struct S {
// CHECK: struct S {
// CHECK: struct S {
// CHECK: }
// CHECK-NEXT: (lldb) type lookup ::S
// CHECK-NEXT: struct S {
// CHECK-NEXT: char a[1];
// CHECK-NEXT: }
// CHECK-NEXT: (lldb) type lookup Outer::S
// CHECK-NEXT: struct S {
// CHECK-NEXT: char a[2];
// CHECK-NEXT: }
// CHECK-NEXT: (lldb) type lookup Outer::Inner1::S
// CHECK-NEXT: struct S {
// CHECK-NEXT: char a[3];
// CHECK-NEXT: }
// CHECK-NEXT: (lldb) type lookup Inner1::S
// CHECK-NEXT: struct S {
// CHECK-NEXT: char a[3];
// CHECK-NEXT: }
// CHECK-NEXT: (lldb) type lookup Outer::Inner1::Inner2::S
// CHECK-NEXT: struct S {
// CHECK-NEXT: char a[4];
// CHECK-NEXT: }
// CHECK-NEXT: (lldb) type lookup Inner2::S
// CHECK-NEXT: struct S {
// CHECK: struct S {
// CHECK: }
// CHECK-NEXT: (lldb) type lookup Outer::Inner2::S
// CHECK-NEXT: struct S {
// CHECK-NEXT: char a[5];
// CHECK-NEXT: }
// CHECK-NEXT: (lldb) type lookup Outer::A
// CHECK-NEXT: struct A {
// CHECK-NEXT: char a[6];
// CHECK-NEXT: }
// CHECK-NEXT: (lldb) type lookup A
// CHECK-NEXT: struct A {
// CHECK: struct A {
// CHECK: }
// CHECK-NEXT: (lldb) type lookup ::A
// CHECK-NEXT: struct A {
// CHECK-NEXT: char a[7];
// CHECK-NEXT: }
// CHECK-NEXT: (lldb) expr sizeof(S)
// CHECK-NEXT: (__size_t) $0 = 1
// CHECK-NEXT: (lldb) expr sizeof(A)
// CHECK-NEXT: (__size_t) $1 = 7
Loading