[Lldb-commits] [PATCH] D62732: [WIP][RISCV] Initial port of LLDB for RISC-V

[Greg Clayton via Phabricator via lldb-commits]

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 &reg : 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