[Lldb-commits] [PATCH] D132510: [RISCV][LLDB] Add initial SysV ABI support

[Emmmer S via Phabricator via lldb-commits]

Emmmer added a subscriber: labath.
Emmmer added a comment.

I have to say sorry after reading your update but not giving you feedback.
I pulled your patch and found it does not compile, and I have fixed them for you :)

In D62732#2306055 <https://reviews.llvm.org/D62732#2306055>, @labath wrote:

> ABI plugins are one of the hardest things to test in lldb, particularly without actual hardware. That's why we've let them be added in the past without any accompanying tests. The situation is not ideal though, because we've accumulated various ABI plugins which are hard to change without breaking (or even to know if they work) because they only work with third-party (possibly proprietary) stubs.

I have tried to work on ABISys_V plugins, but I think it is difficult because I am not able to check whether ABIPlugin works well.
It will be nice if we have some tests.



================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:37-39
+
+LLDB_PLUGIN_DEFINE(ABISysV_riscv)
+
----------------
modify target name.


================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:157-162
+  if (arch.GetTriple().getArch() == llvm::Triple::riscv32 ||
+      arch.GetTriple().getArch() == llvm::Triple::riscv64) {
+    return ABISP(
+        new ABISysV_riscv(std::move(process_sp), MakeMCRegisterInfo(arch),
+                          arch.GetTriple().getArch() == llvm::Triple::riscv64));
+  }
----------------
Using `.isRISCV()` instead of 
`arch.GetTriple().getArch() == llvm::Triple::riscv32 || arch.GetTriple().getArch() == llvm::Triple::riscv64`
and remove braces from `if` statment


================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:175-184
+// PluginInterface protocol
+
+lldb_private::ConstString ABISysV_riscv::GetPluginNameStatic() {
+  static ConstString g_name("sysv-riscv");
+  return g_name;
+}
+
----------------
We can remove this since we defined it in `ABISysV_riscv.h`


================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h:94-98
+  static lldb_private::ConstString GetPluginNameStatic();
+
+  lldb_private::ConstString GetPluginName() override;
+
+  uint32_t GetPluginVersion() override { return 1; }
----------------
GetPluginVersion() is unused.


================
Comment at: lldb/source/Plugins/ABI/RISCV/CMakeLists.txt:1
+add_lldb_library(lldbPluginABISysV_riscv PLUGIN
+  ABISysV_riscv.cpp
----------------
modify target name


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132510/new/

https://reviews.llvm.org/D132510