[Lldb-commits] [PATCH] D138407: [LLDB] Add LoongArch register definitions and operations

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_loongarch64.cpp:194
+
+  uint8_t *dst = const_cast<uint8_t *>(data_sp->GetBytes());
+  ::memcpy(dst, GetGPRBuffer(), GetGPRSize());
----------------
I'm not sure const cast is needed.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_loongarch64.cpp:221
+
+  uint8_t *src = const_cast<uint8_t *>(data_sp->GetBytes());
+  if (src == nullptr) {
----------------
Possibly not needed const cast.


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_loongarch64.h:32
+
+  virtual size_t GetGPRSize();
+
----------------
Some of these can be override I'm pretty sure.

If you don't want to check manually I think there is some "possible override" warning in gcc at least.

(and they don't need to be virtual, unless there's some derived class of this that I've missed)


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_loongarch64.h:60
+  virtual bool WriteGPR() = 0;
+  virtual bool WriteFPR() = 0;
+};
----------------
This doesn't look right, I'd expect `bool ... () override;` here.


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_loongarch64.cpp:138
+size_t RegisterInfoPOSIX_loongarch64::GetRegisterSetCount() const {
+  return k_num_register_sets - 1;
+}
----------------
SixWeining wrote:
> Why `-  1`?
I had the same thought. From the few others I looked at, it seems that it's count not the last index. So if you've got N sets it should return N.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138407