[lldb] Remove all the RegisterInfo name constification code

RegisterInfo's `reg_name`/`reg_alt_name` fields are C-Strings and are supposed
to only be generated from a ConstString. The reason for that is that
`DynamicRegisterInfo::GetRegisterInfo` and
`RegInfoBasedABI::GetRegisterInfoByName` try to optimise finding registers by
name by only comparing the C string pointer values instead of the underlying
strings. This only works if both C strings involved in the comparison come from
a ConstString. If one of the two C strings doesn't come from a ConstString the
comparison won't work (and most likely will silently fail).

I added an assert in b0060c3a7868ef026d95d0cf8a076791ef74f474 which checks that
both strings come from a ConstString. Apparently not all ABI plugins are
generating their register names via ConstString, so this code is now not just
silently failing but also asserting.

In D88375 we did a shady fix for the MIPS plugins by just copying the
ConstString setup code to that plugin, but we still need to fix ABISysV_arc,
ABISysV_ppc and ABISysV_ppc64 plugins.

I would say we just fix the remaining plugins by removing the whole requirement
to have the register names coming from ConstStrings. I really doubt that we
actually save any time with the whole ConstString search trick (searching ~50
strings that have <4 characters doesn't sound more expensive than calling the
really expensive ConstString constructor + comparing the same amount of pointer
values). Also whatever small percentage of LLDB's runtime is actually spend in
this function is anyway not worth the complexity of this approach.

This patch just removes all this and just does a normal string comparison.

Reviewed By: JDevlieghere, labath

Differential Revision: https://reviews.llvm.org/D88490

diff --git a/lldb/include/lldb/Target/ABI.h b/lldb/include/lldb/Target/ABI.h
index b252e4b..131b2ea 100644 (file)
--- a/lldb/include/lldb/Target/ABI.h
+++ b/lldb/include/lldb/Target/ABI.h
@@ -159,7 +159,7 @@ public:
 protected:
   using ABI::ABI;
 
-  bool GetRegisterInfoByName(ConstString name, RegisterInfo &info);
+  bool GetRegisterInfoByName(llvm::StringRef name, RegisterInfo &info);
 
   virtual const RegisterInfo *GetRegisterInfoArray(uint32_t &count) = 0;
 };

diff --git a/lldb/source/Plugins/ABI/ARM/ABIMacOSX_arm.cpp b/lldb/source/Plugins/ABI/ARM/ABIMacOSX_arm.cpp
index ef500cb..06c4590 100644 (file)
--- a/lldb/source/Plugins/ABI/ARM/ABIMacOSX_arm.cpp
+++ b/lldb/source/Plugins/ABI/ARM/ABIMacOSX_arm.cpp
@@ -34,7 +34,7 @@
 using namespace lldb;
 using namespace lldb_private;
 
-static RegisterInfo g_register_infos[] = {
+static const RegisterInfo g_register_infos[] = {
     //  NAME       ALT       SZ OFF ENCODING         FORMAT          EH_FRAME
     //  DWARF               GENERIC                     PROCESS PLUGIN
     //  LLDB NATIVE
@@ -1292,24 +1292,9 @@ static RegisterInfo g_register_infos[] = {
 
 static const uint32_t k_num_register_infos =
     llvm::array_lengthof(g_register_infos);
-static bool g_register_info_names_constified = false;
 
 const lldb_private::RegisterInfo *
 ABIMacOSX_arm::GetRegisterInfoArray(uint32_t &count) {
-  // Make the C-string names and alt_names for the register infos into const
-  // C-string values by having the ConstString unique the names in the global
-  // constant C-string pool.
-  if (!g_register_info_names_constified) {
-    g_register_info_names_constified = true;
-    for (uint32_t i = 0; i < k_num_register_infos; ++i) {
-      if (g_register_infos[i].name)
-        g_register_infos[i].name =
-            ConstString(g_register_infos[i].name).GetCString();
-      if (g_register_infos[i].alt_name)
-        g_register_infos[i].alt_name =
-            ConstString(g_register_infos[i].alt_name).GetCString();
-    }
-  }
   count = k_num_register_infos;
   return g_register_infos;
 }

diff --git a/lldb/source/Plugins/ABI/ARM/ABISysV_arm.cpp b/lldb/source/Plugins/ABI/ARM/ABISysV_arm.cpp
index c63c569..26b3152 100644 (file)
--- a/lldb/source/Plugins/ABI/ARM/ABISysV_arm.cpp
+++ b/lldb/source/Plugins/ABI/ARM/ABISysV_arm.cpp
@@ -36,7 +36,7 @@ using namespace lldb_private;
 
 LLDB_PLUGIN_DEFINE(ABISysV_arm)
 
-static RegisterInfo g_register_infos[] = {
+static const RegisterInfo g_register_infos[] = {
     //  NAME       ALT       SZ OFF ENCODING         FORMAT          EH_FRAME
     //  DWARF               GENERIC                     PROCESS PLUGIN
     //  LLDB NATIVE            VALUE REGS    INVALIDATE REGS
@@ -1295,24 +1295,9 @@ static RegisterInfo g_register_infos[] = {
 
 static const uint32_t k_num_register_infos =
     llvm::array_lengthof(g_register_infos);
-static bool g_register_info_names_constified = false;
 
 const lldb_private::RegisterInfo *
 ABISysV_arm::GetRegisterInfoArray(uint32_t &count) {
-  // Make the C-string names and alt_names for the register infos into const
-  // C-string values by having the ConstString unique the names in the global
-  // constant C-string pool.
-  if (!g_register_info_names_constified) {
-    g_register_info_names_constified = true;
-    for (uint32_t i = 0; i < k_num_register_infos; ++i) {
-      if (g_register_infos[i].name)
-        g_register_infos[i].name =
-            ConstString(g_register_infos[i].name).GetCString();
-      if (g_register_infos[i].alt_name)
-        g_register_infos[i].alt_name =
-            ConstString(g_register_infos[i].alt_name).GetCString();
-    }
-  }
   count = k_num_register_infos;
   return g_register_infos;
 }

diff --git a/lldb/source/Plugins/ABI/Hexagon/ABISysV_hexagon.cpp b/lldb/source/Plugins/ABI/Hexagon/ABISysV_hexagon.cpp
index 32313d4..47aaefd 100644 (file)
--- a/lldb/source/Plugins/ABI/Hexagon/ABISysV_hexagon.cpp
+++ b/lldb/source/Plugins/ABI/Hexagon/ABISysV_hexagon.cpp
@@ -34,7 +34,7 @@ using namespace lldb_private;
 
 LLDB_PLUGIN_DEFINE_ADV(ABISysV_hexagon, ABIHexagon)
 
-static RegisterInfo g_register_infos[] = {
+static const RegisterInfo g_register_infos[] = {
     // hexagon-core.xml
     {"r00",
      "",
@@ -974,24 +974,9 @@ static RegisterInfo g_register_infos[] = {
 
 static const uint32_t k_num_register_infos =
     sizeof(g_register_infos) / sizeof(RegisterInfo);
-static bool g_register_info_names_constified = false;
 
 const lldb_private::RegisterInfo *
 ABISysV_hexagon::GetRegisterInfoArray(uint32_t &count) {
-  // Make the C-string names and alt_names for the register infos into const
-  // C-string values by having the ConstString unique the names in the global
-  // constant C-string pool.
-  if (!g_register_info_names_constified) {
-    g_register_info_names_constified = true;
-    for (uint32_t i = 0; i < k_num_register_infos; ++i) {
-      if (g_register_infos[i].name)
-        g_register_infos[i].name =
-            ConstString(g_register_infos[i].name).GetCString();
-      if (g_register_infos[i].alt_name)
-        g_register_infos[i].alt_name =
-            ConstString(g_register_infos[i].alt_name).GetCString();
-    }
-  }
   count = k_num_register_infos;
   return g_register_infos;
 }

diff --git a/lldb/source/Plugins/ABI/Mips/ABISysV_mips.cpp b/lldb/source/Plugins/ABI/Mips/ABISysV_mips.cpp
index a209fa7..d66e092 100644 (file)
--- a/lldb/source/Plugins/ABI/Mips/ABISysV_mips.cpp
+++ b/lldb/source/Plugins/ABI/Mips/ABISysV_mips.cpp
@@ -75,7 +75,7 @@ enum dwarf_regnums {
   dwarf_pc
 };
 
-static RegisterInfo g_register_infos[] = {
+static const RegisterInfo g_register_infos[] = {
     //  NAME      ALT    SZ OFF ENCODING        FORMAT         EH_FRAME
     //  DWARF                   GENERIC                     PROCESS PLUGINS
     //  LLDB NATIVE            VALUE REGS  INVALIDATE REGS
@@ -542,24 +542,9 @@ static RegisterInfo g_register_infos[] = {
 
 static const uint32_t k_num_register_infos =
     llvm::array_lengthof(g_register_infos);
-static bool g_register_info_names_constified = false;
 
 const lldb_private::RegisterInfo *
 ABISysV_mips::GetRegisterInfoArray(uint32_t &count) {
-  // Make the C-string names and alt_names for the register infos into const
-  // C-string values by having the ConstString unique the names in the global
-  // constant C-string pool.
-  if (!g_register_info_names_constified) {
-    g_register_info_names_constified = true;
-    for (uint32_t i = 0; i < k_num_register_infos; ++i) {
-      if (g_register_infos[i].name)
-        g_register_infos[i].name =
-            ConstString(g_register_infos[i].name).GetCString();
-      if (g_register_infos[i].alt_name)
-        g_register_infos[i].alt_name =
-            ConstString(g_register_infos[i].alt_name).GetCString();
-    }
-  }
   count = k_num_register_infos;
   return g_register_infos;
 }

diff --git a/lldb/source/Plugins/ABI/Mips/ABISysV_mips64.cpp b/lldb/source/Plugins/ABI/Mips/ABISysV_mips64.cpp
index 9a07c33..7515557 100644 (file)
--- a/lldb/source/Plugins/ABI/Mips/ABISysV_mips64.cpp
+++ b/lldb/source/Plugins/ABI/Mips/ABISysV_mips64.cpp
@@ -75,7 +75,7 @@ enum dwarf_regnums {
   dwarf_pc
 };
 
-static RegisterInfo g_register_infos_mips64[] = {
+static const RegisterInfo g_register_infos_mips64[] = {
     //  NAME      ALT    SZ OFF ENCODING        FORMAT         EH_FRAME
     //  DWARF                   GENERIC                     PROCESS PLUGIN
     //  LLDB NATIVE
@@ -542,24 +542,9 @@ static RegisterInfo g_register_infos_mips64[] = {
 
 static const uint32_t k_num_register_infos =
     llvm::array_lengthof(g_register_infos_mips64);
-static bool g_register_info_names_constified = false;
 
 const lldb_private::RegisterInfo *
 ABISysV_mips64::GetRegisterInfoArray(uint32_t &count) {
-  // Make the C-string names and alt_names for the register infos into const
-  // C-string values by having the ConstString unique the names in the global
-  // constant C-string pool.
-  if (!g_register_info_names_constified) {
-    g_register_info_names_constified = true;
-    for (uint32_t i = 0; i < k_num_register_infos; ++i) {
-      if (g_register_infos_mips64[i].name)
-        g_register_infos_mips64[i].name =
-            ConstString(g_register_infos_mips64[i].name).GetCString();
-      if (g_register_infos_mips64[i].alt_name)
-        g_register_infos_mips64[i].alt_name =
-            ConstString(g_register_infos_mips64[i].alt_name).GetCString();
-    }
-  }
   count = k_num_register_infos;
   return g_register_infos_mips64;
 }

diff --git a/lldb/source/Plugins/ABI/SystemZ/ABISysV_s390x.cpp b/lldb/source/Plugins/ABI/SystemZ/ABISysV_s390x.cpp
index 29ad4d9..22a6417 100644 (file)
--- a/lldb/source/Plugins/ABI/SystemZ/ABISysV_s390x.cpp
+++ b/lldb/source/Plugins/ABI/SystemZ/ABISysV_s390x.cpp
@@ -118,7 +118,7 @@ enum dwarf_regnums {
          nullptr, nullptr, nullptr, 0                                          \
   }
 
-static RegisterInfo g_register_infos[] = {
+static const RegisterInfo g_register_infos[] = {
     DEFINE_REG(r0, 8, nullptr, LLDB_INVALID_REGNUM),
     DEFINE_REG(r1, 8, nullptr, LLDB_INVALID_REGNUM),
     DEFINE_REG(r2, 8, "arg1", LLDB_REGNUM_GENERIC_ARG1),
@@ -173,24 +173,9 @@ static RegisterInfo g_register_infos[] = {
 
 static const uint32_t k_num_register_infos =
     llvm::array_lengthof(g_register_infos);
-static bool g_register_info_names_constified = false;
 
 const lldb_private::RegisterInfo *
 ABISysV_s390x::GetRegisterInfoArray(uint32_t &count) {
-  // Make the C-string names and alt_names for the register infos into const
-  // C-string values by having the ConstString unique the names in the global
-  // constant C-string pool.
-  if (!g_register_info_names_constified) {
-    g_register_info_names_constified = true;
-    for (uint32_t i = 0; i < k_num_register_infos; ++i) {
-      if (g_register_infos[i].name)
-        g_register_infos[i].name =
-            ConstString(g_register_infos[i].name).GetCString();
-      if (g_register_infos[i].alt_name)
-        g_register_infos[i].alt_name =
-            ConstString(g_register_infos[i].alt_name).GetCString();
-    }
-  }
   count = k_num_register_infos;
   return g_register_infos;
 }

diff --git a/lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp b/lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp
index 443638a..fd9e192 100644 (file)
--- a/lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp
+++ b/lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp
@@ -151,10 +151,8 @@ DynamicRegisterInfo::SetRegisterInfo(const StructuredData::Dictionary &dict,
               const uint32_t msbyte = msbit / 8;
               const uint32_t lsbyte = lsbit / 8;
 
-              ConstString containing_reg_name(reg_name_str);
-
               const RegisterInfo *containing_reg_info =
-                  GetRegisterInfo(containing_reg_name);
+                  GetRegisterInfo(reg_name_str);
               if (containing_reg_info) {
                 const uint32_t max_bit = containing_reg_info->byte_size * 8;
                 if (msbit < max_bit && lsbit < max_bit) {
@@ -189,7 +187,7 @@ DynamicRegisterInfo::SetRegisterInfo(const StructuredData::Dictionary &dict,
                 }
               } else {
                 printf("error: invalid concrete register \"%s\"\n",
-                       containing_reg_name.GetCString());
+                       reg_name_str.c_str());
               }
             } else {
               printf("error: msbit (%u) must be greater than lsbit (%u)\n",
@@ -217,7 +215,7 @@ DynamicRegisterInfo::SetRegisterInfo(const StructuredData::Dictionary &dict,
               if (composite_reg_list->GetItemAtIndexAsString(
                       composite_idx, composite_reg_name, nullptr)) {
                 const RegisterInfo *composite_reg_info =
-                    GetRegisterInfo(composite_reg_name);
+                    GetRegisterInfo(composite_reg_name.GetStringRef());
                 if (composite_reg_info) {
                   composite_offset = std::min(composite_offset,
                                               composite_reg_info->byte_offset);
@@ -357,7 +355,7 @@ DynamicRegisterInfo::SetRegisterInfo(const StructuredData::Dictionary &dict,
           if (invalidate_reg_list->GetItemAtIndexAsString(
                   idx, invalidate_reg_name)) {
             const RegisterInfo *invalidate_reg_info =
-                GetRegisterInfo(invalidate_reg_name);
+                GetRegisterInfo(invalidate_reg_name.GetStringRef());
             if (invalidate_reg_info) {
               m_invalidate_regs_map[i].push_back(
                   invalidate_reg_info->kinds[eRegisterKindLLDB]);
@@ -737,16 +735,10 @@ void DynamicRegisterInfo::Dump() const {
   }
 }
 
-const lldb_private::RegisterInfo *DynamicRegisterInfo::GetRegisterInfo(
-    lldb_private::ConstString reg_name) const {
-  for (auto &reg_info : m_regs) {
-    // We can use pointer comparison since we used a ConstString to set the
-    // "name" member in AddRegister()
-    assert(ConstString(reg_info.name).GetCString() == reg_info.name &&
-           "reg_info.name not from a ConstString?");
-    if (reg_info.name == reg_name.GetCString()) {
+const lldb_private::RegisterInfo *
+DynamicRegisterInfo::GetRegisterInfo(llvm::StringRef reg_name) const {
+  for (auto &reg_info : m_regs)
+    if (reg_info.name == reg_name)
       return &reg_info;
-    }
-  }
   return nullptr;
 }

diff --git a/lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.h b/lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.h
index 4893937..d5e6f90 100644 (file)
--- a/lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.h
+++ b/lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.h
@@ -75,7 +75,7 @@ protected:
   typedef std::map<uint32_t, dwarf_opcode> dynamic_reg_size_map;
 
   const lldb_private::RegisterInfo *
-  GetRegisterInfo(lldb_private::ConstString reg_name) const;
+  GetRegisterInfo(llvm::StringRef reg_name) const;
 
   void MoveFrom(DynamicRegisterInfo &&info);
 

diff --git a/lldb/source/Target/ABI.cpp b/lldb/source/Target/ABI.cpp
index 4320eb9..d50afb9 100644 (file)
--- a/lldb/source/Target/ABI.cpp
+++ b/lldb/source/Target/ABI.cpp
@@ -42,27 +42,22 @@ ABI::FindPlugin(lldb::ProcessSP process_sp, const ArchSpec &arch) {
 
 ABI::~ABI() = default;
 
-bool RegInfoBasedABI::GetRegisterInfoByName(ConstString name, RegisterInfo &info) {
+bool RegInfoBasedABI::GetRegisterInfoByName(llvm::StringRef name,
+                                            RegisterInfo &info) {
   uint32_t count = 0;
   const RegisterInfo *register_info_array = GetRegisterInfoArray(count);
   if (register_info_array) {
-    const char *unique_name_cstr = name.GetCString();
     uint32_t i;
     for (i = 0; i < count; ++i) {
       const char *reg_name = register_info_array[i].name;
-      assert(ConstString(reg_name).GetCString() == reg_name &&
-             "register_info_array[i].name not from a ConstString?");
-      if (reg_name == unique_name_cstr) {
+      if (reg_name == name) {
         info = register_info_array[i];
         return true;
       }
     }
     for (i = 0; i < count; ++i) {
       const char *reg_alt_name = register_info_array[i].alt_name;
-      assert((reg_alt_name == nullptr ||
-              ConstString(reg_alt_name).GetCString() == reg_alt_name) &&
-             "register_info_array[i].alt_name not from a ConstString?");
-      if (reg_alt_name == unique_name_cstr) {
+      if (reg_alt_name == name) {
         info = register_info_array[i];
         return true;
       }
@@ -224,7 +219,7 @@ void RegInfoBasedABI::AugmentRegisterInfo(RegisterInfo &info) {
     return;
 
   RegisterInfo abi_info;
-  if (!GetRegisterInfoByName(ConstString(info.name), abi_info))
+  if (!GetRegisterInfoByName(info.name, abi_info))
     return;
 
   if (info.kinds[eRegisterKindEHFrame] == LLDB_INVALID_REGNUM)