[LLDB][NativePDB] Don't complete static members' types when completing a record type.

[zequanwu]

Summary

UdtRecordCompleter shouldn't complete static members' types. static members' types are going to be completed when the types are called in SymbolFile::CompleteType.

[rnk]

A similar loop exists between a class and class methods, the type of the this parameter usually references the class, and the same is true for any parameters. Is there some existing code to handle this for methods?

[zequanwu]

In D121030#3361338, @rnk wrote:

A similar loop exists between a class and class methods, the type of the this parameter usually references the class, and the same is true for any parameters. Is there some existing code to handle this for methods?

For function parameter type and return type, it will not try to complete their types. It just create a skeleton record type. And the skeleton record type is created and cached before it's trying to complete the type.

[labath]

This defers the completion of static data members by caching them and complete them later

Are you sure that we need to complete static members at all? They have no impact on the layout of the class, so ideally they would only be completed when they are actually used.

[zequanwu]

In D121030#3367945, @labath wrote:

This defers the completion of static data members by caching them and complete them later

Are you sure that we need to complete static members at all?

Do you mean when completing class type, ignore those static members?

so ideally they would only be completed when they are actually used.

When will they be actually used?

This is what happens now: When TypeSystemClang::DeclContextFindDeclByName is used to find out a decl by name. It parses decls inside each TU until find a decl with the name. Then PdbAstBuilder will try to complete all types and cache them in that PDB file, because we can't filter types by TU.
D82160 also requires constant static member's type width to be known, but this happens when we try to complete the constant static member.

[labath]

In D121030#3370871, @zequanwu wrote:

In D121030#3367945, @labath wrote:

This defers the completion of static data members by caching them and complete them later

Are you sure that we need to complete static members at all?

Do you mean when completing class type, ignore those static members?

Yes.

so ideally they would only be completed when they are actually used.

When will they be actually used?

In the expression evaluator, for one. I'm not sure if there are any other cases.

This is what happens now: When TypeSystemClang::DeclContextFindDeclByName is used to find out a decl by name. It parses decls inside each TU until find a decl with the name. Then PdbAstBuilder will try to complete all types and cache them in that PDB file, because we can't filter types by TU.

I'm not very familiar with this stuff, but have you checked what dwarf would do under the same circumstances. I would be surprised if the answer is "it completes all static member types".

[zequanwu]

In the expression evaluator, for one. I'm not sure if there are any other cases.

If we ignore static members when completing class types, we don't know if CompleteType is called for completing a static member whose type is the class or other cases. For example, when we want to complete the type of static members whose type is the class (e.g. A) itself during expression evaluation, we are trying to complete the class A, but we don't know if we should ignore its static members.

I'm not very familiar with this stuff, but have you checked what dwarf would do under the same circumstances. I would be surprised if the answer is "it completes all static member types".

I checked dwarf but still don't understand how it evaluates static members without going into the loop.

[labath]

In D121030#3371399, @zequanwu wrote:

In the expression evaluator, for one. I'm not sure if there are any other cases.

If we ignore static members when completing class types, we don't know if CompleteType is called for completing a static member whose type is the class or other cases. For example, when we want to complete the type of static members whose type is the class (e.g. A) itself during expression evaluation, we are trying to complete the class A, but we don't know if we should ignore its static members.

I was imagining you would create the static member type as a forward decl -- create some ast like class Member { ... }; class StaticMember; class A { Member m; static StaticMember sm; };. And then you'd get a completion request for StaticMember when the type of A::sm is required.

I'm not very familiar with this stuff, but have you checked what dwarf would do under the same circumstances. I would be surprised if the answer is "it completes all static member types".

I checked dwarf but still don't understand how it evaluates static members without going into the loop.

I think that's because it creates them as forward declarations -- as I outlined above. DWARFASTParserClang::ParseTypeFromDWARF creates a forward-declared type, and ::CompleteTypeFromDwarf completes it. When parsing a class, the dwarf parser calls DWARFDIE::ResolveTypeUID (line 2673 of DWARFASTParserClang.cpp), which (after going through too many layers) ends up in DWARFASTParserClang::ParseTypeFromDWARF.

Also, note that the DWARF parser also has code which should prevent loops in regular members (which should only be possible for malformed inputs).

[zequanwu]

We simply shouldn't complete the types of static members at all, which is how dwarf does it.
Their types are eventually completed when called by SymbolFile::CompleteType.

[labath]

This is cool, just one question about the test.

lldb/test/Shell/SymbolFile/NativePDB/local-variables.cpp

8–26 ↗

(On Diff #414529)

Would it be possible to put this into a new test case, one that does not require running the binary? If we're able to read global variables without a running process (we can do it with elf+dwarf, but I don't know what's the state with coff+pdb), then we could run this test on non-windows hosts as well (which is great for test coverage).

[zequanwu]

Testing with image lookup -type

lldb/test/Shell/SymbolFile/NativePDB/local-variables.cpp

8–26 ↗

(On Diff #414529)

Printing doesn't work with any expression involves :: in NativePDB. So, I changed the test to image lookup -type which also tries to complete given type.

[labath]

lldb/test/Shell/SymbolFile/NativePDB/local-variables.cpp

8–26 ↗

(On Diff #414529)

That's cool, but I see the new test still has a REQUIRES: system-windows. Would it be possible to drop that requirement (if not, then why)?

[zequanwu]

Remove REQUIRES: system-windows.

lldb/test/Shell/SymbolFile/NativePDB/local-variables.cpp

8–26 ↗

(On Diff #414529)

This is not necessary. Removed.

[labath]

awesome

lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp

clang::QualType PdbAstBuilder::CreateEnumType(PdbTypeSymId id,
   TypeSystemClang::SetHasExternalStorage(enum_ct.GetOpaqueQualType(), true);
 
   return clang::QualType::getFromOpaquePtr(enum_ct.GetOpaqueQualType());
 }
 
 clang::QualType PdbAstBuilder::CreateArrayType(const ArrayRecord &ar) {
   clang::QualType element_type = GetOrCreateType(ar.ElementType);
 
-  uint64_t element_count =
-      ar.Size / GetSizeOfType({ar.ElementType}, m_index.tpi());
+  uint64_t element_size = GetSizeOfType({ar.ElementType}, m_index.tpi());
+  if (element_size == 0)
+    return {};
+  uint64_t element_count = ar.Size / element_size;
 
   CompilerType array_ct = m_clang.CreateArrayType(ToCompilerType(element_type),
                                                   element_count, false);
   return clang::QualType::getFromOpaquePtr(array_ct.GetOpaqueQualType());
 }
 
 clang::QualType PdbAstBuilder::CreateFunctionType(
     TypeIndex args_type_idx, TypeIndex return_type_idx,

lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp

Error UdtRecordCompleter::visitKnownMember(CVMemberRecord &cvr,
   return Error::success();
 }
 
 Error UdtRecordCompleter::visitKnownMember(
     CVMemberRecord &cvr, StaticDataMemberRecord &static_data_member) {
   clang::QualType member_type =
       m_ast_builder.GetOrCreateType(PdbTypeSymId(static_data_member.Type));
 
-  m_ast_builder.CompleteType(member_type);
-
   CompilerType member_ct = m_ast_builder.ToCompilerType(member_type);
 
   lldb::AccessType access =
       TranslateMemberAccess(static_data_member.getAccess());
   auto decl = TypeSystemClang::AddVariableToRecordType(
       m_derived_ct, static_data_member.Name, member_ct, access);
 
   // Static constant members may be a const[expr] declaration.
   // Query the symbol's value as the variable initializer if valid.
-  if (member_ct.IsConst()) {
+  if (member_ct.IsConst() && member_ct.IsCompleteType()) {
     std::string qual_name = decl->getQualifiedNameAsString();
 
     auto results =
         m_index.globals().findRecordsByName(qual_name, m_index.symrecords());
 
     for (const auto &result : results) {
       if (result.second.kind() == SymbolKind::S_CONSTANT) {
         ConstantSym constant(SymbolRecordKind::ConstantSym);

lldb/test/Shell/SymbolFile/NativePDB/Inputs/lookup-by-types.lldbinit

+image lookup -type A
+image lookup -type B
+
+quit
 No newline at end of file

lldb/test/Shell/SymbolFile/NativePDB/lookup-by-types.cpp

+// clang-format off
+
+// RUN: %build -o %t.exe -- %s
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f %t.exe -s \
+// RUN:     %p/Inputs/lookup-by-types.lldbinit 2>&1 | FileCheck %s
+
+class B;
+class A {
+public:
+    static const A constA;
+    static A a;
+    static B b;
+    int val = 1;
+};
+class B {
+public:
+    static A a;
+    int val = 2;
+};
+A varA;
+B varB;
+const A A::constA = varA;
+A A::a = varA;
+B A::b = varB;
+A B::a = varA;
+
+int main(int argc, char **argv) {
+  return varA.val + varB.val;
+}
+
+// CHECK:      image lookup -type A
+// CHECK-NEXT: 1 match found in {{.*}}.exe
+// CHECK-NEXT: compiler_type = "class A {
+// CHECK-NEXT:     static const A constA;
+// CHECK-NEXT:     static A a;
+// CHECK-NEXT:     static B b;
+// CHECK-NEXT: public:
+// CHECK-NEXT:     int val;
+// CHECK-NEXT: }"
+// CHECK:      image lookup -type B
+// CHECK-NEXT: 1 match found in {{.*}}.exe
+// CHECK-NEXT:  compiler_type = "class B {
+// CHECK-NEXT:     static A a;
+// CHECK-NEXT: public:
+// CHECK-NEXT:     int val;
+// CHECK-NEXT: }"