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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
