[LLDB][NativePDB] Forcefully complete a record type if it has empty debug info and is required to have complete type.

[zequanwu]

Summary

It's required in following situations:

1. As a base class.
2. As a data member.
3. As an array element type.

[rnk]

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

681 ↗

(On Diff #460854)

Is this what we do for DWARF? The same kind of situation seems like it can arise, where clang requires a type to be complete, but the information is missing, and we still try to proceed with evaluation.

[zequanwu]

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

681 ↗

(On Diff #460854)

I think it's here: https://github.com/llvm/llvm-project/blob/main/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp#L221-L250, relevant: https://reviews.llvm.org/D85968.

[zequanwu]

It seems not correct. The dwarf test expects an error. I should figure out how to reduce that original crash.

[labath]

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

681 ↗

(On Diff #460854)

The trick is to do the forced completion only when the type is used within contexts where the c++ rules require it to be complete. something like class A; A* a; is perfectly legal c++. class A; class B : A {}; is not. You can't do this from within the completion callback. In DWARF code, we check for this when we're parsing the enclosing entity (so, when we're parsing B, we'd check, and if needed, "forcefully complete" the class A.

[zequanwu]

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

681 ↗

(On Diff #460854)

What does dwarf plugin do in this case that we don't have debug info for class A: class A; class B : A {};? The problem I'm trying to fix is when the base class has no debug info to complete it.

[zequanwu]

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

681 ↗

(On Diff #460854)

@labath, Do you see anything would go wrong if we forcefully complete a class that has no debug info? It seems fine, for the chrome crash report I was checking.

[labath]

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

681 ↗

(On Diff #460854)

I don't think anything will blow up, if that's what you're asking. The way I think this will manifest itself is that various opaque types that are supposed to be incomplete (e.g. this is often the case with the FILE type in the C library) will suddenly become complete (and empty). That in turn can have various (more or less subtle) effects on the way these objects can be viewed and manipulated by the user. It's not the end of the world, but given that this forceful completion business is essentially a giant hack, I think it makes sense to limit its scope as much as possible.

[zequanwu]

It shouldn't reach the code path to complete a class with empty debug info.

[labath]

In D134066#3903831, @zequanwu wrote:

It shouldn't reach the code path to complete a class with empty debug info.

How about this?

$ cat a.cc
struct A { int x; A(); }; A::A() : x(47) {}

$ cat b.cc
struct A { int x; A(); };
struct B : A {};

B b;
int main(){}

$ bin\clang a.cc -o a.o -c

$ bin\clang.exe a.o b.cc -g -o b.exe

$ bin\lldb b.exe
(lldb) target create "b.exe"
(lldb) Current executable set to 'b.exe' (x86_64).
(lldb) b main
Breakpoint 1: where = b.exe`main at b.cc:5, address = 0x0000000140007100
(lldb) r
(lldb) Process 14288 launched: 'b.exe' (x86_64)
Process 14288 stopped
* thread #1, stop reason = breakpoint 1.1
    frame #0: 0x00007ff6f37f7100 b.exe`main at b.cc:5
   2    struct B : A {};
   3
   4    B b;
-> 5    int main(){}
(lldb) p b
Assertion failed: DD && "queried property of class with no definition", file clang\include\clang/AST/DeclCXX.h, line 443

[zequanwu]

Update to forcefully complete a record type only if it has empty debug info and is required to have complete type.

I basically copied RequireCompleteType from dwarf plugin. Calling it when the tag type is one of 1. an element type 2. an base class 3. a class member. This is the same way that dwarf plugin is doing.

[labath]

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

1 ↗

(On Diff #473077)

oops?

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

96–98

I suppose this won't hurt, but RequireCompleteType will not actually do anything for method types, right ?

Regarding method types, there is a slightly different problem. C++ rules require that in code like
struct B1 { virtual A1* f(); }; struct B2:B1 { A2* f(); };, the types A1 and A2 must be complete (because that code is only valid if A1 is a base of A2). However, I think that's better left to a separate patch.

159

Are you sure about static member part? Something like struct A; struct B { static A a; }; will compile fine (unlike struct A; struct B { A a; };), so I'd hope that the expression evaluator could handle an incomplete static member. It would be better if p B::a produces something like error: variable has incomplete type 'A' instead of printing an empty struct.

lldb/test/Shell/SymbolFile/NativePDB/incomplete-tag-type.cpp

26

The A a_array[] case is more interesting, because here the class might be (and probably will be) completed as a part of completing B. It would also be better to use a different incomplete class for each test case, as a class can be completed only once.

[zequanwu]

Update.

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

96–98

I suppose this won't hurt, but RequireCompleteType will not actually do anything for method types, right ?

Yeah, it should do nothing. I just replaced all occurrence of CompleteType with RequireCompleteType.

159

Removed.

lldb/test/Shell/SymbolFile/NativePDB/incomplete-tag-type.cpp

26

Printing A a_array[] will give error: use of undeclared identifier. Because the element type size is 0 and the number of element is calculated by total_size/element_type_size, it won't able to create type in this case. The size of B is 4 bytes, so it's able to print b_array.

[zequanwu]

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

96–98

Regarding method types, there is a slightly different problem. C++ rules require that in code like
struct B1 { virtual A1* f(); }; struct B2:B1 { A2* f(); };, the types A1 and A2 must be complete (because that code is only valid if A1 is a base of A2). However, I think that's better left to a separate patch.

I tried this example. It crashed and reminds me of a similar crash stack I seen at https://bugs.chromium.org/p/chromium/issues/detail?id=1359144#c4. I tried to call PdbAstBuilder::RequireCompleteType with the method return type, still not working. Not sure what's missing.

[zequanwu]

Removed PdbAstBuilder::RequireCompleteType from PdbAstBuilder::CreateArrayType as it's not necessary at all.

1. When the element type info is missing, its size info is missing in pdb, which is 0. Thus we can't calculate the number of element in the array.
2. When the element type info is present, it will be completed later as needed.

[labath]

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

90 ↗

(On Diff #473336)

I forgot about this the first time around, but it'd be nice if this code could be shared between the pdb and dwarf plugins. Maybe by putting it into TypeSystemClang ?

lldb/test/Shell/SymbolFile/NativePDB/incomplete-tag-type.cpp

21

Maybe you could leave this in (but change the test so that it expects the expression to fail)?

[zequanwu]

Adding a test for incomplete record type as class static member. It returns error, because its type not required to be completed.

[labath]

Looks good, but I think you missed one comment.

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

90 ↗

(On Diff #473336)

Did you perhaps miss this comment/request?

[zequanwu]

Move RequireCompleteType to TypeSystemClang.

[rnk]

Following the direction of the DWARF plugin sounds good to me.

[labath]

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

9815–9818

Please move this comment to the header.

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

static void ForcefullyCompleteType(CompilerType type) {
   bool started = TypeSystemClang::StartTagDeclarationDefinition(type);
   lldbassert(started && "Unable to start a class type definition.");
   TypeSystemClang::CompleteTagDeclarationDefinition(type);
   const clang::TagDecl *td = ClangUtil::GetAsTagDecl(type);
   auto &ts = llvm::cast<TypeSystemClang>(*type.GetTypeSystem());
   ts.GetMetadata(td)->SetIsForcefullyCompleted();
 }
 
-/// Complete a type from debug info, or mark it as forcefully completed if
-/// there is no definition of the type in the current Module. Call this function
-/// in contexts where the usual C++ rules require a type to be complete (base
-/// class, member, etc.).
-static void RequireCompleteType(CompilerType type) {
-  // Technically, enums can be incomplete too, but we don't handle those as they
-  // are emitted even under -flimit-debug-info.
-  if (!TypeSystemClang::IsCXXClassType(type))
-    return;
-
-  if (type.GetCompleteType())
-    return;
-
-  // No complete definition in this module.  Mark the class as complete to
-  // satisfy local ast invariants, but make a note of the fact that
-  // it is not _really_ complete so we can later search for a definition in a
-  // different module.
-  // Since we provide layout assistance, layouts of types containing this class
-  // will be correct even if we  are not able to find the definition elsewhere.
-  ForcefullyCompleteType(type);
-}
-
 /// This function serves a similar purpose as RequireCompleteType above, but it
 /// avoids completing the type if it is not immediately necessary. It only
 /// ensures we _can_ complete the type later.
 static void PrepareContextToReceiveMembers(TypeSystemClang &ast,
                                            ClangASTImporter &ast_importer,
                                            clang::DeclContext *decl_ctx,
                                            DWARFDIE die,
                                            const char *type_name_cstr) {
DWARFASTParserClang::ParseArrayType(const DWARFDIE &die,
   uint32_t bit_stride = attrs.bit_stride;
   if (array_info) {
     byte_stride = array_info->byte_stride;
     bit_stride = array_info->bit_stride;
   }
   if (byte_stride == 0 && bit_stride == 0)
     byte_stride = element_type->GetByteSize(nullptr).value_or(0);
   CompilerType array_element_type = element_type->GetForwardCompilerType();
-  RequireCompleteType(array_element_type);
+  TypeSystemClang::RequireCompleteType(array_element_type);
 
   uint64_t array_element_bit_stride = byte_stride * 8 + bit_stride;
   CompilerType clang_type;
   if (array_info && array_info->element_orders.size() > 0) {
     uint64_t num_elements = 0;
     auto end = array_info->element_orders.rend();
     for (auto pos = array_info->element_orders.rbegin(); pos != end; ++pos) {
       num_elements = *pos;
if (die.HasChildren()) {
     if (!bases.empty()) {
       // Make sure all base classes refer to complete types and not forward
       // declarations. If we don't do this, clang will crash with an
       // assertion in the call to clang_type.TransferBaseClasses()
       for (const auto &base_class : bases) {
         clang::TypeSourceInfo *type_source_info =
             base_class->getTypeSourceInfo();
         if (type_source_info)
-          RequireCompleteType(m_ast.GetType(type_source_info->getType()));
+          TypeSystemClang::RequireCompleteType(
+              m_ast.GetType(type_source_info->getType()));
       }
 
       m_ast.TransferBaseClasses(clang_type.GetOpaqueQualType(),
                                 std::move(bases));
     }
   }
 
   m_ast.AddMethodOverridesForCXXRecordType(clang_type.GetOpaqueQualType());
if (member_clang_type.IsArrayType(&member_array_element_type,
         }
 
         member_clang_type =
             m_ast.CreateArrayType(member_array_element_type, 0, false);
       }
     }
   }
 
-  RequireCompleteType(member_clang_type);
+  TypeSystemClang::RequireCompleteType(member_clang_type);
 
   clang::FieldDecl *field_decl = TypeSystemClang::AddFieldToRecordType(
       class_clang_type, attrs.name, member_clang_type, attrs.accessibility,
       attrs.bit_size);
 
   m_ast.SetMetadataAsUserID(field_decl, die.GetID());
 
   layout_info.field_offsets.insert(

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

 
 void UdtRecordCompleter::AddMethod(llvm::StringRef name, TypeIndex type_idx,
                                    MemberAccess access, MethodOptions options,
                                    MemberAttributes attrs) {
   clang::QualType method_qt =
       m_ast_builder.GetOrCreateType(PdbTypeSymId(type_idx));
   if (method_qt.isNull())
     return;
-  m_ast_builder.CompleteType(method_qt);
   CompilerType method_ct = m_ast_builder.ToCompilerType(method_qt);
-  lldb::opaque_compiler_type_t derived_opaque_ty = m_derived_ct.GetOpaqueQualType();
+  TypeSystemClang::RequireCompleteType(method_ct);
+  lldb::opaque_compiler_type_t derived_opaque_ty =
+      m_derived_ct.GetOpaqueQualType();
   auto iter = m_cxx_record_map.find(derived_opaque_ty);
   if (iter != m_cxx_record_map.end()) {
     if (iter->getSecond().contains({name, method_ct})) {
       return;
     }
   }
 
   lldb::AccessType access_type = TranslateMemberAccess(access);

 Error UdtRecordCompleter::visitKnownMember(
     CVMemberRecord &cvr, StaticDataMemberRecord &static_data_member) {
   clang::QualType member_type =
       m_ast_builder.GetOrCreateType(PdbTypeSymId(static_data_member.Type));
   if (member_type.isNull())
     return llvm::Error::success();
 
   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() && member_ct.IsCompleteType()) {
if (cvt.kind() == LF_BITFIELD) {
       bitfield_width = bfr.BitSize;
       ti = bfr.Type;
     }
   }
 
   clang::QualType member_qt = m_ast_builder.GetOrCreateType(PdbTypeSymId(ti));
   if (member_qt.isNull())
     return Error::success();
-  m_ast_builder.CompleteType(member_qt);
+  TypeSystemClang::RequireCompleteType(m_ast_builder.ToCompilerType(member_qt));
   lldb::AccessType access = TranslateMemberAccess(data_member.getAccess());
   size_t field_size =
       bitfield_width ? bitfield_width : GetSizeOfType(ti, m_index.tpi()) * 8;
   if (field_size == 0)
     return Error::success();
   m_record.CollectMember(data_member.Name, offset, field_size, member_qt, access,
                 bitfield_width);
   return Error::success();
void UdtRecordCompleter::complete() {
   llvm::stable_sort(m_bases, llvm::less_first());
 
   std::vector<std::unique_ptr<clang::CXXBaseSpecifier>> bases;
   bases.reserve(m_bases.size());
   for (auto &ib : m_bases)
     bases.push_back(std::move(ib.second));
 
   TypeSystemClang &clang = m_ast_builder.clang();
+  // Make sure all base classes refer to complete types and not forward
+  // declarations. If we don't do this, clang will crash with an
+  // assertion in the call to clang_type.TransferBaseClasses()
+  for (const auto &base_class : bases) {
+    clang::TypeSourceInfo *type_source_info =
+        base_class->getTypeSourceInfo();
+    if (type_source_info) {
+      TypeSystemClang::RequireCompleteType(
+          clang.GetType(type_source_info->getType()));
+    }
+  }
+
   clang.TransferBaseClasses(m_derived_ct.GetOpaqueQualType(), std::move(bases));
 
   clang.AddMethodOverridesForCXXRecordType(m_derived_ct.GetOpaqueQualType());
   FinishRecord();
   TypeSystemClang::BuildIndirectFields(m_derived_ct);
   TypeSystemClang::CompleteTagDeclarationDefinition(m_derived_ct);
 
   if (auto *record_decl = llvm::dyn_cast<clang::CXXRecordDecl>(&m_tag_decl)) {

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h

#endif
 
   clang::LangOptions *GetLangOpts() const {
     return m_language_options_up.get();
   }
   clang::SourceManager *GetSourceMgr() const {
     return m_source_manager_up.get();
   }
 
+  /// Complete a type from debug info, or mark it as forcefully completed if
+  /// there is no definition of the type in the current Module. Call this
+  /// function in contexts where the usual C++ rules require a type to be
+  /// complete (base class, member, etc.).
+  static void RequireCompleteType(CompilerType type);
+
 private:
   /// Returns the PrintingPolicy used when generating the internal type names.
   /// These type names are mostly used for the formatter selection.
   clang::PrintingPolicy GetTypePrintingPolicy();
   /// Returns the internal type name for the given NamedDecl using the
   /// type printing policy.
   std::string GetTypeNameForDecl(const clang::NamedDecl *named_decl,
                                  bool qualified = true);

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

 TypeSystemClang::DeclContextGetTypeSystemClang(const CompilerDeclContext &dc) {
   TypeSystemClang *ast =
       llvm::dyn_cast_or_null<TypeSystemClang>(dc.GetTypeSystem());
   if (ast)
     return &ast->getASTContext();
   return nullptr;
 }
 
+void TypeSystemClang::RequireCompleteType(CompilerType type) {
+  // Technically, enums can be incomplete too, but we don't handle those as they
+  // are emitted even under -flimit-debug-info.
+  if (!TypeSystemClang::IsCXXClassType(type))
+    return;
+
+  if (type.GetCompleteType())
+    return;
+
+  // No complete definition in this module.  Mark the class as complete to
+  // satisfy local ast invariants, but make a note of the fact that
+  // it is not _really_ complete so we can later search for a definition in a
+  // different module.
+  // Since we provide layout assistance, layouts of types containing this class
+  // will be correct even if we  are not able to find the definition elsewhere.
+  bool started = TypeSystemClang::StartTagDeclarationDefinition(type);
+  lldbassert(started && "Unable to start a class type definition.");
+  TypeSystemClang::CompleteTagDeclarationDefinition(type);
+  const clang::TagDecl *td = ClangUtil::GetAsTagDecl(type);
+  auto &ts = llvm::cast<TypeSystemClang>(*type.GetTypeSystem());
+  ts.GetMetadata(td)->SetIsForcefullyCompleted();
+}
+
 namespace {
 /// A specialized scratch AST used within ScratchTypeSystemClang.
 /// These are the ASTs backing the different IsolatedASTKinds. They behave
 /// like a normal ScratchTypeSystemClang but they don't own their own
 /// persistent  storage or target reference.
 class SpecializedScratchAST : public TypeSystemClang {
 public:
   /// \param name The display name of the TypeSystemClang instance.

lldb/test/Shell/SymbolFile/NativePDB/Inputs/incomplete-tag-type.cpp

+struct A {
+  int x;
+  A();
+};
+A::A() : x(47) {}
+
+struct C {
+  C();
+};
+C::C() = default;
+
+struct E {
+  E();
+};
+E::E() = default;

lldb/test/Shell/SymbolFile/NativePDB/incomplete-tag-type.cpp

+// clang-format off
+// REQUIRES: lld, x86
+
+// RUN: %clang_cl --target=x86_64-windows-msvc -c /Fo%t1.obj -- %p/Inputs/incomplete-tag-type.cpp
+// RUN: %clang_cl --target=x86_64-windows-msvc /O1 /Z7 -c /Fo%t2.obj -- %s
+// RUN: lld-link /debug:full /nodefaultlib /entry:main %t1.obj %t2.obj /out:%t.exe /pdb:%t.pdb
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f %t.exe -o \
+// RUN:   "settings set interpreter.stop-command-source-on-error false" \
+// RUN:   -o "p b" -o "p d" -o "p static_e_ref" -o "exit" 2>&1 | FileCheck %s
+
+// CHECK: (lldb) p b
+// CHECK: (B) $0 = {}
+// CHECK: (lldb) p d
+// CHECK: (D) $1 = {}
+// CHECK: (lldb) p static_e_ref
+// CHECK: error: expression failed to parse:
+// CHECK: error: {{.*}}: incomplete type 'E' where a complete type is required
+// CHECK: static_e_ref
+// CHECK: ^
+
+// Complete base class.
+struct A { int x; A(); };
+struct B : A {};
+B b;
+
+// Complete data member.
+struct C {
+  C();
+};
+
+struct D {
+  C c;
+};
+D d;
+
+// Incomplete static data member should return error.
+struct E {
+  E();
+};
+
+struct F {
+  static E static_e;
+};
+
+E F::static_e = E();
+E& static_e_ref = F::static_e;
+
+int main(){}