[lldb] Remove all the RegisterInfo name constification code

[teemperor]

Summary

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.

[teemperor]

I saw Pavel's comment about MCBasedABI in D88375 when this was already done, so I'll just put this up here. Not sure if I'll get around (or even be the right person) for doing that conversion, but I guess this patch is straightforward code removal that gets rid of some technical debt, so it's at least a step in the right direction.

[labath]

Yeah, this is good stuff independently of the MCBasedABI business.

[JDevlieghere]

Thanks!

lldb/include/lldb/Target/ABI.h

 
 class RegInfoBasedABI : public ABI {
 public:
   void AugmentRegisterInfo(RegisterInfo &info) override;
 
 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;
 };
 
 class MCBasedABI : public ABI {
 public:
   void AugmentRegisterInfo(RegisterInfo &info) override;
 

lldb/source/Plugins/ABI/ARM/ABIMacOSX_arm.cpp

 
 #include "Plugins/Process/Utility/ARMDefines.h"
 #include "Utility/ARM_DWARF_Registers.h"
 #include "Utility/ARM_ehframe_Registers.h"
 
 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
     //  ========== =======   == === =============    ============
     //  ======================= =================== ===========================
     //  ======================= ======================
     {"r0",
      "arg1",
eFormatHex,
       LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM},
      nullptr,
      nullptr,
      nullptr,
      0}};
 
 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;
 }
 
 size_t ABIMacOSX_arm::GetRedZoneSize() const { return 0; }
 
 // Static Functions
 

lldb/source/Plugins/ABI/ARM/ABISysV_arm.cpp

 #include "Utility/ARM_DWARF_Registers.h"
 #include "Utility/ARM_ehframe_Registers.h"
 
 using namespace lldb;
 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
     //  ========== =======   == === =============    ============
     //  ======================= =================== ===========================
     //  ======================= ====================== ==========
     //  ===============
     {"r0",
eFormatHex,
       LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM},
      nullptr,
      nullptr,
      nullptr,
      0}};
 
 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;
 }
 
 size_t ABISysV_arm::GetRedZoneSize() const { return 0; }
 
 // Static Functions
 

lldb/source/Plugins/ABI/Hexagon/ABISysV_hexagon.cpp

 #include "lldb/Utility/RegisterValue.h"
 #include "lldb/Utility/Status.h"
 
 using namespace lldb;
 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",
      "",
      4,
      0,
      eEncodingUint,
      eFormatAddressInfo,
      {0, 0, LLDB_INVALID_REGNUM, 0, 0},
static const RegisterInfo g_register_infos[] = {
      {83, 83, LLDB_INVALID_REGNUM, 83, 83},
      nullptr,
      nullptr,
      nullptr,
      0}};
 
 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;
 }
 
 /*
     http://en.wikipedia.org/wiki/Red_zone_%28computing%29
 
     In computing, a red zone is a fixed size area in memory beyond the stack

lldb/source/Plugins/ABI/Mips/ABISysV_mips.cpp

enum dwarf_regnums {
   dwarf_sr,
   dwarf_lo,
   dwarf_hi,
   dwarf_bad,
   dwarf_cause,
   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
     //  ========  ======  == === =============  ===========    ============
     //  ==============          ============                =================
     //  ===================     ========== =================
     {"r0",
      "zero",
static const RegisterInfo g_register_infos[] = {
      nullptr,
      nullptr,
      nullptr,
      0},
 };
 
 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;
 }
 
 size_t ABISysV_mips::GetRedZoneSize() const { return 0; }
 
 // Static Functions
 

lldb/source/Plugins/ABI/Mips/ABISysV_mips64.cpp

enum dwarf_regnums {
   dwarf_sr,
   dwarf_lo,
   dwarf_hi,
   dwarf_bad,
   dwarf_cause,
   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
     //  ========  ======  == === =============  ==========     =============
     //  =================       ====================        =================
     //  ====================
     {"r0",
      "zero",
static const RegisterInfo g_register_infos_mips64[] = {
      nullptr,
      nullptr,
      nullptr,
      0},
 };
 
 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;
 }
 
 size_t ABISysV_mips64::GetRedZoneSize() const { return 0; }
 
 // Static Functions
 

lldb/source/Plugins/ABI/SystemZ/ABISysV_s390x.cpp

 #define DEFINE_REG(name, size, alt, generic)                                   \
   {                                                                            \
     #name, alt, size, 0, eEncodingUint, eFormatHex,                            \
         {dwarf_##name##_s390x, dwarf_##name##_s390x, generic,                  \
          LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM },                           \
          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),
     DEFINE_REG(r3, 8, "arg2", LLDB_REGNUM_GENERIC_ARG2),
     DEFINE_REG(r4, 8, "arg3", LLDB_REGNUM_GENERIC_ARG3),
     DEFINE_REG(r5, 8, "arg4", LLDB_REGNUM_GENERIC_ARG4),
     DEFINE_REG(r6, 8, "arg5", LLDB_REGNUM_GENERIC_ARG5),
     DEFINE_REG(r7, 8, nullptr, LLDB_INVALID_REGNUM),
static const RegisterInfo g_register_infos[] = {
     DEFINE_REG(f12, 8, nullptr, LLDB_INVALID_REGNUM),
     DEFINE_REG(f13, 8, nullptr, LLDB_INVALID_REGNUM),
     DEFINE_REG(f14, 8, nullptr, LLDB_INVALID_REGNUM),
     DEFINE_REG(f15, 8, nullptr, LLDB_INVALID_REGNUM),
 };
 
 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;
 }
 
 size_t ABISysV_s390x::GetRedZoneSize() const { return 0; }
 
 // Static Functions
 

lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.h

protected:
   typedef std::vector<uint32_t> reg_num_collection;
   typedef std::vector<reg_num_collection> set_reg_num_collection;
   typedef std::vector<lldb_private::ConstString> name_collection;
   typedef std::map<uint32_t, reg_num_collection> reg_to_regs_map;
   typedef std::vector<uint8_t> dwarf_opcode;
   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);
 
   reg_collection m_regs;
   set_collection m_sets;
   set_reg_num_collection m_set_reg_nums;
   name_collection m_set_names;
   reg_to_regs_map m_value_regs_map;
   reg_to_regs_map m_invalidate_regs_map;
   dynamic_reg_size_map m_dynamic_reg_size_map;
   size_t m_reg_data_byte_size = 0u; // The number of bytes required to store
                                     // all registers
   bool m_finalized = false;
 };
 #endif // LLDB_SOURCE_PLUGINS_PROCESS_UTILITY_DYNAMICREGISTERINFO_H

lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp

if (reg_info.byte_offset == UINT32_MAX) {
               StringConvert::ToUInt32(msbit_str.c_str(), UINT32_MAX);
           const uint32_t lsbit =
               StringConvert::ToUInt32(lsbit_str.c_str(), UINT32_MAX);
           if (msbit != UINT32_MAX && lsbit != UINT32_MAX) {
             if (msbit > lsbit) {
               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) {
                   m_invalidate_regs_map[containing_reg_info
                                             ->kinds[eRegisterKindLLDB]]
                       .push_back(i);
                   m_value_regs_map[i].push_back(
                       containing_reg_info->kinds[eRegisterKindLLDB]);
if (reg_info.byte_offset == UINT32_MAX) {
                            msbit, max_bit);
                   else
                     printf("error: lsbit (%u) must be less than the bitsize "
                            "of the register (%u)\n",
                            lsbit, max_bit);
                 }
               } 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",
                      msbit, lsbit);
             }
           } else {
             printf("error: msbit (%u) and lsbit (%u) must be valid\n", msbit,
                    lsbit);
if (reg_info.byte_offset == UINT32_MAX) {
           if (num_composite_regs > 0) {
             uint32_t composite_offset = UINT32_MAX;
             for (uint32_t composite_idx = 0; composite_idx < num_composite_regs;
                  ++composite_idx) {
               ConstString composite_reg_name;
               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);
                   m_value_regs_map[i].push_back(
                       composite_reg_info->kinds[eRegisterKindLLDB]);
                   m_invalidate_regs_map[composite_reg_info
                                             ->kinds[eRegisterKindLLDB]]
                       .push_back(i);
if (reg_info_dict->GetValueForKeyAsArray("invalidate-regs",
       const size_t num_regs = invalidate_reg_list->GetSize();
       if (num_regs > 0) {
         for (uint32_t idx = 0; idx < num_regs; ++idx) {
           ConstString invalidate_reg_name;
           uint64_t invalidate_reg_num;
           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]);
             } else {
               // TODO: print error invalid slice string that doesn't follow the
               // format
               printf("error: failed to find a 'invalidate-regs' register for "
                      "\"%s\" while parsing register \"%s\"\n",
s.Printf("set[%" PRIu64 "] name = %s, regs = [", (uint64_t)i,
              m_sets[i].name);
     for (size_t idx = 0; idx < m_sets[i].num_registers; ++idx) {
       s.Printf("%s ", m_regs[m_sets[i].registers[idx]].name);
     }
     s.Printf("]\n");
   }
 }
 
-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;
 }

lldb/source/Target/ABI.cpp

if (abi_sp)
       return abi_sp;
   }
   abi_sp.reset();
   return abi_sp;
 }
 
 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;
       }
     }
   }
   return false;
 }
 

 }
 
 void RegInfoBasedABI::AugmentRegisterInfo(RegisterInfo &info) {
   if (info.kinds[eRegisterKindEHFrame] != LLDB_INVALID_REGNUM &&
       info.kinds[eRegisterKindDWARF] != LLDB_INVALID_REGNUM)
     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)
     info.kinds[eRegisterKindEHFrame] = abi_info.kinds[eRegisterKindEHFrame];
   if (info.kinds[eRegisterKindDWARF] == LLDB_INVALID_REGNUM)
     info.kinds[eRegisterKindDWARF] = abi_info.kinds[eRegisterKindDWARF];
   if (info.kinds[eRegisterKindGeneric] == LLDB_INVALID_REGNUM)
     info.kinds[eRegisterKindGeneric] = abi_info.kinds[eRegisterKindGeneric];