mhorne added reviewers: compnerd, phosek.
mhorne marked 2 inline comments as done.
mhorne added a comment.
Add some libunwind contributors for additional review.
================
Comment at: libunwind/src/Registers.hpp:3756
+inline double Registers_riscv::getFloatRegister(int regNum) const {
+#ifdef __riscv_float_abi_double
+ assert(validFloatRegister(regNum));
----------------
lenary wrote:
> 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.
That all makes sense, thank you for the explanation.
What I was referring to was more on the subject of cross-unwinding than
cross-compiling, e.g. unwinding a RISC-V target from an x86 host. In this case
the x86 code would obviously be compiled without the `__riscv_flen` macro, but
wouldn't it still need to be able to access the `_float` registers of
`Registers_riscv`? Cross-unwinding is stated as a future goal of libunwind [1],
rather than something that is currently supported, but still I wonder if my
thinking is correct on this.
[1] https://bcain-llvm.readthedocs.io/projects/libunwind/en/latest/
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68362/new/
https://reviews.llvm.org/D68362
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
