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

[Mitchell Horne via Phabricator via cfe-commits]

mhorne marked 3 inline comments as done.
mhorne added inline comments.


================
Comment at: libunwind/include/__libunwind_config.h:26
 #define _LIBUNWIND_HIGHEST_DWARF_REGISTER_SPARC     31
+#define _LIBUNWIND_HIGHEST_DWARF_REGISTER_RISCV     63
 
----------------
luismarques wrote:
> The highest dwarf register number is 64, the Alternate Frame Return Column. See https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md#dwarf-register-numbers-
This was made a little more complicated recently as they added CSRs to the DWARF encoding space, but I'm thinking for this patch we should just limit it to 64 until there's a reason to use those values.


================
Comment at: libunwind/src/Registers.hpp:3545
+  void      setSP(uint64_t value) { _registers[2] = value; }
+  uint64_t  getIP() const         { return _registers[1]; }
+  void      setIP(uint64_t value) { _registers[1] = value; }
----------------
luismarques wrote:
> `x1` is the return address. Is this the the `IP` to which we unwind to? How do we get the original `ra` value at that unwind point?
> I vaguely recall seeing this IP/x1 pattern in another unwind library, so it's probably correct, but it would be good to confirm and document such details.
I'm not sure that I know how to properly answer your question. 

What most architectures appear to do is add an extra `__pc` register to the register state and use that for get/set of IP. In the jumpto() assembly routines they will restore the value of `__pc` into the return register, ignoring whatever is saved for the return register in the register state's GPR array. What has been implemented for RISC-V should behave the same, we are just avoiding the extra `__pc` in the register state by using `_registers[1]` directly.


================
Comment at: libunwind/src/Registers.hpp:3756
+inline double Registers_riscv::getFloatRegister(int regNum) const {
+#ifdef __riscv_float_abi_double
+  assert(validFloatRegister(regNum));
----------------
luismarques wrote:
> luismarques wrote:
> > lenary wrote:
> > > mhorne wrote:
> > > > lenary wrote:
> > > > > 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`.
> > > > An ABI issue, in my opinion. The unwind frame will always contain space for the float registers, but accessing them should be disallowed for soft-float configurations as the intent of soft-float is that the FPRs will not be used at all. I'd say there is precedent for this in the MIPS implementation, since it checks for `defined(__mips_hard_float) && __mips_fpr == 64`.
> > > I had a discussion with @asb about this. The ISA vs ABI issue in RISC-V is complex. The TL;DR is we both think you need to be using `__riscv_flen == 64` here.
> > > 
> > > The reason for this is that if you compile with `-march=rv64imfd` but `-mabi=lp64`, the architecture still has floating point registers, it just does not use the floating-point calling convention. This means there are still `D`-extension instructions in the stream of instructions, just that "No floating-point registers, if present, are preserved across calls." (see [[ https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md#integer-calling-convention | psABI Integer Calling Convention ]]) This effectively means that with this combination, `f0-f31` are treated exactly the same as `t0-t6`, and so should be able to be restored when unwinding. It is not necessarily the case that with a soft float ABI, `f0-f31` are not used at all. This is similar to ARM's `soft` vs `softfp` calling conventions.
> > > 
> > > The expectation is that if you are compiling your programs with a specific `-march`, then you should be compiling your runtime libraries with the same `-march`. Eventually there should be enough details in the ELF file to allow you to ensure both `-march` and `-mabi` match when linking programs, but support for this is not widespread.
> > A soft-float *ABI* doesn't mean that FPRs aren't used at all, it means that floating-point arguments aren't passed in the floating-point registers. From a quick Google search I got the impression that `__mips_hard_float` was used for a mips softfloat target (i.e. without hardware floating-point support, not for a soft-float ABI), so that's probably not a comparable example.
> I just saw @lenary's reply. I agree with his more detailed analysis.
Thanks Sam and Luis for the detailed replies.

I definitely agree with you that `__riscv_flen == 64` is the more appropriate check. But now I'm reconsidering if a floating point check is needed at all. By adding it are we not preventing access to the FPRs for cross/remote unwinding?


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

https://reviews.llvm.org/D68362