[Lldb-commits] [lldb] Reapply [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #98361)

[Pavel Labath via lldb-commits]

================
@@ -1631,26 +1638,48 @@ bool SymbolFileDWARF::CompleteType(CompilerType &compiler_type) {
     return true;
   }
 
-  DWARFDIE dwarf_die = GetDIE(die_it->getSecond());
-  if (dwarf_die) {
-    // Once we start resolving this type, remove it from the forward
-    // declaration map in case anyone child members or other types require this
-    // type to get resolved. The type will get resolved when all of the calls
-    // to SymbolFileDWARF::ResolveClangOpaqueTypeDefinition are done.
-    GetForwardDeclCompilerTypeToDIE().erase(die_it);
+  DWARFDIE decl_die = GetDIE(die_it->getSecond());
+  // Once we start resolving this type, remove it from the forward
+  // declaration map in case anyone's child members or other types require this
+  // type to get resolved.
+  GetForwardDeclCompilerTypeToDIE().erase(die_it);
+  DWARFDIE def_die = FindDefinitionDIE(decl_die);
+  if (!def_die) {
+    SymbolFileDWARFDebugMap *debug_map_symfile = GetDebugMapSymfile();
+    if (debug_map_symfile) {
+      // We weren't able to find a full declaration in this DWARF, see
+      // if we have a declaration anywhere else...
+      def_die = debug_map_symfile->FindDefinitionDIE(decl_die);
+    }
+  }
+  if (!def_die) {
+    // No definition found. Proceed with the declaration die. We can use it to
+    // create a forward-declared type.
+    def_die = decl_die;
+  }
 
-    Type *type = GetDIEToType().lookup(dwarf_die.GetDIE());
+  Type *type = ResolveType(def_die);
----------------
labath wrote:

> We need it to do the work when finding existing type in the UniqueDWARFASTTypeMap,

Why do we need to find that type? We already know which type we're completing (we get it as an argument to `SymbolFileDWARF::CompleteType`)...

> It doesn't create a loop or recursion. We need it to do the work when finding existing type in the UniqueDWARFASTTypeMap, which is basically this block: https://github.com/llvm/llvm-project/pull/98361/files/37b6878b9125c314c75053f7d5b0ba520111e9a3#diff-5dd6736e4d6c38623630df16c4494c1a8b099716ee0f05c9af54b4bafb1d864eR1648-R1674.

That's only true if `UniqueDWARFASTTypeMap` functions perfectly and returns the same type we started out with. My point is we shouldn't/don't need to rely on that.

> Those code are important to keep some maps(`GetDIEToType()`, `LinkDeclContextToDIE`, `UniqueDWARFASTTypeMap`) updated

Yes, we need to update those maps, but I think we can/should just update those directly, instead of relying on it to be populated as a side-effect of the second ParseTypeFromDWARF call.  Like, we already have the type and the definition die, so why don't we just do a `GetDIEToType()[defn_die] = type`? And for the UniqueDWARFASTTypeMap, we can use the original declaration die to perform the lookup. This will be both faster (we can add a specialized lookup function that assumes that the lookup die matches one of the `udt.m_die` members) and guaranteed to be correct.

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