[PATCH] D68362: [libunwind][RISCV] Add 64-bit RISC-V support

[Sam Elliott via Phabricator via cfe-commits]

lenary added a comment.

Nice! Thanks for adding support for RISC-V. I like the use of the ABI register names rather than the numeric names.

I have a few queries/nits, below.



================
Comment at: libunwind/include/libunwind.h:835
 
+// 64-bit RISC-V registers
+enum {
----------------
Please include a link to the RISC-V ELF psABI spec which defines these register numbers.


================
Comment at: libunwind/src/Registers.hpp:3553
+
+  GPRs    _registers;
+  double  _floats[32];
----------------
Why is this a special GPR struct type, but the FPRs are not?


================
Comment at: libunwind/src/Registers.hpp:3585
+
+inline uint64_t Registers_riscv::getRegister(int regNum) const {
+  if (regNum == UNW_REG_IP)
----------------
Do you want to include an override in this function for `regNum == UNW_RISCV_X0` to always return zero?

The reason I ask is because I worry that `Registers_riscv::Registers_riscv(const void *registers)` could initialise a non-zero value into `_registers.__x[0]`, which could lead to really confusing bugs.

It might also make sense to include `assert(validRegister(regNum));` in both this function and `setRegister` as you've done with the float registers.


================
Comment at: libunwind/src/UnwindCursor.hpp:999
+#if defined (_LIBUNWIND_TARGET_RISCV)
+  int stepWithCompactEncoding(Registers_riscv &) {
+    return UNW_EINVAL;
----------------
Nit: This should be formatted like the version for `Registers_sparc` above.


================
Comment at: libunwind/src/UnwindCursor.hpp:1071
+#if defined (_LIBUNWIND_TARGET_RISCV)
+  bool compactSaysUseDwarf(Registers_riscv &, uint32_t *) const {
+    return true;
----------------
Nit: This should be formatted like the version for `Registers_sparc` above.


================
Comment at: libunwind/src/UnwindCursor.hpp:1143
+#if defined (_LIBUNWIND_TARGET_RISCV)
+  compact_unwind_encoding_t dwarfEncoding(Registers_riscv &) const {
+    return 0;
----------------
Nit: This should be formatted like the version for `Registers_sparc` above.


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

https://reviews.llvm.org/D68362