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

[Sam Elliott via Phabricator via cfe-commits]

lenary added a comment.

One query, but this patch is starting to look good.

I'm not a libunwind expert - it would be good to have one of the libunwind contributors look over this patch yet. Can you add one as a reviewer?



================
Comment at: libunwind/src/Registers.hpp:3677
+  case UNW_RISCV_X31:
+    return "t6";
+  case UNW_RISCV_F0:
----------------
Good catch, thanks!


================
Comment at: libunwind/src/Registers.hpp:3756
+inline double Registers_riscv::getFloatRegister(int regNum) const {
+#ifdef __riscv_float_abi_double
+  assert(validFloatRegister(regNum));
----------------
Is this an ABI or an architecture issue? I'm not sure what other libunwind "backends" do for similar cases.

The difference is, if I compile libunwind with `-march=rv64g -mabi=lp64`, `__riscv_float_abi_double` is not defined (because you're using a soft-float ABI), but `__riscv_flen == 64` (because the machine does have hardware floating-point registers).

I'm not sure what the intended behaviour of libunwind is in this case.

`__riscv_float_abi_double` implies `__riscv_flen >= 64`.


================
Comment at: libunwind/src/UnwindCursor.hpp:999
+#if defined (_LIBUNWIND_TARGET_RISCV)
+  int stepWithCompactEncoding(Registers_riscv &) {
+    return UNW_EINVAL;
----------------
mhorne wrote:
> 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.
Ah, no worries then. Don't update the sparc formatting in this patch.


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

https://reviews.llvm.org/D68362