[LLDB][NativePDB] Use typedef compiler type for typedef types #156250
Conversation
|
@llvm/pr-subscribers-lldb Author: nerix (Nerixyz) ChangesBefore this PR, the native PDB plugin would create the following LLDB with this PR, the following is created: This matches the behavior of the DIA PDB plugin and works towards making I added a similar test to the Full diff: https://github.com/llvm/llvm-project/pull/156250.diff 2 Files Affected:
diff --git a/lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp b/lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
index 112eb06e462fc..9f20746e095e6 100644
--- a/lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ b/lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -2045,14 +2045,17 @@ TypeSP SymbolFileNativePDB::CreateTypedef(PdbGlobalSymId id) {
if (!ts)
return nullptr;
- ts->GetNativePDBParser()->GetOrCreateTypedefDecl(id);
+ auto *typedef_decl = ts->GetNativePDBParser()->GetOrCreateTypedefDecl(id);
+
+ CompilerType ct = target_type->GetForwardCompilerType();
+ if (auto *clang = llvm::dyn_cast_or_null<TypeSystemClang>(ts.get()))
+ ct = clang->GetType(clang->getASTContext().getTypeDeclType(typedef_decl));
Declaration decl;
return MakeType(toOpaqueUid(id), ConstString(udt.Name),
llvm::expectedToOptional(target_type->GetByteSize(nullptr)),
nullptr, target_type->GetID(),
- lldb_private::Type::eEncodingIsTypedefUID, decl,
- target_type->GetForwardCompilerType(),
+ lldb_private::Type::eEncodingIsTypedefUID, decl, ct,
lldb_private::Type::ResolveState::Forward);
}
diff --git a/lldb/test/Shell/SymbolFile/NativePDB/simple-types.cpp b/lldb/test/Shell/SymbolFile/NativePDB/simple-types.cpp
new file mode 100644
index 0000000000000..6c0b68d86a09a
--- /dev/null
+++ b/lldb/test/Shell/SymbolFile/NativePDB/simple-types.cpp
@@ -0,0 +1,128 @@
+// REQUIRES: lld
+
+// Test that simple types can be found
+// RUN: %build --std=c++20 --nodefaultlib --arch=64 -o %t.exe -- %s
+// RUN: lldb-test symbols %t.exe | FileCheck %s
+
+bool *PB;
+bool &RB = *PB;
+bool *&RPB = PB;
+const bool &CRB = RB;
+bool *const BC = 0;
+const bool *const CBC = 0;
+
+long AL[2];
+
+const volatile short CVS = 0;
+const short CS = 0;
+volatile short VS;
+
+struct ReturnedStruct1 {};
+struct ReturnedStruct2 {};
+
+struct MyStruct {
+ static ReturnedStruct1 static_fn(char *) { return {}; }
+ ReturnedStruct2 const_member_fn(char *) const { return {}; }
+ void volatile_member_fn() volatile {};
+ void member_fn() {};
+};
+
+void (*PF)(int, bool *, const float, ...);
+
+using Func = void(char16_t, MyStruct &);
+Func *PF2;
+
+using SomeTypedef = long;
+SomeTypedef ST;
+
+int main() {
+ bool b;
+ char c;
+ unsigned char uc;
+ char8_t c8;
+
+ short s;
+ unsigned short us;
+ wchar_t wc;
+ char16_t c16;
+
+ int i;
+ unsigned int ui;
+ long l;
+ unsigned long ul;
+ char32_t c32;
+
+ long long ll;
+ unsigned long long ull;
+
+ float f;
+ double d;
+
+ MyStruct my_struct;
+
+ decltype(nullptr) np;
+}
+
+// CHECK-DAG: Type{{.*}} , name = "std::nullptr_t", size = 0, compiler_type = 0x{{[0-9a-f]+}} nullptr_t
+
+// CHECK-DAG: Type{{.*}} , name = "bool", size = 1, compiler_type = 0x{{[0-9a-f]+}} _Bool
+// CHECK-DAG: Type{{.*}} , name = "char", size = 1, compiler_type = 0x{{[0-9a-f]+}} char
+// CHECK-DAG: Type{{.*}} , name = "unsigned char", size = 1, compiler_type = 0x{{[0-9a-f]+}} unsigned char
+// CHECK-DAG: Type{{.*}} , name = "char8_t", size = 1, compiler_type = 0x{{[0-9a-f]+}} char8_t
+
+// CHECK-DAG: Type{{.*}} , size = 2, compiler_type = 0x{{[0-9a-f]+}} short
+
+// CHECK-DAG: Type{{.*}} , name = "unsigned short", size = 2, compiler_type = 0x{{[0-9a-f]+}} unsigned short
+// CHECK-DAG: Type{{.*}} , name = "wchar_t", size = 2, compiler_type = 0x{{[0-9a-f]+}} wchar_t
+// CHECK-DAG: Type{{.*}} , name = "char16_t", size = 2, compiler_type = 0x{{[0-9a-f]+}} char16_t
+
+// CHECK-DAG: Type{{.*}} , name = "int", size = 4, compiler_type = 0x{{[0-9a-f]+}} int
+// CHECK-DAG: Type{{.*}} , name = "unsigned", size = 4, compiler_type = 0x{{[0-9a-f]+}} unsigned int
+// CHECK-DAG: Type{{.*}} , name = "long", size = 4, compiler_type = 0x{{[0-9a-f]+}} long
+// CHECK-DAG: Type{{.*}} , name = "unsigned long", size = 4, compiler_type = 0x{{[0-9a-f]+}} unsigned long
+// CHECK-DAG: Type{{.*}} , name = "char32_t", size = 4, compiler_type = 0x{{[0-9a-f]+}} char32_t
+
+// CHECK-DAG: Type{{.*}} , name = "int64_t", size = 8, compiler_type = 0x{{[0-9a-f]+}} long long
+// CHECK-DAG: Type{{.*}} , name = "uint64_t", size = 8, compiler_type = 0x{{[0-9a-f]+}} unsigned long long
+
+// CHECK-DAG: Type{{.*}} , name = "float", size = 4, compiler_type = 0x{{[0-9a-f]+}} float
+// CHECK-DAG: Type{{.*}} , name = "float", size = 4, compiler_type = 0x{{[0-9a-f]+}} const float
+// CHECK-DAG: Type{{.*}} , name = "double", size = 8, compiler_type = 0x{{[0-9a-f]+}} double
+
+// CHECK-DAG: Type{{.*}} , name = "ReturnedStruct1", size = 1, decl = simple-types.cpp:20, compiler_type = 0x{{[0-9a-f]+}} struct ReturnedStruct1 {
+// CHECK-DAG: Type{{.*}} , name = "ReturnedStruct2", size = 1, decl = simple-types.cpp:21, compiler_type = 0x{{[0-9a-f]+}} struct ReturnedStruct2 {
+// CHECK-DAG: Type{{.*}} , name = "MyStruct", size = 1, decl = simple-types.cpp:23, compiler_type = 0x{{[0-9a-f]+}} struct MyStruct {
+
+// CHECK-DAG: Type{{.*}} , size = 8, compiler_type = 0x{{[0-9a-f]+}} struct MyStruct *const
+// CHECK-DAG: Type{{.*}} , size = 8, compiler_type = 0x{{[0-9a-f]+}} const struct MyStruct *const
+// CHECK-DAG: Type{{.*}} , size = 8, compiler_type = 0x{{[0-9a-f]+}} volatile struct MyStruct *const
+// CHECK-DAG: Type{{.*}} , size = 8, compiler_type = 0x{{[0-9a-f]+}} struct MyStruct &
+
+// CHECK-DAG: Type{{.*}} , name = "bool", size = 1, compiler_type = 0x{{[0-9a-f]+}} const _Bool
+// CHECK-DAG: Type{{.*}} , size = 8, compiler_type = 0x{{[0-9a-f]+}} _Bool &
+// CHECK-DAG: Type{{.*}} , size = 8, compiler_type = 0x{{[0-9a-f]+}} _Bool *
+// CHECK-DAG: Type{{.*}} , size = 8, compiler_type = 0x{{[0-9a-f]+}} _Bool *&
+// CHECK-DAG: Type{{.*}} , size = 8, compiler_type = 0x{{[0-9a-f]+}} const _Bool &
+// CHECK-DAG: Type{{.*}} , size = 8, compiler_type = 0x{{[0-9a-f]+}} _Bool *const
+// CHECK-DAG: Type{{.*}} , size = 8, compiler_type = 0x{{[0-9a-f]+}} const _Bool *const
+
+// CHECK-DAG: Type{{.*}} , size = 2, compiler_type = 0x{{[0-9a-f]+}} const volatile short
+// CHECK-DAG: Type{{.*}} , size = 2, compiler_type = 0x{{[0-9a-f]+}} const short
+// CHECK-DAG: Type{{.*}} , size = 2, compiler_type = 0x{{[0-9a-f]+}} volatile short
+
+// CHECK-DAG: Type{{.*}} , name = "SomeTypedef", size = 4, compiler_type = 0x{{[0-9a-f]+}} typedef SomeTypedef
+// CHECK-DAG: Type{{.*}} , name = "Func", size = 0, compiler_type = 0x{{[0-9a-f]+}} typedef Func
+
+// CHECK-DAG: Type{{.*}} , size = 0, compiler_type = 0x{{[0-9a-f]+}} int (void)
+// CHECK-DAG: Type{{.*}} , size = 0, compiler_type = 0x{{[0-9a-f]+}} void (void)
+
+// CHECK-DAG: Type{{.*}} , size = 0, compiler_type = 0x{{[0-9a-f]+}} void (int, _Bool *, const float, ...)
+// CHECK-DAG: Type{{.*}} , size = 8, compiler_type = 0x{{[0-9a-f]+}} void (*)(int, _Bool *, const float, ...)
+
+// CHECK-DAG: Type{{.*}} , size = 0, compiler_type = 0x{{[0-9a-f]+}} void (char16_t, struct MyStruct &)
+// CHECK-DAG: Type{{.*}} , size = 8, compiler_type = 0x{{[0-9a-f]+}} void (*)(char16_t, struct MyStruct &)
+
+// CHECK-DAG: Type{{.*}} , size = 0, compiler_type = 0x{{[0-9a-f]+}} struct ReturnedStruct1 (char *)
+// CHECK-DAG: Type{{.*}} , size = 0, compiler_type = 0x{{[0-9a-f]+}} struct ReturnedStruct2 (char *)
+
+// CHECK-DAG: Type{{.*}} , size = 8, compiler_type = 0x{{[0-9a-f]+}} long[2]
|
|
|
||
| CompilerType ct = target_type->GetForwardCompilerType(); | ||
| if (auto *clang = llvm::dyn_cast_or_null<TypeSystemClang>(ts.get())) | ||
| ct = clang->GetType(clang->getASTContext().getTypeDeclType(typedef_decl)); |
There was a problem hiding this comment.
This feels a bit wrong, because PdbAstParser::GetOrCreateTypedefDecl already knows this compiler type. However, it returns a decl, so we need to go back through the AST to resolve the type.
The method isn't used elsewhere, so the return type could be changed to a CompilerType.
There was a problem hiding this comment.
What use does GetOrCreateType have here? If that's not the thing that creates the correct TypedefType? Or in other words, could you elaborate on how this fixes the issue?
Is it that GetOrCreateTypedefDecl creates a new decl, but the associated TypedefType is different from the type created by GetOrCreateType? Does that mean we create two different typedefs?
There was a problem hiding this comment.
What use does
GetOrCreateTypehave here? If that's not the thing that creates the correctTypedefType? Or in other words, could you elaborate on how this fixes the issue?
Ah, I left out the confusing/unintuitive part:
Currently, in the PDBs produced by both MSVC and LLVM, type aliases are not in the "type system" (TPI stream/LF_* records; https://redirect.github.com/llvm/llvm-project/pull/153936 attempts to change this in LLVM).
GetOrCreateType looks in the PDB type stream TPI. The typedefs we currently have are from S_UDT records in the globals stream, so they're represented as "symbols" and GetOrCreateType would not be able to create the type.
Because of this, they're handled differently.
Does that mean we create two different typedefs?
No, clang::ASTContext::getTypeDeclType will use TypeForDecl to get back to the type.
There was a problem hiding this comment.
Ah I see, thanks for the context
The typedefs we currently have are from S_UDT records in the globals stream, so they're represented as "symbols" and GetOrCreateType would not be able to create the type
So what does GetOrCreateType achieve in this case? If it's not able to create the TypedefType.
There was a problem hiding this comment.
So what does
GetOrCreateTypeachieve in this case? If it's not able to create theTypedefType.
Oh, I see what you mean. The GetOrCreateType(udt.Type) above creates the typedef'd type. So for using Foo = Bar, it would get/create Bar.
| void member_fn() {}; | ||
| }; | ||
|
|
||
| void (*PF)(int, bool *, const float, ...); |
There was a problem hiding this comment.
Missing: Return types and parameters that are "simple" (i.e. TypeIndex::isSimple()) aren't created as types, because they don't have a type record in the TPI stream. So here, we're missing a Type for void. Non-simple types are created, because they will have a type record.
In the DIA typedefs.test (source), many simple types are only used in parameters. So the output of the native plugin is missing these. I'm not sure if it should add them at all.
There was a problem hiding this comment.
This variable seems unused. Can we add a test case to show what you mean by return and parameters missing? What would the typename look like?
There was a problem hiding this comment.
This variable seems unused.
It's unused, but still in the binary, because it's neither static nor inline. And it is present in the debug info.
Can we add a test case to show what you mean by return and parameters missing? What would the typename look like?
The type of the function itself gets created. This is checked at the bottom of the file:
// CHECK-DAG: Type{{.*}} , size = 0, compiler_type = 0x{{[0-9a-f]+}} void (int, _Bool *, const float, ...) // CHECK-DAG: Type{{.*}} , size = 8, compiler_type = 0x{{[0-9a-f]+}} void (*)(int, _Bool *, const float, ...)
However, the LLDB Types for simple/primitive types don't get created. I updated the test and moved double to a function parameter. Now, it's not created as an LLDB Type with the native plugin. DWARF and DIA do create a Type.
| decltype(nullptr) np; | ||
| } | ||
|
|
||
| // CHECK-DAG: Type{{.*}} , name = "std::nullptr_t", size = 0, compiler_type = 0x{{[0-9a-f]+}} nullptr_t |
There was a problem hiding this comment.
Here, the size of this type is 0. This is explicitly set in the code. The DIA plugin doesn't set the size to 0 if types are unsized (e.g. the function types below).
I'm not sure if it's bad that the native plugin sets this to 0. When calling Type::GetByteSize, the size is set to 0 anyway (because the compiler type would return 0 there).
There was a problem hiding this comment.
FWIW, the DWARF plugin doesn't set the size explicitly to 0, in which case Type::Dump omits it:
0xaacd26800: Type{0x0000046b} , name = "decltype(nullptr)", compiler_type = 0x0000000aad18ef40 nullptr_t
Can we do the same?
There was a problem hiding this comment.
Sure, I can do this in a followup PR.
| // CHECK-DAG: Type{{.*}} , name = "unsigned char", size = 1, compiler_type = 0x{{[0-9a-f]+}} unsigned char | ||
| // CHECK-DAG: Type{{.*}} , name = "char8_t", size = 1, compiler_type = 0x{{[0-9a-f]+}} char8_t | ||
|
|
||
| // CHECK-DAG: Type{{.*}} , size = 2, compiler_type = 0x{{[0-9a-f]+}} short |
There was a problem hiding this comment.
Missing: short doesn't have a name.
| // CHECK-DAG: Type{{.*}} , name = "int64_t", size = 8, compiler_type = 0x{{[0-9a-f]+}} long long | ||
| // CHECK-DAG: Type{{.*}} , name = "uint64_t", size = 8, compiler_type = 0x{{[0-9a-f]+}} unsigned long long |
There was a problem hiding this comment.
Missing/incompatible: (unsigned)? long long will generate types named after the typedefs in stdint.h. They should probably use the same name as the compiler type.
|
Ping - I'd like to use the added test from this PR as the "current state" and fix the missing things from above in other PRs. |
Will review some time today. Sorry for the delay! |
Before this PR, the native PDB plugin would create the following LLDB
Typeforusing SomeTypedef = long:with this PR, the following is created:
This matches the behavior of the DIA PDB plugin and works towards making
Shell/SymbolFile/PDB/typedefs.testpass with the native plugin.I added a similar test to the
NativePDBshell tests to capture the current state, which doesn't quite match that of DIA yet. I'll add some comments on what's missing on this PR, because I'm not fully sure what the correct output would be.