[Lldb-commits] [PATCH] D128250: [LLDB][RISCV]Add initial support for lldb-server.

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

Overall this looks fine. Without the ability to test it I don't think we would accept it yet, so I didn't look into the details too much.

> To use gdbserver by lldb requires the participation of lldb-server, so my current plan is to allow lldb to co-op with gdbserver, then let lldb be able to use lldb-server, pass the unit tests, and finally add riscv32 support.

Sounds good. If those lldb changes can be tested by existing bots then they can go in first. For anything else it's probably better to build a tree of your changes and when you've got something that ends in a testable lldb-server then put them into review.

I appreciate it's difficult to test a lot of the enabling code e.g. the breakpoint code in this change. That'll be exercised by the test suite in general once more things are working. So it would be fine to say here is a series that ends in an lldb/lldb-server that can run a large amount of existing tests.

If you find anything non-riscv specific e.g. some gdb protocol difference that can go into review whenever. Anything to improve lldb-gdb compatibility is welcome.

> We use Open Build Server as the riscv build bot.

I'm not familiar with this infrastructure. In a future where your changes are in llvm `main` will developers be notified when they break the riscv lldb build?



================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_riscv64.cpp:249
+bool NativeRegisterContextLinux_riscv64::IsGPR(unsigned reg) const {
+  if (GetRegisterInfo().GetRegisterSetFromRegisterIndex(reg) ==
+      RegisterInfoPOSIX_riscv64::GPRegSet)
----------------
```
  return GetRegisterInfo().GetRegisterSetFromRegisterIndex(reg) == RegisterInfoPOSIX_riscv64::GPRegSet;
```


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_riscv64.cpp:256
+bool NativeRegisterContextLinux_riscv64::IsFPR(unsigned reg) const {
+  return false;
+}
----------------
Seems like this should do a lookup?


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_riscv64.cpp:323
+
+void NativeRegisterContextLinux_riscv64::InvalidateAllRegisters() {
+  m_gpr_is_valid = false;
----------------
Minor thing but if you call this in the constructor it's one less place to `= false` all the things.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_riscv64.cpp:341
+llvm::Error NativeRegisterContextLinux_riscv64::ReadHardwareDebugInfo() {
+  return llvm::Error::success();
+}
----------------
This also a placeholder I assume (I'm not sure what this does for other targets tbh).


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_riscv64.cpp:345
+    NativeRegisterContextLinux_riscv64::DREGType hwbType) {
+  return llvm::Error::success();
+}
----------------
This is a placeholder?


================
Comment at: lldb/source/Plugins/Process/Utility/NativeRegisterContextDBReg_riscv64.cpp:19
+constexpr uint32_t g_enable_bit = 1;
+// PAC (bits 2:1): 0b10
+constexpr uint32_t g_pac_bits = (2 << 1);
----------------
and PAC means? (please add it to the comment)


================
Comment at: lldb/source/Plugins/Process/Utility/NativeRegisterContextDBReg_riscv64.cpp:34
+  Log *log = GetLog(LLDBLog::Breakpoints);
+  llvm::Error error = ReadHardwareDebugInfo();
+  if (error) {
----------------
I don't see an implementation of this yet, not one that actually reads anything. Intentional? It'll just see 0 breakpoints I guess.


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_riscv64.cpp:51
+  switch (target_arch.GetMachine()) {
+  case llvm::Triple::riscv32:
+  case llvm::Triple::riscv64:
----------------
So the way lldb-server works, at least at the moment, is that one binary does one architecture. So for example the AArch64 linux build doesn't handle AArch32 mode, you'd need to use an AArch32 built binary for that.

Maybe you can make this work with riscv but I'd start with just looking for riscv64 here.


================
Comment at: lldb/source/Utility/RISCV64_DWARF_Registers.h:54
+
+} // namespace riscv64_dwarf
+
----------------
No DWARF numbers for the FPU regs?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128250