https://github.com/DavidSpickett created https://github.com/llvm/llvm-project/pull/215578
Follow up to #213897. Where it was pointed out that the type name based caching would break if multiple `<feature>` elements contained a type with the same ID, subtype and size. For example: ``` <feature name="A"> <flags id="abc" <...> /> <register <...> size="4" type="abc"/> </feature> <feature name="B"> <flags id="abc" <...> /> <register <...> size="4" type="abc"/> </feature> ``` If asked for A::abc first, the builder would reuse it for B::abc even though B::abc might be a different set of flags. Vice versa if B::abc is requested first. This could be something like feature A is the unprivleged set of registers and B is the privleged set. Same names, same sizes, different contents. The type builder has no notion of `<feature>`s, and I don't think it should at least right now. Later we could consider modelling features as namespaces, something along those lines. A simple solution for now is to cache based on register type object address and the size of register it was created for. Doing away with the type names entirely. We may want type names for later features but right now all they were used for was this cache lookup. Note that due to https://github.com/llvm/llvm-project/issues/214444, this bug cannot be triggered yet. Hence there are no new tests in this change. It should get tested indirectly by the tests for the eventual fix for that issue. >From 29503c074c8d6d2747f4e542c10fb6100284870c Mon Sep 17 00:00:00 2001 From: David Spickett <[email protected]> Date: Tue, 11 Aug 2026 14:33:35 +0000 Subject: [PATCH] [lldb] Cache register types based on object address and register size Follow up to #213897. Where it was pointed out that the type name based caching would break if multiple `<feature>` elements contained a type with the same ID, subtype and size. For example: ``` <feature name="A"> <flags id="abc" <...> /> <register <...> size="4" type="abc"/> </feature> <feature name="B"> <flags id="abc" <...> /> <register <...> size="4" type="abc"/> </feature> ``` If asked for A::abc first, the builder would reuse it for B::abc even though B::abc might be a different set of flags. Vice versa if B::abc is requested first. This could be something like feature A is the unprivleged set of registers and B is the privleged set. Same names, same sizes, different contents. The type builder has no notion of `<feature>`s, and I don't think it should at least right now. Later we could consider modelling features as namespaces, something along those lines. A simple solution for now is to cache based on register type object address and the size of register it was created for. Doing away with the type names entirely. We may want type names for later features but right now all they were used for was this cache lookup. Note that due to https://github.com/llvm/llvm-project/issues/214444, this bug cannot be triggered yet. Hence there are no new tests in this change. It should get tested indirectly by the tests for the eventual fix for that issue. --- .../RegisterTypeBuilderClang.cpp | 74 ++++++++----------- .../RegisterTypeBuilderClang.h | 24 +++++- 2 files changed, 52 insertions(+), 46 deletions(-) diff --git a/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.cpp b/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.cpp index 001cd35604620..e567a24d7e50d 100644 --- a/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.cpp +++ b/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.cpp @@ -33,48 +33,25 @@ RegisterTypeBuilderClang::CreateInstance(Target &target) { RegisterTypeBuilderClang::RegisterTypeBuilderClang(Target &target) : m_target(target) {} -static std::string MakeTypeName(const RegisterType &type_info, - uint32_t register_byte_size) { - std::string type_name = "__lldb_register_"; - switch (type_info.getKind()) { - case RegisterType::eRegisterTypeKindFlags: - type_name += "flags_"; - break; - case RegisterType::eRegisterTypeKindEnum: - // Enums can be used by many registers and the size of each register - // may be different. The register size is used as the underlying size - // of the enumerators, so we must make one enum type per register size - // it is used with. - type_name += "enum_" + std::to_string(register_byte_size) + "_"; - break; - } - - return type_name + type_info.GetID(); -} - CompilerType -RegisterTypeBuilderClang::BuildEnumType(const RegisterTypeEnum &enum_type_info, +RegisterTypeBuilderClang::BuildEnumType(const RegisterTypeEnum *enum_type_info, uint32_t register_byte_size, lldb::TypeSystemClangSP type_system) { - std::string enum_type_name = MakeTypeName(enum_type_info, register_byte_size); - - // Reuse existing type if we can. - if (CompilerType enum_type = - type_system->GetTypeForIdentifier<clang::EnumDecl>( - type_system->getASTContext(), enum_type_name)) - return enum_type; + if (auto maybe_compiler_type = + GetExistingCompilerType(enum_type_info, register_byte_size)) + return *maybe_compiler_type; CompilerType register_uint_type = type_system->GetBuiltinTypeForEncodingAndBitSize(lldb::eEncodingUint, register_byte_size * 8); CompilerType enum_type = type_system->CreateEnumerationType( - enum_type_name, type_system->GetTranslationUnitDecl(), - OptionalClangModuleID(), Declaration(), register_uint_type, false); + "", type_system->GetTranslationUnitDecl(), OptionalClangModuleID(), + Declaration(), register_uint_type, false); type_system->StartTagDeclarationDefinition(enum_type); Declaration decl; - for (const auto &enumerator : enum_type_info.GetEnumerators()) { + for (const auto &enumerator : enum_type_info->GetEnumerators()) { type_system->AddEnumerationValueToEnumerationType( enum_type, decl, enumerator.m_name.c_str(), enumerator.m_value, register_byte_size * 8); @@ -86,15 +63,11 @@ RegisterTypeBuilderClang::BuildEnumType(const RegisterTypeEnum &enum_type_info, } CompilerType RegisterTypeBuilderClang::BuildFlagsType( - const lldb_private::RegisterTypeFlags &flags_info, + const lldb_private::RegisterTypeFlags *flags_info, uint32_t register_byte_size, lldb::TypeSystemClangSP type_system) { - std::string register_type_name = MakeTypeName(flags_info, register_byte_size); - - // Reuse existing type if we can. - if (CompilerType flags_type = - type_system->GetTypeForIdentifier<clang::CXXRecordDecl>( - type_system->getASTContext(), register_type_name)) - return flags_type; + if (auto maybe_compiler_type = + GetExistingCompilerType(flags_info, register_byte_size)) + return *maybe_compiler_type; // In most ABI, a change of field type means a change in storage unit. // We want it all in one unit, so we use a field type the same as the @@ -104,17 +77,17 @@ CompilerType RegisterTypeBuilderClang::BuildFlagsType( register_byte_size * 8); CompilerType flags_type = type_system->CreateRecordType( - nullptr, OptionalClangModuleID(), register_type_name, + nullptr, OptionalClangModuleID(), "", llvm::to_underlying(clang::TagTypeKind::Struct), lldb::eLanguageTypeC); type_system->StartTagDeclarationDefinition(flags_type); - for (auto field : flags_info.GetFields()) { + for (auto field : flags_info->GetFields()) { CompilerType field_type = field_uint_type; if (const RegisterTypeEnum *enum_type_info = field.GetEnum()) if (!enum_type_info->GetEnumerators().empty()) field_type = - BuildEnumType(*enum_type_info, register_byte_size, type_system); + BuildEnumType(enum_type_info, register_byte_size, type_system); type_system->AddFieldToRecordType(flags_type, field.GetName(), field_type, field.GetSizeInBits()); @@ -127,7 +100,7 @@ CompilerType RegisterTypeBuilderClang::BuildFlagsType( // This should be true if RegisterTypeFlags padded correctly. assert( llvm::expectedToOptional(flags_type.GetByteSize(nullptr)).value_or(0) == - flags_info.GetSize()); + flags_info->GetSize()); return flags_type; } @@ -141,14 +114,27 @@ RegisterTypeBuilderClang::GetRegisterType(const RegisterInfo ®_info) { if (!reg_info.register_type) return CompilerType(); + // Note that we do not check the type cache here because types can be nested. + // There is a cache check in each of the Build<subtype> methods, and those + // methods may call each other (Flags may use Enums for example). + switch (reg_info.register_type->getKind()) { case RegisterType::eRegisterTypeKindFlags: return BuildFlagsType( - *llvm::dyn_cast<RegisterTypeFlags>(reg_info.register_type), + llvm::dyn_cast<RegisterTypeFlags>(reg_info.register_type), reg_info.byte_size, type_system); case RegisterType::eRegisterTypeKindEnum: return BuildEnumType( - *llvm::dyn_cast<RegisterTypeEnum>(reg_info.register_type), + llvm::dyn_cast<RegisterTypeEnum>(reg_info.register_type), reg_info.byte_size, type_system); } } + +std::optional<CompilerType> RegisterTypeBuilderClang::GetExistingCompilerType( + const RegisterType *register_type, uint32_t register_byte_size) { + auto cached = m_type_cache.find({register_type, register_byte_size}); + if (cached != m_type_cache.end()) + return cached->second; + + return {}; +} diff --git a/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.h b/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.h index ae2bea69c70a8..d9d8d1fc295f9 100644 --- a/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.h +++ b/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.h @@ -33,15 +33,35 @@ class RegisterTypeBuilderClang : public RegisterTypeBuilder { CompilerType GetRegisterType(const RegisterInfo ®_info) override; private: - CompilerType BuildEnumType(const RegisterTypeEnum &enum_type_info, + CompilerType BuildEnumType(const RegisterTypeEnum *enum_type_info, uint32_t register_byte_size, lldb::TypeSystemClangSP type_system); - CompilerType BuildFlagsType(const RegisterTypeFlags &flags_info, + CompilerType BuildFlagsType(const RegisterTypeFlags *flags_info, uint32_t register_byte_size, lldb::TypeSystemClangSP type_system); Target &m_target; + + // A cache of previously created types. We do not cache by element ID because + // IDs are not unique across xml <feature> elements and this class does not + // know anything about features. + // + // The key is the address of the type we built, and the size of the register + // we made it for. We assume the address is unique, and we need the + // size because some types (enums for example) use the size of the + // register in their type. They must be made again if the requested size is + // different. + // + // 8 is chosen because types are only made when needed, and most lldb commands + // do not need them. + llvm::SmallDenseMap<std::pair<const RegisterType *, uint32_t>, CompilerType, + 8> + m_type_cache; + + std::optional<CompilerType> + GetExistingCompilerType(const RegisterType *register_type, + uint32_t register_byte_size); }; } // namespace lldb_private _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
