[lldb] Refactor RegisterTypeBuilder

[DavidSpickett]

This prepares it for emitting union types. Major changes:

• Entry function is now a dispatcher to builder functions for each type.
• Name mangling is standardised and the base name is generic.
• The register name parameter is no longer needed and so was removed.
• RegisterInfo is passed around until we need specific fields from it.

[llvmorg-github-actions]

@llvm/pr-subscribers-lldb

Author: David Spickett (DavidSpickett)

Changes

This prepares it for emitting union types. Major changes:

• Entry function is now a dispatcher to builder functions for each type.
• Name mangling is standardised.
• The register name parameter is no longer needed and so was removed.

---

_{Stack created with <a href="https://github.com/github/gh-stack"&gt;GitHub Stacks CLI</a> • <a href="https://gh.io/stacks-feedback"&gt;Give Feedback 💬</a>}

---

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

6 Files Affected:

• (modified) lldb/include/lldb/Target/RegisterTypeBuilder.h (+2-3)
• (modified) lldb/include/lldb/Target/Target.h (+2-3)
• (modified) lldb/source/Core/DumpRegisterValue.cpp (+2-2)
• (modified) lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.cpp (+107-86)
• (modified) lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.h (+12-3)
• (modified) lldb/source/Target/Target.cpp (+2-4)

diff --git a/lldb/include/lldb/Target/RegisterTypeBuilder.h b/lldb/include/lldb/Target/RegisterTypeBuilder.h
index c24d218962e39..5a37109661da0 100644
--- a/lldb/include/lldb/Target/RegisterTypeBuilder.h
+++ b/lldb/include/lldb/Target/RegisterTypeBuilder.h
@@ -19,9 +19,8 @@ class RegisterTypeBuilder : public PluginInterface {
   ~RegisterTypeBuilder() override = default;
 
   virtual CompilerType
-  GetRegisterType(const std::string &name,
-                  const lldb_private::RegisterType &type_info,
-                  uint32_t byte_size) = 0;
+  GetRegisterType(const lldb_private::RegisterType &type_info,
+                  uint32_t register_byte_size) = 0;
 
 protected:
   RegisterTypeBuilder() = default;
diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h
index 39602421cfd96..3e1914513bad4 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -1564,9 +1564,8 @@ class Target : public std::enable_shared_from_this<Target>,
   ///     if none can be found.
   llvm::Expected<lldb_private::Address> GetEntryPointAddress();
 
-  CompilerType GetRegisterType(const std::string &name,
-                               const lldb_private::RegisterType &type_info,
-                               uint32_t byte_size);
+  CompilerType GetRegisterType(const lldb_private::RegisterType &type_info,
+                               uint32_t register_byte_size);
 
   /// Sends a breakpoint notification event.
   void NotifyBreakpointChanged(Breakpoint &bp,
diff --git a/lldb/source/Core/DumpRegisterValue.cpp b/lldb/source/Core/DumpRegisterValue.cpp
index 7096cfec5e11c..4ecaf0f08a693 100644
--- a/lldb/source/Core/DumpRegisterValue.cpp
+++ b/lldb/source/Core/DumpRegisterValue.cpp
@@ -129,8 +129,8 @@ void lldb_private::DumpRegisterValue(const RegisterValue &reg_val, Stream &s,
       (reg_info.byte_size != 4 && reg_info.byte_size != 8))
     return;
 
-  CompilerType register_compiler_type = target_sp->GetRegisterType(
-      reg_info.name, *reg_info.register_type, reg_info.byte_size);
+  CompilerType register_compiler_type =
+      target_sp->GetRegisterType(*reg_info.register_type, reg_info.byte_size);
   if (!register_compiler_type.IsValid())
     return;
 
diff --git a/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.cpp b/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.cpp
index d63c7e2e71bc1..9577a21077805 100644
--- a/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.cpp
+++ b/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.cpp
@@ -8,10 +8,8 @@
 
 #include "clang/AST/DeclCXX.h"
 
-#include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
 #include "RegisterTypeBuilderClang.h"
 #include "lldb/Core/PluginManager.h"
-#include "lldb/Utility/RegisterTypeFlags.h"
 #include "lldb/lldb-enumerations.h"
 
 using namespace lldb_private;
@@ -35,94 +33,117 @@ 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_fields_";
+  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,
+                                        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;
+
+  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->StartTagDeclarationDefinition(enum_type);
+
+  Declaration decl;
+  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);
+  }
+
+  type_system->CompleteTagDeclarationDefinition(enum_type);
+
+  return enum_type;
+}
+
+CompilerType RegisterTypeBuilderClang::BuildFlagsType(
+    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;
+
+  // 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
+  // register's size.
+  CompilerType field_uint_type =
+      type_system->GetBuiltinTypeForEncodingAndBitSize(lldb::eEncodingUint,
+                                                       register_byte_size * 8);
+
+  CompilerType flags_type = type_system->CreateRecordType(
+      nullptr, OptionalClangModuleID(), register_type_name,
+      llvm::to_underlying(clang::TagTypeKind::Struct), lldb::eLanguageTypeC);
+  type_system->StartTagDeclarationDefinition(flags_type);
+
+  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);
+
+    type_system->AddFieldToRecordType(flags_type, field.GetName(), field_type,
+                                      field.GetSizeInBits());
+  }
+
+  type_system->CompleteTagDeclarationDefinition(flags_type);
+  // So that the size of the type matches the size of the register.
+  type_system->SetIsPacked(flags_type);
+
+  // This should be true if RegisterTypeFlags padded correctly.
+  assert(
+      llvm::expectedToOptional(flags_type.GetByteSize(nullptr)).value_or(0) ==
+      flags_info.GetSize());
+
+  return flags_type;
+}
+
 CompilerType RegisterTypeBuilderClang::GetRegisterType(
-    const std::string &name, const lldb_private::RegisterType &type_info,
-    uint32_t byte_size) {
+    const lldb_private::RegisterType &type_info, uint32_t register_byte_size) {
   lldb::TypeSystemClangSP type_system =
       ScratchTypeSystemClang::GetForTarget(m_target);
   assert(type_system);
 
-  std::string register_type_name = "__lldb_register_fields_" + name;
-  // For now we can only build sets of flags.
-  const RegisterTypeFlags *flags =
-      llvm::dyn_cast<RegisterTypeFlags>(&type_info);
-  if (!flags)
-    return {};
-
-  // See if we have made this type before and can reuse it.
-  CompilerType fields_type =
-      type_system->GetTypeForIdentifier<clang::CXXRecordDecl>(
-          type_system->getASTContext(), register_type_name);
-
-  if (!fields_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
-    // register's size.
-    CompilerType field_uint_type =
-        type_system->GetBuiltinTypeForEncodingAndBitSize(lldb::eEncodingUint,
-                                                         byte_size * 8);
-
-    fields_type = type_system->CreateRecordType(
-        nullptr, OptionalClangModuleID(), register_type_name,
-        llvm::to_underlying(clang::TagTypeKind::Struct), lldb::eLanguageTypeC);
-    type_system->StartTagDeclarationDefinition(fields_type);
-
-    // We assume that RegisterTypeFlags has padded and sorted the fields
-    // already.
-    for (const RegisterTypeFlags::Field &field : flags->GetFields()) {
-      CompilerType field_type = field_uint_type;
-
-      if (const RegisterTypeEnum *enum_type = field.GetEnum()) {
-        const RegisterTypeEnum::Enumerators &enumerators =
-            enum_type->GetEnumerators();
-        if (!enumerators.empty()) {
-          // 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.
-          std::string enum_type_name = "__lldb_register_fields_enum_" +
-                                       enum_type->GetID() + "_" +
-                                       std::to_string(byte_size);
-
-          // Enums can be used by mutiple fields and multiple registers, so we
-          // may have built this one already.
-          CompilerType field_enum_type =
-              type_system->GetTypeForIdentifier<clang::EnumDecl>(
-                  type_system->getASTContext(), enum_type_name);
-
-          if (field_enum_type)
-            field_type = field_enum_type;
-          else {
-            field_type = type_system->CreateEnumerationType(
-                enum_type_name, type_system->GetTranslationUnitDecl(),
-                OptionalClangModuleID(), Declaration(), field_uint_type, false);
-
-            type_system->StartTagDeclarationDefinition(field_type);
-
-            Declaration decl;
-            for (auto enumerator : enumerators) {
-              type_system->AddEnumerationValueToEnumerationType(
-                  field_type, decl, enumerator.m_name.c_str(),
-                  enumerator.m_value, byte_size * 8);
-            }
-
-            type_system->CompleteTagDeclarationDefinition(field_type);
-          }
-        }
-      }
-
-      type_system->AddFieldToRecordType(fields_type, field.GetName(),
-                                        field_type, field.GetSizeInBits());
-    }
-
-    type_system->CompleteTagDeclarationDefinition(fields_type);
-    // So that the size of the type matches the size of the register.
-    type_system->SetIsPacked(fields_type);
-
-    // This should be true if RegisterTypeFlags padded correctly.
-    assert(llvm::expectedToOptional(fields_type.GetByteSize(nullptr))
-               .value_or(0) == flags->GetSize());
+  switch (type_info.getKind()) {
+  case RegisterType::eRegisterTypeKindFlags:
+    return BuildFlagsType(*llvm::dyn_cast<RegisterTypeFlags>(&type_info),
+                          register_byte_size, type_system);
+  case RegisterType::eRegisterTypeKindEnum:
+    return BuildEnumType(*llvm::dyn_cast<RegisterTypeEnum>(&type_info),
+                         register_byte_size, type_system);
   }
-
-  return fields_type;
 }
diff --git a/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.h b/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.h
index f00fdc1a5587d..47fff0699b03c 100644
--- a/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.h
+++ b/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.h
@@ -9,8 +9,10 @@
 #ifndef LLDB_SOURCE_PLUGINS_REGISTERTYPEBUILDER_REGISTERTYPEBUILDERCLANG_H
 #define LLDB_SOURCE_PLUGINS_REGISTERTYPEBUILDER_REGISTERTYPEBUILDERCLANG_H
 
+#include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
 #include "lldb/Target/RegisterTypeBuilder.h"
 #include "lldb/Target/Target.h"
+#include "lldb/Utility/RegisterTypeFlags.h"
 
 namespace lldb_private {
 class RegisterTypeBuilderClang : public RegisterTypeBuilder {
@@ -28,11 +30,18 @@ class RegisterTypeBuilderClang : public RegisterTypeBuilder {
   }
   static lldb::RegisterTypeBuilderSP CreateInstance(Target &target);
 
-  CompilerType GetRegisterType(const std::string &name,
-                               const lldb_private::RegisterType &type_info,
-                               uint32_t byte_size) override;
+  CompilerType GetRegisterType(const lldb_private::RegisterType &type_info,
+                               uint32_t register_byte_size) override;
 
 private:
+  CompilerType BuildEnumType(const RegisterTypeEnum &enum_type_info,
+                             uint32_t register_byte_size,
+                             lldb::TypeSystemClangSP type_system);
+
+  CompilerType BuildFlagsType(const RegisterTypeFlags &flags_info,
+                              uint32_t register_byte_size,
+                              lldb::TypeSystemClangSP type_system);
+
   Target &m_target;
 };
 } // namespace lldb_private
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index 283ddb867c93f..ba3c3dadf279a 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -2730,14 +2730,12 @@ Target::GetScratchTypeSystemForLanguage(lldb::LanguageType language,
 }
 
 CompilerType
-Target::GetRegisterType(const std::string &name,
-                        const lldb_private::RegisterType &type_info,
+Target::GetRegisterType(const lldb_private::RegisterType &type_info,
                         uint32_t byte_size) {
   if (!m_register_type_builder_sp)
     m_register_type_builder_sp = PluginManager::GetRegisterTypeBuilder(*this);
   assert(m_register_type_builder_sp);
-  return m_register_type_builder_sp->GetRegisterType(name, type_info,
-                                                     byte_size);
+  return m_register_type_builder_sp->GetRegisterType(type_info, byte_size);
 }
 
 std::vector<lldb::TypeSystemSP>

[DavidSpickett]

This is the last of the refactoring changes.

https://github.com/llvm/llvm-project/pull/196032/commits has prototypes of union and vector as well, if you want to reuse any of that.

[github-actions]

🐧 Linux x64 Test Results

• 34157 tests passed
• 507 tests skipped

✅ The build succeeded and all tests passed.

[barsolo2000]

I might be wrong here but - MakeTypeName caches by kind, ID, and sometimes size. Two valid types from different features may have the same ID and size but different fields/enumerators. Will GetTypeForIdentifier return the first type for both?

[clayborg]

should this be "__lldb_register_type_" instead of fields? Also, do we want make a namespace named __lldb_register_type instead of prepending this to the name? I see we are appending "flags_" and "enum_" below, so maybe this initial typename prefix should just be "__lldb_register_"?

[DavidSpickett]

Yes we should use a generic name, register or type.

I didn't know I could make a namespace, I will try that.

[barsolo2000]

should we cache by RegisterType object identity/UID, not XML ID alone?

[github-actions]

✅ With the latest revision this PR passed the undef deprecator.

[barsolo2000]

Some code formatting issues are left. There still might be cache collisions, since xml id is feature local, but it can be a follow up

[DavidSpickett]

Some code formatting issues are left.

Fixed.

There still might be cache collisions, since xml id is feature local, but it can be a follow up

Yes that's true.

should we cache by RegisterType object identity/UID, not XML ID alone?

I think this is the way to go for now. Especially given that we don't need the string type names for anything yet. Going to try it out now.

[DavidSpickett]

Going to try it out now.

#215578