Author: Raphael Isemann Date: 2020-10-13T17:10:29+02:00 New Revision: 24e07570cc928b75e894b81639cabe96c660ccef
URL: https://github.com/llvm/llvm-project/commit/24e07570cc928b75e894b81639cabe96c660ccef DIFF: https://github.com/llvm/llvm-project/commit/24e07570cc928b75e894b81639cabe96c660ccef.diff LOG: [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 Added: Modified: lldb/include/lldb/Target/ABI.h lldb/source/Plugins/ABI/ARM/ABIMacOSX_arm.cpp lldb/source/Plugins/ABI/ARM/ABISysV_arm.cpp lldb/source/Plugins/ABI/Hexagon/ABISysV_hexagon.cpp lldb/source/Plugins/ABI/Mips/ABISysV_mips.cpp lldb/source/Plugins/ABI/Mips/ABISysV_mips64.cpp lldb/source/Plugins/ABI/SystemZ/ABISysV_s390x.cpp lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.h lldb/source/Target/ABI.cpp Removed: ################################################################################ diff --git a/lldb/include/lldb/Target/ABI.h b/lldb/include/lldb/Target/ABI.h index b252e4b54f03..131b2eaff765 100644 --- a/lldb/include/lldb/Target/ABI.h +++ b/lldb/include/lldb/Target/ABI.h @@ -159,7 +159,7 @@ class RegInfoBasedABI : public ABI { 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 ef500cb198a8..06c4590b7740 100644 --- 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 c63c569f875a..26b3152bed16 100644 --- 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 32313d4cd815..47aaefd3b228 100644 --- 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 a209fa760556..d66e0926ad99 100644 --- 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 9a07c3398e19..751555722dac 100644 --- 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 29ad4d97e718..22a64170017b 100644 --- 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 443638aa39f6..fd9e1923104d 100644 --- 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 ®_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 ®_info : m_regs) + if (reg_info.name == reg_name) return ®_info; - } - } return nullptr; } diff --git a/lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.h b/lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.h index 48939375a504..d5e6f90832bf 100644 --- a/lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.h +++ b/lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.h @@ -75,7 +75,7 @@ class DynamicRegisterInfo { 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 4320eb93adfc..d50afb9967e3 100644 --- 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) _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits