llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) <details> <summary>Changes</summary> 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::EnumType`s). 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`](https://github.com/llvm/llvm-project/blob/466217eb0334d467ec8e9b96c52ee988aaadc389/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp#L2260-L2262). 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 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; `````````` </details> https://github.com/llvm/llvm-project/pull/125203 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
