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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
