[lldb][SymbolFileDWARF] Ignore Declaration when searching through UniqueDWARFASTTypeList in C++

[Michael137]

This addresses an issue encountered when investigating #120569.

In ParseTypeFromDWARF, we insert the parsed type into UniqueDWARFASTTypeMap using the typename and Declaration obtained with GetUniqueTypeNameAndDeclaration. For C++ GetUniqueTypeNameAndDeclaration will zero the Declaration info (presumably because forward declaration may not have it, and for C++, ODR means we can rely on fully qualified typenames for uniqueing). When we then called CompleteType, we would first MapDeclDIEToDefDIE, updating the UniqueDWARFASTType we inserted previously with the Declaration of the definition DIE (without zeroing it). We would then call into ParseTypeFromDWARF for the same type again, and search the UniqueDWARFASTTypeMap. But we do the search using a zeroed declaration we get from GetUniqueTypeNameAndDeclaration.

This particular example was fixed by #120569 but this still feels a little inconsistent. I'm not entirely sure we can get into that situation after that patch anymore. So the only test-case I could come up with was the unit-test which calls MapDeclDIEToDefDIE directly.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

This addresses an issue encountered when investigating #120569.

In ParseTypeFromDWARF, we insert the parsed type into UniqueDWARFASTTypeMap using the typename and Declaration obtained with GetUniqueTypeNameAndDeclaration. For C++ GetUniqueTypeNameAndDeclaration will zero the Declaration info (presumably because forward declaration may not have it, and for C++, ODR means we can rely on fully qualified typenames for uniqueing). When we then called CompleteType, we would first MapDeclDIEToDefDIE, updating the UniqueDWARFASTType we inserted previously with the Declaration of the definition DIE (without zeroing it). We would then call into ParseTypeFromDWARF for the same type again, and search the UniqueDWARFASTTypeMap. But we do the search using a zeroed declaration we get from GetUniqueTypeNameAndDeclaration.

This particular example was fixed by #120569 but this still feels a little inconsistent. I'm not entirely sure we can get into that situation after that patch anymore. So the only test-case I could come up with was the unit-test which calls MapDeclDIEToDefDIE directly.

---

Full diff: https://github.com/llvm/llvm-project/pull/120809.diff

2 Files Affected:

• (modified) lldb/source/Plugins/SymbolFile/DWARF/UniqueDWARFASTType.cpp (+33-10)
• (modified) lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp (+143)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/UniqueDWARFASTType.cpp b/lldb/source/Plugins/SymbolFile/DWARF/UniqueDWARFASTType.cpp
index 3d201e96f92c3c..b598768b6e49f9 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/UniqueDWARFASTType.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/UniqueDWARFASTType.cpp
@@ -7,8 +7,10 @@
 //===----------------------------------------------------------------------===//
 
 #include "UniqueDWARFASTType.h"
+#include "SymbolFileDWARF.h"
 
 #include "lldb/Core/Declaration.h"
+#include "lldb/Target/Language.h"
 
 using namespace lldb_private::dwarf;
 using namespace lldb_private::plugin::dwarf;
@@ -18,6 +20,33 @@ static bool IsStructOrClassTag(llvm::dwarf::Tag Tag) {
          Tag == llvm::dwarf::Tag::DW_TAG_structure_type;
 }
 
+static bool IsSizeAndDeclarationMatching(UniqueDWARFASTType const &udt,
+                                         DWARFDIE const &die,
+                                         const lldb_private::Declaration &decl,
+                                         const int32_t byte_size,
+                                         bool is_forward_declaration) {
+
+  // If they are not both definition DIEs or both declaration DIEs, then
+  // don't check for byte size and declaration location, because declaration
+  // DIEs usually don't have those info.
+  if (udt.m_is_forward_declaration != is_forward_declaration)
+    return true;
+
+  if (udt.m_byte_size > 0 && byte_size > 0 && udt.m_byte_size != byte_size)
+    return false;
+
+  // For C++, we match the behaviour of
+  // DWARFASTParserClang::GetUniqueTypeNameAndDeclaration. We rely on the
+  // one-definition-rule: for a given fully qualified name there exists only one
+  // definition, and there should only be one entry for such name, so ignore
+  // location of where it was declared vs. defined.
+  if (lldb_private::Language::LanguageIsCPlusPlus(
+          SymbolFileDWARF::GetLanguage(*die.GetCU())))
+    return true;
+
+  return udt.m_declaration == decl;
+}
+
 UniqueDWARFASTType *UniqueDWARFASTTypeList::Find(
     const DWARFDIE &die, const lldb_private::Declaration &decl,
     const int32_t byte_size, bool is_forward_declaration) {
@@ -25,17 +54,11 @@ UniqueDWARFASTType *UniqueDWARFASTTypeList::Find(
     // Make sure the tags match
     if (udt.m_die.Tag() == die.Tag() || (IsStructOrClassTag(udt.m_die.Tag()) &&
                                          IsStructOrClassTag(die.Tag()))) {
-      // If they are not both definition DIEs or both declaration DIEs, then
-      // don't check for byte size and declaration location, because declaration
-      // DIEs usually don't have those info.
-      bool matching_size_declaration =
-          udt.m_is_forward_declaration != is_forward_declaration
-              ? true
-              : (udt.m_byte_size < 0 || byte_size < 0 ||
-                 udt.m_byte_size == byte_size) &&
-                    udt.m_declaration == decl;
-      if (!matching_size_declaration)
+
+      if (!IsSizeAndDeclarationMatching(udt, die, decl, byte_size,
+                                        is_forward_declaration))
         continue;
+
       // The type has the same name, and was defined on the same file and
       // line. Now verify all of the parent DIEs match.
       DWARFDIE parent_arg_die = die.GetParent();
diff --git a/lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp b/lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp
index ee90855437a71e..f22d76b3973e5f 100644
--- a/lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp
+++ b/lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp
@@ -598,3 +598,146 @@ TEST_F(DWARFASTParserClangTests, TestDefaultTemplateParamParsing) {
     }
   }
 }
+
+TEST_F(DWARFASTParserClangTests, TestUniqueDWARFASTTypeMap_CppInsertMapFind) {
+  // This tests the behaviour of UniqueDWARFASTTypeMap under
+  // following scenario:
+  // 1. DWARFASTParserClang parses a forward declaration and
+  // inserts it into the UniqueDWARFASTTypeMap.
+  // 2. We then MapDeclDIEToDefDIE which updates the map
+  // entry with the line number/file information of the definition.
+  // 3. Parse the definition DIE, which should return the previously
+  // parsed type from the UniqueDWARFASTTypeMap.
+
+  const char *yamldata = R"(
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_AARCH64
+DWARF:
+  debug_str:
+    - Foo
+
+  debug_line:      
+    - Version:         4
+      MinInstLength:   1
+      MaxOpsPerInst:   1
+      DefaultIsStmt:   1
+      LineBase:        0
+      LineRange:       0
+      Files:           
+        - Name:            main.cpp
+          DirIdx:          0
+          ModTime:         0
+          Length:          0
+
+  debug_abbrev:
+    - ID:              0
+      Table:
+        - Code:            0x01
+          Tag:             DW_TAG_compile_unit
+          Children:        DW_CHILDREN_yes
+          Attributes:
+            - Attribute:       DW_AT_language
+              Form:            DW_FORM_data2
+            - Attribute:       DW_AT_stmt_list
+              Form:            DW_FORM_sec_offset
+        - Code:            0x02
+          Tag:             DW_TAG_structure_type
+          Children:        DW_CHILDREN_no
+          Attributes:
+            - Attribute:       DW_AT_name
+              Form:            DW_FORM_strp
+            - Attribute:       DW_AT_declaration
+              Form:            DW_FORM_flag_present
+        - Code:            0x03
+          Tag:             DW_TAG_structure_type
+          Children:        DW_CHILDREN_no
+          Attributes:
+            - Attribute:       DW_AT_name
+              Form:            DW_FORM_strp
+            - Attribute:       DW_AT_decl_file
+              Form:            DW_FORM_data1
+            - Attribute:       DW_AT_decl_line
+              Form:            DW_FORM_data1
+
+  debug_info:
+    - Version:         5
+      UnitType:        DW_UT_compile
+      AddrSize:        8
+      Entries:
+# 0x0c: DW_TAG_compile_unit
+#         DW_AT_language [DW_FORM_data2]    (DW_LANG_C_plus_plus)
+#         DW_AT_stmt_list [DW_FORM_sec_offset]
+        - AbbrCode:        0x01
+          Values:
+            - Value:           0x04
+            - Value:           0x0000000000000000
+
+# 0x0d:   DW_TAG_structure_type
+#           DW_AT_name [DW_FORM_strp]       (\"Foo\")
+#           DW_AT_declaration [DW_FORM_flag_present] (true)
+        - AbbrCode:        0x02
+          Values:
+            - Value:           0x00
+
+# 0x0f:   DW_TAG_structure_type
+#           DW_AT_name [DW_FORM_strp]       (\"Foo\")
+#           DW_AT_decl_file [DW_FORM_data1] (main.cpp)
+#           DW_AT_decl_line [DW_FORM_data1] (3)
+        - AbbrCode:        0x03
+          Values:
+            - Value:           0x00
+            - Value:           0x01
+            - Value:           0x03
+
+        - AbbrCode:        0x00 # end of child tags of 0x0c
+...
+)";
+  YAMLModuleTester t(yamldata);
+
+  DWARFUnit *unit = t.GetDwarfUnit();
+  ASSERT_NE(unit, nullptr);
+  const DWARFDebugInfoEntry *cu_entry = unit->DIE().GetDIE();
+  ASSERT_EQ(cu_entry->Tag(), DW_TAG_compile_unit);
+  ASSERT_EQ(unit->GetDWARFLanguageType(), DW_LANG_C_plus_plus);
+  DWARFDIE cu_die(unit, cu_entry);
+
+  auto holder = std::make_unique<clang_utils::TypeSystemClangHolder>("ast");
+  auto &ast_ctx = *holder->GetAST();
+  DWARFASTParserClangStub ast_parser(ast_ctx);
+
+  DWARFDIE decl_die;
+  DWARFDIE def_die;
+  for (auto const &die : cu_die.children()) {
+    if (die.Tag() != DW_TAG_structure_type)
+      continue;
+
+    if (die.GetAttributeValueAsOptionalUnsigned(llvm::dwarf::DW_AT_declaration))
+      decl_die = die;
+    else
+      def_die = die;
+  }
+
+  ASSERT_TRUE(decl_die.IsValid());
+  ASSERT_TRUE(def_die.IsValid());
+  ASSERT_NE(decl_die, def_die);
+
+  ParsedDWARFTypeAttributes attrs(def_die);
+  ASSERT_TRUE(attrs.decl.IsValid());
+
+  SymbolContext sc;
+  bool new_type = false;
+  lldb::TypeSP type_sp = ast_parser.ParseTypeFromDWARF(sc, decl_die, &new_type);
+  ASSERT_NE(type_sp, nullptr);
+
+  ast_parser.MapDeclDIEToDefDIE(decl_die, def_die);
+
+  lldb::TypeSP reparsed_type_sp =
+      ast_parser.ParseTypeFromDWARF(sc, def_die, &new_type);
+  ASSERT_NE(reparsed_type_sp, nullptr);
+
+  ASSERT_EQ(type_sp, reparsed_type_sp);
+}

[Michael137]

If we don't want to introduce language specifics into UniqueDWARFASTTypeList, we could just not update MapDeclDIEToDefDIE with the Declaration info of the definition when we're dealing with C++.

On the other hand, assuming the debuggee doesn't violate ODR might be a bit too strong of an assumption for more complex setups.

[ZequanWu]

LGTM, thanks for fixing it.

[ZequanWu]

This particular example was fixed by #120569 but this still feels a little inconsistent. I'm not entirely sure we can get into that situation after that patch anymore. So the only test-case I could come up with was the unit-test which calls MapDeclDIEToDefDIE directly.

That sounds still possible to happens without this change: parse decl die -> complete its type -> update the map with def die and non-zeroed def die -> parse a second decl die for the same type -> without this change, it will fail to find the existing type in the map because zeroed declaration.