[Lldb-commits] [lldb] 24e0757 - [lldb] Remove all the RegisterInfo name constification code
Raphael Isemann via lldb-commits
lldb-commits at lists.llvm.org
Tue Oct 13 08:10:52 PDT 2020
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)
More information about the lldb-commits
mailing list