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

[Tiezhu Yang via Phabricator via lldb-commits]

seehearfeel added inline comments.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_loongarch64.cpp:27
+// NT_PRSTATUS and NT_FPREGSET definition
+#include <elf.h>
+
----------------
SixWeining wrote:
> [[ https://llvm.org/docs/CodingStandards.html#include-style | Should be sorted lexicographically by the full path ]]. So put it before `sys/uio.h`. 
OK, will modify it, thank you.


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

```
/home/loongson/llvm.git/lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_loongarch64.cpp:221:12: error: cannot initialize a variable of type 'uint8_t *' (aka 'unsigned char *') with an rvalue of type 'const uint8_t *' (aka 'const unsigned char *')
  uint8_t *src = data_sp->GetBytes();
           ^     ~~~~~~~~~~~~~~~~~~~
1 error generated.
```


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_loongarch64.h:60
+  virtual bool WriteGPR() = 0;
+  virtual bool WriteFPR() = 0;
+};
----------------
DavidSpickett wrote:
> This doesn't look right, I'd expect `bool ... () override;` here.
Maybe we should leave it as is, the other archs do the same thing, 
they will be override in the following file which is not implemented now:
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_loongarch64.h/cpp,
otherwise build failed:

```
In file included from /home/loongson/llvm.git/lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_loongarch64.cpp:19:
/home/loongson/llvm.git/lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_loongarch64.h:57:18: error: only virtual member functions can be marked 'override'
  bool ReadGPR() override;
                 ^~~~~~~~
/home/loongson/llvm.git/lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_loongarch64.h:58:18: error: only virtual member functions can be marked 'override'
  bool ReadFPR() override;
                 ^~~~~~~~
/home/loongson/llvm.git/lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_loongarch64.h:59:19: error: only virtual member functions can be marked 'override'
  bool WriteGPR() override;
                  ^~~~~~~~
/home/loongson/llvm.git/lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_loongarch64.h:60:19: error: only virtual member functions can be marked 'override'
  bool WriteFPR() override;
                  ^~~~~~~~
4 errors generated.
```


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_loongarch64.cpp:138
+size_t RegisterInfoPOSIX_loongarch64::GetRegisterSetCount() const {
+  return k_num_register_sets - 1;
+}
----------------
DavidSpickett wrote:
> 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.
You are right, will modify it, thank you.


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfos_loongarch64.h:35
+  {                                                                            \
+    loongarch_dwarf::dwarf_##reg, loongarch_dwarf::dwarf_##reg, generic_kind,          \
+    LLDB_INVALID_REGNUM, reg##_loongarch                                           \
----------------
SixWeining wrote:
> unnecessary indent?
Yes, will modify it, thank you.


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfos_loongarch64.h:56
+
+#define DEFINE_FPR64(reg, generic_kind)                                        \
+  {                                                                            \
----------------
SixWeining wrote:
> Not allow accessing FPR registers through ABI names?
Will modify it, thank you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138407