labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.
This is great, thanks for doing this. Just a couple of minor comments inline.
================
Comment at: lldb/source/Plugins/Process/FreeBSD/FreeBSDThread.cpp:167
case llvm::Triple::aarch64:
break;
case llvm::Triple::arm:
----------------
delete this break.
================
Comment at:
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp:61-62
new RegisterInfoPOSIX_arm(target_arch)) {
switch (target_arch.GetMachine()) {
case llvm::Triple::arm:
+ ::memset(&m_fpr, 0, sizeof(m_fpr));
----------------
Actually, I'd just change this to `assert(target_arch.GetMachine() ==
llvm::Triple::arm)`
================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm.cpp:93-95
+static_assert(((sizeof g_gpr_regnums_arm / sizeof g_gpr_regnums_arm[0]) - 1) ==
+ k_num_gpr_registers,
+ "g_gpr_regnums_arm has wrong number of register infos");
----------------
You can skip this if you want to keep things consistent (or put it in a
separate patch, etc), but I just realized that this is completely unnecessary.
Instead of the assert, we could just make the assert expression _be_ the
definition of the constant (`k_num_gpr_registers = ((sizeof g_gpr_regnums_arm /
sizeof g_gpr_regnums_arm[0]) - 1)`). Given that `llvm::array_lengthof` is
constexpr, it might even work to make this
`llvm::array_lengthof(g_gpr_regnums_arm)-1`.
================
Comment at: lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp:84-85
switch (arch.GetMachine()) {
case llvm::Triple::aarch64:
break;
case llvm::Triple::ppc:
----------------
I think you should either keep both aarch64 and arm cases, or delete both of
them. I'm fine with either.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86962/new/
https://reviews.llvm.org/D86962
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
