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

[Mitchell Horne via Phabricator via cfe-commits]

mhorne marked 5 inline comments as done.
mhorne added a comment.

Thanks for the review!



================
Comment at: libunwind/src/Registers.hpp:3585
+
+inline uint64_t Registers_riscv::getRegister(int regNum) const {
+  if (regNum == UNW_REG_IP)
----------------
lenary wrote:
> 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.
The assert is not necessary, since `_LIBUNWIND_ABORT` is called in the case of an invalid register number.


================
Comment at: libunwind/src/UnwindCursor.hpp:999
+#if defined (_LIBUNWIND_TARGET_RISCV)
+  int stepWithCompactEncoding(Registers_riscv &) {
+    return UNW_EINVAL;
----------------
lenary wrote:
> Nit: This should be formatted like the version for `Registers_sparc` above.
`Registers_sparc` is the outlier in this case, I've formatted it as all the other architectures use. If you'd like I could fix the SPARC formatting in this patch.


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

https://reviews.llvm.org/D68362