clayborg added a comment.
You are following all of the patterns for all of the architectures. It would be
nice for us to cleanup DynamicRegisterInfo.cpp, Platform.cpp, and Thread.cpp
eventually so we don't need to modify these files when a few arch is added by
abstracting things into lldb_private::Architecture, but that is beyond the
scope of this change.
================
Comment at: lldb/source/Plugins/ABI/SysV-riscv/ABISysV_riscv.cpp:41
+ eFormatHex,
+ {0, 0, LLDB_INVALID_REGNUM, 0, 0},
+ nullptr,
----------------
No need to fill in the "eRegisterKindProcessPlugin" field of each register
info. That is designed to be the number that any process plug-in (probably
ProcessGDBRemote.cpp) fills in, so best to not fill this in. The orders are: EH
frame, DWARF, generic, process plug-in and then LLDB. So I would change all of
these to:
```
{0, 0, LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM, 0},
```
================
Comment at: lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp:624-635
+ case llvm::Triple::riscv32:
+ case llvm::Triple::riscv64:
+ for (auto ® : m_regs) {
+ if (strcmp(reg.name, "x1") == 0)
+ reg.kinds[eRegisterKindGeneric] = LLDB_REGNUM_GENERIC_RA;
+ else if (strcmp(reg.name, "x2") == 0)
+ reg.kinds[eRegisterKindGeneric] = LLDB_REGNUM_GENERIC_SP;
----------------
Seems like you filled all of this info in the registers defs already in
ABISysV_riscv.cpp? Do we need this here? Some platforms weren't setting these
correctly and this code was meant to correct that.
================
Comment at: lldb/source/Target/Platform.cpp:1914-1922
+ case llvm::Triple::riscv32:
+ case llvm::Triple::riscv64: {
+ // FIXME: Use ebreak when c_ebreak is not available.
+ static const uint8_t g_riscv_c_opcode[] = {0x02, 0x90}; // c_ebreak
+ trap_opcode = g_riscv_c_opcode;
+ trap_opcode_size = sizeof(g_riscv_c_opcode);
+ break;
----------------
This would be great to factor out into lldb_private::Architecture eventually.
You are correctly following the patterns that are in LLDB. I will see if I can
get this to happen soon so we don't run into these issues again.
================
Comment at: lldb/source/Target/Thread.cpp:2060-2084
switch (machine) {
case llvm::Triple::x86_64:
case llvm::Triple::x86:
case llvm::Triple::arm:
case llvm::Triple::aarch64:
case llvm::Triple::thumb:
case llvm::Triple::mips:
----------------
Seems like the original logic before your mods is not correct. Not sure the
default case ever gets used here. The arch of x86_64, x86, arm, aarch64 or
thumb should all be caught before we get to the default statement for apple
targets. I will add Jason Molenda to the change to get a comment on this.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62732/new/
https://reviews.llvm.org/D62732
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
