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

[Sam Elliott via Phabricator via cfe-commits]

lenary accepted this revision.
lenary added a comment.

Thanks for updating the patch.

I'm happy for this patch to land, but I would like you to wait for @luismarques to approve it as well before landing it.



================
Comment at: libunwind/src/Registers.hpp:3756
+inline double Registers_riscv::getFloatRegister(int regNum) const {
+#ifdef __riscv_float_abi_double
+  assert(validFloatRegister(regNum));
----------------
mhorne wrote:
> 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?
Cross-compiling across RISC-V architectures is very complex. Sadly, using only the target triple is not enough, and nor is matching the ABI, because the architecture is so extensible.

In all of these cases, we expect end users to explicitly compile their required libraries with the correct `-march`/`-mabi` for the RISC-V platform they are using. If they are cross-compiling, then that means the whole sysroot, compiler runtime (libgcc or compiler-rt), and libc should be compiled with an explicitly-set `-march`/`-mabi`. If they do this, then there will be no issues with our code. Importantly, this should still largely work for multilib builds, where there are multiple march/mabi combinations that libraries are compiled for.

This is not optimal from the point-of-view of someone developing for lots of disparate RISC-V targets (like compiler developers), but should be ok for developers developing for single devices.


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

https://reviews.llvm.org/D68362