Page MenuHomePhabricator

[libunwind][RISCV] Add 64-bit RISC-V support
ClosedPublic

Authored by mhorne on Oct 2 2019, 5:25 PM.

Details

Summary

Add unwinding support for 64-bit RISC-V.

This is from the FreeBSD implementation with the following minor
changes:

  • Renamed and renumbered DWARF registers to match the RISC-V ABI [1]
  • Use the ABI mneumonics in getRegisterName() instead of the exact register names
  • Include checks for __riscv_xlen == 64 to facilitate adding the 32-bit ABI in the future.

[1] https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md

Diff Detail

Event Timeline

mhorne created this revision.Oct 2 2019, 5:25 PM
asb added a subscriber: asb.Oct 3 2019, 6:03 AM
lenary added a comment.Oct 8 2019, 5:55 AM

Nice! Thanks for adding support for RISC-V. I like the use of the ABI register names rather than the numeric names.

I have a few queries/nits, below.

libunwind/include/libunwind.h
835

Please include a link to the RISC-V ELF psABI spec which defines these register numbers.

libunwind/src/Registers.hpp
3553

Why is this a special GPR struct type, but the FPRs are not?

3585

Do you want to include an override in this function for regNum == UNW_RISCV_X0 to always return zero?

The reason I ask is because I worry that Registers_riscv::Registers_riscv(const void *registers) could initialise a non-zero value into _registers.__x[0], which could lead to really confusing bugs.

It might also make sense to include assert(validRegister(regNum)); in both this function and setRegister as you've done with the float registers.

libunwind/src/UnwindCursor.hpp
999

Nit: This should be formatted like the version for Registers_sparc above.

1071

Nit: This should be formatted like the version for Registers_sparc above.

1143

Nit: This should be formatted like the version for Registers_sparc above.

mhorne updated this revision to Diff 223971.Oct 8 2019, 6:24 PM

Address lenary's comments.

Add a check for __riscv_float_abi_double in getFloat/setFloat.

mhorne marked 5 inline comments as done.Oct 8 2019, 6:29 PM

Thanks for the review!

libunwind/src/Registers.hpp
3585

The assert is not necessary, since _LIBUNWIND_ABORT is called in the case of an invalid register number.

libunwind/src/UnwindCursor.hpp
999

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.

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?

libunwind/src/Registers.hpp
3677

Good catch, thanks!

3756

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.

libunwind/src/UnwindCursor.hpp
999

Ah, no worries then. Don't update the sparc formatting in this patch.

mhorne marked 2 inline comments as done.Oct 15 2019, 6:24 PM
mhorne added inline comments.
libunwind/src/Registers.hpp
3756

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.

lenary added inline comments.Oct 16 2019, 5:03 AM
libunwind/src/Registers.hpp
3756

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 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.

luismarques added inline comments.Oct 16 2019, 6:11 AM
libunwind/src/Registers.hpp
3756

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.

luismarques added inline comments.Oct 16 2019, 6:39 AM
libunwind/src/Registers.hpp
3545

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.

3756

I just saw @lenary's reply. I agree with his more detailed analysis.

luismarques requested changes to this revision.Oct 22 2019, 1:28 AM
luismarques added inline comments.
libunwind/include/__libunwind_config.h
26

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 revision now requires changes to proceed.Oct 22 2019, 1:28 AM
abidh added a subscriber: abidh.Oct 28 2019, 3:33 AM
mhorne updated this revision to Diff 228799.Nov 11 2019, 7:43 PM
mhorne marked an inline comment as done.

Convert checks for __riscv_float_abi_double to __riscv_flen == 64.

Fix _LIBUNWIND_HIGHEST_DWARF_REGISTER_RISCV.

mhorne marked 3 inline comments as done.Nov 11 2019, 8:17 PM
mhorne added inline comments.
libunwind/include/__libunwind_config.h
26

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.

libunwind/src/Registers.hpp
3545

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.

3756

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?

lenary accepted this revision.Nov 12 2019, 6:30 AM

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.

libunwind/src/Registers.hpp
3756

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.

mhorne marked 2 inline comments as done.

Add some libunwind contributors for additional review.

libunwind/src/Registers.hpp
3756

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/

abidh added inline comments.Nov 22 2019, 2:56 AM
libunwind/src/Registers.hpp
3765

I am building these changes and get the following warning.
warning: extra tokens at end of #ifdef directive [-Wextra-tokens]

You probably meant #if here.

libunwind/src/UnwindRegistersRestore.S
1042

This line generates the following warning when compiled without 'd' extension.

warning: '__riscv_flen' is not defined, evaluates to 0 [-Wundef]

Probably better to make it something like
#if defined(riscv_flen) && riscv_flen == 64

libunwind/src/UnwindRegistersSave.S
1020

Similar problem here as described above.

mhorne updated this revision to Diff 232721.Dec 7 2019, 2:01 PM
mhorne marked 3 inline comments as done.
lenary accepted this revision.Dec 13 2019, 8:18 AM

LGTM! Thanks for correcting the preprocessor issues!

This revision is now accepted and ready to land.Dec 16 2019, 4:52 AM

Thanks Sam and Luis for the review. Could I ask that one of you commit this if you are able?

Yeah, will do.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptDec 16 2019, 8:43 AM