[lldb][TypeSystemClang] Fix enum signedness in CompleteEnumType

[Michael137]

I tried using CompleteEnumType to replace some duplicated code in DWARFASTParserClang::ParseEnum but tests started failing.

CompleteEnumType parses/attaches the child enumerators using the signedness it got from CompilerType::IsIntegerType. However, this would only report the correct signedness for builtin integer types (never for clang::EnumTypes). We have a different API for that in CompilerType::IsIntegerOrEnumerationType which could've been used there instead. This patch calls IsEnumerationIntegerTypeSigned to determine signedness because we always pass an enum type into CompleteEnumType anyway.

Based on git history this has been the case for a long time, but possibly never caused issues because ParseEnum was completing the definition manually instead of through CompleteEnumType.

I couldn't find a good way to test CompleteEnumType on its own because it expects an enum type to be passed to it, which only gets created in ParseEnum (at which point we already call CompleteEnumType). The only other place we call CompleteEnumType at is in CompleteTypeFromDWARF. Though I think we don't actually ever end up calling into that codepath because we eagerly complete enum definitions. Maybe we can remove that call to CompleteEnumType in a follow-up.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

I tried using CompleteEnumType to replace some duplicated code in DWARFASTParserClang::ParseEnum but tests started failing.

CompleteEnumType parses/attaches the child enumerators using the signedness it got from CompilerType::IsIntegerType. However, this would only report the correct signedness for builtin integer types (never for clang::EnumTypes). We have a different API for that in CompilerType::IsIntegerOrEnumerationType which could've been used there instead. This patch calls IsEnumerationIntegerTypeSigned to determine signedness because we always pass an enum type into CompleteEnumType anyway.

Based on git history this has been the case for a long time, but possibly never caused issues because ParseEnum was completing the definition manually instead of through CompleteEnumType.

I couldn't find a good way to test CompleteEnumType on its own because it expects an enum type to be passed to it, which only gets created in ParseEnum (at which point we already call CompleteEnumType). The only other place we call CompleteEnumType at is in CompleteTypeFromDWARF. Though I think we don't actually ever end up calling into that codepath because we eagerly complete enum definitions. Maybe we can remove that call to CompleteEnumType in a follow-up.

---

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

1 Files Affected:

• (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp (+6-14)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 6602dd763ba6930..ee99fd6f16cc44d 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -1020,15 +1020,7 @@ TypeSP DWARFASTParserClang::ParseEnum(const SymbolContext &sc,
   }
 
 
-  if (TypeSystemClang::StartTagDeclarationDefinition(clang_type)) {
-    if (def_die.HasChildren()) {
-      bool is_signed = false;
-      enumerator_clang_type.IsIntegerType(is_signed);
-      ParseChildEnumerators(clang_type, is_signed,
-                            type_sp->GetByteSize(nullptr).value_or(0), def_die);
-    }
-    TypeSystemClang::CompleteTagDeclarationDefinition(clang_type);
-  } else {
+  if (!CompleteEnumType(def_die, type_sp.get(), clang_type)) {
     dwarf->GetObjectFile()->GetModule()->ReportError(
         "DWARF DIE at {0:x16} named \"{1}\" was not able to start its "
         "definition.\nPlease file a bug and attach the file at the "
@@ -2221,13 +2213,13 @@ bool DWARFASTParserClang::CompleteRecordType(const DWARFDIE &die,
 bool DWARFASTParserClang::CompleteEnumType(const DWARFDIE &die,
                                            lldb_private::Type *type,
                                            const CompilerType &clang_type) {
+  assert(clang_type.IsEnumerationType());
+
   if (TypeSystemClang::StartTagDeclarationDefinition(clang_type)) {
-    if (die.HasChildren()) {
-      bool is_signed = false;
-      clang_type.IsIntegerType(is_signed);
-      ParseChildEnumerators(clang_type, is_signed,
+    if (die.HasChildren())
+      ParseChildEnumerators(clang_type, clang_type.IsEnumerationIntegerTypeSigned(),
                             type->GetByteSize(nullptr).value_or(0), die);
-    }
+
     TypeSystemClang::CompleteTagDeclarationDefinition(clang_type);
   }
   return (bool)clang_type;

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.