[Lldb-commits] [lldb] [lldb/DWARF] Don't start class definitions in ParseStructureLikeDIE (PR #96755)

[Pavel Labath via lldb-commits]

================
@@ -1893,72 +1849,21 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
   dwarf->GetUniqueDWARFASTTypeMap().Insert(unique_typename,
                                            *unique_ast_entry_up);
 
-  if (!attrs.is_forward_declaration) {
-    // Always start the definition for a class type so that if the class
-    // has child classes or types that require the class to be created
-    // for use as their decl contexts the class will be ready to accept
-    // these child definitions.
-    if (!def_die.HasChildren()) {
-      // No children for this struct/union/class, lets finish it
-      if (TypeSystemClang::StartTagDeclarationDefinition(clang_type)) {
-        TypeSystemClang::CompleteTagDeclarationDefinition(clang_type);
-      } else {
-        dwarf->GetObjectFile()->GetModule()->ReportError(
-
-            "DWARF DIE {0:x16} named \"{1}\" was not able to start its "
-            "definition.\nPlease file a bug and attach the file at the "
-            "start of this error message",
-            def_die.GetID(), attrs.name.GetCString());
-      }
-
-      // Setting authority byte size and alignment for empty structures.
-      //
-      // If the byte size or alignmenet of the record is specified then
-      // overwrite the ones that would be computed by Clang.
-      // This is only needed as LLDB's TypeSystemClang is always in C++ mode,
-      // but some compilers such as GCC and Clang give empty structs a size of 0
-      // in C mode (in contrast to the size of 1 for empty structs that would be
-      // computed in C++ mode).
-      if (attrs.byte_size || attrs.alignment) {
-        clang::RecordDecl *record_decl =
-            TypeSystemClang::GetAsRecordDecl(clang_type);
-        if (record_decl) {
-          ClangASTImporter::LayoutInfo layout;
-          layout.bit_size = attrs.byte_size.value_or(0) * 8;
-          layout.alignment = attrs.alignment.value_or(0) * 8;
-          GetClangASTImporter().SetRecordLayout(record_decl, layout);
-        }
-      }
-    } else if (clang_type_was_created) {
-      // Start the definition if the class is not objective C since the
-      // underlying decls respond to isCompleteDefinition(). Objective
-      // C decls don't respond to isCompleteDefinition() so we can't
-      // start the declaration definition right away. For C++
-      // class/union/structs we want to start the definition in case the
-      // class is needed as the declaration context for a contained class
-      // or type without the need to complete that type..
-
-      if (attrs.class_language != eLanguageTypeObjC &&
-          attrs.class_language != eLanguageTypeObjC_plus_plus)
-        TypeSystemClang::StartTagDeclarationDefinition(clang_type);
-
-      // Leave this as a forward declaration until we need to know the
-      // details of the type. lldb_private::Type will automatically call
-      // the SymbolFile virtual function
-      // "SymbolFileDWARF::CompleteType(Type *)" When the definition
-      // needs to be defined.
-      assert(!dwarf->GetForwardDeclCompilerTypeToDIE().count(
-                 ClangUtil::RemoveFastQualifiers(clang_type)
-                     .GetOpaqueQualType()) &&
-             "Type already in the forward declaration map!");
-      // Can't assume m_ast.GetSymbolFile() is actually a
-      // SymbolFileDWARF, it can be a SymbolFileDWARFDebugMap for Apple
-      // binaries.
-      dwarf->GetForwardDeclCompilerTypeToDIE().try_emplace(
-          ClangUtil::RemoveFastQualifiers(clang_type).GetOpaqueQualType(),
-          *def_die.GetDIERef());
-      m_ast.SetHasExternalStorage(clang_type.GetOpaqueQualType(), true);
-    }
+  if (clang_type_was_created) {
----------------
labath wrote:

I don't think so, or rather like, if it does, I would say that's a bug somewhere else, as an empty class should be just a special case of "a class". You could think of this as an optimization, but I'd be surprised if it made a difference -- our performance problems are coming from classes that in fact have members. (Also, die.HasChildren() is not a very reliable indicator of an empty class, as e.g. an empty template class will still have children)

https://github.com/llvm/llvm-project/pull/96755