[lldb] Forcefully complete a type when adding nested classes

[labath]

Summary

With -flimit-debug-info, we can run into cases when we only have a class
as a declaration, but we do have a definition of a nested class. In this
case, clang will hit an assertion when adding a member to an incomplete
type (but only if it's adding a c++ class, and not C struct).

It turns out we already had code to handle a similar situation arising
in the -gmodules scenario. This extends the code to handle
-flimit-debug-info as well, and reorganizes bits of other code handling
completion of types to move functions doing similar things closer
together.

[teemperor]

LGTM

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

233

You could just get the TypeSystemClang from the type (passing a different TypeSystemClang that is not type.getTypeSystem() would lead to strange bugs, so we might as well make that situation impossible by not giving the caller the option).

[MaskRay]

lldb/test/Shell/SymbolFile/DWARF/DW_AT_declaration-with-children.s

7–11

In many cases rm -rf %t is not needed. split-file will unlink the output if it was originally a file.

9

No need to change now: -o is recommended (I think the arguments are (1) for aesthetic value (2) when llvm-mc fails, don't leave an empty file)

[labath]

lldb/test/Shell/SymbolFile/DWARF/DW_AT_declaration-with-children.s

7–11

Aha. That seems useful, albeit unexpected.

9

cool

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

 //===-- DWARFASTParserClang.h -----------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_SOURCE_PLUGINS_SYMBOLFILE_DWARF_DWARFASTPARSERCLANG_H
 #define LLDB_SOURCE_PLUGINS_SYMBOLFILE_DWARF_DWARFASTPARSERCLANG_H
 
 #include "clang/AST/CharUnits.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallVector.h"
 
 #include "DWARFASTParser.h"
 #include "DWARFDIE.h"
 #include "DWARFDefines.h"
 #include "DWARFFormValue.h"
lldb::TypeSP ParseEnum(const lldb_private::SymbolContext &sc,
                          const DWARFDIE &die, ParsedDWARFTypeAttributes &attrs);
   lldb::TypeSP ParseSubroutine(const DWARFDIE &die,
                                ParsedDWARFTypeAttributes &attrs);
   // FIXME: attrs should be passed as a const reference.
   lldb::TypeSP ParseArrayType(const DWARFDIE &die,
                               ParsedDWARFTypeAttributes &attrs);
   lldb::TypeSP ParsePointerToMemberType(const DWARFDIE &die,
                                         const ParsedDWARFTypeAttributes &attrs);
-
-  /// Complete a type from debug info, or mark it as forcefully completed if
-  /// there is no 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.).
-  void CompleteType(lldb_private::CompilerType type);
 };
 
 /// Parsed form of all attributes that are relevant for type reconstruction.
 /// Some attributes are relevant for all kinds of types (declaration), while
 /// others are only meaningful to a specific type (is_virtual)
 struct ParsedDWARFTypeAttributes {
   explicit ParsedDWARFTypeAttributes(const DWARFDIE &die);
 

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

if (tag_decl) {
     clang::DeclContext *defn_decl_ctx = GetCachedClangDeclContextForDIE(die);
     if (defn_decl_ctx)
       LinkDeclContextToDIE(defn_decl_ctx, die);
   }
 
   return type_sp;
 }
 
-static void CompleteExternalTagDeclType(TypeSystemClang &ast,
+static void ForcefullyCompleteType(CompilerType type, TypeSystemClang &ast) {
+  bool started = TypeSystemClang::StartTagDeclarationDefinition(type);
+  lldbassert(started && "Unable to start a class type definition.");
+  TypeSystemClang::CompleteTagDeclarationDefinition(type);
+  const clang::TagDecl *td = ClangUtil::GetAsTagDecl(type);
+  ast.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, TypeSystemClang &ast) {
+  // 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, ast);
+}
+
+/// 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) {
   auto *tag_decl_ctx = clang::dyn_cast<clang::TagDecl>(decl_ctx);
   if (!tag_decl_ctx)
+    return; // Non-tag context are always ready.
+
+  // We have already completed the type, or we have found its definition and are
+  // ready to complete it later (cf. ParseStructureLikeDIE).
+  if (tag_decl_ctx->isCompleteDefinition() || tag_decl_ctx->isBeingDefined())
     return;
 
+  // We reach this point of the tag was present in the debug info as a
+  // declaration only. If it was imported from another AST context (in the
+  // gmodules case), we can complete the type by doing a full import.
+
   // If this type was not imported from an external AST, there's nothing to do.
   CompilerType type = ast.GetTypeForDecl(tag_decl_ctx);
-  if (!type || !ast_importer.CanImport(type))
-    return;
-
+  if (type && ast_importer.CanImport(type)) {
     auto qual_type = ClangUtil::GetQualType(type);
-  if (!ast_importer.RequireCompleteType(qual_type)) {
+    if (ast_importer.RequireCompleteType(qual_type))
+      return;
     die.GetDWARF()->GetObjectFile()->GetModule()->ReportError(
         "Unable to complete the Decl context for DIE '%s' at offset "
         "0x%8.8x.\nPlease file a bug report.",
         type_name_cstr ? type_name_cstr : "", die.GetOffset());
-    // We need to make the type look complete otherwise, we might crash in
-    // Clang when adding children.
-    if (TypeSystemClang::StartTagDeclarationDefinition(type))
-      TypeSystemClang::CompleteTagDeclarationDefinition(type);
   }
+
+  // We don't have a type definition and/or the import failed. We must
+  // forcefully complete the type to avoid crashes.
+  ForcefullyCompleteType(type, ast);
 }
 
 ParsedDWARFTypeAttributes::ParsedDWARFTypeAttributes(const DWARFDIE &die) {
   DWARFAttributes attributes;
   size_t num_attributes = die.GetAttributes(attributes);
   for (size_t i = 0; i < num_attributes; ++i) {
     dw_attr_t attr = attributes.AttributeAtIndex(i);
     DWARFFormValue form_value;
TypeSP DWARFASTParserClang::ParseArrayType(const DWARFDIE &die,
   llvm::Optional<SymbolFile::ArrayInfo> array_info = ParseChildArrayInfo(die);
   if (array_info) {
     attrs.byte_stride = array_info->byte_stride;
     attrs.bit_stride = array_info->bit_stride;
   }
   if (attrs.byte_stride == 0 && attrs.bit_stride == 0)
     attrs.byte_stride = element_type->GetByteSize(nullptr).getValueOr(0);
   CompilerType array_element_type = element_type->GetForwardCompilerType();
-  CompleteType(array_element_type);
+  RequireCompleteType(array_element_type, m_ast);
 
   uint64_t array_element_bit_stride =
       attrs.byte_stride * 8 + attrs.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) {
if (llvm::Optional<uint64_t> clang_type_size =
     return std::make_shared<Type>(die.GetID(), dwarf, attrs.name,
                                   *clang_type_size, nullptr, LLDB_INVALID_UID,
                                   Type::eEncodingIsUID, nullptr, clang_type,
                                   Type::ResolveState::Forward);
   }
   return nullptr;
 }
 
-void DWARFASTParserClang::CompleteType(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);
-  m_ast.GetMetadata(td)->SetIsForcefullyCompleted();
-}
-
 TypeSP DWARFASTParserClang::UpdateSymbolContextScopeForType(
     const SymbolContext &sc, const DWARFDIE &die, TypeSP type_sp) {
   if (!type_sp)
     return type_sp;
 
   SymbolFileDWARF *dwarf = die.GetDWARF();
   TypeList &type_list = dwarf->GetTypeList();
   DWARFDIE sc_parent_die = SymbolFileDWARF::GetParentSymbolContextDIE(die);
DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
   assert(tag_decl_kind != -1);
   bool clang_type_was_created = false;
   clang_type.SetCompilerType(
       &m_ast, dwarf->GetForwardDeclDieToClangType().lookup(die.GetDIE()));
   if (!clang_type) {
     clang::DeclContext *decl_ctx =
         GetClangDeclContextContainingDIE(die, nullptr);
 
-    // If your decl context is a record that was imported from another
-    // AST context (in the gmodules case), we need to make sure the type
-    // backing the Decl is complete before adding children to it. This is
-    // not an issue in the non-gmodules case because the debug info will
-    // always contain a full definition of parent types in that case.
-    CompleteExternalTagDeclType(m_ast, GetClangASTImporter(), decl_ctx, die,
+    PrepareContextToReceiveMembers(m_ast, GetClangASTImporter(), decl_ctx, die,
                                    attrs.name.GetCString());
 
     if (attrs.accessibility == eAccessNone && decl_ctx) {
       // Check the decl context that contains this class/struct/union. If
       // it is a class we must give it an accessibility.
       const clang::Decl::Kind containing_decl_kind = decl_ctx->getDeclKind();
       if (DeclKindIsCXXClass(containing_decl_kind))
         attrs.accessibility = default_accessibility;
     }
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)
-          CompleteType(m_ast.GetType(type_source_info->getType()));
+          RequireCompleteType(m_ast.GetType(type_source_info->getType()),
+                              m_ast);
       }
 
       m_ast.TransferBaseClasses(clang_type.GetOpaqueQualType(),
                                 std::move(bases));
     }
   }
 
   m_ast.AddMethodOverridesForCXXRecordType(clang_type.GetOpaqueQualType());
if (tag == DW_TAG_member) {
               }
 
               member_clang_type =
                   m_ast.CreateArrayType(member_array_element_type, 0, false);
             }
           }
         }
 
-        CompleteType(member_clang_type);
+        RequireCompleteType(member_clang_type, m_ast);
 
         field_decl = TypeSystemClang::AddFieldToRecordType(
             class_clang_type, name, member_clang_type, accessibility,
             bit_size);
 
         m_ast.SetMetadataAsUserID(field_decl, die.GetID());
 
         layout_info.field_offsets.insert(

lldb/test/Shell/SymbolFile/DWARF/DW_AT_declaration-with-children.s

-# Test that a forward-declared (DW_AT_declaration) structure is treated as a
-# forward-declaration even if it has children. These types can be produced due
-# to vtable-based type homing, or other -flimit-debug-info optimizations.
+# Test handling of forward-declared (DW_AT_declaration) structures. These types
+# can be produced due to vtable-based type homing, or other -flimit-debug-info
+# optimizations.
 
 # REQUIRES: x86
 
-# RUN: llvm-mc --triple x86_64-pc-linux %s --filetype=obj > %t
-# RUN: %lldb %t -o "expr a" -o exit 2>&1 | FileCheck %s --check-prefix=EXPR
-# RUN: %lldb %t -o "target var a" -o exit 2>&1 | FileCheck %s --check-prefix=VAR
-
-# EXPR: incomplete type 'A' where a complete type is required
-
+# RUN: rm -rf %t
+# RUN: split-file %s %t
+# RUN: llvm-mc --triple x86_64-pc-linux %t/asm --filetype=obj > %t.o
+# RUN: %lldb -o "settings set interpreter.stop-command-source-on-error false" \
+# RUN:   -s %t/commands -o exit %t.o 2>&1 | FileCheck %s
+
+#--- commands
+# Type A should be treated as a forward-declaration even though it has a child.
+target var a
+# CHECK-LABEL: target var a
 # FIXME: This should also produce some kind of an error.
-# VAR: (A) a = {}
+# CHECK: (A) a = {}
+expr a
+# CHECK-LABEL: expr a
+# CHECK: incomplete type 'A' where a complete type is required
+
+# Parsing B::B1 should not crash even though B is incomplete. Note that in this
+# case B must be forcefully completed.
+target var b1
+# CHECK-LABEL: target var b1
+# CHECK: (B::B1) b1 = (ptr = 0x00000000baadf00d)
+expr b1
+# CHECK-LABEL: expr b1
+# CHECK: (B::B1) $0 = (ptr = 0x00000000baadf00d)
 
+#--- asm
         .text
 _ZN1AC2Ev:
         retq
 .LZN1AC2Ev_end:
 
         .data
+        .p2align 4
 a:
-        .quad   $_ZTV1A+16
-        .quad   $0xdeadbeef
+        .quad   _ZTV1A+16
+        .quad   0xdeadbeef
+
+b1:
+        .quad   0xbaadf00d
 
         .section        .debug_abbrev,"",@progbits
         .byte   1                               # Abbreviation Code
         .byte   17                              # DW_TAG_compile_unit
         .byte   1                               # DW_CHILDREN_yes
         .byte   37                              # DW_AT_producer
         .byte   8                               # DW_FORM_string
         .byte   17                              # DW_AT_low_pc
b1:
         .byte   5                               # DW_TAG_formal_parameter
         .byte   0                               # DW_CHILDREN_no
         .byte   73                              # DW_AT_type
         .byte   19                              # DW_FORM_ref4
         .byte   52                              # DW_AT_artificial
         .byte   25                              # DW_FORM_flag_present
         .byte   0                               # EOM(1)
         .byte   0                               # EOM(2)
+        .byte   6                               # Abbreviation Code
+        .byte   2                               # DW_TAG_class_type
+        .byte   1                               # DW_CHILDREN_yes
+        .byte   3                               # DW_AT_name
+        .byte   8                               # DW_FORM_string
+        .byte   0                               # EOM(1)
+        .byte   0                               # EOM(2)
+        .byte   7                               # Abbreviation Code
+        .byte   13                              # DW_TAG_member
+        .byte   0                               # DW_CHILDREN_no
+        .byte   3                               # DW_AT_name
+        .byte   8                               # DW_FORM_string
+        .byte   73                              # DW_AT_type
+        .byte   19                              # DW_FORM_ref4
+        .byte   56                              # DW_AT_data_member_location
+        .byte   11                              # DW_FORM_data1
+        .byte   0                               # EOM(1)
+        .byte   0                               # EOM(2)
         .byte   8                               # Abbreviation Code
         .byte   15                              # DW_TAG_pointer_type
         .byte   0                               # DW_CHILDREN_no
         .byte   73                              # DW_AT_type
         .byte   19                              # DW_FORM_ref4
         .byte   0                               # EOM(1)
         .byte   0                               # EOM(2)
         .byte   10                              # Abbreviation Code

 .Ldebug_info_start0:
         .short  4                               # DWARF version number
         .long   .debug_abbrev                   # Offset Into Abbrev. Section
         .byte   8                               # Address Size (in bytes)
         .byte   1                               # Abbrev [1] DW_TAG_compile_unit
         .asciz  "Hand-written DWARF"            # DW_AT_producer
         .quad   _ZN1AC2Ev                       # DW_AT_low_pc
         .long   .LZN1AC2Ev_end-_ZN1AC2Ev        # DW_AT_high_pc
+
+# Case 1: The compiler has omitted the declaration of the type, but it still
+# produces an entry for its implicit constructor instantiated in this compile
+# unit.
+# Roughly corresponds to this:
+# struct A {
+#   virtual ~A(); // not defined here
+#   // implicit A() {}
+# } a;
         .byte   2                               # Abbrev [2] DW_TAG_variable
         .asciz  "a"                             # DW_AT_name
         .long   .LA-.Lcu_begin0                 # DW_AT_type
         .byte   9                               # DW_AT_location
         .byte   3
         .quad   a
 .LA:
         .byte   3                               # Abbrev [3] DW_TAG_structure_type
.LAptr:
         .byte   11                              # Abbrev [11] DW_TAG_formal_parameter
         .byte   2                               # DW_AT_location
         .byte   145
         .byte   120
         .asciz  "this"                          # DW_AT_name
         .long   .LAptr-.Lcu_begin0              # DW_AT_type
                                                 # DW_AT_artificial
         .byte   0                               # End Of Children Mark
+
+# Case 2: A structure has been emitted as a declaration only, but it contains a
+# nested class, which has a full definition present.
+# Rougly corresponds to this:
+# struct B {
+#   virtual ~B(); // not defined here
+#   class B1 {
+#     A* ptr;
+#   };
+# };
+# B::B1 b1;
+# Note that it is important that the inner type is a class (not struct) as that
+# triggers a clang assertion.
+        .byte   3                               # Abbrev [3] DW_TAG_structure_type
+        .asciz  "B"                             # DW_AT_name
+                                                # DW_AT_declaration
+.LB1:
+        .byte   6                               # Abbrev [6] DW_TAG_class_type
+        .asciz  "B1"                            # DW_AT_name
+                                                # DW_AT_declaration
+        .byte   7                               # Abbrev [5] 0x58:0xc DW_TAG_member
+        .asciz  "ptr"                           # DW_AT_name
+        .long   .LAptr                          # DW_AT_type
+        .byte   0                               # DW_AT_data_member_location
+        .byte   0                               # End Of Children Mark
+        .byte   0                               # End Of Children Mark
+
+        .byte   2                               # Abbrev [2] DW_TAG_variable
+        .asciz  "b1"                            # DW_AT_name
+        .long   .LB1-.Lcu_begin0                # DW_AT_type
+        .byte   9                               # DW_AT_location
+        .byte   3
+        .quad   b1
+
         .byte   0                               # End Of Children Mark
 .Ldebug_info_end0: