Author: Bar Soloveychik Date: 2026-08-14T09:49:03-07:00 New Revision: bc6c2af342f5cd1905c18be05fd391e44b12ea1f
URL: https://github.com/llvm/llvm-project/commit/bc6c2af342f5cd1905c18be05fd391e44b12ea1f DIFF: https://github.com/llvm/llvm-project/commit/bc6c2af342f5cd1905c18be05fd391e44b12ea1f.diff LOG: [lldb] Cache register types by object UID and register size (#215874) XML type IDs are only unique within a feature, so they cannot identify entries in the target-wide `CompilerType` cache. Assign each `RegisterType` a lifetime-unique UID and combine it with the register size for cache keys. This also prevents a new process from aliasing stale entries when an allocator reuses a RegisterType address. Insert completed enum and flags types into the cache, and clear cached CompilerTypes when the target replaces its scratch `TypeSystemClang`. Add direct builder tests for cache reuse, same-ID definitions, size-sensitive enums, object-address reuse, and scratch type-system replacement. --------- Co-authored-by: Bar Soloveychik <[email protected]> Co-authored-by: David Spickett <[email protected]> Added: lldb/unittests/Target/RegisterTypeBuilderClangTest.cpp Modified: lldb/include/lldb/Target/RegisterTypeBuilder.h lldb/include/lldb/Utility/RegisterType.h lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.cpp lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.h lldb/source/Utility/RegisterType.cpp lldb/unittests/Target/CMakeLists.txt Removed: ################################################################################ diff --git a/lldb/include/lldb/Target/RegisterTypeBuilder.h b/lldb/include/lldb/Target/RegisterTypeBuilder.h index 26b97b6194d49..9a5981af75b08 100644 --- a/lldb/include/lldb/Target/RegisterTypeBuilder.h +++ b/lldb/include/lldb/Target/RegisterTypeBuilder.h @@ -18,6 +18,8 @@ class RegisterTypeBuilder : public PluginInterface { public: ~RegisterTypeBuilder() override = default; + /// Do not cache the returned CompilerType. A target may replace its scratch + /// type system, so callers must request the type again whenever needed. virtual CompilerType GetRegisterType(const RegisterInfo ®_info) = 0; protected: diff --git a/lldb/include/lldb/Utility/RegisterType.h b/lldb/include/lldb/Utility/RegisterType.h index 9ecd6c1dfb639..859de9ee65719 100644 --- a/lldb/include/lldb/Utility/RegisterType.h +++ b/lldb/include/lldb/Utility/RegisterType.h @@ -9,6 +9,7 @@ #ifndef LLDB_UTILITY_REGISTERTYPE_H #define LLDB_UTILITY_REGISTERTYPE_H +#include <cstdint> #include <string> #include <unordered_set> #include <vector> @@ -27,8 +28,11 @@ class RegisterType { RegisterTypeKind getKind() const { return m_kind; } - RegisterType(RegisterTypeKind kind, std::string id) - : m_kind(kind), m_id(std::move(id)) {} + RegisterType(RegisterTypeKind kind, std::string id); + RegisterType(const RegisterType &) = delete; + RegisterType &operator=(const RegisterType &) = delete; + RegisterType(RegisterType &&) = delete; + RegisterType &operator=(RegisterType &&) = delete; /// Output XML that describes this type, to be inserted into a target XML /// file. Reserved characters like "<" are replaced with their XML safe @@ -47,6 +51,11 @@ class RegisterType { const std::string &GetID() const { return m_id; } + /// Return an identifier unique among all RegisterType instances constructed + /// during the lifetime of the LLDB host process. The identifier is not + /// reused after this instance is destroyed. + uint64_t GetUID() const { return m_uid; } + void SetDependencies(std::vector<const RegisterType *> dependencies) { m_dependencies = dependencies; } @@ -54,6 +63,7 @@ class RegisterType { private: const RegisterTypeKind m_kind; const std::string m_id; + const uint64_t m_uid; std::vector<const RegisterType *> m_dependencies; }; diff --git a/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.cpp b/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.cpp index 001cd35604620..7a49c75f181bd 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 diff erent. 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); @@ -82,19 +59,17 @@ RegisterTypeBuilderClang::BuildEnumType(const RegisterTypeEnum &enum_type_info, type_system->CompleteTagDeclarationDefinition(enum_type); + m_type_cache.try_emplace( + std::make_pair(enum_type_info->GetUID(), register_byte_size), enum_type); return enum_type; } 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 +79,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,8 +102,10 @@ 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()); + m_type_cache.try_emplace( + std::make_pair(flags_info->GetUID(), register_byte_size), flags_type); return flags_type; } @@ -138,17 +115,36 @@ RegisterTypeBuilderClang::GetRegisterType(const RegisterInfo ®_info) { ScratchTypeSystemClang::GetForTarget(m_target); assert(type_system); + if (m_cached_type_system.lock() != type_system) { + m_type_cache.clear(); + m_cached_type_system = type_system; + } + 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->GetUID(), 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..8d970367a15b2 100644 --- a/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.h +++ b/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.h @@ -33,15 +33,33 @@ 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 contains the process-wide UID of the type and the size of the + // register we made it for. Some types (enums for example) use the register + // size in their type and must be rebuilt for a diff erent size. + // + // 8 is chosen because types are only made when needed, and most lldb commands + // do not need them. + llvm::SmallDenseMap<std::pair<uint64_t, uint32_t>, CompilerType, 8> + m_type_cache; + std::weak_ptr<TypeSystemClang> m_cached_type_system; + + std::optional<CompilerType> + GetExistingCompilerType(const RegisterType *register_type, + uint32_t register_byte_size); }; } // namespace lldb_private diff --git a/lldb/source/Utility/RegisterType.cpp b/lldb/source/Utility/RegisterType.cpp index 9191831bb52c1..a7de0688cff22 100644 --- a/lldb/source/Utility/RegisterType.cpp +++ b/lldb/source/Utility/RegisterType.cpp @@ -8,8 +8,16 @@ #include "lldb/Utility/RegisterType.h" +#include <atomic> + using namespace lldb_private; +static std::atomic<uint64_t> g_next_register_type_uid{1}; + +RegisterType::RegisterType(RegisterTypeKind kind, std::string id) + : m_kind(kind), m_id(std::move(id)), + m_uid(g_next_register_type_uid.fetch_add(1, std::memory_order_relaxed)) {} + void RegisterType::ToXML( Stream &strm, std::unordered_set<const RegisterType *> &previously_emitted, const RegisterType *user) const { diff --git a/lldb/unittests/Target/CMakeLists.txt b/lldb/unittests/Target/CMakeLists.txt index b12c781cf5789..ebfb06e8fc780 100644 --- a/lldb/unittests/Target/CMakeLists.txt +++ b/lldb/unittests/Target/CMakeLists.txt @@ -9,6 +9,7 @@ add_lldb_unittest(TargetTests MemoryTagMapTest.cpp ModuleCacheTest.cpp PathMappingListTest.cpp + RegisterTypeBuilderClangTest.cpp RemoteAwarePlatformTest.cpp ScratchTypeSystemTest.cpp StackFrameRecognizerTest.cpp @@ -26,8 +27,10 @@ add_lldb_unittest(TargetTests lldbPluginPlatformLinux lldbPluginPlatformMacOSX lldbPluginPlatformAndroid + lldbPluginRegisterTypeBuilderClang lldbPluginSymbolFileBreakpad lldbPluginSymbolFileSymtab + lldbPluginTypeSystemClang lldbTarget lldbSymbol lldbUtility diff --git a/lldb/unittests/Target/RegisterTypeBuilderClangTest.cpp b/lldb/unittests/Target/RegisterTypeBuilderClangTest.cpp new file mode 100644 index 0000000000000..c24c3e35957f9 --- /dev/null +++ b/lldb/unittests/Target/RegisterTypeBuilderClangTest.cpp @@ -0,0 +1,163 @@ +//===-- RegisterTypeBuilderClangTest.cpp ----------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.h" +#include "Plugins/Platform/Linux/PlatformLinux.h" +#include "Plugins/TypeSystem/Clang/TypeSystemClang.h" +#include "TestingSupport/SubsystemRAII.h" +#include "TestingSupport/TestUtilities.h" +#include "lldb/Core/Debugger.h" +#include "lldb/Host/FileSystem.h" +#include "lldb/Host/HostInfo.h" +#include "lldb/Utility/ArchSpec.h" +#include "lldb/Utility/RegisterInfo.h" +#include "lldb/Utility/RegisterTypeFlags.h" +#include "gtest/gtest.h" + +#include <mutex> +#include <optional> + +using namespace lldb; +using namespace lldb_private; + +namespace { + +class RegisterTypeBuilderClangTest : public testing::Test { +public: + SubsystemRAII<FileSystem, HostInfo, TypeSystemClang, + platform_linux::PlatformLinux> + subsystems; + +protected: + void SetUp() override { + std::call_once(TestUtilities::g_debugger_initialize_flag, + []() { Debugger::Initialize(nullptr); }); + ArchSpec host_arch("x86_64-pc-linux"); + Platform::SetHostPlatform( + platform_linux::PlatformLinux::CreateInstance(true, &host_arch)); + m_debugger_sp = Debugger::CreateInstance(); + } + + static RegisterInfo MakeRegisterInfo(const RegisterType &type, + uint32_t byte_size) { + RegisterInfo info{}; + info.name = "test"; + info.byte_size = byte_size; + info.register_type = &type; + return info; + } + + DebuggerSP m_debugger_sp; +}; + +TEST_F(RegisterTypeBuilderClangTest, ReusesCachedType) { + Target &target = m_debugger_sp->GetDummyTarget(); + RegisterTypeFlags flags("flags", 4, + {RegisterTypeFlags::Field("field", 0, 31)}); + RegisterInfo info = MakeRegisterInfo(flags, 4); + RegisterTypeBuilderClang builder(target); + + CompilerType first = builder.GetRegisterType(info); + CompilerType second = builder.GetRegisterType(info); + + ASSERT_TRUE(first); + ASSERT_TRUE(second); + EXPECT_EQ(first, second); +} + +// XML type IDs are scoped to a feature, so separate features may define +// diff erent types using the same ID. +TEST_F(RegisterTypeBuilderClangTest, DistinguishesTypesWithTheSameID) { + Target &target = m_debugger_sp->GetDummyTarget(); + RegisterTypeFlags first_flags("flags", 4, + {RegisterTypeFlags::Field("first", 0, 31)}); + RegisterTypeFlags second_flags("flags", 4, + {RegisterTypeFlags::Field("second", 0, 31)}); + RegisterTypeBuilderClang builder(target); + + CompilerType first = + builder.GetRegisterType(MakeRegisterInfo(first_flags, 4)); + CompilerType second = + builder.GetRegisterType(MakeRegisterInfo(second_flags, 4)); + + ASSERT_TRUE(first); + ASSERT_TRUE(second); + EXPECT_NE(first, second); + + std::string first_field_name; + std::string second_field_name; + ASSERT_TRUE( + first.GetFieldAtIndex(0, first_field_name, nullptr, nullptr, nullptr)); + ASSERT_TRUE( + second.GetFieldAtIndex(0, second_field_name, nullptr, nullptr, nullptr)); + EXPECT_EQ(first_field_name, "first"); + EXPECT_EQ(second_field_name, "second"); +} + +TEST_F(RegisterTypeBuilderClangTest, RegisterSizeIsPartOfCacheKey) { + Target &target = m_debugger_sp->GetDummyTarget(); + RegisterTypeEnum type("enum", {{0, "zero"}, {1, "one"}}); + RegisterTypeBuilderClang builder(target); + + CompilerType four_byte = builder.GetRegisterType(MakeRegisterInfo(type, 4)); + CompilerType eight_byte = builder.GetRegisterType(MakeRegisterInfo(type, 8)); + + ASSERT_TRUE(four_byte); + ASSERT_TRUE(eight_byte); + EXPECT_NE(four_byte, eight_byte); + EXPECT_EQ(llvm::expectedToOptional(four_byte.GetByteSize(nullptr)), 4u); + EXPECT_EQ(llvm::expectedToOptional(eight_byte.GetByteSize(nullptr)), 8u); + EXPECT_EQ(four_byte, builder.GetRegisterType(MakeRegisterInfo(type, 4))); + EXPECT_EQ(eight_byte, builder.GetRegisterType(MakeRegisterInfo(type, 8))); +} + +TEST_F(RegisterTypeBuilderClangTest, DistinguishesReusedObjectAddresses) { + Target &target = m_debugger_sp->GetDummyTarget(); + RegisterTypeBuilderClang builder(target); + std::optional<RegisterTypeEnum> type; + + type.emplace("enum", RegisterTypeEnum::Enumerators{{0, "first"}}); + const RegisterTypeEnum *first_address = &*type; + uint64_t first_uid = type->GetUID(); + CompilerType first = builder.GetRegisterType(MakeRegisterInfo(*type, 4)); + ASSERT_TRUE(first); + + type.reset(); + type.emplace("enum", RegisterTypeEnum::Enumerators{{0, "second"}}); + ASSERT_EQ(first_address, &*type); + ASSERT_NE(first_uid, type->GetUID()); + CompilerType second = builder.GetRegisterType(MakeRegisterInfo(*type, 4)); + + ASSERT_TRUE(second); + EXPECT_NE(first, second); +} + +TEST_F(RegisterTypeBuilderClangTest, CacheFollowsScratchTypeSystem) { + Target &target = m_debugger_sp->GetDummyTarget(); + RegisterTypeEnum type("enum", {{0, "zero"}, {1, "one"}}); + RegisterInfo info = MakeRegisterInfo(type, 4); + RegisterTypeBuilderClang builder(target); + + CompilerType first = builder.GetRegisterType(info); + ASSERT_TRUE(first); + std::shared_ptr<TypeSystemClang> first_type_system = + first.GetTypeSystem<TypeSystemClang>(); + ASSERT_TRUE(first_type_system); + + target.ClearModules(/*delete_locations=*/false); + + CompilerType second = builder.GetRegisterType(info); + ASSERT_TRUE(second); + std::shared_ptr<TypeSystemClang> second_type_system = + second.GetTypeSystem<TypeSystemClang>(); + ASSERT_TRUE(second_type_system); + EXPECT_NE(first_type_system, second_type_system); + EXPECT_NE(first, second); +} + +} // namespace _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
