[lldb/DWARF] Unique enums parsed from declarations

[labath]

This is a regression from #96484 caught by @ZequanWu.

Note that we will still create separate enum types for types parsed from two definitions. This is different from how we handle classes, but it is not a regression.

I'm also adding the DieToType check to the class type parsing code, although in this case, the type uniqueness should be enforced by the UniqueDWARFASTType map.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Pavel Labath (labath)

Changes

This is a regression from #96484 caught by @ZequanWu.

Note that we will still create separate enum types for types parsed from two definitions. This is different from how we handle classes, but it is not a regression.

I'm also adding the DieToType check to the class type parsing code, although in this case, the type uniqueness should be enforced by the UniqueDWARFASTType map.

---

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

2 Files Affected:

• (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp (+14)
• (added) lldb/test/Shell/SymbolFile/DWARF/enum-declaration-uniqueness.cpp (+32)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index f36e2af9589b8..c4a3b432ba949 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -870,6 +870,13 @@ TypeSP DWARFASTParserClang::ParseEnum(const SymbolContext &sc,
     }
   }
   if (def_die) {
+    if (auto [it, inserted] = dwarf->GetDIEToType().try_emplace(
+            def_die.GetDIE(), DIE_IS_BEING_PARSED);
+        !inserted) {
+      if (it->getSecond() == nullptr || it->getSecond() == DIE_IS_BEING_PARSED)
+        return nullptr;
+      return it->getSecond()->shared_from_this();
+    }
     attrs = ParsedDWARFTypeAttributes(def_die);
   } else {
     // No definition found. Proceed with the declaration die. We can use it to
@@ -1798,6 +1805,13 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
   }
 
   if (def_die) {
+    if (auto [it, inserted] = dwarf->GetDIEToType().try_emplace(
+            def_die.GetDIE(), DIE_IS_BEING_PARSED);
+        !inserted) {
+      if (it->getSecond() == nullptr || it->getSecond() == DIE_IS_BEING_PARSED)
+        return nullptr;
+      return it->getSecond()->shared_from_this();
+    }
     attrs = ParsedDWARFTypeAttributes(def_die);
   } else {
     // No definition found. Proceed with the declaration die. We can use it to
diff --git a/lldb/test/Shell/SymbolFile/DWARF/enum-declaration-uniqueness.cpp b/lldb/test/Shell/SymbolFile/DWARF/enum-declaration-uniqueness.cpp
new file mode 100644
index 0000000000000..6fc47d7e4b53a
--- /dev/null
+++ b/lldb/test/Shell/SymbolFile/DWARF/enum-declaration-uniqueness.cpp
@@ -0,0 +1,32 @@
+// RUN: %clangxx_host -gdwarf -c -o %t_a.o %s -DFILE_A
+// RUN: %clangxx_host -gdwarf -c -o %t_b.o %s -DFILE_B
+// RUN: %clangxx_host -o %t %t_a.o %t_b.o
+// RUN: %lldb %t \
+// RUN:   -o "target variable my_enum my_enum_ref" -o "image dump ast" \
+// RUN:   -o exit | FileCheck %s
+
+
+// CHECK: (lldb) target variable
+// CHECK: (MyEnum) my_enum = MyEnum_A
+// CHECK: (MyEnum &) my_enum_ref =
+// CHECK-SAME: &::my_enum_ref = MyEnum_A
+
+// CHECK: (lldb) image dump ast
+// CHECK: EnumDecl {{.*}} MyEnum
+// CHECK-NEXT: EnumConstantDecl {{.*}} MyEnum_A 'MyEnum'
+// CHECK-NOT: MyEnum
+
+enum MyEnum : int;
+
+extern MyEnum my_enum;
+
+#ifdef FILE_A
+enum MyEnum : int { MyEnum_A };
+
+MyEnum my_enum = MyEnum_A;
+
+int main() {}
+#endif
+#ifdef FILE_B
+MyEnum &my_enum_ref = my_enum;
+#endif

[dwblaikie]

any chance of reducing/avoiding this duplication? Looks like the same code here as above? (refactor into a common function?)

[labath]

It's possible, but I am not sure if it's worth it, as this code will go away (or at least change substantially) when we switch to lazy definition lookups.

[ZequanWu]

This part seems unnecessary as UniqueDWARFASTType map already handles it, right?

[labath]

For uniqueness, yes the unique map should already catch that (though, like we've seen, it can easily have bugs). However, it's still useful for preventing (infinite) recursion.